CollectionIncompatibleType fixes:

1) Allow the case in which the argument is a supertype of the contained type,
   e.g. it should be legal to check whether a List<HashMap> contains a Map.

2) Deal properly with subtypes that have a different order of type arguments
   than the interface types we're looking for.  For example,
   ClassToInstanceMap<T> implements Map<Class<? extends T>, T>.

RELNOTES: none
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=104155720

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

@@ -19,9 +19,9 @@
 import static com.google.errorprone.BugPattern.Category.JDK;
 import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
-import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.collect.Iterables;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
@@ -33,11 +33,15 @@
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Types;
 
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
+import javax.annotation.Nullable;
+
 /**
  * Checker for calling Object-accepting methods with types that don't match the type arguments of
  * their container types.  Currently this checker detects problems with the following methods on
@@ -63,71 +67,38 @@
 public class CollectionIncompatibleType extends BugChecker implements MethodInvocationTreeMatcher {
 
   /* TODO(eaftan):
-   * 1) Run over the Google codebase to see how robust this check is.
-   * 2) Can we construct a suggested fix?  Anything reasonable to do?
-   * 3) Add new methods.  The list is in Issue 106.  It might be easier to do it incrementally.
-   * 4) Consider whether there is a subset of these that can/should be errors rather than warnings.
-   * 5) Bump maturity to MATURE.
+   * 1) Add new methods.  The list is in Issue 106.  It might be easier to do it incrementally.
+   * 2) Consider whether there is a subset of these that can/should be errors rather than warnings.
+   * 3) Bump maturity to MATURE.
    */
 
-  private static final Matcher<ExpressionTree> METHOD_ARG_0_SHOULD_MATCH_TYPE_ARG_0 =
-      anyOf(
-          instanceMethod()
-              .onDescendantOf("java.util.Collection")
-              .withSignature("contains(java.lang.Object)"),
-          instanceMethod()
-              .onDescendantOf("java.util.Collection")
-              .withSignature("remove(java.lang.Object)"),
-          instanceMethod()
-              .onDescendantOf("java.util.List")
-              .withSignature("indexOf(java.lang.Object)"),
-          instanceMethod()
-              .onDescendantOf("java.util.List")
-              .withSignature("lastIndexOf(java.lang.Object)"),
-          instanceMethod().onDescendantOf("java.util.Map").withSignature("get(java.lang.Object)"),
-          instanceMethod()
-              .onDescendantOf("java.util.Map")
-              .withSignature("containsKey(java.lang.Object)"),
-          instanceMethod()
-              .onDescendantOf("java.util.Map")
-              .withSignature("remove(java.lang.Object)"));
-
-  private static final Matcher<ExpressionTree> METHOD_ARG_0_SHOULD_MATCH_TYPE_ARG_1 =
-      anyOf(
-          instanceMethod()
-              .onDescendantOf("java.util.Map")
-              .withSignature("containsValue(java.lang.Object)"));
+  private static final Iterable<MatcherWithTypeInfo> MATCHERS =
+      Arrays.asList(
+          new MatcherWithTypeInfo("java.util.Collection", "contains(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.Collection", "remove(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.List", "indexOf(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.List", "lastIndexOf(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.Map", "get(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.Map", "containsKey(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.Map", "remove(java.lang.Object)", 0, 0),
+          new MatcherWithTypeInfo("java.util.Map", "containsValue(java.lang.Object)", 1, 0));
+
 
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
 
-    int methodArgIndex;
-    int typeArgIndex;
-
-    if (METHOD_ARG_0_SHOULD_MATCH_TYPE_ARG_0.matches(tree, state)) {
-      methodArgIndex = 0;
-      typeArgIndex = 0;
-    } else if (METHOD_ARG_0_SHOULD_MATCH_TYPE_ARG_1.matches(tree, state)) {
-      methodArgIndex = 0;
-      typeArgIndex = 1;
-    } else {
+    MatchResult result = firstNonNullMatchResult(MATCHERS, tree, state);
+    if (result == null) {
       return Description.NO_MATCH;
     }
 
-    com.sun.tools.javac.util.List<Type> tyargs =
-        ASTHelpers.getReceiverType(tree).getTypeArguments();
-    if (tyargs.size() <= typeArgIndex) {
-      // Collection is raw, nothing we can do.
-      return Description.NO_MATCH;
-    }
+    Types types = state.getTypes();
 
-    Type typeArg = tyargs.get(typeArgIndex);
-    ExpressionTree methodArg = Iterables.get(tree.getArguments(), methodArgIndex);
-    Type methodArgType = ASTHelpers.getType(methodArg);
-    // TODO(eaftan): Allow cases when the lower bound of the type argument is assignable to
-    // the method argument type.
-    Type typeArgUpperBound = ASTHelpers.getUpperBound(typeArg, state.getTypes());
-    if (state.getTypes().isAssignable(methodArgType, typeArgUpperBound)) {
+    Type methodArgTypeErased = types.erasure(result.methodArgType());
+    Type typeArgUpperBoundErased =
+        types.erasure(ASTHelpers.getUpperBound(result.typeArgType(), types));
+    if (state.getTypes().isAssignable(methodArgTypeErased, typeArgUpperBoundErased)
+        || state.getTypes().isAssignable(typeArgUpperBoundErased, methodArgTypeErased)) {
       return Description.NO_MATCH;
     }
 
@@ -136,9 +107,88 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
             String.format(
                 "Argument '%s' should not be passed to this method; its type %s is not compatible "
                     + "with its collection's type argument %s",
-                methodArg,
-                methodArgType,
-                typeArg))
+                result.methodArg(),
+                result.methodArgType(),
+                result.typeArgType()))
         .build();
   }
