Extend library models to mark fields as nullable (uber#878)

This PR extends `LibraryModels` to add support for marking fields `@Nullable`. This feature is required to enable [Annotator](https://github.com/ucr-riple/NullAwayAnnotator) index impact of making fields `@Nullable` on downstream dependencies. Please note, since this feature is mostly going to be used while indexing impacts on downstream dependencies, this PR does not focus on the optimality of the implementation. Also the working set in iterations is not expected to be large. We can optimize the implementation in a follow up PR if needed. --------- Co-authored-by: Manu Sridharan <[email protected]>
msridhar · Dec 15, 2023 · 3ee7072 · 3ee7072
1 parent 5fbee1f
commit 3ee7072
Show file tree

Hide file tree

Showing 10 changed files with 175 additions and 1 deletion.
diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/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);
+    }
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -486,7 +486,8 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    if (Nullness.hasNullableAnnotation(assigned, config)) {
+    if (Nullness.hasNullableAnnotation(assigned, config)
+        || handler.onOverrideFieldNullability(assigned)) {
       // field already annotated
       return Description.NO_MATCH;
     }

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java
@@ -118,6 +118,12 @@ public Nullness onOverrideMethodReturnNullability(
     return returnNullness;
   }
 
+  @Override
+  public boolean onOverrideFieldNullability(Symbol field) {
+    // NoOp
+    return false;
+  }
+
   @Override
   public Nullness[] onOverrideMethodInvocationParametersNullability(
       VisitorState state,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
@@ -135,6 +135,18 @@ public Nullness onOverrideMethodReturnNullability(
     return returnNullness;
   }
 
+  @Override
+  public boolean onOverrideFieldNullability(Symbol field) {
+    for (Handler h : handlers) {
+      if (h.onOverrideFieldNullability(field)) {
+        // If any handler determines that the field is @Nullable, we should acknowledge that and
+        // treat it as such.
+        return true;
+      }
+    }
+    return false;
+  }
+
   @Override
   public Nullness[] onOverrideMethodInvocationParametersNullability(
       VisitorState state,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
@@ -173,6 +173,16 @@ Nullness onOverrideMethodReturnNullability(
       boolean isAnnotated,
       Nullness returnNullness);
 
+  /**
+   * Called to potentially override the nullability of a field which is not annotated as @Nullable.
+   * If the field is decided to be @Nullable by this handler, the field should be treated
+   * as @Nullable anyway.
+   *
+   * @param field The symbol for the field in question.
+   * @return true if the field should be treated as @Nullable, false otherwise.
+   */
+  boolean onOverrideFieldNullability(Symbol field);
+
   /**
    * Called after the analysis determines the nullability of a method's arguments, allowing handlers
    * to override.

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
@@ -22,6 +22,7 @@
 
 package com.uber.nullaway.handlers;
 
+import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef;
 import static com.uber.nullaway.LibraryModels.MethodRef.methodRef;
 import static com.uber.nullaway.Nullness.NONNULL;
 import static com.uber.nullaway.Nullness.NULLABLE;
@@ -57,6 +58,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 +82,25 @@ public LibraryModelsHandler(Config config) {
     libraryModels = loadLibraryModels(config);
   }
 
+  @Override
+  public boolean onOverrideFieldNullability(Symbol field) {
+    return isNullableFieldInLibraryModels(field);
+  }
+
+  @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 +153,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 +257,32 @@ 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();
+      // This check could be optimized further in the future if needed
+      return libraryModels.nullableFields().contains(fieldRef(enclosingClassName, fieldName));
+    }
+    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;

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
@@ -227,4 +227,41 @@ 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();",
+            "   }",
+            "   void assignmentTest() {",
+            "      uwm.nullableFieldUnannotated1 = null;",
+            "   }",
+            "}")
+        .doTest();
+  }
 }
diff --git a/sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java b/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();
+  }
 }
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
@@ -2,6 +2,10 @@
 
 public class UnannotatedWithModels {
 
+  public Object nullableFieldUnannotated1;
+
+  public Object nullableFieldUnannotated2;
+
   public Object returnsNullUnannotated() {
     return 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
@@ -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();
+  }
 }