Skip to content

Commit

Permalink
Avoid suggesting castToNonNull fixes in certain cases (#799)
Browse files Browse the repository at this point in the history 
We avoid suggesting a `castToNonNull` fix if either (1) the expression
is the `null` literal (i.e., don't suggest `castToNonNull(null)`), or
(2) the expression is a `@Nullable` parameter (see code comments for
justification). In these cases, we suggest a `@SuppressWarnings`
annotation instead.

Here we also add some tests to improve coverage of the `ErrorBuilder`
class.

---------

Co-authored-by: Lázaro Clapp <lazaro@uber.com>
  • Loading branch information
@msridhar @lazaroclapp
msridhar and lazaroclapp committed Aug 25, 2023
1 parent 56d5ff1 commit 154b758
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 6 deletions.
44 changes: 40 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
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
// suppressible node here.
Tree suppressibleNode = suppressibleNode(state.getPath());
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
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 {
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();
}
}

0 comments on commit 154b758

Please sign in to comment.