Extend library models to mark fields as nullable

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

@@ -22,6 +22,7 @@
 
 package com.uber.nullaway;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
@@ -131,6 +132,13 @@ public interface LibraryModels {
    */
   ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods();
 
+  /**
+   * Get the set of library fields that may be {@code null}.
+   *
+   * @return set of library fields that may be {@code null}.
+   */
+  ImmutableSet<FieldRef> nullableFields();
+
   /**
    * Get a list of custom stream library specifications.
    *
@@ -244,4 +252,17 @@ public String toString() {
           + '}';
     }
   }
+
+  /** Representation of a field as a qualified class name + a field name */
+  @AutoValue
+  abstract class FieldRef {
+
+    public abstract String getEnclosingClassName();
+
+    public abstract String getFieldName();
+
+    public static FieldRef fieldRef(String enclosingClass, String fieldName) {
+      return new AutoValue_LibraryModels_FieldRef(enclosingClass, fieldName);
+    }
+  }
 }

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

@@ -57,6 +57,7 @@
 import java.util.Set;
 import java.util.function.Function;
 import javax.annotation.Nullable;
+import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode;
 import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode;
 import org.checkerframework.nullaway.dataflow.cfg.node.Node;
 
@@ -80,6 +81,20 @@ public LibraryModelsHandler(Config config) {
     libraryModels = loadLibraryModels(config);
   }
 
