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

Avoid suggesting castToNonNull fixes in certain cases #799

Merged
merged 11 commits into from
Aug 25, 2023
Merged

Avoid suggesting castToNonNull fixes in certain cases #799

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ea7f06
WIP
msridhar Aug 4, 2023
95df551
Avoid using castToNonNull suggested fixes in certain cases
msridhar Aug 4, 2023
6cc3e83
fix NullAway warnings
msridhar Aug 4, 2023
24e4582
Merge branch 'master' into manu/tweak-cast-suppressions
msridhar Aug 12, 2023
dd851b2
Merge branch 'master' into manu/tweak-cast-suppressions
msridhar Aug 19, 2023
9a4ac1a
Merge branch 'master' into manu/tweak-cast-suppressions
msridhar Aug 21, 2023
5b46f06
Merge branch 'master' into manu/tweak-cast-suppressions
msridhar Aug 22, 2023
29902be
add comment
msridhar Aug 24, 2023
b84a4f7
docs and another test
msridhar Aug 24, 2023
eece8d7
improve test coverage
msridhar Aug 24, 2023
fb5406a
Update nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
msridhar Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -198,10 +200,16 @@ 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)) {
builder = addCastToNonNullFix(suggestTree, builder);
} else {
builder = addSuppressWarningsFix(suggestTree, builder, suppressionName);
// 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);
}
}
break;
case CAST_TO_NONNULL_ARG_NONNULL:
Expand Down Expand Up @@ -320,6 +328,34 @@ 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}:
*
* <ol>
* <li>{@code tree} represents the {@code null} literal
* <li>{@code tree} represents a {@code @Nullable} formal parameter of the enclosing method
* </ol>
*/
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 != null
&& symbol.getKind().equals(ElementKind.PARAMETER)
&& hasNullableAnnotation(symbol, config));
default:
return true;
}
}

private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) {
final String fullMethodName = config.getCastToNonNullMethod();
if (fullMethodName == null) {
Expand Down Expand Up @@ -359,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
122 changes: 120 additions & 2 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java
View file
Open in desktop
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 @@ -92,7 +93,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;",
" }",
"}")
Expand All @@ -102,13 +104,65 @@ 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 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()
.addInputLines(
"Test.java",
"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();
}

@Test
public void removeUnnecessaryCastToNonNull() throws IOException {
makeTestHelper()
Expand Down Expand Up @@ -202,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();
}
}