Skip to content

Commit

Permalink
Handle more cases in ReturnMissingNullable.
Browse files Browse the repository at this point in the history
I have no stats about how commonly these occur, but it's easy enough to handle them.

Also, minor cleanups and other notes:

- Document the `NullType` subtyping implementation trick.
- Check `returnType.isPrimitive()`, just as the old code used to. This check wasn't necessary in the new code until this CL's improvements.
- Inline `makeFix`, which doesn't really benefit from being a separate method. Then apply a suggested simplification.

PiperOrigin-RevId: 388295613
  • Loading branch information
cpovirk authored and Error Prone Team committed Aug 2, 2021
1 parent cc5ef9e commit b660308
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,15 @@
import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnnotations;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.ParenthesizedTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.TypeCastTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type;
Expand Down Expand Up @@ -103,6 +106,10 @@ && getOnlyElement(statements) == returnTree
// `@Nullable Void` is accurate but noisy, so some users won't want it.
return NO_MATCH;
}
if (returnType.isPrimitive()) {
// Buggy code, but adding @Nullable just makes it worse.
return NO_MATCH;
}
if (beingConservative && state.getTypes().isArray(returnType)) {
/*
* Type-annotation syntax on arrays can be confusing, and this refactoring doesn't get it
Expand All @@ -123,29 +130,46 @@ && getOnlyElement(statements) == returnTree
return NO_MATCH;
}

return makeFix(state, methodTree, returnTree);
return describeMatch(returnTree, NullnessFixes.makeFix(state, methodTree));
}

// VisitorState will have the TreePath of the `return` statement. That's OK for our purposes.
private static final SimpleTreeVisitor<Boolean, VisitorState> HAS_DEFINITELY_NULL_BRANCH =
new SimpleTreeVisitor<Boolean, VisitorState>() {
@Override
public Boolean visitAssignment(AssignmentTree tree, VisitorState state) {
return visit(tree.getExpression(), state);
}

@Override
public Boolean visitConditionalExpression(
ConditionalExpressionTree tree, VisitorState state) {
return visit(tree.getTrueExpression(), state) || visit(tree.getFalseExpression(), state);
}

@Override
public Boolean visitParenthesized(ParenthesizedTree tree, VisitorState state) {
return visit(tree.getExpression(), state);
}

@Override
public Boolean visitTypeCast(TypeCastTree tree, VisitorState state) {
return visit(tree.getExpression(), state);
}

@Override
protected Boolean defaultAction(Tree tree, VisitorState state) {
/*
* This covers not only "Void" and "CAP#1 extends Void" but also the null literal. (It
* covers the null literal even through parenthesized expressions. Still, we end up
* needing special handling for parenthesized expressions for cases like `(foo ? bar :
* null)`.)
*/
return isVoid(getType(tree), state);
}
};

private static boolean isVoid(Type type, VisitorState state) {
return type != null && state.getTypes().isSubtype(type, JAVA_LANG_VOID_TYPE.get(state));
}

private Description makeFix(VisitorState state, Tree declaration, Tree matchedTree) {
return buildDescription(matchedTree).addFix(NullnessFixes.makeFix(state, declaration)).build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,45 @@ public void testParenthesizedLiteralNullReturn() {
.doTest();
}

@Test
public void testAssignmentOfLiteralNullReturn() {
createCompilationTestHelper()
.addSourceLines(
"com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
"package com.google.errorprone.bugpatterns.nullness;",
"public class LiteralNullReturnTest {",
" String cachedMessage;",
" public String getMessage(boolean b) {",
" if (b) {",
" // BUG: Diagnostic contains: @Nullable",
" return cachedMessage = null;",
" } else {",
" return \"negative\";",
" }",
" }",
"}")
.doTest();
}

@Test
public void testCastLiteralNullReturn() {
createCompilationTestHelper()
.addSourceLines(
"com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
"package com.google.errorprone.bugpatterns.nullness;",
"public class LiteralNullReturnTest {",
" public String getMessage(boolean b) {",
" if (b) {",
" // BUG: Diagnostic contains: @Nullable",
" return (String) null;",
" } else {",
" return \"negative\";",
" }",
" }",
"}")
.doTest();
}

@Test
public void testConditionalLiteralNullReturn() {
createCompilationTestHelper()
Expand All @@ -79,6 +118,21 @@ public void testConditionalLiteralNullReturn() {
.doTest();
}

@Test
public void testParenthesizedConditionalLiteralNullReturn() {
createCompilationTestHelper()
.addSourceLines(
"com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
"package com.google.errorprone.bugpatterns.nullness;",
"public class LiteralNullReturnTest {",
" public String getMessage(int x) {",
" // BUG: Diagnostic contains: @Nullable",
" return (x >= 0 ? null : \"negative\");",
" }",
"}")
.doTest();
}

@Test
public void testVoidReturn() {
createCompilationTestHelper()
Expand Down

0 comments on commit b660308

Please sign in to comment.