From 46073afed1ad6f13f89d4b53a9d5e2ed0ab98ce3 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 12 Dec 2018 17:02:57 -0800 Subject: [PATCH] Various fixes for generating @SuppressWarnings (#271) Fixes #266 Fixes some issues with generating suppressions for method reference and lambda issues (see new tests). Also refactors the code a bit to be more consistent in using the `TreePath` to find the right place to suppress --- .../main/java/com/uber/nullaway/NullAway.java | 67 +++-- .../NullAwayAutoSuggestNoCastTest.java | 228 +++++++++++++++++- 2 files changed, 254 insertions(+), 41 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 71e25c4661..3fc6e60de3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -525,7 +525,7 @@ private Description checkParamOverriding( + overriddenMethod.toString() + " is @Nullable"; return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_PARAM, memberReferenceTree, message, memberReferenceTree); + MessageTypes.WRONG_OVERRIDE_PARAM, memberReferenceTree, message, state.getPath()); } } // for unbound member references, we need to adjust parameter indices by 1 when matching with @@ -581,7 +581,7 @@ private Description checkParamOverriding( errorTree = getTreesInstance(state).getTree(paramSymbol); } return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_PARAM, errorTree, message, errorTree); + MessageTypes.WRONG_OVERRIDE_PARAM, errorTree, message, state.getPath()); } } return Description.NO_MATCH; @@ -714,12 +714,8 @@ && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) { memberReferenceTree != null ? memberReferenceTree : getTreesInstance(state).getTree(overridingMethod); - Tree suggestTree = - memberReferenceTree != null - ? NullabilityUtil.findEnclosingMethodOrLambdaOrInitializer(state.getPath()).getLeaf() - : errorTree; return createErrorDescription( - MessageTypes.WRONG_OVERRIDE_RETURN, errorTree, message, suggestTree); + MessageTypes.WRONG_OVERRIDE_RETURN, errorTree, message, state.getPath()); } // if any parameter in the super method is annotated @Nullable, // overriding method cannot assume @Nonnull @@ -1088,7 +1084,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) { tree, "assigning @Nullable expression to @NonNull field", initializer, - tree); + state.getPath()); } } } @@ -1328,7 +1324,6 @@ private Description checkCastToNonNullTakesNullable( // Initializer block isInitializer = true; } - MethodTree enclosingMethod = ASTHelpers.findEnclosingNode(state.getPath(), MethodTree.class); if (!isInitializer && !mayBeNullExpr(state, actual)) { String message = "passing known @NonNull parameter '" @@ -1961,8 +1956,8 @@ private Description matchDereference( */ private Description createErrorDescription( MessageTypes errorType, Tree errorLocTree, String message, TreePath path) { - MethodTree enclosingMethod = ASTHelpers.findEnclosingNode(path, MethodTree.class); - return createErrorDescription(errorType, errorLocTree, message, enclosingMethod); + Tree enclosingSuppressTree = suppressibleNode(path); + return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); } /** @@ -2033,38 +2028,34 @@ private Description createErrorDescriptionForNullAssignment( String message, @Nullable Tree suggestTreeIfCastToNonNull, @Nullable TreePath suggestTreePathIfSuppression) { - MethodTree enclosingMethod = - ASTHelpers.findEnclosingNode(suggestTreePathIfSuppression, MethodTree.class); - return createErrorDescriptionForNullAssignment( - errorType, errorLocTree, message, suggestTreeIfCastToNonNull, enclosingMethod); + Tree enclosingSuppressTree = suppressibleNode(suggestTreePathIfSuppression); + if (config.getCastToNonNullMethod() != null) { + return createErrorDescription(errorType, errorLocTree, message, suggestTreeIfCastToNonNull); + } else { + return createErrorDescription(errorType, errorLocTree, message, enclosingSuppressTree); + } } /** - * create an error description for a generalized @Nullable value to @NonNull location assignment. + * Adapted from {@link com.google.errorprone.fixes.SuggestedFixes}. * - *

This includes: field assignments, method arguments and method returns - * - * @param errorType the type of error encountered. - * @param errorLocTree the location of the error - * @param message the error message - * @param suggestTreeIfCastToNonNull the location at which a fix suggestion should be made if a - * castToNonNull method is available (usually the expression to cast) - * @param suggestTreeIfSuppression the location at which a fix suggestion should be made if a - * castToNonNull method is not available (usually the enclosing method, or any place - * where @SuppressWarnings can be added). - * @return the error description. + *

TODO: actually use {@link + * com.google.errorprone.fixes.SuggestedFixes#addSuppressWarnings(VisitorState, String)} instead */ - private Description createErrorDescriptionForNullAssignment( - MessageTypes errorType, - Tree errorLocTree, - String message, - @Nullable Tree suggestTreeIfCastToNonNull, - @Nullable Tree suggestTreeIfSuppression) { - if (config.getCastToNonNullMethod() != null) { - return createErrorDescription(errorType, errorLocTree, message, suggestTreeIfCastToNonNull); - } else { - return createErrorDescription(errorType, errorLocTree, message, suggestTreeIfSuppression); - } + @Nullable + private static Tree suppressibleNode(@Nullable TreePath path) { + if (path == null) { + return null; + } + return StreamSupport.stream(path.spliterator(), false) + .filter( + tree -> + tree instanceof MethodTree + || (tree instanceof ClassTree + && ((ClassTree) tree).getSimpleName().length() != 0) + || tree instanceof VariableTree) + .findFirst() + .orElse(null); } private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java index 76f658efdd..33533e260c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java @@ -24,7 +24,6 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.ErrorProneFlags; -import java.io.IOException; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -57,7 +56,7 @@ public void setup() { } @Test - public void suggestSuppressionWithComment() throws IOException { + public void suggestSuppressionWithComment() { BugCheckerRefactoringTestHelper bcr = BugCheckerRefactoringTestHelper.newInstance(new NullAway(flags), getClass()); @@ -82,7 +81,7 @@ public void suggestSuppressionWithComment() throws IOException { } @Test - public void suggestSuppressionWithoutComment() throws IOException { + public void suggestSuppressionWithoutComment() { BugCheckerRefactoringTestHelper bcr = BugCheckerRefactoringTestHelper.newInstance( new NullAway(flagsNoAutoFixSuppressionComment), getClass()); @@ -106,4 +105,227 @@ public void suggestSuppressionWithoutComment() throws IOException { "}"); bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + + @Test + public void suggestSuppressionFieldLambdaDeref() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Object foo;", + " private final Runnable runnable =", + " () -> {", + " foo.toString();", + " };", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Object foo;", + " @SuppressWarnings(\"NullAway\")", + " private final Runnable runnable =", + " () -> {", + " foo.toString();", + " };", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestSuppressionFieldLambdaUnbox() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " static int id(int x) { return x; }", + " private final Runnable runnable =", + " () -> {", + " id(foo);", + " };", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " static int id(int x) { return x; }", + " @SuppressWarnings(\"NullAway\")", + " private final Runnable runnable =", + " () -> {", + " id(foo);", + " };", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestSuppressionFieldLambdaAssignment() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " static int id(int x) { return x; }", + " private final Runnable runnable =", + " () -> {", + " int x = foo + 1;", + " };", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " static int id(int x) { return x; }", + " private final Runnable runnable =", + " () -> {", + " @SuppressWarnings(\"NullAway\")", + " int x = foo + 1;", + " };", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestLambdaAssignInMethod() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " @Nullable private java.util.function.Function f;", + " void m1() {", + " f = (x) -> { return foo + 1; };", + " }", + " void m2() {", + " java.util.function.Function g = (x) -> { return foo + 1; };", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable private Integer foo;", + " @Nullable private java.util.function.Function f;", + " @SuppressWarnings(\"NullAway\")", + " void m1() {", + " f = (x) -> { return foo + 1; };", + " }", + " void m2() {", + " @SuppressWarnings(\"NullAway\")", + " java.util.function.Function g = (x) -> { return foo + 1; };", + " }", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suppressMethodRefOverrideParam() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static interface I {", + " public void foo(@Nullable Object o);", + " }", + " static void biz(Object p) {}", + " static void callFoo(I i) { i.foo(null); }", + " static void bar() {", + " callFoo(Test::biz);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static interface I {", + " public void foo(@Nullable Object o);", + " }", + " static void biz(Object p) {}", + " static void callFoo(I i) { i.foo(null); }", + " @SuppressWarnings(\"NullAway\")", + " static void bar() {", + " callFoo(Test::biz);", + " }", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suppressMethodRefOverrideReturn() { + BugCheckerRefactoringTestHelper bcr = + BugCheckerRefactoringTestHelper.newInstance( + new NullAway(flagsNoAutoFixSuppressionComment), getClass()); + + bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath()); + bcr.addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static interface I {", + " public Object foo();", + " }", + " @Nullable", + " static Object biz() { return null; }", + " static void callFoo(I i) { i.foo(); }", + " static void bar() {", + " callFoo(Test::biz);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static interface I {", + " public Object foo();", + " }", + " @Nullable", + " static Object biz() { return null; }", + " static void callFoo(I i) { i.foo(); }", + " @SuppressWarnings(\"NullAway\")", + " static void bar() {", + " callFoo(Test::biz);", + " }", + "}"); + bcr.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } }