Treat Dagger @Component.Builder and @Subcomponent.Builder setter …

…methods as ignorable.

#checkreturnvalue

PiperOrigin-RevId: 595433986

core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java

@@ -22,6 +22,8 @@
 import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoBuilders;
 import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValueBuilders;
 import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoValues;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.DaggerRules.componentBuilders;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.DaggerRules.subcomponentBuilders;
 import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.annotationOnVoid;
 import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.conflictingAnnotations;
 import static com.google.errorprone.bugpatterns.checkreturnvalue.ErrorMessages.invocationResultIgnored;
@@ -166,6 +168,8 @@ public MethodKind getMethodKind(MethodSymbol method) {
                 autoValues(),
                 autoValueBuilders(),
                 autoBuilders(),
+                componentBuilders(),
+                subcomponentBuilders(),
 
                 // This is conceptually lower precedence than the above rules.
                 externalIgnoreList());

core/src/main/java/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) {

core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/DaggerRules.java

@@ -0,0 +1,78 @@
+/*
+ * Copyright 2024 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns.checkreturnvalue;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.ResultUsePolicy.OPTIONAL;
+import static com.google.errorprone.bugpatterns.checkreturnvalue.Rules.returnsEnclosingType;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
+import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.isAbstract;
+
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.checkreturnvalue.Rules.ErrorProneMethodRule;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import java.util.Optional;
+
+/** Rules for methods on Dagger {@code Component.Builder} and {@code Subcomponent.Builder} types. */
+public final class DaggerRules {
+
+  /**
+   * Returns a rule that handles {@code @dagger.Component.Builder} types, making their fluent setter
+   * methods' results ignorable.
+   */
+  public static ResultUseRule<VisitorState, Symbol> componentBuilders() {
+    return new DaggerRule("dagger.Component.Builder");
+  }
+
+  /**
+   * Returns a rule that handles {@code @dagger.Subcomponent.Builder} types, making their fluent
+   * setter methods' results ignorable.
+   */
+  public static ResultUseRule<VisitorState, Symbol> subcomponentBuilders() {
+    return new DaggerRule("dagger.Subcomponent.Builder");
+  }
+
+  /** Rules for methods on Dagger components and subcomponents. */
+  private static final class DaggerRule extends ErrorProneMethodRule {
+
+    private final String annotationName;
+
+    DaggerRule(String annotationName) {
+      this.annotationName = checkNotNull(annotationName);
+    }
+
+    @Override
+    public String id() {
+      return "@" + annotationName;
+    }
+
+    @Override
+    public Optional<ResultUsePolicy> evaluateMethod(MethodSymbol method, VisitorState state) {
+      if (method.getParameters().size() == 1
+          && isAbstract(method)
+          && hasAnnotation(enclosingClass(method), annotationName, state)
+          && returnsEnclosingType(method, state)) {
+        return Optional.of(OPTIONAL);
+      }
+      return Optional.empty();
+    }
+  }
+
+  private DaggerRules() {}
+}

core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java

@@ -17,7 +17,9 @@
 package com.google.errorprone.bugpatterns.checkreturnvalue;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.errorprone.util.ASTHelpers.enclosingClass;
 import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
+import static com.google.errorprone.util.ASTHelpers.isSameType;
 
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.checkreturnvalue.ResultUseRule.GlobalRule;
@@ -31,7 +33,10 @@
 /** Factories for common kinds of {@link ResultUseRule}s. */
 public final class Rules {
 
-  private Rules() {}
+  /** Returns true if {@code method} returns the same type as its enclosing class. */
+  static boolean returnsEnclosingType(MethodSymbol method, VisitorState state) {
+    return isSameType(enclosingClass(method).type, method.getReturnType(), state);
+  }
 
   /** A {@link MethodRule} for Error Prone. */
   abstract static class ErrorProneMethodRule
@@ -109,4 +114,6 @@ public Optional<ResultUsePolicy> evaluate(Symbol symbol, VisitorState context) {
       return symbol.isConstructor() ? constructorDefault : methodDefault;
     }
   }
+
+  private Rules() {}
 }

core/src/test/java/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();
   }
 }

core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java

@@ -616,6 +616,62 @@ public void autoBuilderSetterMethods_withInterface() {
         .doTest();
   }
 
+  @Test
+  public void daggerComponentBuilderSetters() {
+    compilationHelper
+        .addSourceLines(
+            "Builder.java",
+            "package com.google.frobber;",
+            "import com.google.errorprone.annotations.CheckReturnValue;",
+            "import dagger.Component;",
+            "@CheckReturnValue",
+            "@Component.Builder",
+            "interface Builder {",
+            "  Builder setName(String name);",
+            "  String build();",
+            "}")
+        .addSourceLines(
+            "ComponentBuilderCaller.java",
+            "package com.google.frobber;",
+            "public final class ComponentBuilderCaller {",
+            "  static void testSetters(Builder builder) {",
+            // Dagger Component setters are implicitly @CIRV
+            "    builder.setName(\"Kurt\");",
+            "    // BUG: Diagnostic contains: CheckReturnValue",
+            "    builder.build();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void daggerSubcomponentBuilderSetters() {
+    compilationHelper
+        .addSourceLines(
+            "Builder.java",
+            "package com.google.frobber;",
+            "import com.google.errorprone.annotations.CheckReturnValue;",
+            "import dagger.Subcomponent;",
+            "@CheckReturnValue",
+            "@Subcomponent.Builder",
+            "interface Builder {",
+            "  Builder setName(String name);",
+            "  String build();",
+            "}")
+        .addSourceLines(
+            "SubcomponentBuilderCaller.java",
+            "package com.google.frobber;",
+            "public final class SubcomponentBuilderCaller {",
+            "  static void testSetters(Builder builder) {",
+            // Dagger Subcomponent setters are implicitly @CIRV
+            "    builder.setName(\"Kurt\");",
+            "    // BUG: Diagnostic contains: CheckReturnValue",
+            "    builder.build();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper compilationHelperLookingAtAllConstructors() {
     return compilationHelper.setArgs(
         "-XepOpt:" + CheckReturnValue.CHECK_ALL_CONSTRUCTORS + "=true");