From bb5862c8a8eca0af0777fbb4a43179b5deca5536 Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Wed, 13 Apr 2022 09:55:45 -0700 Subject: [PATCH] PUBLIC: Implicitly treat `@AutoBuilder` setter methods as `@CanIgnoreReturnValue`. #checkreturnvalue RELNOTES=n/a PiperOrigin-RevId: 441508644 --- .../matchers/UnusedReturnValueMatcher.java | 31 ++++--- core/pom.xml | 7 ++ .../bugpatterns/CheckReturnValueTest.java | 87 +++++++++++++++++++ 3 files changed, 113 insertions(+), 12 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java b/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java index be872b489cd..34222fa5f41 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java @@ -81,8 +81,11 @@ public final class UnusedReturnValueMatcher implements Matcher { UnusedReturnValueMatcher::exceptionTesting, AllowReason.RETURNS_JAVA_LANG_VOID, UnusedReturnValueMatcher::returnsJavaLangVoid, - AllowReason.AUTO_VALUE_BUILDER_SETTER, - UnusedReturnValueMatcher::isAutoValueBuilderSetter); + // TODO(kak): this exclusion really doesn't belong here, since the context of the calling + // code doesn't matter; known builder setters are _always_ treated as CIRV, and the + // surrounding context doesn't matter. + AllowReason.KNOWN_BUILDER_SETTER, + UnusedReturnValueMatcher::isKnownBuilderSetter); private static final ImmutableSet DISALLOW_EXCEPTION_TESTING = Sets.immutableEnumSet( @@ -155,13 +158,11 @@ public Stream getAllowReasons(ExpressionTree tree, VisitorState sta .filter(reason -> ALLOW_MATCHERS.get(reason).matches(tree, state)); } - private static final String AUTO_VALUE_BUILDER = "com.google.auto.value.AutoValue.Builder"; - /** - * Returns {@code true} if the given tree is a method call to an abstract AutoValue Builder setter - * method. + * Returns {@code true} if the given tree is a method call to an abstract setter method inside of + * a known builder class. */ - private static boolean isAutoValueBuilderSetter(ExpressionTree tree, VisitorState state) { + private static boolean isKnownBuilderSetter(ExpressionTree tree, VisitorState state) { Symbol symbol = getSymbol(tree); if (!(symbol instanceof MethodSymbol)) { return false; @@ -175,12 +176,18 @@ private static boolean isAutoValueBuilderSetter(ExpressionTree tree, VisitorStat MethodSymbol method = (MethodSymbol) symbol; ClassSymbol enclosingClass = method.enclClass(); - // If the enclosing class is not an @AutoValue.Builder, return false. - if (!hasAnnotation(enclosingClass, AUTO_VALUE_BUILDER, state)) { + // Setters always have exactly 1 param + if (method.getParameters().size() != 1) { + return false; + } + + // If the enclosing class is not a known builder type, return false. + if (!hasAnnotation(enclosingClass, "com.google.auto.value.AutoValue.Builder", state) + && !hasAnnotation(enclosingClass, "com.google.auto.value.AutoBuilder", state)) { return false; } - // If the method return type is not the same as the enclosing type (the AutoValue Builder), + // If the method return type is not the same as the enclosing type (the builder itself), // e.g., the abstract `build()` method on the Builder, return false. if (!isSameType(method.getReturnType(), enclosingClass.asType(), state)) { return false; @@ -321,7 +328,7 @@ public enum AllowReason { MOCKING_CALL, /** The method returns {@code java.lang.Void} at this use-site. */ RETURNS_JAVA_LANG_VOID, - /** The method is an AutoValue Builder setter method. */ - AUTO_VALUE_BUILDER_SETTER + /** The method is a known Builder setter method (that always returns "this"). */ + KNOWN_BUILDER_SETTER, } } diff --git a/core/pom.xml b/core/pom.xml index 6046c211042..813e137579f 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -105,6 +105,13 @@ ${autovalue.version} compile + + + com.google.auto.value + auto-value + ${autovalue.version} + test + com.google.errorprone diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java index adbbc9b7a5b..65f5f6cf2b6 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import com.google.auto.value.processor.AutoBuilderProcessor; import com.google.auto.value.processor.AutoValueProcessor; import com.google.common.collect.ImmutableList; import com.google.errorprone.CompilationTestHelper; @@ -912,6 +913,92 @@ public void testAutoValueBuilderSetterMethods() { .doTest(); } + @Test + public void testAutoBuilderSetterMethods() { + compilationHelper + .addSourceLines( + "Person.java", + "package com.google.frobber;", + "public final class Person {", + " public Person(String name, int id) {}", + "}") + .addSourceLines( + "PersonBuilder.java", + "package com.google.frobber;", + "import com.google.auto.value.AutoBuilder;", + "import com.google.errorprone.annotations.CheckReturnValue;", + "@CheckReturnValue", + "@AutoBuilder(ofClass = Person.class)", + "abstract class PersonBuilder {", + " static PersonBuilder personBuilder() {", + " return new AutoBuilder_PersonBuilder();", + " }", + " abstract PersonBuilder setName(String name);", + " abstract PersonBuilder setId(int id);", + " abstract Person build();", + "}") + .addSourceLines( + "PersonCaller.java", + "package com.google.frobber;", + "public final class PersonCaller {", + " static void testPersonBuilder() {", + " // BUG: Diagnostic contains: Ignored return value of 'personBuilder'", + " PersonBuilder.personBuilder();", + " PersonBuilder builder = PersonBuilder.personBuilder();", + " builder.setName(\"kurt\");", // AutoBuilder setters are implicitly @CIRV + " builder.setId(42);", // AutoBuilder setters are implicitly @CIRV + " // BUG: Diagnostic contains: Ignored return value of 'build'", + " builder.build();", + " }", + "}") + .setArgs(ImmutableList.of("-processor", AutoBuilderProcessor.class.getName())) + .doTest(); + } + + @Test + public void testAutoBuilderSetterMethods_withInterface() { + compilationHelper + .addSourceLines( + "LogUtil.java", + "package com.google.frobber;", + "import java.util.logging.Level;", + "public class LogUtil {", + " public static void log(Level severity, String message) {}", + "}") + .addSourceLines( + "Caller.java", + "package com.google.frobber;", + "import com.google.auto.value.AutoBuilder;", + "import java.util.logging.Level;", + "import com.google.errorprone.annotations.CheckReturnValue;", + "@CheckReturnValue", + "@AutoBuilder(callMethod = \"log\", ofClass = LogUtil.class)", + "public interface Caller {", + " static Caller logCaller() {", + " return new AutoBuilder_Caller();", + " }", + " Caller setSeverity(Level level);", + " Caller setMessage(String message);", + " void call(); // calls: LogUtil.log(severity, message)", + "}") + .addSourceLines( + "LogCaller.java", + "package com.google.frobber;", + "import java.util.logging.Level;", + "public final class LogCaller {", + " static void testLogCaller() {", + " // BUG: Diagnostic contains: Ignored return value of 'logCaller'", + " Caller.logCaller();", + " Caller caller = Caller.logCaller();", + " caller.setMessage(\"hi\");", // AutoBuilder setters are implicitly @CIRV + " caller.setSeverity(Level.FINE);", // AutoBuilder setters are implicitly @CIRV + " caller.call();", + " }", + "}") + .setArgs(ImmutableList.of("-processor", AutoBuilderProcessor.class.getName())) + .doTest(); + } + private CompilationTestHelper compilationHelperLookingAtAllConstructors() { return compilationHelper.setArgs( "-XepOpt:" + CheckReturnValue.CHECK_ALL_CONSTRUCTORS + "=true");