Make library models override annotations by default.

This change removes the short-lived `AcknowledgeLibraryModelsOfAnnotatedCode`
configuration flag.

It sets the behavior of NullAway to always allow library models to override
the nullability of annotated code, with an important caveat regarding #445:

Whenever we have a method `D.m` which overrides a method `C.m` for which we
have a library model, NullAway's behavior depends on whether `D.m` is an
annotated or unannotated method:

1) If `D.m` is within unannotated / third-party code, we consider the library
   model of `C.m` to extend to `D.m` (and to all such third-party overriding
   methods of `C.m`). This minimizes redundancy in our library models and makes
   them more robust to the changes within third-party library internals.
2) However, for an overriding `D.m` within annotated code, we don't propagate
   the library model, requiring either `D.m` to be annotated in a compatible
   manner or else a separate library model for it.

This makes it so that libraries for which the user doesn't have the source code
(i.e. can't add annotations), but which they wish to add to the `AnnotatedPackages`
list, require more comprehensive library models than "unannotated" libraries.
However, this is fully consistent with the intent behind marking code as annotated,
which is stricter checking of its nullability.

guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java

@@ -41,8 +41,7 @@ public void setup() {
                 Arrays.asList(
                     "-d",
                     temporaryFolder.getRoot().getAbsolutePath(),
-                    "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common",
-                    "-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=true"));
+                    "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common"));
   }
 
   @Test

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

@@ -129,8 +129,6 @@ public abstract class AbstractConfig implements Config {
 
   protected FixSerializationConfig fixSerializationConfig;
 
-  protected boolean acknowledgeLibraryModelsOfAnnotatedCode;
-
   @Override
   public boolean serializationIsActive() {
     return serializationActivationFlag;
@@ -351,9 +349,4 @@ public String getErrorURL() {
   public boolean acknowledgeAndroidRecent() {
     return acknowledgeAndroidRecent;
   }
-
-  @Override
-  public boolean acknowledgeLibraryModelsOfAnnotatedCode() {
-    return acknowledgeLibraryModelsOfAnnotatedCode;
-  }
 }

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

@@ -292,12 +292,4 @@ public interface Config {
    *     similarly for {@code @RecentlyNonNull}
    */
   boolean acknowledgeAndroidRecent();
-
-  /**
-   * Checks if {@link LibraryModels} can override annotations on annotated source code.
-   *
-   * @return true if NullAway should use information provided by {@link LibraryModels} on annotated
-   *     source code.
-   */
-  boolean acknowledgeLibraryModelsOfAnnotatedCode();
 }

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

@@ -215,9 +215,4 @@ public String getErrorURL() {
   public boolean acknowledgeAndroidRecent() {
     throw new IllegalStateException(ERROR_MESSAGE);
   }
-
-  @Override
-  public boolean acknowledgeLibraryModelsOfAnnotatedCode() {
-    throw new IllegalStateException(ERROR_MESSAGE);
-  }
 }

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

@@ -84,9 +84,6 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
   static final String FL_FIX_SERIALIZATION_CONFIG_PATH =
       EP_FL_NAMESPACE + ":FixSerializationConfigPath";
 
-  static final String FL_ACKNOWLEDGE_LIBRARY_MODELS_OF_ANNOTATED_CODE =
-      EP_FL_NAMESPACE + ":AcknowledgeLibraryModelsOfAnnotatedCode";
-
   private static final String DELIMITER = ",";
 
   static final ImmutableSet<String> DEFAULT_CLASS_ANNOTATIONS_TO_EXCLUDE =
@@ -251,8 +248,6 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
               + FL_SUGGEST_SUPPRESSIONS
               + ")");
     }
-    acknowledgeLibraryModelsOfAnnotatedCode =
-        flags.getBoolean(FL_ACKNOWLEDGE_LIBRARY_MODELS_OF_ANNOTATED_CODE).orElse(false);
   }
 
   private static ImmutableSet<String> getFlagStringSet(ErrorProneFlags flags, String flagName) {

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

@@ -107,17 +107,22 @@ public boolean onOverrideMayBeNullExpr(
     if (expr.getKind() == Tree.Kind.METHOD_INVOCATION) {
       OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
       Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) ASTHelpers.getSymbol(expr);
-      if (!config.acknowledgeLibraryModelsOfAnnotatedCode()
-          && !getClassAnnotationInfo(state.context)
-              .isSymbolUnannotated(methodSymbol, this.config)) {
-        // We only look at library models for unannotated (i.e. third-party) code.
-        return exprMayBeNull;
-      } else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes())
+      // When looking up library models of annotated code, we match the exact method signature only,
+      // overriding implementations that share the same model must be explicitly given their own
+      // library model.
+      // When dealing with unannotated code, we default to generality: a model applies to a method
+      // and
+      // any of its overriding implementations.
+      // see https://github.com/uber/NullAway/issues/445 for why this is needed.
+      boolean isMethodAnnotated =
+          !getClassAnnotationInfo(state.context).isSymbolUnannotated(methodSymbol, this.config);
+      if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isMethodAnnotated)
           || !optLibraryModels.nullImpliesNullParameters(methodSymbol).isEmpty()) {
         // These mean the method might be null, depending on dataflow and arguments. We force
         // dataflow to run.
         return analysis.nullnessFromDataflow(state, expr) || exprMayBeNull;
-      } else if (optLibraryModels.hasNonNullReturn(methodSymbol, state.getTypes())) {
+      } else if (optLibraryModels.hasNonNullReturn(
+          methodSymbol, state.getTypes(), !isMethodAnnotated)) {
         // This means the method can't be null, so we return false outright.
         return false;
       }
