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 b330a796cb6..afc18d1632d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/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 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 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(); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java index 0a69ca162bd..0b7697db610 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/dagger/ProvidesNull.java +++ b/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 any 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; - } } diff --git a/core/src/main/java/com/google/errorprone/util/ASTHelpers.java b/core/src/main/java/com/google/errorprone/util/ASTHelpers.java index a78ec263b81..847eb3b042f 100644 --- a/core/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/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. * 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 ffd37fdaabc..a2453191357 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java +++ b/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(); diff --git a/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java b/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java index 298771db310..75907bc05b6 100644 --- a/core/src/test/java/com/google/errorprone/util/ASTHelpersTest.java +++ b/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 {