From a4ccd1db9875e324df82c941306f0951ed013748 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Sat, 16 Dec 2023 12:34:22 -0800 Subject: [PATCH] Revert "Extend library models to mark fields as nullable (#878)" This reverts commit 3ee7072af654fbcd1ec9a76509db41635bc521ed. --- .../java/com/uber/nullaway/LibraryModels.java | 21 ------ .../main/java/com/uber/nullaway/NullAway.java | 3 +- .../nullaway/handlers/BaseNoOpHandler.java | 6 -- .../nullaway/handlers/CompositeHandler.java | 12 ---- .../com/uber/nullaway/handlers/Handler.java | 10 --- .../handlers/LibraryModelsHandler.java | 68 ------------------- .../NullAwayCustomLibraryModelsTests.java | 37 ---------- .../modelexample/ExampleLibraryModels.java | 5 -- .../unannotated/UnannotatedWithModels.java | 4 -- .../testlibrarymodels/TestLibraryModels.java | 10 --- 10 files changed, 1 insertion(+), 175 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 1eeb219f82..a0816b3712 100644 --- 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 castToNonNullMethods(); - /** - * Get the set of library fields that may be {@code null}. - * - * @return set of library fields that may be {@code null}. - */ - ImmutableSet 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 index 01a43ae229..f7ee780e5b 100644 --- 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 index 9260711d74..4fc0c0c8f2 100644 --- 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 index f857d06fef..3602a5ff03 100644 --- 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 index 7033e40ea5..d509d7bdda 100644 --- 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 index 09f1c41ffb..e5e943c553 100644 --- 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 nonNullReturns() { public ImmutableSetMultimap castToNonNullMethods() { return CAST_TO_NONNULL_METHODS; } - - @Override - public ImmutableSet 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 nonNullReturns; - private final ImmutableSet nullableFields; - private final ImmutableSetMultimap castToNonNullMethods; private final ImmutableList customStreamNullabilitySpecs; @@ -928,7 +870,6 @@ public CombinedLibraryModels(Iterable models, Config config) { new ImmutableSetMultimap.Builder<>(); ImmutableList.Builder customStreamNullabilitySpecsBuilder = new ImmutableList.Builder<>(); - ImmutableSet.Builder nullableFieldsBuilder = new ImmutableSet.Builder<>(); for (LibraryModels libraryModels : models) { for (Map.Entry entry : libraryModels.failIfNullParameters().entries()) { if (shouldSkipModel(entry.getKey())) { @@ -991,9 +932,6 @@ public CombinedLibraryModels(Iterable 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 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 nonNullReturns() { return nonNullReturns; } - @Override - public ImmutableSet nullableFields() { - return nullableFields; - } - @Override public ImmutableSetMultimap 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 index 098b98b6b3..937752c0bc 100644 --- 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 index 5c17aca966..a1aa99d79d 100644 --- 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 nonNullReturns() { public ImmutableSetMultimap castToNonNullMethods() { return ImmutableSetMultimap.of(); } - - @Override - public ImmutableSet 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 index a9783336d2..412042dbbb 100644 --- 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 index 01304a9ec3..c94dfef501 100644 --- 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 customStreamNullabilitySpecs() { .withPassthroughMethodFromSignature("distinct()") .end(); } - - @Override - public ImmutableSet nullableFields() { - return ImmutableSet.builder() - .add( - fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated1"), - fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated2")) - .build(); - } }