Don't fire CanIgnoreReturnValueSuggester on Dagger `@Component.Buil…

…der` or `@Subcomponent.Builder` methods (they will soon be treated as implicitly ignorable via a custom rule). #checkreturnvalue PiperOrigin-RevId: 595417817
google · Jan 3, 2024 · 64ec061 · 64ec061
1 parent 4ef5e53
commit 64ec061
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 35 deletions.
diff --git a/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/...ava/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java
@@ -84,6 +84,13 @@ public final class CanIgnoreReturnValueSuggester extends BugChecker implements M
           "com.google.errorprone.annotations.CheckReturnValue",
           "com.google.errorprone.refaster.annotation.AfterTemplate");
 
+  private static final ImmutableSet<String> EXEMPTING_CLASS_ANNOTATIONS =
+      ImmutableSet.of(
+          "com.google.auto.value.AutoValue.Builder",
+          "com.google.auto.value.AutoBuilder",
+          "dagger.Component.Builder",
+          "dagger.Subcomponent.Builder");
+
   private static final Supplier<Type> PROTO_BUILDER =
       VisitorState.memoize(s -> s.getTypeFromString("com.google.protobuf.MessageLite.Builder"));
 
@@ -93,7 +100,7 @@ public final class CanIgnoreReturnValueSuggester extends BugChecker implements M
   private final ImmutableSet<String> exemptingMethodAnnotations;
 
   @Inject
-  public CanIgnoreReturnValueSuggester(ErrorProneFlags errorProneFlags) {
+  CanIgnoreReturnValueSuggester(ErrorProneFlags errorProneFlags) {
     this.exemptingMethodAnnotations =
         Sets.union(
                 EXEMPTING_METHOD_ANNOTATIONS,
@@ -107,7 +114,7 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
 
     // If the method has an exempting annotation, then bail out.
     if (exemptingMethodAnnotations.stream()
-        .anyMatch(annotation -> ASTHelpers.hasAnnotation(methodSymbol, annotation, state))) {
+        .anyMatch(annotation -> hasAnnotation(methodSymbol, annotation, state))) {
       return Description.NO_MATCH;
     }
 
@@ -148,8 +155,8 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    // skip @AutoValue and @AutoBuilder methods
-    if (isAbstractAutoValueOrAutoBuilderMethod(methodSymbol, state)) {
+    // skip builder setter methods defined on "known safe" types
+    if (isKnownSafeBuilderInterface(methodSymbol, state)) {
       return Description.NO_MATCH;
     }
 
@@ -179,14 +186,17 @@ private Description annotateWithCanIgnoreReturnValue(MethodTree methodTree, Visi
     return describeMatch(methodTree, fix.build());
   }
 
-  private static boolean isAbstractAutoValueOrAutoBuilderMethod(
+  private static boolean isKnownSafeBuilderInterface(
       MethodSymbol methodSymbol, VisitorState state) {
-    Symbol owner = methodSymbol.owner;
-    // TODO(kak): use ResultEvaluator instead of duplicating _some_ of the logic (right now we only
-    // exclude @AutoValue.Builder's and @AutoBuilder's)
-    return isAbstract(methodSymbol)
-        && (hasAnnotation(owner, AUTO_VALUE + ".Builder", state)
-            || hasAnnotation(owner, "com.google.auto.value.AutoBuilder", state));
+    // TODO(kak): use ResultEvaluator instead of duplicating logic
+    if (isAbstract(methodSymbol)) {
+      for (String annotation : EXEMPTING_CLASS_ANNOTATIONS) {
+        if (hasAnnotation(methodSymbol.owner, annotation, state)) {
+          return true;
+        }
+      }
+    }
+    return false;
   }
 
   private static boolean classLooksLikeBuilder(Symbol owner, VisitorState state) {

diff --git a/...com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java b/...com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java
@@ -885,18 +885,7 @@ public void daggerComponentBuilder_b318407972() {
             "  Builder setName(String name);",
             "  String build();",
             "}")
-        .addOutputLines(
-            "Builder.java",
-            "package com.google.frobber;",
-            "import com.google.errorprone.annotations.CanIgnoreReturnValue;",
-            "import dagger.Component;",
-            "@Component.Builder",
-            "interface Builder {",
-            // TODO(b/318407972): we shouldn't suggest @CIRV on Dagger Component.Builder setters
-            "  @CanIgnoreReturnValue",
-            "  Builder setName(String name);",
-            "  String build();",
-            "}")
+        .expectUnchanged()
         .doTest();
   }
 
@@ -912,18 +901,7 @@ public void daggerSubcomponentBuilder_b318407972() {
             "  Builder setName(String name);",
             "  String build();",
             "}")
-        .addOutputLines(
-            "Builder.java",
-            "package com.google.frobber;",
-            "import com.google.errorprone.annotations.CanIgnoreReturnValue;",
-            "import dagger.Subcomponent;",
-            "@Subcomponent.Builder",
-            "interface Builder {",
-            // TODO(b/318407972): we shouldn't suggest @CIRV on Dagger Subcomponent.Builder setters
-            "  @CanIgnoreReturnValue",
-            "  Builder setName(String name);",
-            "  String build();",
-            "}")
+        .expectUnchanged()
         .doTest();
   }
 }