Skip to content

Commit

Permalink
Various fixes for generating @SuppressWarnings (#271)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@msridhar
msridhar committed Dec 13, 2018
1 parent 33152f1 commit 46073af
Show file tree
Hide file tree
Showing 2 changed files with 254 additions and 41 deletions.
67 changes: 29 additions & 38 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1088,7 +1084,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
tree,
"assigning @Nullable expression to @NonNull field",
initializer,
tree);
state.getPath());
}
}
}
Expand Down Expand Up @@ -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 '"
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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}.
*
* <p>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.
* <p>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) {
Expand Down
228 changes: 225 additions & 3 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -57,7 +56,7 @@ public void setup() {
}

@Test
public void suggestSuppressionWithComment() throws IOException {
public void suggestSuppressionWithComment() {
BugCheckerRefactoringTestHelper bcr =
BugCheckerRefactoringTestHelper.newInstance(new NullAway(flags), getClass());

Expand All @@ -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());
Expand All @@ -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<Object, Integer> f;",
" void m1() {",
" f = (x) -> { return foo + 1; };",
" }",
" void m2() {",
" java.util.function.Function<Object,Integer> 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<Object, Integer> f;",
" @SuppressWarnings(\"NullAway\")",
" void m1() {",
" f = (x) -> { return foo + 1; };",
" }",
" void m2() {",
" @SuppressWarnings(\"NullAway\")",
" java.util.function.Function<Object,Integer> 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);
}
}

0 comments on commit 46073af

Please sign in to comment.