From 49e4b34998e8b23e8e076feed96311983b56c624 Mon Sep 17 00:00:00 2001
From: cpovirk <cpovirk@google.com>
Date: Wed, 13 Jul 2022 11:59:55 -0700
Subject: [PATCH] Don't suggest `@Nullable` on implementations of methods like
 `Map.put` 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 https://github.com/google/error-prone/issues/2910

PiperOrigin-RevId: 460772102
---
 .../nullness/ReturnMissingNullable.java       | 21 ++++++++++++-
 .../nullness/ReturnMissingNullableTest.java   | 30 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
index bb557cc181f..feff9fa1bd4 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
@@ -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;
@@ -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
@@ -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());
             }
           }
         }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
index df2d5e4c575..64592ffda59 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
@@ -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()