Make CheckReturnValue match on any annotation with the simple name "C…

…heckReturnValue"

This is a revised version of PR #435.

Fixes #422

RELNOTES: CheckReturnValue checker now respects any annotation
named "CheckReturnValue," not just the JSR 305 one.

MOE_MIGRATED_REVID=128535032

core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java

@@ -30,6 +30,7 @@
 import static com.google.errorprone.util.ASTHelpers.enclosingClass;
 import static com.google.errorprone.util.ASTHelpers.enclosingPackage;
 import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
+import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
 
 import com.google.common.base.Optional;
 import com.google.errorprone.BugPattern;
@@ -67,11 +68,13 @@
 public class CheckReturnValue extends AbstractReturnValueIgnored
     implements MethodTreeMatcher, ClassTreeMatcher {
 
+  private static final String CHECK_RETURN_VALUE = "CheckReturnValue";
+
   private static Optional<Boolean> shouldCheckReturnValue(Symbol sym, VisitorState state) {
     if (hasAnnotation(sym, CanIgnoreReturnValue.class, state)) {
       return Optional.of(false);
     }
-    if (hasAnnotation(sym, javax.annotation.CheckReturnValue.class, state)) {
+    if (hasDirectAnnotationWithSimpleName(sym, CHECK_RETURN_VALUE)) {
       return Optional.of(true);
     }
     return Optional.absent();
@@ -171,7 +174,7 @@ public Matcher<MethodInvocationTree> specializedMatcher() {
   public Description matchMethod(MethodTree tree, VisitorState state) {
     MethodSymbol method = ASTHelpers.getSymbol(tree);
 
-    boolean checkReturn = hasAnnotation(method, javax.annotation.CheckReturnValue.class, state);
+    boolean checkReturn = hasDirectAnnotationWithSimpleName(method, CHECK_RETURN_VALUE);
     boolean canIgnore = hasAnnotation(method, CanIgnoreReturnValue.class, state);
 
     if (checkReturn && canIgnore) {
@@ -204,7 +207,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
    */
   @Override
   public Description matchClass(ClassTree tree, VisitorState state) {
-    if (hasAnnotation(tree, javax.annotation.CheckReturnValue.class, state)
+    if (hasDirectAnnotationWithSimpleName(ASTHelpers.getSymbol(tree), CHECK_RETURN_VALUE)
         && hasAnnotation(tree, CanIgnoreReturnValue.class, state)) {
       return buildDescription(tree).setMessage(String.format(BOTH_ERROR, "class")).build();
     }

core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java

@@ -36,8 +36,6 @@
 import com.sun.source.tree.Tree.Kind;
 import com.sun.source.util.TreePath;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
-import javax.lang.model.element.AnnotationMirror;
-
 /**
  * Bug checker for null-returning methods annotated with {@code @Provides} but not
  * {@code @Nullable}.
@@ -88,7 +86,7 @@ public Description matchReturn(ReturnTree returnTree, VisitorState state) {
     }
 
     if (!ASTHelpers.hasAnnotation(enclosingMethodSym, "dagger.Provides", state)
-        || hasAnyNullableAnnotation(enclosingMethodSym)) {
+        || ASTHelpers.hasDirectAnnotationWithSimpleName(enclosingMethodSym, "Nullable")) {
       return Description.NO_MATCH;
     }
 
@@ -118,17 +116,4 @@ public Description matchReturn(ReturnTree returnTree, VisitorState state) {
           .build();
     }
   }
-
-  /**
-   * Returns true iff this method is directly annotated with <em>any</em> annotation with the simple
-   * name "Nullable".
-   */
-  private static boolean hasAnyNullableAnnotation(MethodSymbol methodSym) {
-    for (AnnotationMirror annotation : methodSym.getAnnotationMirrors()) {
-      if (annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable")) {
-        return true;
-      }
-    }
-    return false;
-  }
 }

core/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -84,6 +84,7 @@
 import java.util.List;
 import java.util.Set;
 import javax.annotation.Nullable;
+import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.type.TypeKind;
 
@@ -534,6 +535,23 @@ public static boolean hasAnnotation(
     return hasAnnotation(sym, annotationType.getName(), state);
   }
 
+  /**
+   * Check for the presence of an annotation with a specific simple name directly on this symbol.
+   * Does *not* consider annotation inheritance.
+   *
+   * @param sym the symbol to check for the presence of the annotation
+   * @param simpleName the simple name of the annotation to look for, e.g. "Nullable" or
+   *     "CheckReturnValue"
+   */
+  public static boolean hasDirectAnnotationWithSimpleName(Symbol sym, String simpleName) {
+    for (AnnotationMirror annotation : sym.getAnnotationMirrors()) {
+      if (annotation.getAnnotationType().asElement().getSimpleName().contentEquals(simpleName)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /**
    * Retrieve an annotation, considering annotation inheritance.
    *

core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java

@@ -50,6 +50,30 @@ public void testPositiveCases() throws Exception {
     compilationHelper.addSourceFile("CheckReturnValuePositiveCases.java").doTest();
   }
 
+  @Test
+  public void testCustomCheckReturnValueAnnotation() {
+    compilationHelper
+        .addSourceLines(
+            "foo/bar/CheckReturnValue.java",
+            "package foo.bar;",
+            "public @interface CheckReturnValue {}")
+        .addSourceLines(
+            "test/TestCustomCheckReturnValueAnnotation.java",
+            "package test;",
+            "import foo.bar.CheckReturnValue;",
+            "public class TestCustomCheckReturnValueAnnotation {",
+            "  @CheckReturnValue",
+            "  public String getString() {",
+            "    return \"string\";",
+            "  }",
+            "  public void doIt() {",
+            "    // BUG: Diagnostic contains:",
+            "    getString();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testNegativeCase() throws Exception {
     compilationHelper.addSourceFile("CheckReturnValueNegativeCases.java").doTest();

core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java

@@ -22,8 +22,17 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Verify;
+import com.google.common.io.ByteStreams;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.Category;
+import com.google.errorprone.BugPattern.MaturityLevel;
+import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.matchers.CompilerBasedAbstractTest;
+import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.scanner.Scanner;
@@ -33,21 +42,35 @@
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.LiteralTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.ReturnTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
+import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Type.TypeVar;
 import com.sun.tools.javac.parser.Tokens.Comment;
 import com.sun.tools.javac.tree.JCTree.JCLiteral;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
 import org.junit.After;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
+/** Tests for {@link ASTHelpers}. */
 @RunWith(JUnit4.class)
 public class ASTHelpersTest extends CompilerBasedAbstractTest {
 
@@ -496,7 +519,7 @@ public Void visitMethodInvocation(MethodInvocationTree tree, VisitorState state)
     tests.add(scanner);
     assertCompiles(scanner);
   }
-  
+
   @Test
   public void testCommentTokens() {
     writeFile(
@@ -544,6 +567,97 @@ public Void visitNewClass(NewClassTree tree, VisitorState state) {
     assertCompiles(scanner);
   }
 
+  @Test
+  public void testHasDirectAnnotationWithSimpleName() {
+    writeFile(
+        "A.java", //
+        "public class A {",
+        "  @Deprecated public void doIt() {}",
+        "}");
+    TestScanner scanner =
+        new TestScanner() {
+          @Override
+          public Void visitMethod(MethodTree tree, VisitorState state) {
+            if (tree.getName().contentEquals("doIt")) {
+              setAssertionsComplete();
+              Symbol sym = ASTHelpers.getSymbol(tree);
+              assertThat(ASTHelpers.hasDirectAnnotationWithSimpleName(sym, "Deprecated")).isTrue();
+              assertThat(ASTHelpers.hasDirectAnnotationWithSimpleName(sym, "Nullable")).isFalse();
+            }
+            return super.visitMethod(tree, state);
+          }
+        };
+    tests.add(scanner);
+    assertCompiles(scanner);
+  }
+
+  @BugPattern(
+    name = "HasDirectAnnotationWithSimpleNameChecker",
+    category = Category.ONE_OFF,
+    maturity = MaturityLevel.MATURE,
+    severity = SeverityLevel.ERROR,
+    summary =
+        "Test checker to ensure that ASTHelpers.hasDirectAnnotationWithSimpleName() "
+            + "does require the annotation symbol to be on the classpath"
+  )
+  public static class HasDirectAnnotationWithSimpleNameChecker extends BugChecker
+      implements MethodInvocationTreeMatcher {
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+      if (ASTHelpers.hasDirectAnnotationWithSimpleName(
+          ASTHelpers.getSymbol(tree), "CheckReturnValue")) {
+        return describeMatch(tree);
+      }
+      return Description.NO_MATCH;
+    }
+  }
+
+  /** Test class containing a method annotated with a custom @CheckReturnValue. */
+  public static class CustomCRVTest {
+    /** A custom @CRV annotation. */
+    @Retention(RetentionPolicy.RUNTIME)
+    public @interface CheckReturnValue {}
+
+    @CheckReturnValue
+    public static String hello() {
+      return "Hello!";
+    }
+  }
+
+  @Rule public final TemporaryFolder tempFolder = new TemporaryFolder();
+
+  static void addClassToJar(JarOutputStream jos, Class<?> clazz) throws IOException {
+    String entryPath = clazz.getName().replace('.', '/') + ".class";
+    try (InputStream is = clazz.getClassLoader().getResourceAsStream(entryPath)) {
+      jos.putNextEntry(new JarEntry(entryPath));
+      ByteStreams.copy(is, jos);
+    }
+  }
+
+  @Test
+  public void testHasDirectAnnotationWithSimpleNameWithoutAnnotationOnClasspath()
+      throws IOException {
+    File libJar = tempFolder.newFile("lib.jar");
+    try (FileOutputStream fis = new FileOutputStream(libJar);
+        JarOutputStream jos = new JarOutputStream(fis)) {
+      addClassToJar(jos, CustomCRVTest.class);
+      addClassToJar(jos, ASTHelpersTest.class);
+      addClassToJar(jos, CompilerBasedAbstractTest.class);
+    }
+
+    CompilationTestHelper.newInstance(HasDirectAnnotationWithSimpleNameChecker.class, getClass())
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void m() {",
+            "    // BUG: Diagnostic contains:",
+            "    com.google.errorprone.util.ASTHelpersTest.CustomCRVTest.hello();",
+            "  }",
+            "}")
+        .setArgs(Arrays.asList("-cp", libJar.toString()))
+        .doTest();
+  }
+
   /* Test infrastructure */
 
   private static abstract class TestScanner extends Scanner {