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.

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

Avoid suggesting castToNonNull fixes in certain cases #799

Merged
merged 11 commits into from
Aug 25, 2023
7 changes: 5 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
msridhar marked this conversation as resolved.
Show resolved Hide resolved
// suppressible node here.
Tree suppressibleNode = suppressibleNode(state.getPath());
msridhar marked this conversation as resolved.
Show resolved Hide resolved
if (suppressibleNode != null) {
builder = addSuppressWarningsFix(suppressibleNode, builder, suppressionName);
Expand Down Expand Up @@ -392,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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comment noting which cases these are and referencing the ErrorBuilder docs? Just because when not seen as part of the PR it might be hard to remember why we want this behavior.

Also, can we add a case where null is assigned to a non-null field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b84a4f7

makeTestHelper()
Expand All @@ -119,24 +124,41 @@ 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;",
" }",
" @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();",
" @SuppressWarnings(\"NullAway\") Object test1(@Nullable Object o) {",
" return o;",
" }",
" @SuppressWarnings(\"NullAway\") Object test2() {",
" return null;",
" }",
" @SuppressWarnings(\"NullAway\") void test3() {",
" f = null;",
" }",
" @Nullable Object m() { return null; }",
" Object shouldAddCast() {",
" return castToNonNull(m());",
" }",
"}")
.doTest();
}
Expand Down Expand Up @@ -234,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();
}
}