Update the interface contract of MultiMatcher

to return all the matching nodes, as opposed to just one of the matching
nodes, and use this new contract in MoreThanOneScopeAnnotationOnClass.

Also, adjusts the behavior of MoreThanOneScopeAnnotationOnClass to
ignore Dagger components/subcomponents, and no longer suggest the
deletion of any particular scope annotation.

RELNOTES: Adjusts the behavior of MoreThanOneScopeAnnotationOnClass to
ignore Dagger components/subcomponents, and no longer suggest the
deletion of any particular scope annotation.

MOE_MIGRATED_REVID=131232952

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

@@ -446,8 +446,7 @@ public boolean matches(TryTree tryTree, VisitorState state) {
   /**
    * Matches if any two of the given list of expressions are integer literals with different values.
    */
-  private static class UnequalIntegerLiteralMatcher
-      implements MultiMatcher<MethodInvocationTree, ExpressionTree> {
+  private static class UnequalIntegerLiteralMatcher implements Matcher<MethodInvocationTree> {
 
     private final Matcher<ExpressionTree> methodSelectMatcher;
 
@@ -476,11 +475,6 @@ private boolean matches(List<? extends ExpressionTree> expressionTrees) {
       }
       return false;
     }
-
-    @Override
-    public ExpressionTree getMatchingNode() {
-      throw new UnsupportedOperationException();
-    }
   }
 
   private static class ChildOfTryMatcher extends ChildMultiMatcher<TryTree, Tree> {

core/src/main/java/com/google/errorprone/bugpatterns/inject/MoreThanOneScopeAnnotationOnClass.java

@@ -19,65 +19,60 @@
 import static com.google.errorprone.BugPattern.Category.INJECT;
 import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
-import static com.google.errorprone.matchers.InjectMatchers.GUICE_SCOPE_ANNOTATION;
-import static com.google.errorprone.matchers.InjectMatchers.JAVAX_SCOPE_ANNOTATION;
-import static com.google.errorprone.matchers.Matchers.hasAnnotation;
+import static com.google.errorprone.matchers.InjectMatchers.hasScopingAnnotations;
+import static com.google.errorprone.matchers.InjectMatchers.isDaggerComponent;
 
+import com.google.common.base.Joiner;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
-import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
 import com.google.errorprone.matchers.Description;
-import com.google.errorprone.matchers.Matcher;
-import com.google.errorprone.matchers.Matchers;
+import com.google.errorprone.matchers.MultiMatcher;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.ClassTree;
-import com.sun.source.tree.ModifiersTree;
+import com.sun.source.tree.Tree;
+import java.util.List;
 
 /**
- * This checker matches if a class has more than one annotation that is a scope
- * annotation(that is, the annotation is either annotated with Guice's
- * {@code @ScopeAnnotation} or Javax's {@code @Scope}).
+ * This checker matches if a class has more than one annotation that is a scope annotation(that is,
+ * the annotation is either annotated with Guice's {@code @ScopeAnnotation} or Javax's
+ * {@code @Scope}).
  *
  * @author sgoldfeder@google.com (Steven Goldfeder)
  */
 @BugPattern(
   name = "InjectMoreThanOneScopeAnnotationOnClass",
-  summary = "A class can be annotated with at most one scope annotation",
+  summary = "A class can be annotated with at most one scope annotation.",
   explanation =
       "Annotating a class with more than one scope annotation is "
           + "invalid according to the JSR-330 specification. ",
   category = INJECT,
   severity = ERROR,
   maturity = EXPERIMENTAL
 )
-public class MoreThanOneScopeAnnotationOnClass extends BugChecker implements AnnotationTreeMatcher {
-
-  /**
-   * Matches annotations that are themselves annotated with with
-   * {@code @ScopeAnnotation(Guice)} or {@code @Scope(Javax)}.
-   */
-  private final Matcher<AnnotationTree> scopeAnnotationMatcher =
-      Matchers.<AnnotationTree>anyOf(
-          hasAnnotation(GUICE_SCOPE_ANNOTATION), hasAnnotation(JAVAX_SCOPE_ANNOTATION));
+public class MoreThanOneScopeAnnotationOnClass extends BugChecker implements ClassTreeMatcher {
 
   @Override
-  public final Description matchAnnotation(AnnotationTree annotationTree, VisitorState state) {
-    int numberOfScopeAnnotations = 0;
-    // check if this annotation is on a class and is a scope annotation
-    if (scopeAnnotationMatcher.matches(annotationTree, state)
-        && state.getPath().getParentPath().getParentPath().getLeaf() instanceof ClassTree) {
-      for (AnnotationTree annotation :
-          ((ModifiersTree) state.getPath().getParentPath().getLeaf()).getAnnotations()) {
-        if (scopeAnnotationMatcher.matches(annotation, state)) {
-          numberOfScopeAnnotations++;
-        }
+  public final Description matchClass(ClassTree classTree, VisitorState state) {
+    MultiMatcher<Tree, AnnotationTree> scopeFinder = hasScopingAnnotations();
+    if (scopeFinder.matches(classTree, state) && !isDaggerComponent().matches(classTree, state)) {
+      List<AnnotationTree> scopeAnnotations = scopeFinder.getMatchingNodes();
+      if (scopeAnnotations.size() > 1) {
+        return buildDescription(classTree)
+            .setMessage(
+                "This class is annotated with more than one scope annotation: "
+                    + annotationDebugString(scopeAnnotations)
+                    + ". However, classes can only have one scope annotation applied to them. "
+                    + "Please remove all but one of them.")
+            .build();
       }
-    }
-    if (numberOfScopeAnnotations > 1) {
-      return describeMatch(annotationTree, SuggestedFix.delete(annotationTree));
+
     }
     return Description.NO_MATCH;
   }
+
+  private String annotationDebugString(List<AnnotationTree> scopeAnnotations) {
+    return Joiner.on(", ").join(scopeAnnotations);
+  }
 }

core/src/main/java/com/google/errorprone/bugpatterns/inject/guice/AssistedInjectScoping.java

@@ -85,11 +85,9 @@ public class AssistedInjectScoping extends BugChecker implements ClassTreeMatche
       constructor(AT_LEAST_ONE, InjectMatchers.<MethodTree>hasInjectAnnotation());
 
   /**
-   * Matches if:
-   * 1) If there is a constructor that is annotated with @Inject and that constructor has at least
-   *    one parameter that is annotated with @Assisted.
-   * 2) If there is no @Inject constructor and at least one constructor is annotated with
-   *    @AssistedInject.
+   * Matches if: 1) If there is a constructor that is annotated with @Inject and that constructor
+   * has at least one parameter that is annotated with @Assisted. 2) If there is no @Inject
+   * constructor and at least one constructor is annotated with @AssistedInject.
    */
   private static final Matcher<ClassTree> assistedMatcher =
       new Matcher<ClassTree>() {
@@ -99,7 +97,7 @@ public boolean matches(ClassTree classTree, VisitorState state) {
             // Check constructor with @Inject annotation for parameter with @Assisted annotation.
             return methodHasParameters(
                     AT_LEAST_ONE, Matchers.<VariableTree>hasAnnotation(ASSISTED_ANNOTATION))
-                .matches(constructorWithInjectMatcher.getMatchingNode(), state);
+                .matches(constructorWithInjectMatcher.getMatchingNodes().get(0), state);
           }
 
           return constructor(
@@ -115,7 +113,7 @@ public final Description matchClass(ClassTree classTree, VisitorState state) {
       return Description.NO_MATCH;
     }
 
-    AnnotationTree annotationWithScopeAnnotation = classAnnotationMatcher.getMatchingNode();
+    AnnotationTree annotationWithScopeAnnotation = classAnnotationMatcher.getMatchingNodes().get(0);
     if (annotationWithScopeAnnotation == null) {
       throw new IllegalStateException(
           "Expected to find an annotation that was annotated with @ScopeAnnotation");

core/src/main/java/com/google/errorprone/matchers/ChildMultiMatcher.java

@@ -16,8 +16,9 @@
 
 package com.google.errorprone.matchers;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.auto.value.AutoValue;
-import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.errorprone.VisitorState;
@@ -38,8 +39,21 @@ public abstract class ChildMultiMatcher<T extends Tree, N extends Tree>
     implements MultiMatcher<T, N> {
 
   public enum MatchType {
+    /**
+     * Matches if all of the child elements match the matcher. If the parent element has no child
+     * elements, this matcher returns true.
+     */
     ALL,
+    /**
+     * Matches if at least one of the child elements match the matcher. If the parent element has no
+     * child elements, this matcher returns false.
+     */
     AT_LEAST_ONE,
+    /**
+     * Matches if the last child element matches the matcher, regardless of whether or not any of
+     * the other child elements would match the matcher. If the parent element has no child
+     * elements, this matcher returns false.
+     */
     LAST
   }
 
@@ -55,23 +69,23 @@ public static <T extends Tree> Matchable<T> create(T tree, VisitorState state) {
 
   @AutoValue
   abstract static class MatchResult<T extends Tree> {
-    public abstract Optional<T> matchingNode();
+    public abstract ImmutableList<T> matchingNodes();
     public abstract boolean matches();
 
     public static <T extends Tree> MatchResult<T> none() {
-      return create(Optional.<T>absent(), false);
-    }
-
-    public static <T extends Tree> MatchResult<T> match() {
-      return create(Optional.<T>absent(), true);
+      return create(ImmutableList.<T>of(), false);
     }
-    
+
     public static <T extends Tree> MatchResult<T> match(T matchingNode) {
-      return create(Optional.of(matchingNode), true);
+      return create(ImmutableList.of(matchingNode), true);
+    }
+
+    public static <T extends Tree> MatchResult<T> match(ImmutableList<T> matchingNodes) {
+      return create(matchingNodes, true);
     }
 
     private static <T extends Tree> MatchResult<T> create(
-        Optional<T> matchingNode, boolean matches) {
+        ImmutableList<T> matchingNode, boolean matches) {
       return new AutoValue_ChildMultiMatcher_MatchResult<>(matchingNode, matches);
     }
   }
@@ -102,12 +116,14 @@ public static <N extends Tree> ListMatcher<N> create(MatchType matchType) {
   private static class AllMatcher<N extends Tree> extends ListMatcher<N> {
     @Override
     public MatchResult<N> matches(List<Matchable<N>> matchables, Matcher<N> nodeMatcher) {
+      ImmutableList.Builder<N> matchingTrees = ImmutableList.builder();
       for (Matchable<N> matchable : matchables) {
         if (!nodeMatcher.matches(matchable.tree(), matchable.state())) {
           return MatchResult.none();
         }
+        matchingTrees.add(matchable.tree());
       }
-      return MatchResult.match();
+      return MatchResult.match(matchingTrees.build());
     }
   }
 
@@ -117,12 +133,14 @@ public MatchResult<N> matches(List<Matchable<N>> matchables, Matcher<N> nodeMatc
   private static class AtLeastOneMatcher<N extends Tree> extends ListMatcher<N> {
     @Override
     public MatchResult<N> matches(List<Matchable<N>> matchables, Matcher<N> nodeMatcher) {
+      ImmutableList.Builder<N> matchingTrees = ImmutableList.builder();
       for (Matchable<N> matchable : matchables) {
         if (nodeMatcher.matches(matchable.tree(), matchable.state())) {
-          return MatchResult.match(matchable.tree());
+          matchingTrees.add(matchable.tree());
         }
       }
-      return MatchResult.none();
+      ImmutableList<N> allTheTrees = matchingTrees.build();
+      return allTheTrees.isEmpty() ? MatchResult.<N>none() : MatchResult.match(allTheTrees);
     }
   }
 
@@ -167,14 +185,15 @@ public boolean matches(T tree, VisitorState state) {
   }
 
   @Override
-  public N getMatchingNode() {
-    return matchResult.matchingNode().get();
+  public List<N> getMatchingNodes() {
+    checkState(matchResult != null, "getMatchingNodes() called before matches()");
+    return matchResult.matchingNodes();
   }
-  
+
   /**
    * Returns the set of child nodes to match. The nodes must be immediate children of the current
    * node to ensure the TreePath calculation is correct. MultiMatchers with other requirements
-   * should not subclass ChildMultiMatcher (see {@link HasIdentifier} for an example).
+   * should not subclass ChildMultiMatcher.
    */
   protected abstract Iterable<? extends N> getChildNodes(T tree, VisitorState state);
 }

core/src/main/java/com/google/errorprone/matchers/HasIdentifier.java

@@ -29,7 +29,7 @@
  *
  * @author alexloh@google.com (Alex Loh)
  */
-public class HasIdentifier implements MultiMatcher<Tree, IdentifierTree> {
+public class HasIdentifier implements Matcher<Tree> {
 
   private final Matcher<IdentifierTree> nodeMatcher;
 
@@ -71,9 +71,4 @@ public Boolean visitClass(ClassTree node, Void v) {
       return firstNonNull(super.visitClass(node, v), false);
     }
   }
-
-  @Override
-  public IdentifierTree getMatchingNode() {
-    throw new IllegalStateException();
-  }
 }

core/src/main/java/com/google/errorprone/matchers/InjectMatchers.java

@@ -16,6 +16,8 @@
 
 package com.google.errorprone.matchers;
 
+import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
+import static com.google.errorprone.matchers.Matchers.annotations;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.hasAnnotation;
 
@@ -49,4 +51,19 @@ private InjectMatchers() {} // no instantiation
   public static <T extends Tree> Matcher<T> hasInjectAnnotation() {
     return (Matcher<T>) HAS_INJECT_ANNOTATION_MATCHER;
   }
+
+  private static final Matcher<Tree> DAGGER_COMPONENT_MATCHER =
+      anyOf(hasAnnotation("dagger.Component"), hasAnnotation("dagger.Subcomponent"));
+
+  @SuppressWarnings("unchecked") // Safe contravariant cast
+  public static <T extends Tree> Matcher<T> isDaggerComponent() {
+    return (Matcher<T>) DAGGER_COMPONENT_MATCHER;
+  }
+
+  public static MultiMatcher<Tree, AnnotationTree> hasScopingAnnotations() {
+    return annotations(
+        AT_LEAST_ONE,
+        Matchers.<AnnotationTree>anyOf(
+            hasAnnotation(GUICE_SCOPE_ANNOTATION), hasAnnotation(JAVAX_SCOPE_ANNOTATION)));
+  }
 }

core/src/main/java/com/google/errorprone/matchers/Matchers.java

@@ -1037,8 +1037,7 @@ public boolean matches(BinaryTree t, VisitorState state) {
    *
    * @param nodeMatcher Which identifiers to look for
    */
-  public static MultiMatcher<Tree, IdentifierTree> hasIdentifier(
-      Matcher<IdentifierTree> nodeMatcher) {
+  public static Matcher<Tree> hasIdentifier(Matcher<IdentifierTree> nodeMatcher) {
     return new HasIdentifier(nodeMatcher);
   }
 

core/src/main/java/com/google/errorprone/matchers/MultiMatcher.java

@@ -17,6 +17,7 @@
 package com.google.errorprone.matchers;
 
 import com.sun.source.tree.Tree;
+import java.util.List;
 
 /**
  * An matcher that applies a single matcher across multiple tree nodes.
@@ -28,7 +29,14 @@
 public interface MultiMatcher<T extends Tree, N extends Tree> extends Matcher<T> {
 
   /**
-   * @return the matching node, iff a single node was matched.
+   * This method is only valid to call after calling {@link #matches}. If that call returned true,
+   * this method will return all nodes that matched (which could be empty if the multi matcher had
+   * no nodes to process, so the child nodes vacuously matched the matcher). If that call returned
+   * false (i.e.: this multimatcher did not match the matcher), this function will return an empty
+   * list.
+   *
+   * @return all the child nodes that matched the matcher.
+   * @throws IllegalStateException if {@link #matches} wasn't called beforehand.
    */
-  N getMatchingNode();
+  List<N> getMatchingNodes();
 }

core/src/test/java/com/google/errorprone/bugpatterns/inject/testdata/MoreThanOneScopeAnnotationOnClassNegativeCases.java

@@ -16,8 +16,12 @@
 
 package com.google.errorprone.bugpatterns.inject.testdata;
 
-import com.google.inject.Singleton;
 import com.google.inject.Provides;
+import com.google.inject.Singleton;
+import com.google.inject.servlet.SessionScoped;
+import dagger.Component;
+import dagger.Subcomponent;
+
 /**
  * @author sgoldfeder@google.com(Steven Goldfeder)
  */
@@ -46,12 +50,25 @@ public class TestClass3 {}
   public class TestClass4 {}
 
   /**
-   * Class has two annotations, one of which is a scoping annotation. Class
-   * also has a method with a scoping annotation.
+   * Class has two annotations, one of which is a scoping annotation. Class also has a method with a
+   * scoping annotation.
    */
-   @SuppressWarnings("foo")
+  @SuppressWarnings("foo")
   public class TestClass5 {
-  @Singleton @Provides
-  public void foo(){}
+    @Singleton
+    @Provides
+    public void foo() {}
   }
-}
+
+  /** Class has two scoped annotations, but is a Dagger component */
+  @Singleton
+  @SessionScoped
+  @Component
+  public class DaggerComponent {}
+
+  /** Class has two scoped annotations, but is a Dagger subcomponent */
+  @Singleton
+  @SessionScoped
+  @Subcomponent
+  public class DaggerSubComponent {}
+}