From cc42b47ddef99c6941d0bf873eeea11a936f18f3 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:43:13 -0800 Subject: [PATCH 01/15] add fields to library models --- .../java/com/uber/nullaway/LibraryModels.java | 61 +++++++++++++++++++ .../handlers/LibraryModelsHandler.java | 61 +++++++++++++++++++ .../NullAwayCustomLibraryModelsTests.java | 30 +++++++++ .../modelexample/ExampleLibraryModels.java | 5 ++ .../unannotated/UnannotatedWithModels.java | 4 ++ .../testlibrarymodels/TestLibraryModels.java | 10 +++ 6 files changed, 171 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index a0816b3712..a106b558b9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -131,6 +131,13 @@ public interface LibraryModels { */ ImmutableSetMultimap castToNonNullMethods(); + /** + * Get the set of library fields that may be null. + * + * @return set of library fields that may be null + */ + ImmutableSet nullableFields(); + /** * Get a list of custom stream library specifications. * @@ -244,4 +251,58 @@ public String toString() { + '}'; } } + + /** Representation of a field as a qualified class name + a field name */ + final class FieldRef { + + public final String enclosingClass; + + public final String fieldName; + + private FieldRef(String enclosingClass, String fieldName) { + this.enclosingClass = enclosingClass; + this.fieldName = fieldName; + } + + /** + * Construct a field reference. + * + * @param enclosingClass containing flat name class + * @param fieldName field name + * @return corresponding {@link FieldRef} + */ + public static FieldRef fieldRef(String enclosingClass, String fieldName) { + return new FieldRef(enclosingClass, fieldName); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + FieldRef fieldRef = (FieldRef) o; + return Objects.equals(enclosingClass, fieldRef.enclosingClass) + && Objects.equals(fieldName, fieldRef.fieldName); + } + + @Override + public int hashCode() { + return Objects.hash(enclosingClass, fieldName); + } + + @Override + public String toString() { + return "FieldRef{" + + "enclosingClass='" + + enclosingClass + + '\'' + + ", field name='" + + fieldName + + '\'' + + '}'; + } + } } 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..3b136b04ce 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -57,6 +57,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 +81,20 @@ public LibraryModelsHandler(Config config) { libraryModels = loadLibraryModels(config); } + @Override + public NullnessHint onDataflowVisitFieldAccess( + FieldAccessNode node, + Symbol symbol, + Types types, + Context context, + AccessPath.AccessPathContext apContext, + AccessPathNullnessPropagation.SubNodeValues inputs, + AccessPathNullnessPropagation.Updates updates) { + return isNullableFieldInLibraryModel(symbol) + ? NullnessHint.HINT_NULLABLE + : NullnessHint.UNKNOWN; + } + @Override public Nullness[] onOverrideMethodInvocationParametersNullability( VisitorState state, @@ -132,6 +147,9 @@ public boolean onOverrideMayBeNullExpr( @Nullable Symbol exprSymbol, VisitorState state, boolean exprMayBeNull) { + if (isNullableFieldInLibraryModel(exprSymbol)) { + return true; + } if (!(expr.getKind() == Tree.Kind.METHOD_INVOCATION && exprSymbol instanceof Symbol.MethodSymbol)) { return exprMayBeNull; @@ -233,6 +251,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 isNullableFieldInLibraryModel(@Nullable Symbol symbol) { + 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 enclosingClass = classSymbol.flatName().toString(); + // check presence + return libraryModels.nullableFields().stream() + .anyMatch( + fieldRef -> + fieldRef.fieldName.equals(fieldName) + && fieldRef.enclosingClass.equals(enclosingClass)); + } + return false; + } + private void setConditionalArgumentNullness( AccessPathNullnessPropagation.Updates thenUpdates, AccessPathNullnessPropagation.Updates elseUpdates, @@ -824,6 +868,11 @@ public ImmutableSet nonNullReturns() { public ImmutableSetMultimap castToNonNullMethods() { return CAST_TO_NONNULL_METHODS; } + + @Override + public ImmutableSet nullableFields() { + return ImmutableSet.of(); + } } private static class CombinedLibraryModels implements LibraryModels { @@ -846,6 +895,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 +921,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 +984,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 +998,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 +1045,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..97a6db6d8e 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -227,4 +227,34 @@ 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;", + " }", + " }", + "}") + .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(); + } } From db44cdcf4b501fdde4017727c580fd4736d81ceb Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:45:45 -0800 Subject: [PATCH 02/15] model to modles --- .../com/uber/nullaway/handlers/LibraryModelsHandler.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 3b136b04ce..60fa559e78 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -90,7 +90,7 @@ public NullnessHint onDataflowVisitFieldAccess( AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates updates) { - return isNullableFieldInLibraryModel(symbol) + return isNullableFieldInLibraryModels(symbol) ? NullnessHint.HINT_NULLABLE : NullnessHint.UNKNOWN; } @@ -147,7 +147,7 @@ public boolean onOverrideMayBeNullExpr( @Nullable Symbol exprSymbol, VisitorState state, boolean exprMayBeNull) { - if (isNullableFieldInLibraryModel(exprSymbol)) { + if (isNullableFieldInLibraryModels(exprSymbol)) { return true; } if (!(expr.getKind() == Tree.Kind.METHOD_INVOCATION @@ -257,7 +257,7 @@ public NullnessHint onDataflowVisitMethodInvocation( * @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 isNullableFieldInLibraryModel(@Nullable Symbol symbol) { + private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { if (symbol instanceof Symbol.VarSymbol && symbol.getKind().isField()) { Symbol.VarSymbol varSymbol = (Symbol.VarSymbol) symbol; Symbol.ClassSymbol classSymbol = varSymbol.enclClass(); From d982af85d399551b121c60152855a3f88cb85db2 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:47:17 -0800 Subject: [PATCH 03/15] add name to class --- .../java/com/uber/nullaway/handlers/LibraryModelsHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 60fa559e78..138b5cd26c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -266,13 +266,13 @@ private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { return false; } String fieldName = varSymbol.getSimpleName().toString(); - String enclosingClass = classSymbol.flatName().toString(); + String enclosingClassName = classSymbol.flatName().toString(); // check presence return libraryModels.nullableFields().stream() .anyMatch( fieldRef -> fieldRef.fieldName.equals(fieldName) - && fieldRef.enclosingClass.equals(enclosingClass)); + && fieldRef.enclosingClass.equals(enclosingClassName)); } return false; } From 5b721ace4b08ddf777c6d243011184edfee3b1bd Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:52:07 -0800 Subject: [PATCH 04/15] update javadoc --- nullaway/src/main/java/com/uber/nullaway/LibraryModels.java | 3 ++- .../java/com/uber/nullaway/handlers/LibraryModelsHandler.java | 1 + 2 files changed, 3 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 a106b558b9..56b21804b2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -132,7 +132,8 @@ public interface LibraryModels { ImmutableSetMultimap castToNonNullMethods(); /** - * Get the set of library fields that may be null. + * Get the set of library fields that may be null. This is mostly used to index the impact of + * making fields nullable on downstream dependencies. * * @return set of library fields that may be null */ 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 138b5cd26c..c84b2e275c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -871,6 +871,7 @@ public ImmutableSetMultimap castToNonNullMethods() { @Override public ImmutableSet nullableFields() { + // No nullable field by default. return ImmutableSet.of(); } } From 90b3155576529ddde0525dda7595704b97eefb42 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:53:25 -0800 Subject: [PATCH 05/15] update javadoc --- nullaway/src/main/java/com/uber/nullaway/LibraryModels.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 56b21804b2..3ae8f07de9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -133,9 +133,9 @@ public interface LibraryModels { /** * Get the set of library fields that may be null. This is mostly used to index the impact of - * making fields nullable on downstream dependencies. + * making fields @Nullable on downstream dependencies. * - * @return set of library fields that may be null + * @return set of library fields that may be null. */ ImmutableSet nullableFields(); From a31b800ddf1fc9e7e91a8de219416ffe9e92f88f Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Mon, 11 Dec 2023 22:56:43 -0800 Subject: [PATCH 06/15] add comment --- .../java/com/uber/nullaway/handlers/LibraryModelsHandler.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 c84b2e275c..7da8ad2dfc 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -267,7 +267,9 @@ private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { } String fieldName = varSymbol.getSimpleName().toString(); String enclosingClassName = classSymbol.flatName().toString(); - // check presence + // check presence. This is a bit inefficient, but it's only used while indexing impact of + // making fields @Nullable on downstream dependencies. Also, the set of nullable fields is not + // expected to be large. We can optimize in future if needed. return libraryModels.nullableFields().stream() .anyMatch( fieldRef -> From 95c14ba5fba2d0912a1853c4cfc81c87f97be55b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 13 Dec 2023 08:42:42 -0800 Subject: [PATCH 07/15] tweak documentation --- nullaway/src/main/java/com/uber/nullaway/LibraryModels.java | 5 ++--- .../com/uber/nullaway/handlers/LibraryModelsHandler.java | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 3ae8f07de9..1a94107fd6 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -132,10 +132,9 @@ public interface LibraryModels { ImmutableSetMultimap castToNonNullMethods(); /** - * Get the set of library fields that may be null. This is mostly used to index the impact of - * making fields @Nullable on downstream dependencies. + * Get the set of library fields that may be {@code null}. * - * @return set of library fields that may be null. + * @return set of library fields that may be {@code null}. */ ImmutableSet nullableFields(); 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 7da8ad2dfc..f70f7a8edf 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -873,7 +873,7 @@ public ImmutableSetMultimap castToNonNullMethods() { @Override public ImmutableSet nullableFields() { - // No nullable field by default. + // No nullable fields by default. return ImmutableSet.of(); } } From f15b983c1b68079923bc25477ddf7c1df515d237 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Wed, 13 Dec 2023 11:52:38 -0800 Subject: [PATCH 08/15] used @AutoValue --- .../java/com/uber/nullaway/LibraryModels.java | 52 +++---------------- .../handlers/LibraryModelsHandler.java | 4 +- 2 files changed, 8 insertions(+), 48 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 3ae8f07de9..fdfe6b2021 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; @@ -254,56 +255,15 @@ public String toString() { } /** Representation of a field as a qualified class name + a field name */ - final class FieldRef { + @AutoValue + abstract class FieldRef { - public final String enclosingClass; - - public final String fieldName; + public abstract String getEnclosingClassName(); - private FieldRef(String enclosingClass, String fieldName) { - this.enclosingClass = enclosingClass; - this.fieldName = fieldName; - } + public abstract String getFieldName(); - /** - * Construct a field reference. - * - * @param enclosingClass containing flat name class - * @param fieldName field name - * @return corresponding {@link FieldRef} - */ public static FieldRef fieldRef(String enclosingClass, String fieldName) { - return new FieldRef(enclosingClass, fieldName); - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - FieldRef fieldRef = (FieldRef) o; - return Objects.equals(enclosingClass, fieldRef.enclosingClass) - && Objects.equals(fieldName, fieldRef.fieldName); - } - - @Override - public int hashCode() { - return Objects.hash(enclosingClass, fieldName); - } - - @Override - public String toString() { - return "FieldRef{" - + "enclosingClass='" - + enclosingClass - + '\'' - + ", field name='" - + fieldName - + '\'' - + '}'; + return new AutoValue_LibraryModels_FieldRef(enclosingClass, fieldName); } } } 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 7da8ad2dfc..5f30599bc4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -273,8 +273,8 @@ private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { return libraryModels.nullableFields().stream() .anyMatch( fieldRef -> - fieldRef.fieldName.equals(fieldName) - && fieldRef.enclosingClass.equals(enclosingClassName)); + fieldRef.getFieldName().equals(fieldName) + && fieldRef.getEnclosingClassName().equals(enclosingClassName)); } return false; } From 819f75e48fa87ef50eae02ec008fc19fbdfbcadf Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Wed, 13 Dec 2023 11:58:43 -0800 Subject: [PATCH 09/15] add empty check --- .../java/com/uber/nullaway/handlers/LibraryModelsHandler.java | 4 ++++ 1 file changed, 4 insertions(+) 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 b6329308b8..c75478b35c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -258,6 +258,10 @@ public NullnessHint onDataflowVisitMethodInvocation( * @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(); From 2ffb5d614c70cc572c5a17e2e8ee4cb44d1aa5e9 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Wed, 13 Dec 2023 12:01:45 -0800 Subject: [PATCH 10/15] minor formatting on test src --- .../com/uber/nullaway/NullAwayCustomLibraryModelsTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 97a6db6d8e..4582a65d1b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -249,9 +249,9 @@ public void libraryModelsAndOverridingFieldNullability() { " 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;", + " if(uwm.nullableFieldUnannotated1 != null) {", + " // no error here, to check that library models only initialize flow store", + " this.nonnullField = uwm.nullableFieldUnannotated1;", " }", " }", "}") From 0f73d506c657bea941a9a22147e3049af1ee966b Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Wed, 13 Dec 2023 12:10:09 -0800 Subject: [PATCH 11/15] add a dereference test --- .../com/uber/nullaway/NullAwayCustomLibraryModelsTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 4582a65d1b..3563334bfc 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -254,6 +254,10 @@ public void libraryModelsAndOverridingFieldNullability() { " this.nonnullField = uwm.nullableFieldUnannotated1;", " }", " }", + " String dereferenceTest() {", + " // BUG: Diagnostic contains: dereferenced expression uwm.nullableFieldUnannotated1 is @Nullable", + " return uwm.nullableFieldUnannotated1.toString();", + " }", "}") .doTest(); } From 8347141ce8ddc3323b3cc5231759d881031b5c12 Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Thu, 14 Dec 2023 11:13:08 -0800 Subject: [PATCH 12/15] support assignment for fields in librarymodels --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 5 ++++- .../java/com/uber/nullaway/handlers/BaseNoOpHandler.java | 6 ++++++ .../com/uber/nullaway/handlers/CompositeHandler.java | 8 ++++++++ .../main/java/com/uber/nullaway/handlers/Handler.java | 9 +++++++++ .../com/uber/nullaway/handlers/LibraryModelsHandler.java | 8 ++++++++ .../uber/nullaway/NullAwayCustomLibraryModelsTests.java | 5 ++++- 6 files changed, 39 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f7ee780e5b..f630dda466 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -486,7 +486,10 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { return Description.NO_MATCH; } - if (Nullness.hasNullableAnnotation(assigned, config)) { + if (Nullness.hasNullableAnnotation(assigned, config) + || handler + .onOverrideFieldNullability(assigned, Nullness.NONNULL) + .equals(Nullness.NULLABLE)) { // 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..e19aefa11e 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 Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { + // NoOp + return fieldNullness; + } + @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..edecf365f3 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,14 @@ public Nullness onOverrideMethodReturnNullability( return returnNullness; } + @Override + public Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { + for (Handler h : handlers) { + fieldNullness = h.onOverrideFieldNullability(field, fieldNullness); + } + return fieldNullness; + } + @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..cb2c38e538 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,15 @@ Nullness onOverrideMethodReturnNullability( boolean isAnnotated, Nullness returnNullness); + /** + * Called to potentially override the nullability of a field which is not annotated as @Nullable. + * + * @param field The symbol for the field in question. + * @param fieldNullness field Nullness computed by upstream handlers or NullAway core. + * @return Updated field nullability computed by this handler. + */ + Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness); + /** * 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 c75478b35c..4ea0fe1b43 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -81,6 +81,14 @@ public LibraryModelsHandler(Config config) { libraryModels = loadLibraryModels(config); } + @Override + public Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { + if (fieldNullness.equals(NONNULL) && isNullableFieldInLibraryModels(field)) { + return NULLABLE; + } + return fieldNullness; + } + @Override public NullnessHint onDataflowVisitFieldAccess( FieldAccessNode node, diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 3563334bfc..098b98b6b3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -250,7 +250,7 @@ public void libraryModelsAndOverridingFieldNullability() { " }", " void flowTest() {", " if(uwm.nullableFieldUnannotated1 != null) {", - " // no error here, to check that library models only initialize flow store", + " // no error here, to check that library models only initialize flow store", " this.nonnullField = uwm.nullableFieldUnannotated1;", " }", " }", @@ -258,6 +258,9 @@ public void libraryModelsAndOverridingFieldNullability() { " // BUG: Diagnostic contains: dereferenced expression uwm.nullableFieldUnannotated1 is @Nullable", " return uwm.nullableFieldUnannotated1.toString();", " }", + " void assignmentTest() {", + " uwm.nullableFieldUnannotated1 = null;", + " }", "}") .doTest(); } From 4d8187131ec57e117903d302a8d7d096b21b250b Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Thu, 14 Dec 2023 13:47:54 -0800 Subject: [PATCH 13/15] update onOverrideFieldNullability signature --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 4 +--- .../com/uber/nullaway/handlers/BaseNoOpHandler.java | 4 ++-- .../com/uber/nullaway/handlers/CompositeHandler.java | 10 +++++++--- .../main/java/com/uber/nullaway/handlers/Handler.java | 7 ++++--- .../uber/nullaway/handlers/LibraryModelsHandler.java | 7 ++----- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f630dda466..01a43ae229 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -487,9 +487,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { } if (Nullness.hasNullableAnnotation(assigned, config) - || handler - .onOverrideFieldNullability(assigned, Nullness.NONNULL) - .equals(Nullness.NULLABLE)) { + || 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 e19aefa11e..9260711d74 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -119,9 +119,9 @@ public Nullness onOverrideMethodReturnNullability( } @Override - public Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { + public boolean onOverrideFieldNullability(Symbol field) { // NoOp - return fieldNullness; + return false; } @Override 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 edecf365f3..f857d06fef 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -136,11 +136,15 @@ public Nullness onOverrideMethodReturnNullability( } @Override - public Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { + public boolean onOverrideFieldNullability(Symbol field) { for (Handler h : handlers) { - fieldNullness = h.onOverrideFieldNullability(field, fieldNullness); + 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 fieldNullness; + return false; } @Override 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 cb2c38e538..7033e40ea5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -175,12 +175,13 @@ Nullness onOverrideMethodReturnNullability( /** * 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. - * @param fieldNullness field Nullness computed by upstream handlers or NullAway core. - * @return Updated field nullability computed by this handler. + * @return true if the field should be treated as @Nullable, false otherwise. */ - Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness); + boolean onOverrideFieldNullability(Symbol field); /** * Called after the analysis determines the nullability of a method's arguments, allowing handlers 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 4ea0fe1b43..d151c4a1f7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -82,11 +82,8 @@ public LibraryModelsHandler(Config config) { } @Override - public Nullness onOverrideFieldNullability(Symbol field, Nullness fieldNullness) { - if (fieldNullness.equals(NONNULL) && isNullableFieldInLibraryModels(field)) { - return NULLABLE; - } - return fieldNullness; + public boolean onOverrideFieldNullability(Symbol field) { + return isNullableFieldInLibraryModels(field); } @Override From 1728198fabe668b5f93929efbf1b5d517d6f3a3b Mon Sep 17 00:00:00 2001 From: nimakarimipour Date: Thu, 14 Dec 2023 19:43:10 -0800 Subject: [PATCH 14/15] refactor --- .../com/uber/nullaway/handlers/LibraryModelsHandler.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 d151c4a1f7..5fb5ee2d35 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; @@ -279,11 +280,7 @@ private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { // check presence. This is a bit inefficient, but it's only used while indexing impact of // making fields @Nullable on downstream dependencies. Also, the set of nullable fields is not // expected to be large. We can optimize in future if needed. - return libraryModels.nullableFields().stream() - .anyMatch( - fieldRef -> - fieldRef.getFieldName().equals(fieldName) - && fieldRef.getEnclosingClassName().equals(enclosingClassName)); + return libraryModels.nullableFields().contains(fieldRef(enclosingClassName, fieldName)); } return false; } From b16a3a5af9b28cd89f3db63e26dcef8eede73c76 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 14 Dec 2023 19:52:34 -0800 Subject: [PATCH 15/15] tweak comment --- .../java/com/uber/nullaway/handlers/LibraryModelsHandler.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 5fb5ee2d35..09f1c41ffb 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -277,9 +277,7 @@ private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { } String fieldName = varSymbol.getSimpleName().toString(); String enclosingClassName = classSymbol.flatName().toString(); - // check presence. This is a bit inefficient, but it's only used while indexing impact of - // making fields @Nullable on downstream dependencies. Also, the set of nullable fields is not - // expected to be large. We can optimize in future if needed. + // This check could be optimized further in the future if needed return libraryModels.nullableFields().contains(fieldRef(enclosingClassName, fieldName)); } return false;