Skip to content

Commit

Permalink
Don't suggest @Nullable on implementations of methods like `Map.put…
Browse files Browse the repository at this point in the history
…` if those implementations always throw or they are `@DoNotCall`.

Also, provide a different error message for this "implementations of well-known methods" case, one that is different from the usual "Method returns a definitely null value" message.

(I considered continuing the make the suggestion in "aggressive" mode, but I'm not sure if it's worth the trouble even then, since it may lead to a discussion every time the suggestion appears.)

Fixes #2910

PiperOrigin-RevId: 460772102
  • Loading branch information
cpovirk authored and Error Prone Team committed Jul 13, 2022
1 parent d6ee78c commit 49e4b34
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@
import static com.google.errorprone.util.ASTHelpers.constValue;
import static com.google.errorprone.util.ASTHelpers.findEnclosingMethod;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
import static com.google.errorprone.util.ASTHelpers.methodCanBeOverridden;
import static com.sun.source.tree.Tree.Kind.NULL_LITERAL;
import static com.sun.source.tree.Tree.Kind.THROW;
import static java.lang.Boolean.FALSE;
import static java.util.regex.Pattern.compile;
import static javax.lang.model.type.TypeKind.TYPEVAR;
Expand Down Expand Up @@ -280,6 +282,17 @@ void doVisitMethod(MethodTree tree) {
return;
}

if (tree.getBody() != null
&& tree.getBody().getStatements().size() == 1
&& getOnlyElement(tree.getBody().getStatements()).getKind() == THROW) {
return;
}

if (hasAnnotation(
tree, "com.google.errorprone.annotations.DoNotCall", stateForCompilationUnit)) {
return;
}

for (MethodSymbol methodKnownToReturnNull :
METHODS_KNOWN_TO_RETURN_NULL.get(stateForCompilationUnit)) {
if (stateForCompilationUnit
Expand All @@ -289,7 +302,13 @@ void doVisitMethod(MethodTree tree) {
fixByAddingNullableAnnotationToReturnType(
stateForCompilationUnit.withPath(getCurrentPath()), tree);
if (!fix.isEmpty()) {
stateForCompilationUnit.reportMatch(describeMatch(tree, fix));
stateForCompilationUnit.reportMatch(
buildDescription(tree)
.setMessage(
"Nearly all implementations of this method must return null, but it is"
+ " not annotated @Nullable")
.addFix(fix)
.build());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,36 @@ public void testImplementsMap() {
.doTest();
}

@Test
public void testImplementsMapButAlwaysThrows() {
createCompilationTestHelper()
.addSourceLines(
"MyMap.java",
"import java.util.Map;",
"abstract class MyMap<K, V> implements Map<K, V> {",
" @Override",
" public V put(K k, V v) {",
" throw new UnsupportedOperationException();",
" }",
"}")
.doTest();
}

@Test
public void testImplementsMapButDoNotCall() {
createCompilationTestHelper()
.addSourceLines(
"MyMap.java",
"import com.google.errorprone.annotations.DoNotCall;",
"import java.util.Map;",
"interface MyMap<K, V> extends Map<K, V> {",
" @DoNotCall",
" @Override",
" V put(K k, V v);",
"}")
.doTest();
}

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

0 comments on commit 49e4b34

Please sign in to comment.