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 fc98d56a252..e99d1f227b3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.bugpatterns.CheckReturnValue.MessageTrailerStyle.NONE; import static com.google.errorprone.bugpatterns.checkreturnvalue.AutoValueRules.autoBuilders; @@ -40,7 +41,9 @@ import static com.google.errorprone.util.ASTHelpers.isGeneratedConstructor; import static com.google.errorprone.util.ASTHelpers.isLocal; import static com.sun.source.tree.Tree.Kind.METHOD; +import static java.util.stream.Collectors.joining; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; @@ -70,6 +73,7 @@ import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.processing.JavacProcessingEnvironment; +import java.util.List; import java.util.Optional; import javax.lang.model.element.ElementKind; import org.checkerframework.checker.nullness.qual.Nullable; @@ -86,6 +90,8 @@ public class CheckReturnValue extends AbstractReturnValueIgnored private static final String CHECK_RETURN_VALUE = "CheckReturnValue"; private static final String CAN_IGNORE_RETURN_VALUE = "CanIgnoreReturnValue"; + private static final ImmutableList MUTUALLY_EXCLUSIVE_ANNOTATIONS = + ImmutableList.of(CHECK_RETURN_VALUE, CAN_IGNORE_RETURN_VALUE); public static final String CHECK_ALL_CONSTRUCTORS = "CheckReturnValue:CheckAllConstructors"; public static final String CHECK_ALL_METHODS = "CheckReturnValue:CheckAllMethods"; @@ -206,9 +212,6 @@ public boolean isCovered(ExpressionTree tree, VisitorState state) { .orElse(ImmutableMap.of()); } - private static final String BOTH_ERROR = - "@CheckReturnValue and @CanIgnoreReturnValue cannot both be applied to the same %s"; - /** * Validate {@code @CheckReturnValue} and {@link CanIgnoreReturnValue} usage on methods. * @@ -220,24 +223,20 @@ public boolean isCovered(ExpressionTree tree, VisitorState state) { @Override public Description matchMethod(MethodTree tree, VisitorState state) { MethodSymbol method = ASTHelpers.getSymbol(tree); - - boolean checkReturn = hasDirectAnnotationWithSimpleName(method, CHECK_RETURN_VALUE); - boolean canIgnore = hasDirectAnnotationWithSimpleName(method, CAN_IGNORE_RETURN_VALUE); - - // TODO(cgdecker): We can check this with evaluator.checkForConflicts now, though I want to - // think more about how we build and format error messages in that. - if (checkReturn && canIgnore) { - return buildDescription(tree).setMessage(String.format(BOTH_ERROR, "method")).build(); + ImmutableList presentAnnotations = presentCrvRelevantAnnotations(method); + switch (presentAnnotations.size()) { + case 0: + return Description.NO_MATCH; + case 1: + break; + default: + // TODO(cgdecker): We can check this with evaluator.checkForConflicts now, though I want to + // think more about how we build and format error messages in that. + return buildDescription(tree) + .setMessage(conflictingAnnotations(presentAnnotations, "method")) + .build(); } - String annotationToValidate; - if (checkReturn) { - annotationToValidate = CHECK_RETURN_VALUE; - } else if (canIgnore) { - annotationToValidate = CAN_IGNORE_RETURN_VALUE; - } else { - return Description.NO_MATCH; - } if (method.getKind() != ElementKind.METHOD) { // skip constructors (which javac thinks are void-returning) return Description.NO_MATCH; @@ -246,20 +245,36 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } String message = - String.format("@%s may not be applied to void-returning methods", annotationToValidate); + String.format( + "@%s may not be applied to void-returning methods", presentAnnotations.get(0)); return buildDescription(tree).setMessage(message).build(); } + private static ImmutableList presentCrvRelevantAnnotations(Symbol symbol) { + return MUTUALLY_EXCLUSIVE_ANNOTATIONS.stream() + .filter(a -> hasDirectAnnotationWithSimpleName(symbol, a)) + .collect(toImmutableList()); + } + + private static String conflictingAnnotations(List annotations, String targetType) { + return annotations.stream().map(a -> "@" + a).collect(joining(" and ")) + + " cannot be applied to the same " + + targetType; + } + /** * Validate that at most one of {@code CheckReturnValue} and {@code CanIgnoreReturnValue} are * applied to a class (or interface or enum). */ @Override public Description matchClass(ClassTree tree, VisitorState state) { - if (hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CHECK_RETURN_VALUE) - && hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CAN_IGNORE_RETURN_VALUE)) { - return buildDescription(tree).setMessage(String.format(BOTH_ERROR, "class")).build(); + ImmutableList presentAnnotations = presentCrvRelevantAnnotations(getSymbol(tree)); + if (presentAnnotations.size() > 1) { + return buildDescription(tree) + .setMessage(conflictingAnnotations(presentAnnotations, "class")) + .build(); } + return Description.NO_MATCH; } 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 9bcb59a71ba..35d99d4cf05 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java @@ -346,7 +346,7 @@ public void errorBothClass() { "@com.google.errorprone.annotations.CanIgnoreReturnValue", "@com.google.errorprone.annotations.CheckReturnValue", "// BUG: Diagnostic contains: @CheckReturnValue and @CanIgnoreReturnValue cannot" - + " both be applied to the same class", + + " be applied to the same class", "class Test {}") .doTest(); } @@ -360,7 +360,7 @@ public void errorBothMethod() { " @com.google.errorprone.annotations.CanIgnoreReturnValue", " @com.google.errorprone.annotations.CheckReturnValue", " // BUG: Diagnostic contains: @CheckReturnValue and @CanIgnoreReturnValue cannot" - + " both be applied to the same method", + + " be applied to the same method", " void m() {}", "}") .doTest();