Add command line option to skip specific library models. (#741)

This PR adds the CLI option `-XepOpt:NullAway:IgnoreLibraryModelsFor` which takes a list of methods, given as fully qualified class name + method simple name (i.e. independent of argument types). Methods matching this list will be skipped when loading library models (from any implementation of the `LibraryModels`'s `@AutoService` as well as our included models). This can be used in a per-compilation target basis to disable NullAway's library models for ease of upgrading to new versions with stricter modeling of common JDK or third-party APIs. We considered a few alternative approaches here, but decided against: - Simply using another instance of `LibraryModels` to "invert" the models given by NullAway's default library models: This would have required no code changes, but doesn't work in all cases. If NullAway's default models make the return of method `foo()` `@Nullable`, for example, then a new model that makes the return `@NonNull` will break `@Nullable` overrides of `foo()`. In general, we want to go back to "optimistic assumptions" rather than just replace the library model. - We could have a list of methods in the `LibraryModels` for which to ignore previous models, and have those override any models on those methods coming from a different `LibraryModels` implementation. But, from the point of view of the user configuring NullAway, this is complex: they need to have an instance of custom library models in their build, and changing java plugin classpath deps on a per-target basis is more complex than changing CLI arguments (e.g. due to JVM re-use by the build system). - We could provide more specific disabling of library models (e.g. a specific method signature or removing only one particular kind of model from a method, such as keeping the model on the return value, but removing it from an argument, or removing a null-implies-false model or similar). We could revisit this in the future, but supporting this would make the syntax of the CLI values a lot more complex. For now, we believe just turning off all models for a given method is a sufficient degree of granularity. - Per-package/per-class/regex based ignore specs: See above. Avoiding complexity until we need it. Note: If and when this lands, it needs a Wiki documentation update! --------- Co-authored-by: Manu Sridharan <[email protected]>
uber · Mar 6, 2023 · 9b6d93f · 9b6d93f
1 parent 49cda49
commit 9b6d93f
Show file tree

Hide file tree

Showing 8 changed files with 209 additions and 6 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/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 {
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/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 ---
 
   /**

diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/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);

diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/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);

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/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;

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/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();
+  }
 }
diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java b/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;
+  }
+}
diff --git a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java b/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