+  @Override
+  public NullnessHint onDataflowVisitFieldAccess(
+      FieldAccessNode node,
+      Symbol symbol,
+      Types types,
+      Context context,
+      AccessPath.AccessPathContext apContext,
+      AccessPathNullnessPropagation.SubNodeValues inputs,
+      AccessPathNullnessPropagation.Updates updates) {
+    return isNullableFieldInLibraryModels(symbol)
+        ? NullnessHint.HINT_NULLABLE
+        : NullnessHint.UNKNOWN;
+  }
+
   @Override
   public Nullness[] onOverrideMethodInvocationParametersNullability(
       VisitorState state,
@@ -132,6 +147,9 @@ public boolean onOverrideMayBeNullExpr(
       @Nullable Symbol exprSymbol,
       VisitorState state,
       boolean exprMayBeNull) {
+    if (isNullableFieldInLibraryModels(exprSymbol)) {
+      return true;
+    }
     if (!(expr.getKind() == Tree.Kind.METHOD_INVOCATION
         && exprSymbol instanceof Symbol.MethodSymbol)) {
       return exprMayBeNull;
@@ -233,6 +251,38 @@ public NullnessHint onDataflowVisitMethodInvocation(
     }
   }
 
+  /**
+   * Check if the given symbol is a field that is marked as nullable in any of our library models.
+   *
+   * @param symbol The symbol to check.
+   * @return True if the symbol is a field that is marked as nullable in any of our library models.
+   */
+  private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) {
+    if (libraryModels.nullableFields().isEmpty()) {
+      // no need to do any work if there are no nullable fields.
+      return false;
+    }
+    if (symbol instanceof Symbol.VarSymbol && symbol.getKind().isField()) {
+      Symbol.VarSymbol varSymbol = (Symbol.VarSymbol) symbol;
+      Symbol.ClassSymbol classSymbol = varSymbol.enclClass();
+      if (classSymbol == null) {
+        // e.g. .class expressions
+        return false;
+      }
+      String fieldName = varSymbol.getSimpleName().toString();
+      String enclosingClassName = classSymbol.flatName().toString();
+      // check presence. This is a bit inefficient, but it's only used while indexing impact of
+      // making fields @Nullable on downstream dependencies. Also, the set of nullable fields is not
+      // expected to be large. We can optimize in future if needed.
+      return libraryModels.nullableFields().stream()
+          .anyMatch(
+              fieldRef ->
+                  fieldRef.getFieldName().equals(fieldName)
+                      && fieldRef.getEnclosingClassName().equals(enclosingClassName));
+    }
+    return false;
+  }
+
   private void setConditionalArgumentNullness(
       AccessPathNullnessPropagation.Updates thenUpdates,
       AccessPathNullnessPropagation.Updates elseUpdates,
@@ -824,6 +874,12 @@ public ImmutableSet<MethodRef> nonNullReturns() {
     public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {
       return CAST_TO_NONNULL_METHODS;
     }
+
+    @Override
+    public ImmutableSet<FieldRef> nullableFields() {
+      // No nullable fields by default.
+      return ImmutableSet.of();
+    }
   }
 
   private static class CombinedLibraryModels implements LibraryModels {
@@ -846,6 +902,8 @@ private static class CombinedLibraryModels implements LibraryModels {
 
     private final ImmutableSet<MethodRef> nonNullReturns;
 
+    private final ImmutableSet<FieldRef> nullableFields;
+
     private final ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods;
 
     private final ImmutableList<StreamTypeRecord> customStreamNullabilitySpecs;
@@ -870,6 +928,7 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
           new ImmutableSetMultimap.Builder<>();
       ImmutableList.Builder<StreamTypeRecord> customStreamNullabilitySpecsBuilder =
           new ImmutableList.Builder<>();
+      ImmutableSet.Builder<FieldRef> nullableFieldsBuilder = new ImmutableSet.Builder<>();
       for (LibraryModels libraryModels : models) {
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.failIfNullParameters().entries()) {
           if (shouldSkipModel(entry.getKey())) {
@@ -932,6 +991,9 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
         for (StreamTypeRecord streamTypeRecord : libraryModels.customStreamNullabilitySpecs()) {
           customStreamNullabilitySpecsBuilder.add(streamTypeRecord);
         }
+        for (FieldRef fieldRef : libraryModels.nullableFields()) {
+          nullableFieldsBuilder.add(fieldRef);
+        }
       }
       failIfNullParameters = failIfNullParametersBuilder.build();
       explicitlyNullableParameters = explicitlyNullableParametersBuilder.build();
@@ -943,6 +1005,7 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
       nonNullReturns = nonNullReturnsBuilder.build();
       castToNonNullMethods = castToNonNullMethodsBuilder.build();
       customStreamNullabilitySpecs = customStreamNullabilitySpecsBuilder.build();
+      nullableFields = nullableFieldsBuilder.build();
     }
 
     private boolean shouldSkipModel(MethodRef key) {
@@ -989,6 +1052,11 @@ public ImmutableSet<MethodRef> nonNullReturns() {
       return nonNullReturns;
     }
 
+    @Override
+    public ImmutableSet<FieldRef> nullableFields() {
+      return nullableFields;
+    }
+
     @Override
     public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {
       return castToNonNullMethods;

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

@@ -227,4 +227,38 @@ public void libraryModelsAndSelectiveSkippingViaCommandLineOptions2() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void libraryModelsAndOverridingFieldNullability() {
+    makeLibraryModelsTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.UnannotatedWithModels;",
+            "public class Test {",
+            "   UnannotatedWithModels uwm = new UnannotatedWithModels();",
+            "   Object nonnullField = new Object();",
+            "   void assignNullableFromLibraryModelField() {",
+            "      // BUG: Diagnostic contains: assigning @Nullable",
+            "      this.nonnullField = uwm.nullableFieldUnannotated1;",
+            "      // BUG: Diagnostic contains: assigning @Nullable",
+            "      this.nonnullField = uwm.nullableFieldUnannotated2;",
+            "   }",
+            "   void flowTest() {",
+            "      if(uwm.nullableFieldUnannotated1 != null) {",
+            "         // no error here, to check that library models only initialize flow store",
+            "         this.nonnullField = uwm.nullableFieldUnannotated1;",
+            "      }",
+            "   }",
+            "   String dereferenceTest() {",
+            "      // BUG: Diagnostic contains: dereferenced expression uwm.nullableFieldUnannotated1 is @Nullable",
+            "      return uwm.nullableFieldUnannotated1.toString();",
+            "   }",
+            "}")
+        .doTest();
+  }
 }

sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java

@@ -71,4 +71,9 @@ public ImmutableSet<MethodRef> nonNullReturns() {
   public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {
     return ImmutableSetMultimap.of();
   }
+
+  @Override
+  public ImmutableSet<FieldRef> nullableFields() {
+    return ImmutableSet.of();
+  }
 }

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

@@ -2,6 +2,10 @@
 
 public class UnannotatedWithModels {
 
+  public Object nullableFieldUnannotated1;
+
+  public Object nullableFieldUnannotated2;
+
   public Object returnsNullUnannotated() {
     return null;
   }

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

@@ -15,6 +15,7 @@
  */
 package com.uber.nullaway.testlibrarymodels;
 
+import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef;
 import static com.uber.nullaway.LibraryModels.MethodRef.methodRef;
 
 import com.google.auto.service.AutoService;
@@ -122,4 +123,13 @@ public ImmutableList<StreamTypeRecord> customStreamNullabilitySpecs() {
         .withPassthroughMethodFromSignature("distinct()")
         .end();
   }
+
+  @Override
+  public ImmutableSet<FieldRef> nullableFields() {
+    return ImmutableSet.<FieldRef>builder()
+        .add(
+            fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated1"),
+            fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated2"))
+        .build();
+  }
 }