@@ -169,11 +174,8 @@ public NullnessHint onDataflowVisitMethodInvocation(
       AccessPathNullnessPropagation.Updates bothUpdates) {
     Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree());
     Preconditions.checkNotNull(callee);
-    if (!config.acknowledgeLibraryModelsOfAnnotatedCode()
-        && !getClassAnnotationInfo(context).isSymbolUnannotated(callee, this.config)) {
-      // Ignore annotated methods, library models should only apply to "unannotated" code.
-      return NullnessHint.UNKNOWN;
-    }
+    boolean isMethodAnnotated =
+        !getClassAnnotationInfo(context).isSymbolUnannotated(callee, this.config);
     setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee, context, apContext);
     setConditionalArgumentNullness(
         thenUpdates, elseUpdates, node.getArguments(), callee, context, apContext);
@@ -192,9 +194,9 @@ public NullnessHint onDataflowVisitMethodInvocation(
       }
       return anyNull ? NullnessHint.HINT_NULLABLE : NullnessHint.FORCE_NONNULL;
     }
-    if (getOptLibraryModels(context).hasNonNullReturn(callee, types)) {
+    if (getOptLibraryModels(context).hasNonNullReturn(callee, types, !isMethodAnnotated)) {
       return NullnessHint.FORCE_NONNULL;
-    } else if (getOptLibraryModels(context).hasNullableReturn(callee, types)) {
+    } else if (getOptLibraryModels(context).hasNullableReturn(callee, types, !isMethodAnnotated)) {
       return NullnessHint.HINT_NULLABLE;
     } else {
       return NullnessHint.UNKNOWN;
@@ -895,12 +897,12 @@ public OptimizedLibraryModels(LibraryModels models, Context context) {
       castToNonNullMethods = makeOptimizedIntSetLookup(names, models.castToNonNullMethods());
     }
 
-    public boolean hasNonNullReturn(Symbol.MethodSymbol symbol, Types types) {
-      return lookupHandlingOverrides(symbol, types, nonNullRet) != null;
+    public boolean hasNonNullReturn(Symbol.MethodSymbol symbol, Types types, boolean checkSuper) {
+      return lookupHandlingOverrides(symbol, types, nonNullRet, checkSuper) != null;
     }
 
-    public boolean hasNullableReturn(Symbol.MethodSymbol symbol, Types types) {
-      return lookupHandlingOverrides(symbol, types, nullableRet) != null;
+    public boolean hasNullableReturn(Symbol.MethodSymbol symbol, Types types, boolean checkSuper) {
+      return lookupHandlingOverrides(symbol, types, nullableRet, checkSuper) != null;
     }
 
     ImmutableSet<Integer> failIfNullParameters(Symbol.MethodSymbol symbol) {
@@ -968,14 +970,23 @@ private <T> NameIndexedMap<T> makeOptimizedLookup(
      */
     @Nullable
     private static Symbol.MethodSymbol lookupHandlingOverrides(
-        Symbol.MethodSymbol symbol, Types types, NameIndexedMap<Boolean> optLookup) {
+        Symbol.MethodSymbol symbol,
+        Types types,
+        NameIndexedMap<Boolean> optLookup,
+        boolean checkSuperTypes) {
       if (optLookup.nameNotPresent(symbol)) {
         // no model matching the method name, so we don't need to check for overridden methods
         return null;
       }
       if (optLookup.get(symbol) != null) {
         return symbol;
       }
+      if (checkSuperTypes == false) {
+        // Consider only a model on the exact class and method, used when checking annotated code
+        return null;
+      }
+      // For unannotated code, we allow a single model to cover all overriding implementations /
+      // subtypes
       for (Symbol.MethodSymbol superSymbol : ASTHelpers.findSuperMethods(symbol, types)) {
         if (optLookup.get(superSymbol) != null) {
           return superSymbol;

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

@@ -51,8 +51,7 @@ public void allowLibraryModelsOverrideAnnotations() {
             Arrays.asList(
                 "-d",
                 temporaryFolder.getRoot().getAbsolutePath(),
-                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
-                "-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=true"))
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
         .addSourceLines(
             "Foo.java",
             "package com.uber;",
@@ -75,34 +74,6 @@ public void allowLibraryModelsOverrideAnnotations() {
         .doTest();
   }
 
-  @Test
-  public void allowLibraryModelsOverrideAnnotationsFlagTest() {
-    makeLibraryModelsTestHelperWithArgs(
-            Arrays.asList(
-                "-d",
-                temporaryFolder.getRoot().getAbsolutePath(),
-                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
-                "-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode=false"))
-        .addSourceLines(
-            "Foo.java",
-            "package com.uber;",
-            "public class Foo {",
-            "   Object field = new Object();",
-            "   Object bar() {",
-            "      return new Object();",
-            "   }",
-            "   Object nullableReturn() {",
-            "       return bar();",
-            "   }",
-            "   void run() {",
-            "       // just to make sure, flow analysis is not impacted by library models information",
-            "      Object temp = bar();",
-            "      this.field = temp;",
-            "   }",
-            "}")
-        .doTest();
-  }
-
   @Test
   public void libraryModelsOverrideRestrictiveAnnotations() {
     makeLibraryModelsTestHelperWithArgs(