From 3ee7072af654fbcd1ec9a76509db41635bc521ed Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Thu, 14 Dec 2023 20:14:38 -0800 Subject: [PATCH] Extend library models to mark fields as nullable (#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 --- .../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, 175 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index a0816b3712..1eeb219f82 100644 --- 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 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. * @@ -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 index f7ee780e5b..01a43ae229 100644 --- 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 index 4fc0c0c8f2..9260711d74 100644 --- 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 index 3602a5ff03..f857d06fef 100644 --- 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 index d509d7bdda..7033e40ea5 100644 --- 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 index e5e943c553..09f1c41ffb 100644 --- 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 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 { @@ -846,6 +902,8 @@ private static class CombinedLibraryModels implements LibraryModels { private final ImmutableSet nonNullReturns; + private final ImmutableSet nullableFields; + private final ImmutableSetMultimap castToNonNullMethods; private final ImmutableList customStreamNullabilitySpecs; @@ -870,6 +928,7 @@ 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())) { @@ -932,6 +991,9 @@ 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(); @@ -943,6 +1005,7 @@ public CombinedLibraryModels(Iterable 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 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 937752c0bc..098b98b6b3 100644 --- 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 index a1aa99d79d..5c17aca966 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,4 +71,9 @@ 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 412042dbbb..a9783336d2 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,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 index c94dfef501..01304a9ec3 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,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 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(); + } }