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

This reverts commit 3ee7072.
msridhar · Dec 16, 2023 · a4ccd1d · a4ccd1d
1 parent 3ee7072
commit a4ccd1d
Show file tree

Hide file tree

Showing 10 changed files with 1 addition and 175 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java
@@ -22,7 +22,6 @@
 
 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;
@@ -132,13 +131,6 @@ 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.
    *
@@ -252,17 +244,4 @@ 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,8 +486,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    if (Nullness.hasNullableAnnotation(assigned, config)
-        || handler.onOverrideFieldNullability(assigned)) {
+    if (Nullness.hasNullableAnnotation(assigned, config)) {
       // 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,12 +118,6 @@ 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,18 +135,6 @@ 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,16 +173,6 @@ 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,7 +22,6 @@
 
 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;
@@ -58,7 +57,6 @@
 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;
 
@@ -82,25 +80,6 @@ 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,
@@ -153,9 +132,6 @@ 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;
@@ -257,32 +233,6 @@ 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,
@@ -874,12 +824,6 @@ 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 {
@@ -902,8 +846,6 @@ 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;
@@ -928,7 +870,6 @@ 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())) {
@@ -991,9 +932,6 @@ 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();
@@ -1005,7 +943,6 @@ public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
       nonNullReturns = nonNullReturnsBuilder.build();
       castToNonNullMethods = castToNonNullMethodsBuilder.build();
       customStreamNullabilitySpecs = customStreamNullabilitySpecsBuilder.build();
-      nullableFields = nullableFieldsBuilder.build();
     }
 
     private boolean shouldSkipModel(MethodRef key) {
@@ -1052,11 +989,6 @@ 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,41 +227,4 @@ 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,9 +71,4 @@ 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,10 +2,6 @@
 
 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,7 +15,6 @@
  */
 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;
@@ -123,13 +122,4 @@ 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();
-  }
 }