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(); + } }