+
+  @AutoValue
+  abstract static class MatchResult {
+    public abstract ExpressionTree methodArg();
+    public abstract Type methodArgType();
+    public abstract Type typeArgType();
+
+    public static MatchResult create(
+        ExpressionTree methodArg, Type methodArgType, Type typeArgType) {
+      return new AutoValue_CollectionIncompatibleType_MatchResult(
+          methodArg, methodArgType, typeArgType);
+    }
+  }
+
+  /**
+   * Wraps a method invocation matcher with extra information about the method matched.  Extracts
+   * the relevant method argument and its type, and the type of the relevant type argument, and
+   * returns those along with enough information to produce a helpful error message.
+   */
+  private static class MatcherWithTypeInfo {
+
+    private final Matcher<ExpressionTree> methodMatcher;
+    private final String typeName;
+    private final int typeArgIndex;
+    private final int methodArgIndex;
+
+    /**
+     * @param typeName The fully-qualified name of the type whose descendants to match on
+     * @param signature The signature of the method to match on
+     * @param typeArgIndex The index of the type argument that should match the method argument
+     * @param methodArgIndex The index of the method argument that should match the type argument
+     */
+    public MatcherWithTypeInfo(
+        String typeName, String signature, int typeArgIndex, int methodArgIndex) {
+      this.methodMatcher = instanceMethod().onDescendantOf(typeName).withSignature(signature);
+      this.typeName = typeName;
+      this.typeArgIndex = typeArgIndex;
+      this.methodArgIndex = methodArgIndex;
+    }
+
+    @Nullable
+    public MatchResult matches(MethodInvocationTree tree, VisitorState state) {
+      if (!methodMatcher.matches(tree, state)) {
+        return null;
+      }
+
+      Types types = state.getTypes();
+
+      // Find instantiated parameterized type for the receiver expression that matches the
+      // owner of the method we were looking for to handle the case when a subtype has different
+      // type arguments than the expected type.  For example, ClassToInstanceMap<T> implements
+      // Map<Class<? extends T>, T>.
+      Type collectionType =
+          types.asSuper(ASTHelpers.getReceiverType(tree), state.getSymbolFromString(typeName));
+      com.sun.tools.javac.util.List<Type> tyargs = collectionType.getTypeArguments();
+      if (tyargs.size() <= typeArgIndex) {
+        // Collection is raw, nothing we can do.
+        return null;
+      }
+
+      ExpressionTree methodArg = Iterables.get(tree.getArguments(), methodArgIndex);
+      Type methodArgType = ASTHelpers.getType(methodArg);
+      Type typeArgType = tyargs.get(typeArgIndex);
+      return MatchResult.create(methodArg, methodArgType, typeArgType);
+    }
+  }
+
+  @Nullable
+  private static MatchResult firstNonNullMatchResult(
+      Iterable<MatcherWithTypeInfo> matchers, MethodInvocationTree tree, VisitorState state) {
+    for (MatcherWithTypeInfo matcher : matchers) {
+      MatchResult result = matcher.matches(tree, state);
+      if (result != null) {
+        return result;
+      }
+    }
+
+    return null;
+  }
 }

core/src/main/java/com/google/errorprone/predicates/TypePredicate.java

@@ -20,7 +20,9 @@
 
 import com.sun.tools.javac.code.Type;
 
+import java.io.Serializable;
+
 /** A predicate for testing {@link Type}s. */
