Merge branch 'master' into generics-ternary-operator-assignment-changes

README.md

@@ -84,7 +84,7 @@ A complete Android `build.gradle` example is [here](https://gist.github.com/msri
 
 #### Annotation Processors / Generated Code
 
-Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code.  This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build.  Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.13 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle).  To use, figure out which directory contains the generated code, and add that directory to the excluded path regex.
+Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code.  This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build.  Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.1.3 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle).  To use, figure out which directory contains the generated code, and add that directory to the excluded path regex.
 
 **Note for Dagger users**: Dagger versions older than 2.12 can have bad interactions with NullAway; see [here](https://github.com/uber/NullAway/issues/48#issuecomment-340018409).  Please update to Dagger 2.12 to fix the problem.
 

nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java

@@ -101,6 +101,8 @@ public abstract class AbstractConfig implements Config {
 
   protected String autofixSuppressionComment;
 
+  protected ImmutableSet<String> skippedLibraryModels;
+
   /** --- JarInfer configs --- */
   protected boolean jarInferEnabled;
 
@@ -292,6 +294,11 @@ public boolean isContractAnnotation(String annotationName) {
     return contractAnnotations.contains(annotationName);
   }
 
+  @Override
+  public boolean isSkippedLibraryModel(String classDotMethod) {
+    return skippedLibraryModels.contains(classDotMethod);
+  }
+
   @AutoValue
   abstract static class MethodClassAndName {
 

nullaway/src/main/java/com/uber/nullaway/Config.java

@@ -249,6 +249,20 @@ public interface Config {
    */
   String getAutofixSuppressionComment();
 
+  /**
+   * Checks if the given library model should be skipped/ignored.
+   *
+   * <p>For ease of configuration in the command line, this works at the level of the (class, method
+   * name) pair, meaning it applies for all methods with the same name in the same class, even if
+   * they have different signatures, and to all library models applicable to that method (i.e. on
+   * the method's return, arguments, etc).
+   *
+   * @param classDotMethod The method from the model, in [fully_qualified_class_name].[method_name]
+   *     format (no args)
+   * @return True if the library model should be skipped.
+   */
+  boolean isSkippedLibraryModel(String classDotMethod);
+
   // --- JarInfer configs ---
 
   /**

nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java

@@ -185,6 +185,11 @@ public String getAutofixSuppressionComment() {
     throw new IllegalStateException(ERROR_MESSAGE);
   }
 
+  @Override
+  public boolean isSkippedLibraryModel(String classDotMethod) {
+    throw new IllegalStateException(ERROR_MESSAGE);
+  }
+
   @Override
   public boolean isJarInferEnabled() {
     throw new IllegalStateException(ERROR_MESSAGE);

nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java

@@ -72,6 +72,9 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
   static final String FL_OPTIONAL_CLASS_PATHS =
       EP_FL_NAMESPACE + ":CheckOptionalEmptinessCustomClasses";
   static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment";
+
+  static final String FL_SKIP_LIBRARY_MODELS = EP_FL_NAMESPACE + ":IgnoreLibraryModelsFor";
+
   /** --- JarInfer configs --- */
   static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
 
@@ -209,6 +212,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
       throw new IllegalStateException(
           "Invalid -XepOpt:" + FL_SUPPRESS_COMMENT + " value. Comment must be single line.");
     }
+    skippedLibraryModels = getFlagStringSet(flags, FL_SKIP_LIBRARY_MODELS);
     /** --- JarInfer configs --- */
     jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false);
     jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false);

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

@@ -75,7 +75,7 @@ public class LibraryModelsHandler extends BaseNoOpHandler {
   public LibraryModelsHandler(Config config) {
     super();
     this.config = config;
-    libraryModels = loadLibraryModels();
+    libraryModels = loadLibraryModels(config);
   }
 
   @Override
@@ -283,12 +283,12 @@ private void setUnconditionalArgumentNullness(
     }
   }
 
-  private static LibraryModels loadLibraryModels() {
+  private static LibraryModels loadLibraryModels(Config config) {
     Iterable<LibraryModels> externalLibraryModels =
         ServiceLoader.load(LibraryModels.class, LibraryModels.class.getClassLoader());
     ImmutableSet.Builder<LibraryModels> libModelsBuilder = new ImmutableSet.Builder<>();
     libModelsBuilder.add(new DefaultLibraryModels()).addAll(externalLibraryModels);
-    return new CombinedLibraryModels(libModelsBuilder.build());
+    return new CombinedLibraryModels(libModelsBuilder.build(), config);
   }
 
   private static class DefaultLibraryModels implements LibraryModels {
@@ -751,6 +751,8 @@ public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {
 
   private static class CombinedLibraryModels implements LibraryModels {
 
+    private final Config config;
+
     private final ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters;
 
     private final ImmutableSetMultimap<MethodRef, Integer> explicitlyNullableParameters;
@@ -769,7 +771,8 @@ private static class CombinedLibraryModels implements LibraryModels {
 
     private final ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods;
 
-    public CombinedLibraryModels(Iterable<LibraryModels> models) {
+    public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
+      this.config = config;
       ImmutableSetMultimap.Builder<MethodRef, Integer> failIfNullParametersBuilder =
           new ImmutableSetMultimap.Builder<>();
       ImmutableSetMultimap.Builder<MethodRef, Integer> explicitlyNullableParametersBuilder =
@@ -788,34 +791,61 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
           new ImmutableSetMultimap.Builder<>();
       for (LibraryModels libraryModels : models) {
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.failIfNullParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           failIfNullParametersBuilder.put(entry);
         }
         for (Map.Entry<MethodRef, Integer> entry :
             libraryModels.explicitlyNullableParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           explicitlyNullableParametersBuilder.put(entry);
         }
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.nonNullParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           nonNullParametersBuilder.put(entry);
         }
         for (Map.Entry<MethodRef, Integer> entry :
             libraryModels.nullImpliesTrueParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           nullImpliesTrueParametersBuilder.put(entry);
         }
         for (Map.Entry<MethodRef, Integer> entry :
             libraryModels.nullImpliesFalseParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           nullImpliesFalseParametersBuilder.put(entry);
         }
         for (Map.Entry<MethodRef, Integer> entry :
             libraryModels.nullImpliesNullParameters().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           nullImpliesNullParametersBuilder.put(entry);
         }
         for (MethodRef name : libraryModels.nullableReturns()) {
+          if (shouldSkipModel(name)) {
+            continue;
+          }
           nullableReturnsBuilder.add(name);
         }
         for (MethodRef name : libraryModels.nonNullReturns()) {
+          if (shouldSkipModel(name)) {
+            continue;
+          }
           nonNullReturnsBuilder.add(name);
         }
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.castToNonNullMethods().entries()) {
+          if (shouldSkipModel(entry.getKey())) {
+            continue;
+          }
           castToNonNullMethodsBuilder.put(entry);
         }
       }
@@ -830,6 +860,10 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
       castToNonNullMethods = castToNonNullMethodsBuilder.build();
     }
 
+    private boolean shouldSkipModel(MethodRef key) {
+      return config.isSkippedLibraryModel(key.enclosingClass + "." + key.methodName);
+    }
+
     @Override
     public ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters() {
       return failIfNullParameters;

nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java

@@ -107,4 +107,122 @@ public void libraryModelsOverrideRestrictiveAnnotations() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void libraryModelsAndSelectiveSkippingViaCommandLineOptions() {
+    // First test with the library models in effect
+    makeLibraryModelsTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated"))
+        .addSourceLines(
+            "CallMethods.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.UnannotatedWithModels;",
+            "import javax.annotation.Nullable;",
+            "public class CallMethods {",
+            "  Object testWithoutCheck(UnannotatedWithModels u) {",
+            "     // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
+            "     return u.returnsNullUnannotated();",
+            "  }",
+            "  Object testWithCheck(@Nullable Object o) {",
+            "     if (UnannotatedWithModels.isNonNull(o)) {",
+            "       return o;",
+            "     }",
+            "     return new Object();",
+            "  }",
+            "}")
+        .addSourceLines(
+            "OverrideCheck.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.UnannotatedWithModels;",
+            "import javax.annotation.Nullable;",
+            "public class OverrideCheck extends UnannotatedWithModels {",
+            "  @Nullable",
+            "  public Object returnsNullUnannotated() {",
+            "     return null;",
+            "  }",
+            "}")
+        .doTest();
+    // Now test disabling the library model
+    makeLibraryModelsTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.UnannotatedWithModels.returnsNullUnannotated,com.uber.lib.unannotated.UnannotatedWithModels.isNonNull"))
+        .addSourceLines(
+            "CallMethods.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.UnannotatedWithModels;",
+            "import javax.annotation.Nullable;",
+            "public class CallMethods {",
+            "  Object testWithoutCheck(UnannotatedWithModels u) {",
+            "     // Ok. Library model ignored",
+            "     return u.returnsNullUnannotated();",
+            "  }",
+            "  Object testWithoutCheckNonIgnoredMethod(UnannotatedWithModels u) {",
+            "     // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
+            "     return u.returnsNullUnannotated2();",
+            "  }",
+            "  Object testWithCheck(@Nullable Object o) {",
+            "     if (UnannotatedWithModels.isNonNull(o)) {",
+            "       // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
+            "       return o;",
+            "     }",
+            "     return new Object();",
+            "  }",
+            "}")
+        .addSourceLines(
+            "OverrideCheck.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.UnannotatedWithModels;",
+            "import javax.annotation.Nullable;",
+            "public class OverrideCheck extends UnannotatedWithModels {",
+            "  @Nullable", // Still safe, because the method is not @NonNull, it's unannotated
+            // without model!
+            "  public Object returnsNullUnannotated() {",
+            "     return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void libraryModelsAndSelectiveSkippingViaCommandLineOptions2() {
+    makeLibraryModelsTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true",
+                "-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride.apply"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride;",
+            "import javax.annotation.Nullable;",
+            "public class Test {",
+            "  void bar(RestrictivelyAnnotatedFIWithModelOverride f) {",
+            "     // Param is @NullableDecl in bytecode, and library model making it non-null is skipped",
+            "     f.apply(null);",
+            "  }",
+            "  void foo() {",
+            "    RestrictivelyAnnotatedFIWithModelOverride func = (x) -> {",
+            "     // Param is @NullableDecl in bytecode, and overriding library model is ignored, thus unsafe",
+            "     // BUG: Diagnostic contains: dereferenced expression x is @Nullable",
+            "     return x.toString();",
+            "    };",
+            "  }",
+            "  void baz() {",
+            "     // BUG: Diagnostic contains: unbound instance method reference cannot be used",
+            "     bar(Object::toString);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java

@@ -0,0 +1,16 @@
+package com.uber.lib.unannotated;
+
+public class UnannotatedWithModels {
+
+  public Object returnsNullUnannotated() {
+    return null;
+  }
+
+  public Object returnsNullUnannotated2() {
+    return null;
+  }
+
+  public static boolean isNonNull(Object o) {
+    return o != null;
+  }
+}

test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java

@@ -53,7 +53,9 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesTrueParameters() {
 
   @Override
   public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
-    return ImmutableSetMultimap.of();
+    return ImmutableSetMultimap.of(
+        methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "isNonNull(java.lang.Object)"),
+        0);
   }
 
   @Override
@@ -63,7 +65,10 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
 
   @Override
   public ImmutableSet<MethodRef> nullableReturns() {
-    return ImmutableSet.of(methodRef("com.uber.Foo", "bar()"));
+    return ImmutableSet.of(
+        methodRef("com.uber.Foo", "bar()"),
+        methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"),
+        methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()"));
   }
 
   @Override