From e8cb551502f042f21d11ac014ee0e8b1909b8a7b Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Wed, 3 Jan 2024 10:08:46 -0800 Subject: [PATCH] Treat Dagger `@Component.Builder` and `@Subcomponent.Builder` setter methods as ignorable. #checkreturnvalue PiperOrigin-RevId: 595433986 --- .../bugpatterns/CheckReturnValue.java | 4 + .../CanIgnoreReturnValueSuggester.java | 32 +++++--- .../checkreturnvalue/DaggerRules.java | 78 +++++++++++++++++++ .../bugpatterns/checkreturnvalue/Rules.java | 9 ++- .../CanIgnoreReturnValueSuggesterTest.java | 26 +------ ...heckReturnValueWellKnownLibrariesTest.java | 56 +++++++++++++ 6 files changed, 169 insertions(+), 36 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/DaggerRules.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java index e242bd2bdd9e..2b5ebeea0932 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/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()); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java index 77dacbfeef0b..865c83fe2a5c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggester.java +++ b/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 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 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 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/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/DaggerRules.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/DaggerRules.java new file mode 100644 index 000000000000..13a6c45b4862 --- /dev/null +++ b/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 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 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 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() {} +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java b/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java index 1cd5bd2b5b2e..66745194175d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Rules.java +++ b/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 evaluate(Symbol symbol, VisitorState context) { return symbol.isConstructor() ? constructorDefault : methodDefault; } } + + private Rules() {} } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java index 905313686f24..f7deed364f3e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CanIgnoreReturnValueSuggesterTest.java +++ b/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(); } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java index 522ce8e00fc7..22cb90033323 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/CheckReturnValueWellKnownLibrariesTest.java +++ b/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");