Internal Change.

PiperOrigin-RevId: 479604041
google · Oct 7, 2022 · 47f34a9 · 47f34a9
1 parent 59eda39
commit 47f34a9
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 25 deletions.
diff --git 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<String> 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<String> 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<String> presentCrvRelevantAnnotations(Symbol symbol) {
+    return MUTUALLY_EXCLUSIVE_ANNOTATIONS.stream()
+        .filter(a -> hasDirectAnnotationWithSimpleName(symbol, a))
+        .collect(toImmutableList());
+  }
+
+  private static String conflictingAnnotations(List<String> 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<String> 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
@@ -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();