From 7f37024762c6a5fecbe53a8623a4a863561af1b9 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 7 Jun 2024 11:46:27 -0700 Subject: [PATCH] Generalize InjectScopeOrQualifierAnnotationRetention to MissingRuntimeRetention PiperOrigin-RevId: 641313551 --- ...tion.java => MissingRuntimeRetention.java} | 104 ++++++++++++------ .../scanner/BuiltInCheckerSuppliers.java | 4 +- ....java => MissingRuntimeRetentionTest.java} | 15 +-- ...MissingRuntimeRetentionNegativeCases.java} | 2 +- ...MissingRuntimeRetentionPositiveCases.java} | 2 +- ...etention.md => MissingRuntimeRetention.md} | 0 6 files changed, 77 insertions(+), 50 deletions(-) rename core/src/main/java/com/google/errorprone/bugpatterns/inject/{ScopeOrQualifierAnnotationRetention.java => MissingRuntimeRetention.java} (56%) rename core/src/test/java/com/google/errorprone/bugpatterns/inject/{ScopeOrQualifierAnnotationRetentionTest.java => MissingRuntimeRetentionTest.java} (87%) rename core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/{ScopeOrQualifierAnnotationRetentionNegativeCases.java => MissingRuntimeRetentionNegativeCases.java} (97%) rename core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/{ScopeOrQualifierAnnotationRetentionPositiveCases.java => MissingRuntimeRetentionPositiveCases.java} (97%) rename docs/bugpattern/{InjectScopeOrQualifierAnnotationRetention.md => MissingRuntimeRetention.md} (100%) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetention.java b/core/src/main/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetention.java similarity index 56% rename from core/src/main/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetention.java rename to core/src/main/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetention.java index 2e6165a847d..924f0bb9176 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetention.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetention.java @@ -14,9 +14,11 @@ package com.google.errorprone.bugpatterns.inject; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.bugpatterns.inject.ElementPredicates.doesNotHaveRuntimeRetention; import static com.google.errorprone.bugpatterns.inject.ElementPredicates.hasSourceRetention; +import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.InjectMatchers.DAGGER_MAP_KEY_ANNOTATION; import static com.google.errorprone.matchers.InjectMatchers.GUICE_BINDING_ANNOTATION; import static com.google.errorprone.matchers.InjectMatchers.GUICE_MAP_KEY_ANNOTATION; @@ -24,12 +26,13 @@ import static com.google.errorprone.matchers.InjectMatchers.JAVAX_QUALIFIER_ANNOTATION; import static com.google.errorprone.matchers.InjectMatchers.JAVAX_SCOPE_ANNOTATION; import static com.google.errorprone.matchers.Matchers.allOf; -import static com.google.errorprone.matchers.Matchers.anyOf; -import static com.google.errorprone.matchers.Matchers.hasAnnotation; -import static com.google.errorprone.matchers.Matchers.kindIs; +import static com.google.errorprone.util.ASTHelpers.annotationsAmong; +import static com.google.errorprone.util.ASTHelpers.getDeclaredSymbol; import static com.sun.source.tree.Tree.Kind.ANNOTATION_TYPE; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -37,62 +40,91 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.InjectMatchers; -import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.util.Name; import java.lang.annotation.Retention; +import java.util.Collections; +import java.util.Set; +import java.util.stream.Stream; import javax.annotation.Nullable; -// TODO(b/180081278): Rename this check to MissingRuntimeRetention /** * @author sgoldfeder@google.com (Steven Goldfeder) */ @BugPattern( - name = "InjectScopeOrQualifierAnnotationRetention", + name = "MissingRuntimeRetention", + altNames = "InjectScopeOrQualifierAnnotationRetention", summary = "Scoping and qualifier annotations must have runtime retention.", severity = ERROR) -public class ScopeOrQualifierAnnotationRetention extends BugChecker implements ClassTreeMatcher { +public class MissingRuntimeRetention extends BugChecker implements ClassTreeMatcher { private static final String RETENTION_ANNOTATION = "java.lang.annotation.Retention"; - /** Matches classes that are qualifier, scope annotation or map binding keys. */ - private static final Matcher SCOPE_OR_QUALIFIER_ANNOTATION_MATCHER = - allOf( - kindIs(ANNOTATION_TYPE), - anyOf( - hasAnnotation(GUICE_SCOPE_ANNOTATION), - hasAnnotation(JAVAX_SCOPE_ANNOTATION), - hasAnnotation(GUICE_BINDING_ANNOTATION), - hasAnnotation(JAVAX_QUALIFIER_ANNOTATION), - hasAnnotation(GUICE_MAP_KEY_ANNOTATION), - hasAnnotation(DAGGER_MAP_KEY_ANNOTATION))); + private static final Supplier> INJECT_ANNOTATIONS = + VisitorState.memoize( + state -> + Stream.of( + GUICE_SCOPE_ANNOTATION, + JAVAX_SCOPE_ANNOTATION, + GUICE_BINDING_ANNOTATION, + JAVAX_QUALIFIER_ANNOTATION, + GUICE_MAP_KEY_ANNOTATION, + DAGGER_MAP_KEY_ANNOTATION) + .map(state::binaryNameFromClassname) + .collect(toImmutableSet())); + + private static final Supplier> ANNOTATIONS = + VisitorState.memoize( + state -> + Streams.concat( + Stream.of("com.google.apps.framework.annotations.ProcessorAnnotation") + .map(state::binaryNameFromClassname), + INJECT_ANNOTATIONS.get(state).stream()) + .collect(toImmutableSet())); @Override public final Description matchClass(ClassTree classTree, VisitorState state) { - if (SCOPE_OR_QUALIFIER_ANNOTATION_MATCHER.matches(classTree, state)) { - ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); - if (hasSourceRetention(classSymbol)) { - return describe(classTree, state, ASTHelpers.getAnnotation(classSymbol, Retention.class)); - } + if (!classTree.getKind().equals(ANNOTATION_TYPE)) { + return NO_MATCH; + } + Set annotations = + annotationsAmong(getDeclaredSymbol(classTree), ANNOTATIONS.get(state), state); + if (annotations.isEmpty()) { + return NO_MATCH; + } + ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); + if (!doesNotHaveRuntimeRetention(classSymbol)) { + return NO_MATCH; + } + if (!Collections.disjoint(annotations, INJECT_ANNOTATIONS.get(state)) + && exemptInjectAnnotation(state, classSymbol)) { + return NO_MATCH; + } + return describe(classTree, state, ASTHelpers.getAnnotation(classSymbol, Retention.class)); + } - // TODO(glorioso): This is a poor hack to exclude android apps that are more likely to not - // have reflective DI than normal java. JSR spec still says the annotations should be - // runtime-retained, but this reduces the instances that are flagged. - if (!state.isAndroidCompatible() && doesNotHaveRuntimeRetention(classSymbol)) { - // Is this in a dagger component? - ClassTree outer = ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class); - if (outer != null - && allOf(InjectMatchers.IS_DAGGER_COMPONENT_OR_MODULE).matches(outer, state)) { - return Description.NO_MATCH; - } - return describe(classTree, state, ASTHelpers.getAnnotation(classSymbol, Retention.class)); - } + private static boolean exemptInjectAnnotation(VisitorState state, ClassSymbol classSymbol) { + // TODO(glorioso): This is a poor hack to exclude android apps that are more likely to not + // have reflective DI than normal java. JSR spec still says the annotations should be + // runtime-retained, but this reduces the instances that are flagged. + if (hasSourceRetention(classSymbol)) { + return false; + } + if (state.isAndroidCompatible()) { + return true; + } + // Is this in a dagger component? + ClassTree outer = ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class); + if (outer != null + && allOf(InjectMatchers.IS_DAGGER_COMPONENT_OR_MODULE).matches(outer, state)) { + return true; } - return Description.NO_MATCH; + return false; } private Description describe( diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 8929178147f..23a64368749 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -499,6 +499,7 @@ import com.google.errorprone.bugpatterns.inject.JavaxInjectOnAbstractMethod; import com.google.errorprone.bugpatterns.inject.JavaxInjectOnFinalField; import com.google.errorprone.bugpatterns.inject.MisplacedScopeAnnotations; +import com.google.errorprone.bugpatterns.inject.MissingRuntimeRetention; import com.google.errorprone.bugpatterns.inject.MoreThanOneInjectableConstructor; import com.google.errorprone.bugpatterns.inject.MoreThanOneQualifier; import com.google.errorprone.bugpatterns.inject.MoreThanOneScopeAnnotationOnClass; @@ -506,7 +507,6 @@ import com.google.errorprone.bugpatterns.inject.QualifierOrScopeOnInjectMethod; import com.google.errorprone.bugpatterns.inject.QualifierWithTypeUse; import com.google.errorprone.bugpatterns.inject.ScopeAnnotationOnInterfaceOrAbstractClass; -import com.google.errorprone.bugpatterns.inject.ScopeOrQualifierAnnotationRetention; import com.google.errorprone.bugpatterns.inject.dagger.AndroidInjectionBeforeSuper; import com.google.errorprone.bugpatterns.inject.dagger.EmptySetMultibindingContributions; import com.google.errorprone.bugpatterns.inject.dagger.PrivateConstructorForNoninstantiableModule; @@ -1179,6 +1179,7 @@ public static ScannerSupplier warningChecks() { MethodCanBeStatic.class, MissingBraces.class, MissingDefault.class, + MissingRuntimeRetention.class, MixedArrayDimensions.class, MockitoDoSetup.class, MoreThanOneQualifier.class, @@ -1205,7 +1206,6 @@ public static ScannerSupplier warningChecks() { ReturnMissingNullable.class, ReturnsNullCollection.class, ScopeOnModule.class, - ScopeOrQualifierAnnotationRetention.class, StaticOrDefaultInterfaceMethod.class, StaticQualifiedUsingExpression.class, StringFormatWithLiteral.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetentionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetentionTest.java similarity index 87% rename from core/src/test/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetentionTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetentionTest.java index e4a1d05d8e0..3a0c3f0a9eb 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inject/ScopeOrQualifierAnnotationRetentionTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inject/MissingRuntimeRetentionTest.java @@ -27,27 +27,22 @@ * @author sgoldfeder@google.com (Steven Goldfeder) */ @RunWith(JUnit4.class) -public class ScopeOrQualifierAnnotationRetentionTest { +public class MissingRuntimeRetentionTest { private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance( - ScopeOrQualifierAnnotationRetention.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(MissingRuntimeRetention.class, getClass()); private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(ScopeOrQualifierAnnotationRetention.class, getClass()); + CompilationTestHelper.newInstance(MissingRuntimeRetention.class, getClass()); @Test public void positiveCase() { - compilationHelper - .addSourceFile("ScopeOrQualifierAnnotationRetentionPositiveCases.java") - .doTest(); + compilationHelper.addSourceFile("MissingRuntimeRetentionPositiveCases.java").doTest(); } @Test public void negativeCase() { - compilationHelper - .addSourceFile("ScopeOrQualifierAnnotationRetentionNegativeCases.java") - .doTest(); + compilationHelper.addSourceFile("MissingRuntimeRetentionNegativeCases.java").doTest(); } @Test diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionNegativeCases.java similarity index 97% rename from core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionNegativeCases.java rename to core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionNegativeCases.java index aef6a2ca962..1bd1409d3cf 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionNegativeCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionNegativeCases.java @@ -31,7 +31,7 @@ /** * @author sgoldfeder@google.com (Steven Goldfeder) */ -public class ScopeOrQualifierAnnotationRetentionNegativeCases { +public class MissingRuntimeRetentionNegativeCases { /** A scoping (@Scope) annotation with runtime retention */ @Scope @Target({TYPE, METHOD}) diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionPositiveCases.java similarity index 97% rename from core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionPositiveCases.java rename to core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionPositiveCases.java index 4a6da5a0291..574b4d6fbd8 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/ScopeOrQualifierAnnotationRetentionPositiveCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MissingRuntimeRetentionPositiveCases.java @@ -30,7 +30,7 @@ /** * @author sgoldfeder@google.com (Steven Goldfeder) */ -public class ScopeOrQualifierAnnotationRetentionPositiveCases { +public class MissingRuntimeRetentionPositiveCases { /** A scoping (@Scope) annotation with SOURCE retention */ @Scope @Target({TYPE, METHOD}) diff --git a/docs/bugpattern/InjectScopeOrQualifierAnnotationRetention.md b/docs/bugpattern/MissingRuntimeRetention.md similarity index 100% rename from docs/bugpattern/InjectScopeOrQualifierAnnotationRetention.md rename to docs/bugpattern/MissingRuntimeRetention.md