Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Various fixes for generating @SuppressWarnings #271

Merged
merged 4 commits into from Dec 13, 2018
Merged

Various fixes for generating @SuppressWarnings #271

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3adc98a
Suppress properly for deref on RHS of field initializer
msridhar Dec 12, 2018
3fac3f1
handle unboxing
msridhar Dec 12, 2018
42b19b9
more fixes
msridhar Dec 12, 2018
fa2d13f
another test
msridhar Dec 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 29 additions & 38 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this iterate over the full path even if the suppressible node is close? Or is it a lazy iterator / generator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. From what I can see it bottoms out in an iterator and goes one by one checking the predicate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry by "I think so" I meant that I think it's lazy

}

private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) {
Expand Down
188 changes: 185 additions & 3 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestNoCastTest.java
View file
Open in desktop
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,187 @@ 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;",
" };",
"}");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you assign a lambda within a method, where does this put the suppression? And, does that suppression work?

e.g.

public void foo() {
  this.f = () -> { ... };
  Object o = () -> { ... };
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case. For the this.f assignment the suppression goes on the method, whereas for the Object o case it goes on the assignment itself (you can suppress on variable declarations).

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