Ensure classes null-marked by library model detected in all places (#1197)

Fixes #1194 

Previously we were sloppy about allowing a null `Handler` to be passed
into `CodeAnnotationInfo` and then caching the result. Here we change
the code to always pass a non-null `Handler` in to public APIs of
`CodeAnnotationInfo`, and we avoid caching if the handler is ever `null`
from an internal call.

Author: msridhar

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

@@ -115,9 +115,9 @@ && hasDirectAnnotationWithSimpleName(
   public boolean isGenerated(Symbol symbol, Config config) {
     Symbol.ClassSymbol classSymbol = enclosingClass(symbol);
     if (classSymbol == null) {
+      // One known case where this can happen: int.class, void.class, etc.
       Preconditions.checkArgument(
-          isClassFieldOfPrimitiveType(
-              symbol), // One known case where this can happen: int.class, void.class, etc.
+          isClassFieldOfPrimitiveType(symbol),
           String.format(
               "Unexpected symbol passed to CodeAnnotationInfo.isGenerated(...) with null enclosing class: %s",
               symbol));
@@ -152,7 +152,7 @@ private static boolean isClassFieldOfPrimitiveType(Symbol symbol) {
    * @return true if symbol represents an entity contained in a class that is unannotated; false
    *     otherwise
    */
-  public boolean isSymbolUnannotated(Symbol symbol, Config config, @Nullable Handler handler) {
+  public boolean isSymbolUnannotated(Symbol symbol, Config config, Handler handler) {
     Symbol.ClassSymbol classSymbol;
     if (symbol instanceof Symbol.ClassSymbol) {
       classSymbol = (Symbol.ClassSymbol) symbol;
@@ -245,7 +245,11 @@ record = new ClassCacheRecord(recordForEnclosing.outermostClassSymbol, isAnnotat
       record =
           new ClassCacheRecord(classSymbol, isAnnotatedTopLevelClass(classSymbol, config, handler));
     }
-    classCache.put(classSymbol, record);
+    // Don't update the cache if the handler is null, as we may not have full info about classes
+    // being null-marked via library models
+    if (handler != null) {
+      classCache.put(classSymbol, record);
+    }
     return record;
   }
 

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

@@ -2601,7 +2601,7 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
               "unexpected null symbol for dereference expression " + state.getSourceForNode(expr));
         }
         exprMayBeNull =
-            NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, codeAnnotationInfo);
+            NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, handler, codeAnnotationInfo);
         break;
       case IDENTIFIER:
         if (exprSymbol == null) {
@@ -2610,7 +2610,8 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
         }
         if (exprSymbol.getKind() == ElementKind.FIELD) {
           exprMayBeNull =
-              NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, codeAnnotationInfo);
+              NullabilityUtil.mayBeNullFieldFromType(
+                  exprSymbol, config, handler, codeAnnotationInfo);
         } else {
           // rely on dataflow analysis for local variables
           exprMayBeNull = true;

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

@@ -48,6 +48,7 @@
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.tree.JCTree;
 import com.sun.tools.javac.util.JCDiagnostic;
+import com.uber.nullaway.handlers.Handler;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -476,10 +477,10 @@ private static boolean isTypeOfNestedClass(Type type) {
    *     the field might be null; false otherwise
    */
   public static boolean mayBeNullFieldFromType(
-      Symbol symbol, Config config, CodeAnnotationInfo codeAnnotationInfo) {
+      Symbol symbol, Config config, Handler handler, CodeAnnotationInfo codeAnnotationInfo) {
     return !(symbol.getSimpleName().toString().equals("class")
             || symbol.isEnum()
-            || codeAnnotationInfo.isSymbolUnannotated(symbol, config, null))
+            || codeAnnotationInfo.isSymbolUnannotated(symbol, config, handler))
         && Nullness.hasNullableOrMonotonicNonNullAnnotation(symbol, config);
   }
 

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

@@ -751,7 +751,8 @@ public TransferResult<Nullness, NullnessStore> visitFieldAccess(
         break;
       case UNKNOWN:
         fieldMayBeNull =
-            NullabilityUtil.mayBeNullFieldFromType(symbol, config, getCodeAnnotationInfo(state));
+            NullabilityUtil.mayBeNullFieldFromType(
+                symbol, config, handler, getCodeAnnotationInfo(state));
         break;
       default:
         // Should be unreachable unless NullnessHint changes, cases above are exhaustive!

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

@@ -50,10 +50,12 @@ public static Handler buildDefault(Config config) {
     // requested it
     boolean acknowledgeRestrictive =
         config.acknowledgeRestrictiveAnnotations() || config.isJSpecifyMode();
+    RestrictiveAnnotationHandler restrictiveAnnotationHandler = null;
     if (acknowledgeRestrictive) {
       // This runs before LibraryModelsHandler, so that library models can override third-party
       // bytecode annotations
-      handlerListBuilder.add(new RestrictiveAnnotationHandler(config));
+      restrictiveAnnotationHandler = new RestrictiveAnnotationHandler(config);
+      handlerListBuilder.add(restrictiveAnnotationHandler);
     }
     if (config.handleTestAssertionLibraries()) {
       handlerListBuilder.add(new AssertionHandler(methodNameUtil));
@@ -85,8 +87,15 @@ public static Handler buildDefault(Config config) {
     }
     handlerListBuilder.add(new LombokHandler(config));
     handlerListBuilder.add(new FluentFutureHandler(config));
+    CompositeHandler mainHandler = new CompositeHandler(handlerListBuilder.build());
 
-    return new CompositeHandler(handlerListBuilder.build());
+    // Initialize the handlers that need to be aware of the main handler
+    if (restrictiveAnnotationHandler != null) {
+      restrictiveAnnotationHandler.initMainHandler(mainHandler);
+    }
+    libraryModelsHandler.initMainHandler(mainHandler);
+
+    return mainHandler;
   }
 
   /**

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

@@ -46,6 +46,7 @@
 import com.uber.nullaway.LibraryModels.MethodRef;
 import com.uber.nullaway.NullAway;
 import com.uber.nullaway.Nullness;
+import com.uber.nullaway.annotations.Initializer;
 import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
 import com.uber.nullaway.handlers.stream.StreamTypeRecord;
@@ -73,6 +74,7 @@
 public class LibraryModelsHandler extends BaseNoOpHandler {
 
   private final Config config;
+  private Handler mainHandler;
   private final LibraryModels libraryModels;
 
   private @Nullable OptimizedLibraryModels optLibraryModels;
@@ -83,6 +85,11 @@ public LibraryModelsHandler(Config config) {
     libraryModels = loadLibraryModels(config);
   }
 
+  @Initializer
+  public void initMainHandler(Handler mainHandler) {
+    this.mainHandler = mainHandler;
+  }
+
   @Override
   public boolean onOverrideFieldNullability(Symbol field) {
     return isNullableFieldInLibraryModels(field);
@@ -169,7 +176,8 @@ public boolean onOverrideMayBeNullExpr(
     // and any of its overriding implementations.
     // see https://github.com/uber/NullAway/issues/445 for why this is needed.
     boolean isMethodUnannotated =
-        getCodeAnnotationInfo(state.context).isSymbolUnannotated(methodSymbol, this.config, null);
+        getCodeAnnotationInfo(state.context)
+            .isSymbolUnannotated(methodSymbol, this.config, mainHandler);
     if (exprMayBeNull) {
       // This is the only case in which we may switch the result from @Nullable to @NonNull:
       return !optLibraryModels.hasNonNullReturn(
@@ -226,7 +234,7 @@ public NullnessHint onDataflowVisitMethodInvocation(
       AccessPathNullnessPropagation.Updates elseUpdates,
       AccessPathNullnessPropagation.Updates bothUpdates) {
     boolean isMethodAnnotated =
-        !getCodeAnnotationInfo(state.context).isSymbolUnannotated(callee, this.config, null);
+        !getCodeAnnotationInfo(state.context).isSymbolUnannotated(callee, this.config, mainHandler);
     setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee, state, apContext);
     setConditionalArgumentNullness(
         thenUpdates, elseUpdates, node.getArguments(), callee, state, apContext);

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

@@ -33,6 +33,7 @@
 import com.uber.nullaway.NullAway;
 import com.uber.nullaway.NullabilityUtil;
 import com.uber.nullaway.Nullness;
+import com.uber.nullaway.annotations.Initializer;
 import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
 import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode;
@@ -42,11 +43,17 @@
 public class RestrictiveAnnotationHandler extends BaseNoOpHandler {
 
   private final Config config;
+  private Handler mainHandler;
 
   RestrictiveAnnotationHandler(Config config) {
     this.config = config;
   }
 
+  @Initializer
+  public void initMainHandler(Handler mainHandler) {
+    this.mainHandler = mainHandler;
+  }
+
   /**
    * Returns true iff the symbol is considered unannotated but restrictively annotated
    * {@code @Nullable} under {@code AcknowledgeRestrictiveAnnotations=true} logic.
@@ -61,7 +68,7 @@ public class RestrictiveAnnotationHandler extends BaseNoOpHandler {
    */
   private boolean isSymbolRestrictivelyNullable(Symbol symbol, Context context) {
     CodeAnnotationInfo codeAnnotationInfo = getCodeAnnotationInfo(context);
-    return (codeAnnotationInfo.isSymbolUnannotated(symbol, config, null)
+    return (codeAnnotationInfo.isSymbolUnannotated(symbol, config, mainHandler)
         // with the generated-as-unannotated option enabled, we want to ignore annotations in
         // generated code no matter what
         && !(config.treatGeneratedAsUnannotated() && codeAnnotationInfo.isGenerated(symbol, config))

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

@@ -264,4 +264,29 @@ public void libraryModelsAndOverridingFieldNullability() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void issue1194() {
+    makeLibraryModelsTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:JSpecifyMode=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.ProviderNullMarkedViaModel;",
+            "import org.jspecify.annotations.Nullable;",
+            "public class Test {",
+            "  void use(Object o) {}",
+            "  void f(Object o) {",
+            "    use(o);",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter",
+            "    use(provider.get());",
+            "  }",
+            "  ProviderNullMarkedViaModel<@Nullable Object> provider = () -> null;",
+            "}")
+        .doTest();
+  }
 }

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

@@ -0,0 +1,5 @@
+package com.uber.lib.unannotated;
+
+public interface ProviderNullMarkedViaModel<T> {
+  T get();
+}

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

@@ -132,4 +132,14 @@ public ImmutableSet<FieldRef> nullableFields() {
             fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated2"))
         .build();
   }
+
+  @Override
+  public ImmutableSetMultimap<String, Integer> typeVariablesWithNullableUpperBounds() {
+    return ImmutableSetMultimap.of("com.uber.lib.unannotated.ProviderNullMarkedViaModel", 0);
+  }
+
+  @Override
+  public ImmutableSet<String> nullMarkedClasses() {
+    return ImmutableSet.of("com.uber.lib.unannotated.ProviderNullMarkedViaModel");
+  }
 }