Allow any package's @CanIgnoreReturnValue to suppress CanIgnoreReturn…

…ValueSuggester This brings it into alignment with the CheckReturnValue checker logic. PiperOrigin-RevId: 658248419
google · Aug 1, 2024 · 6ff6f35 · 6ff6f35
1 parent ba7e3b3
commit 6ff6f35
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 7 deletions.
diff --git a/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java
@@ -25,6 +25,7 @@
 import static com.google.errorprone.util.ASTHelpers.getReturnType;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
 import static com.google.errorprone.util.ASTHelpers.isAbstract;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
 import static com.google.errorprone.util.ASTHelpers.isSubtype;
@@ -78,10 +79,9 @@ public final class CanIgnoreReturnValueSuggester extends BugChecker implements M
 
   private static final String AUTO_VALUE = "com.google.auto.value.AutoValue";
   private static final String IMMUTABLE = "com.google.errorprone.annotations.Immutable";
-  private static final String CIRV = "com.google.errorprone.annotations.CanIgnoreReturnValue";
+  private static final String CIRV_SIMPLE_NAME = "CanIgnoreReturnValue";
   private static final ImmutableSet<String> EXEMPTING_METHOD_ANNOTATIONS =
       ImmutableSet.of(
-          CIRV,
           "com.google.errorprone.annotations.CheckReturnValue",
           "com.google.errorprone.refaster.annotation.AfterTemplate");
 
@@ -118,7 +118,12 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    // If the method has an exempting annotation, then bail out.
+    // If the method is @CanIgnoreReturnValue (in any package), then bail out.
+    if (hasDirectAnnotationWithSimpleName(methodSymbol, CIRV_SIMPLE_NAME)) {
+      return Description.NO_MATCH;
+    }
+
+    // If the method has another exempting annotation, then bail out.
     if (exemptingMethodAnnotations.stream()
         .anyMatch(annotation -> hasAnnotation(methodSymbol, annotation, state))) {
       return Description.NO_MATCH;
@@ -187,7 +192,8 @@ private Description annotateWithCanIgnoreReturnValue(MethodTree methodTree, Visi
     }
 
     // now annotate it with @CanIgnoreReturnValue
-    fix.prefixWith(methodTree, "@" + qualifyType(state, fix, CIRV) + "\n");
+    String cirv = "com.google.errorprone.annotations.CanIgnoreReturnValue";
+    fix.prefixWith(methodTree, "@" + qualifyType(state, fix, cirv) + "\n");
 
     return describeMatch(methodTree, fix.build());
   }
@@ -307,9 +313,8 @@ private boolean isIgnorableMethodCallOnSameInstance(
               || isIdentifier(receiver, "super")) {
             // If the method we're calling is @CIRV and the enclosing class could be represented by
             // the object being returned by the other method, then it's probable that the other
-            // method is likely to
-            // be an ignorable result.
-            return hasAnnotation(calledMethod, CIRV, state)
+            // method is likely to be an ignorable result.
+            return hasDirectAnnotationWithSimpleName(calledMethod, CIRV_SIMPLE_NAME)
                 && isSubtype(enclosingClassType, methodReturnType, state)
                 && isSubtype(enclosingClassType, getReturnType(mit), state);
           }

diff --git a/...com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java b/...com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java
@@ -418,6 +418,43 @@ public void simpleCaseAlreadyAnnotatedWithCirv() {
         .doTest();
   }
 
+  @Test
+  public void alreadyAnnotatedWithProtobufCirv_b356526159() {
+    helper
+        .addInputLines(
+            "Client.java",
+            "package com.google.protobuf;",
+            "public final class Client {",
+            "  private String name;",
+            "  @CanIgnoreReturnValue",
+            "  public Client setName(String name) {",
+            "    this.name = name;",
+            "    return this;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
+  @Test
+  public void alreadyAnnotatedWithArbitraryCirv_b356526159() {
+    helper
+        .addInputLines(
+            "Client.java",
+            "package com.google.frobber;",
+            "public final class Client {",
+            "  @interface CanIgnoreReturnValue {}",
+            "  private String name;",
+            "  @CanIgnoreReturnValue",
+            "  public Client setName(String name) {",
+            "    this.name = name;",
+            "    return this;",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
+
   @Test
   public void simpleCaseAlreadyAnnotatedWithCrv() {
     helper