-public interface TypePredicate {
+public interface TypePredicate extends Serializable {
   boolean apply(Type type, VisitorState state);
 }

core/src/main/java/com/google/errorprone/suppliers/Supplier.java

@@ -18,10 +18,12 @@
 
 import com.google.errorprone.VisitorState;
 
+import java.io.Serializable;
+
 /**
  * Simple supplier pattern, which allows delayed binding to access to runtime elements.
  * @author alexeagle@google.com (Alex Eagle)
  */
-public interface Supplier<T> {
+public interface Supplier<T> extends Serializable {
   T get(VisitorState state);
 }

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

@@ -19,6 +19,7 @@
 import com.google.errorprone.CompilationTestHelper;
 
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -38,22 +39,41 @@ public void setUp() {
   }
 
   @Test
-  public void testPositiveCase() throws Exception {
+  public void testPositiveCase() {
     compilationHelper.addSourceFile("CollectionIncompatibleTypePositiveCases.java").doTest();
   }
 
   @Test
-  public void testNegativeCase() throws Exception {
+  public void testNegativeCase() {
     compilationHelper.addSourceFile("CollectionIncompatibleTypeNegativeCases.java").doTest();
   }
 
   @Test
-  public void testOutOfBounds() throws Exception {
+  public void testOutOfBounds() {
     compilationHelper.addSourceFile("CollectionIncompatibleTypeOutOfBounds.java").doTest();
   }
 
   @Test
-  public void testClassCast() throws Exception {
+  public void testClassCast() {
     compilationHelper.addSourceFile("CollectionIncompatibleTypeClassCast.java").doTest();
   }
+
+  // This test is disabled because calling Types#asSuper in the check removes the upper bound on K.
+  @Test
+  @Ignore
+  public void testBoundedTypeParameters() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import java.util.HashMap;",
+            "public class Test {",
+            "  private static class MyHashMap<K extends Integer, V extends String>",
+            "      extends HashMap<K, V> {}",
+            "  public boolean boundedTypeParameters(MyHashMap<?, ?> myHashMap) {",
+            "    // BUG: Diagnostic contains:",
+            "    return myHashMap.containsKey(\"bad\");",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/testdata/CollectionIncompatibleTypeNegativeCases.java

@@ -16,6 +16,9 @@
 
 package com.google.errorprone.bugpatterns;
 
+import com.google.common.base.Optional;
+import com.google.common.collect.ClassToInstanceMap;
+
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
@@ -80,11 +83,17 @@ public boolean mapSubtype() {
 
   private class B extends Date {}
 
-  public boolean extendsContainedType() {
+  public boolean argTypeExtendsContainedType() {
     Collection<Date> collection = new ArrayList<>();
     return collection.contains(new B());
   }
 
+  public boolean containedTypeExtendsArgType() {
+    Collection<String> collection = new ArrayList<>();
+    Object actuallyAString = "ok";
+    return collection.contains(actuallyAString);
+  }
+
   public boolean boundedWildcard() {
     Collection<? extends Date> collection = new ArrayList<>();
     return collection.contains(new Date()) || collection.contains(new B());
@@ -109,7 +118,30 @@ public boolean doesntExtendCollection() {
     DoesntExtendCollection<String> collection = new DoesntExtendCollection<>();
     return collection.contains(new Date());
   }
+
+  private static class Pair<A, B> {
+    public A first;
+    public B second;
+  }
+
+  public boolean declaredTypeVsExpressionType(Pair<Integer, String> pair, List<Integer> list) {
+    return list.contains(pair.first);
+  }
+
+  public boolean containsParameterizedType(
+      Collection<Class<? extends String>> collection, Class<?> clazz) {
+    return collection.contains(clazz);
+  }
 
+  public boolean containsWildcard(Collection<String> collection, Optional<?> optional) {
+    return collection.contains(optional.get());
+  }
+
+  public <T extends String> T subclassHasDifferentTypeParameters(
+      ClassToInstanceMap<String> map, Class<T> klass) {
+    return klass.cast(map.get(klass));
+  }
+
   // Ensure we don't match Hashtable.contains and ConcurrentHashtable.contains because there is a
   // separate check, HashtableContains, specifically for them.
   public boolean hashtableContains() {

core/src/test/java/com/google/errorprone/bugpatterns/testdata/CollectionIncompatibleTypePositiveCases.java

@@ -16,6 +16,8 @@
 
 package com.google.errorprone.bugpatterns;
 
+import com.google.common.collect.ClassToInstanceMap;
+
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
@@ -94,10 +96,18 @@ public boolean boundedWildcard() {
     return collection.contains("bad");
   }
 
-  private static class MyHashMap<K extends Integer, V extends String> extends HashMap<K, V> {}
+  private static class Pair<A, B> {
+    public A first;
+    public B second;
+  }
 
-  public boolean boundedTypeParameters(MyHashMap<?, ?> myHashMap) {
+  public boolean declaredTypeVsExpressionType(Pair<Integer, String> pair, List<Integer> list) {
+    // BUG: Diagnostic contains:
+    return list.contains(pair.second);
+  }
+
+  public String subclassHasDifferentTypeParameters(ClassToInstanceMap<String> map, String s) {
     // BUG: Diagnostic contains:
-    return myHashMap.containsKey("bad");
+    return map.get(s);
   }
 }