Automated rollback of commit a3fdf64.

*** Reason for rollback *** Requires a large number of fixes throughout the depot. I am not sure that that was intended, but if it was then those fixes should be made separately from the regularly-scheduled JavaBuilder release. A *subset* of the fixes, before I gave up, is here: unknown commit. *** Original change description *** Generalize InjectScopeOrQualifierAnnotationRetention to MissingRuntimeRetention *** PiperOrigin-RevId: 646486667
google · Jun 25, 2024 · 4b7f0e4 · 4b7f0e4
1 parent a2b0ff6
commit 4b7f0e4
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 77 deletions.
diff --git a/...terns/inject/MissingRuntimeRetention.java → .../ScopeOrQualifierAnnotationRetention.java b/...terns/inject/MissingRuntimeRetention.java → .../ScopeOrQualifierAnnotationRetention.java
@@ -14,117 +14,85 @@
 
 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;
 import static com.google.errorprone.matchers.InjectMatchers.GUICE_SCOPE_ANNOTATION;
 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.util.ASTHelpers.annotationsAmong;
-import static com.google.errorprone.util.ASTHelpers.getDeclaredSymbol;
+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.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;
 import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
 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 [email protected] (Steven Goldfeder)
  */
 @BugPattern(
-    name = "MissingRuntimeRetention",
-    altNames = "InjectScopeOrQualifierAnnotationRetention",
+    name = "InjectScopeOrQualifierAnnotationRetention",
     summary = "Scoping and qualifier annotations must have runtime retention.",
     severity = ERROR)
-public class MissingRuntimeRetention extends BugChecker implements ClassTreeMatcher {
+public class ScopeOrQualifierAnnotationRetention extends BugChecker implements ClassTreeMatcher {
 
   private static final String RETENTION_ANNOTATION = "java.lang.annotation.Retention";
 
-  private static final Supplier<ImmutableSet<Name>> 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<ImmutableSet<Name>> ANNOTATIONS =
-      VisitorState.memoize(
-          state ->
-              Streams.concat(
-                      Stream.of("com.google.apps.framework.annotations.ProcessorAnnotation")
-                          .map(state::binaryNameFromClassname),
-                      INJECT_ANNOTATIONS.get(state).stream())
-                  .collect(toImmutableSet()));
+  /** Matches classes that are qualifier, scope annotation or map binding keys. */
+  private static final Matcher<ClassTree> 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)));
 
   @Override
   public final Description matchClass(ClassTree classTree, VisitorState state) {
-    if (!classTree.getKind().equals(ANNOTATION_TYPE)) {
-      return NO_MATCH;
-    }
-    Set<Name> 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));
-  }
+    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));
+      }
 
-  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;
+      // 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));
+      }
     }
-    return false;
+    return Description.NO_MATCH;
   }
 
   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
@@ -499,14 +499,14 @@
 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;
 import com.google.errorprone.bugpatterns.inject.OverlappingQualifierAndScopeAnnotation;
 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,7 +1179,6 @@ public static ScannerSupplier warningChecks() {
           MethodCanBeStatic.class,
           MissingBraces.class,
           MissingDefault.class,
-          MissingRuntimeRetention.class,
           MixedArrayDimensions.class,
           MockitoDoSetup.class,
           MoreThanOneQualifier.class,
@@ -1206,6 +1205,7 @@ public static ScannerSupplier warningChecks() {
           ReturnMissingNullable.class,
           ReturnsNullCollection.class,
           ScopeOnModule.class,
+          ScopeOrQualifierAnnotationRetention.class,
           StaticOrDefaultInterfaceMethod.class,
           StaticQualifiedUsingExpression.class,
           StringFormatWithLiteral.class,

diff --git a/...s/inject/MissingRuntimeRetentionTest.java → ...peOrQualifierAnnotationRetentionTest.java b/...s/inject/MissingRuntimeRetentionTest.java → ...peOrQualifierAnnotationRetentionTest.java
@@ -27,22 +27,27 @@
  * @author [email protected] (Steven Goldfeder)
  */
 @RunWith(JUnit4.class)
-public class MissingRuntimeRetentionTest {
+public class ScopeOrQualifierAnnotationRetentionTest {
 
   private final BugCheckerRefactoringTestHelper refactoringTestHelper =
-      BugCheckerRefactoringTestHelper.newInstance(MissingRuntimeRetention.class, getClass());
+      BugCheckerRefactoringTestHelper.newInstance(
+          ScopeOrQualifierAnnotationRetention.class, getClass());
 
   private final CompilationTestHelper compilationHelper =
-      CompilationTestHelper.newInstance(MissingRuntimeRetention.class, getClass());
+      CompilationTestHelper.newInstance(ScopeOrQualifierAnnotationRetention.class, getClass());
 
   @Test
   public void positiveCase() {
-    compilationHelper.addSourceFile("MissingRuntimeRetentionPositiveCases.java").doTest();
+    compilationHelper
+        .addSourceFile("ScopeOrQualifierAnnotationRetentionPositiveCases.java")
+        .doTest();
   }
 
   @Test
   public void negativeCase() {
-    compilationHelper.addSourceFile("MissingRuntimeRetentionNegativeCases.java").doTest();
+    compilationHelper
+        .addSourceFile("ScopeOrQualifierAnnotationRetentionNegativeCases.java")
+        .doTest();
   }
 
   @Test

diff --git a/...MissingRuntimeRetentionNegativeCases.java → ...fierAnnotationRetentionNegativeCases.java b/...MissingRuntimeRetentionNegativeCases.java → ...fierAnnotationRetentionNegativeCases.java
@@ -31,7 +31,7 @@
 /**
  * @author [email protected] (Steven Goldfeder)
  */
-public class MissingRuntimeRetentionNegativeCases {
+public class ScopeOrQualifierAnnotationRetentionNegativeCases {
   /** A scoping (@Scope) annotation with runtime retention */
   @Scope
   @Target({TYPE, METHOD})

diff --git a/...MissingRuntimeRetentionPositiveCases.java → ...fierAnnotationRetentionPositiveCases.java b/...MissingRuntimeRetentionPositiveCases.java → ...fierAnnotationRetentionPositiveCases.java
@@ -30,7 +30,7 @@
 /**
  * @author [email protected] (Steven Goldfeder)
  */
-public class MissingRuntimeRetentionPositiveCases {
+public class ScopeOrQualifierAnnotationRetentionPositiveCases {
   /** A scoping (@Scope) annotation with SOURCE retention */
   @Scope
   @Target({TYPE, METHOD})

diff --git a/docs/bugpattern/MissingRuntimeRetention.md → ...ectScopeOrQualifierAnnotationRetention.md b/docs/bugpattern/MissingRuntimeRetention.md → ...ectScopeOrQualifierAnnotationRetention.md