Skip to content

Commit

Permalink
PUBLIC: Implicitly treat @AutoBuilder setter methods as `@CanIgnore…
Browse files Browse the repository at this point in the history 
…ReturnValue`.

#checkreturnvalue

RELNOTES=n/a
PiperOrigin-RevId: 441508644
  • Loading branch information
@kluever
kluever authored and Error Prone Team committed Apr 13, 2022
1 parent 1636144 commit bb5862c
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 12 deletions.
31 changes: 19 additions & 12 deletions check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java
View file
Expand Up @@ -81,8 +81,11 @@ public final class UnusedReturnValueMatcher implements Matcher<ExpressionTree> {
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<AllowReason> DISALLOW_EXCEPTION_TESTING =
Sets.immutableEnumSet(
Expand Down Expand Up @@ -155,13 +158,11 @@ public Stream<AllowReason> 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;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
}
}
7 changes: 7 additions & 0 deletions core/pom.xml
View file
Expand Up @@ -105,6 +105,13 @@
<version>${autovalue.version}</version>
<scope>compile</scope>
</dependency>
<dependency>
<!-- Apache 2.0 -->
<groupId>com.google.auto.value</groupId>
<artifactId>auto-value</artifactId>
<version>${autovalue.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<!-- Apache 2.0 -->
<groupId>com.google.errorprone</groupId>
Expand Down
87 changes: 87 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit bb5862c

Please sign in to comment.