From 1ea7f067de1e3d0a6fbc4951e3010e4bcc0e5f94 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 4 Aug 2023 15:08:45 -0700 Subject: [PATCH 1/7] WIP --- .../java/com/uber/nullaway/ErrorBuilder.java | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 629e477e6c..67a0026432 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -198,7 +198,7 @@ private Description.Builder addSuggestedSuppression( case PASS_NULLABLE: case ASSIGN_FIELD_NULLABLE: case SWITCH_EXPRESSION_NULLABLE: - if (config.getCastToNonNullMethod() != null) { + if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree, state)) { builder = addCastToNonNullFix(suggestTree, builder); } else { builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); @@ -320,6 +320,25 @@ private Tree suppressibleNode(@Nullable TreePath path) { .orElse(null); } + /** + * Checks if it would be appropriate to wrap {@code tree} in a {@code castToNonNull} call. There + * are two cases where this method returns {@code false}: + * + *
    + *
  1. {@code tree} represents the {@code null} literal + *
  2. {@code tree} represents a {@code @Nullable} formal parameter of the enclosing method + *
+ */ + private boolean canBeCastToNonNull(Tree tree, VisitorState state) { + switch (tree.getKind()) { + case NULL_LITERAL: + return false; + case IDENTIFIER: + default: + return true; + } + } + private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { final String fullMethodName = config.getCastToNonNullMethod(); if (fullMethodName == null) { From 95df551526b468787b3db8e441edbe7a7d3a363c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 4 Aug 2023 16:43:39 -0700 Subject: [PATCH 2/7] Avoid using castToNonNull suggested fixes in certain cases --- .../java/com/uber/nullaway/ErrorBuilder.java | 17 +++++++-- .../nullaway/NullAwayAutoSuggestTest.java | 36 +++++++++++++++++-- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 67a0026432..287dfb041c 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -31,6 +31,7 @@ import static com.uber.nullaway.NullAway.INITIALIZATION_CHECK_NAME; import static com.uber.nullaway.NullAway.OPTIONAL_CHECK_NAME; import static com.uber.nullaway.NullAway.getTreesInstance; +import static com.uber.nullaway.Nullness.hasNullableAnnotation; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; @@ -61,6 +62,7 @@ import java.util.stream.StreamSupport; import javax.annotation.Nullable; import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; import javax.tools.JavaFileObject; /** A class to construct error message to be displayed after the analysis finds error. */ @@ -198,10 +200,11 @@ private Description.Builder addSuggestedSuppression( case PASS_NULLABLE: case ASSIGN_FIELD_NULLABLE: case SWITCH_EXPRESSION_NULLABLE: - if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree, state)) { + if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) { builder = addCastToNonNullFix(suggestTree, builder); } else { - builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + builder = + addSuppressWarningsFix(suppressibleNode(state.getPath()), builder, suppressionName); } break; case CAST_TO_NONNULL_ARG_NONNULL: @@ -329,11 +332,19 @@ private Tree suppressibleNode(@Nullable TreePath path) { *
  • {@code tree} represents a {@code @Nullable} formal parameter of the enclosing method * */ - private boolean canBeCastToNonNull(Tree tree, VisitorState state) { + private boolean canBeCastToNonNull(Tree tree) { switch (tree.getKind()) { case NULL_LITERAL: + // never do castToNonNull(null) return false; case IDENTIFIER: + // Don't wrap a @Nullable parameter in castToNonNull, as this misleads callers into thinking + // they can pass in null without causing an NPE. A more appropriate fix would likely be to + // make the parameter @NonNull and add casts at call sites, but that is beyond the scope of + // our suggested fixes + Symbol symbol = ASTHelpers.getSymbol(tree); + return !(symbol.getKind().equals(ElementKind.PARAMETER) + && hasNullableAnnotation(symbol, config)); default: return true; } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index 92138b2f79..4f12d25b99 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -92,7 +92,8 @@ public void suggestCastToNonNull() throws IOException { "package com.uber;", "import javax.annotation.Nullable;", "class Test {", - " Object test1(@Nullable Object o) {", + " @Nullable Object o;", + " Object test1() {", " return o;", " }", "}") @@ -102,13 +103,44 @@ public void suggestCastToNonNull() throws IOException { "import static com.uber.nullaway.testdata.Util.castToNonNull;", "import javax.annotation.Nullable;", "class Test {", - " Object test1(@Nullable Object o) {", + " @Nullable Object o;", + " Object test1() {", " return castToNonNull(o);", " }", "}") .doTest(); } + @Test + public void suppressInsteadOfCastToNonNull() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object test1(@Nullable Object o) {", + " return o;", + " }", + " Object test2() {", + " return null;", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings(\"NullAway\") Object test1(@Nullable Object o) {", + " return o;", + " }", + " @SuppressWarnings(\"NullAway\") Object test2() {", + " return null;", + " }", + "}") + .doTest(); + } + @Test public void removeUnnecessaryCastToNonNull() throws IOException { makeTestHelper() From 6cc3e83fe52a23f331cfd24102801f031ac0143c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 4 Aug 2023 16:54:31 -0700 Subject: [PATCH 3/7] fix NullAway warnings --- .../src/main/java/com/uber/nullaway/ErrorBuilder.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 287dfb041c..28ef27c9a7 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -203,8 +203,10 @@ private Description.Builder addSuggestedSuppression( if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) { builder = addCastToNonNullFix(suggestTree, builder); } else { - builder = - addSuppressWarningsFix(suppressibleNode(state.getPath()), builder, suppressionName); + Tree suppressibleNode = suppressibleNode(state.getPath()); + if (suppressibleNode != null) { + builder = addSuppressWarningsFix(suppressibleNode, builder, suppressionName); + } } break; case CAST_TO_NONNULL_ARG_NONNULL: @@ -343,7 +345,8 @@ private boolean canBeCastToNonNull(Tree tree) { // make the parameter @NonNull and add casts at call sites, but that is beyond the scope of // our suggested fixes Symbol symbol = ASTHelpers.getSymbol(tree); - return !(symbol.getKind().equals(ElementKind.PARAMETER) + return !(symbol != null + && symbol.getKind().equals(ElementKind.PARAMETER) && hasNullableAnnotation(symbol, config)); default: return true; From 29902bee3deeb430ac5e33bad6d58512628427b9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 24 Aug 2023 09:14:27 -0700 Subject: [PATCH 4/7] add comment --- nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 28ef27c9a7..2e97588932 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -203,6 +203,9 @@ private Description.Builder addSuggestedSuppression( if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) { builder = addCastToNonNullFix(suggestTree, builder); } else { + // When there is a castToNonNull method, suggestTree is set to the expression to be + // casted, which is not suppressible. For simplicity, we just always recompute the + // suppressible node here. Tree suppressibleNode = suppressibleNode(state.getPath()); if (suppressibleNode != null) { builder = addSuppressWarningsFix(suppressibleNode, builder, suppressionName); From b84a4f76c595c6e30903ed04db79ddf945a9d4e8 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 24 Aug 2023 09:19:20 -0700 Subject: [PATCH 5/7] docs and another test --- .../com/uber/nullaway/NullAwayAutoSuggestTest.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index 4f12d25b99..41fa3ed9b1 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -24,6 +24,7 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.ErrorProneFlags; +import com.sun.source.tree.Tree; import com.uber.nullaway.testlibrarymodels.TestLibraryModels; import java.io.IOException; import org.junit.Before; @@ -111,6 +112,10 @@ public void suggestCastToNonNull() throws IOException { .doTest(); } + /** + * Test for cases where we heuristically decide not to wrap an expression in castToNonNull; see + * {@link ErrorBuilder#canBeCastToNonNull(Tree)} + */ @Test public void suppressInsteadOfCastToNonNull() throws IOException { makeTestHelper() @@ -119,24 +124,32 @@ public void suppressInsteadOfCastToNonNull() throws IOException { "package com.uber;", "import javax.annotation.Nullable;", "class Test {", + " Object f = new Object();", " Object test1(@Nullable Object o) {", " return o;", " }", " Object test2() {", " return null;", " }", + " void test3() {", + " f = null;", + " }", "}") .addOutputLines( "out/Test.java", "package com.uber;", "import javax.annotation.Nullable;", "class Test {", + " Object f = new Object();", " @SuppressWarnings(\"NullAway\") Object test1(@Nullable Object o) {", " return o;", " }", " @SuppressWarnings(\"NullAway\") Object test2() {", " return null;", " }", + " @SuppressWarnings(\"NullAway\") void test3() {", + " f = null;", + " }", "}") .doTest(); } From eece8d7ea745457784f2c0c8e73e28826111b879 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 24 Aug 2023 09:30:51 -0700 Subject: [PATCH 6/7] improve test coverage --- .../java/com/uber/nullaway/ErrorBuilder.java | 4 +- .../nullaway/NullAwayAutoSuggestTest.java | 73 +++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 2e97588932..0ad1392e49 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -395,8 +395,8 @@ private Description.Builder removeCastToNonNullFix( } /** - * Reports initialization errors where a constructor fails to guarantee non-fields are initialized - * along all paths at exit points. + * Reports initialization errors where a constructor fails to guarantee non-null fields are + * initialized along all paths at exit points. * * @param methodSymbol Constructor symbol. * @param message Error message. diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index 41fa3ed9b1..d025fa9a7c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -134,10 +134,15 @@ public void suppressInsteadOfCastToNonNull() throws IOException { " void test3() {", " f = null;", " }", + " @Nullable Object m() { return null; }", + " Object shouldAddCast() {", + " return m();", + " }", "}") .addOutputLines( "out/Test.java", "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", "import javax.annotation.Nullable;", "class Test {", " Object f = new Object();", @@ -150,6 +155,10 @@ public void suppressInsteadOfCastToNonNull() throws IOException { " @SuppressWarnings(\"NullAway\") void test3() {", " f = null;", " }", + " @Nullable Object m() { return null; }", + " Object shouldAddCast() {", + " return castToNonNull(m());", + " }", "}") .doTest(); } @@ -247,4 +256,68 @@ public void suggestSuppressionOnMethodRef() throws IOException { "}") .doTest(); } + + @Test + public void suggestInitSuppressionOnConstructor() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + " Object g;", + " Test() {}", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + " Object g;", + " @SuppressWarnings(\"NullAway.Init\") Test() {}", + "}") + .doTest(); + } + + @Test + public void suggestInitSuppressionOnField() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings(\"NullAway.Init\") Object f;", + "}") + .doTest(); + } + + @Test + public void updateExtantSuppressWarnings() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings(\"unused\") Object f;", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings({\"unused\",\"NullAway.Init\"}) Object f;", + "}") + .doTest(); + } } From fb5406a4dd965005deabe32a2e2643a14de988e1 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 25 Aug 2023 10:48:47 -0700 Subject: [PATCH 7/7] Update nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lázaro Clapp --- nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 0ad1392e49..5188f823b1 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -204,7 +204,7 @@ private Description.Builder addSuggestedSuppression( builder = addCastToNonNullFix(suggestTree, builder); } else { // When there is a castToNonNull method, suggestTree is set to the expression to be - // casted, which is not suppressible. For simplicity, we just always recompute the + // casted, which is not suppressible. For simplicity, we just always recompute the // suppressible node here. Tree suppressibleNode = suppressibleNode(state.getPath()); if (suppressibleNode != null) {