fix #1025 Error prone StrictCollectionIncompatibleType to guard col…

…lection usage

This check is an improvement over `CollectionIncompatibleType` because it
validates that values exist in the same type hierarchy, where it could
theoretically be possible the input implements the collection type,
but the type system doesn't have enough information to be confident.
This check allows both subtypes and supertypes, but does not check
for shared supertypes, which are not common.

README.md

@@ -176,6 +176,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
 - `ReadReturnValueIgnored`: The result of a read call must be checked to know if EOF has been reached or the expected number of bytes have been consumed.
 - `FinalClass`: A class should be declared final if all of its constructors are private.
 - `RedundantModifier`: Avoid using redundant modifiers.
+- `StrictCollectionIncompatibleType`: Likely programming error due to using the wrong type in a method that accepts Object.
 
 ### Programmatic Application
 

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleType.java

@@ -0,0 +1,209 @@
+/*
+ * (c) Copyright 2017 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import com.google.auto.service.AutoService;
+import com.google.common.collect.ImmutableList;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.method.MethodMatchers;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Type;
+import java.util.Collection;
+import java.util.Deque;
+import java.util.Dictionary;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Stack;
+import java.util.Vector;
+import javax.annotation.Nullable;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "StrictCollectionIncompatibleType",
+        // Idea provides a similar check, avoid noise when that warning is already suppressed.
+        // https://github.com/JetBrains/intellij-community/blob/master/java/java-analysis-impl/src/com/intellij/codeInspection/miscGenerics/SuspiciousCollectionsMethodCallsInspection.java
+        altNames = "SuspiciousMethodCalls",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        severity = BugPattern.SeverityLevel.WARNING,
+        summary = "Likely programming error due to using incompatible types as "
+                + "arguments for a collection method that accepts Object.")
+public final class StrictCollectionIncompatibleType
+        extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {
+
+    private static final String COLLECTION = Collection.class.getName();
+    private static final String DEQUE = Deque.class.getName();
+    private static final String DICTIONARY = Dictionary.class.getName();
+    private static final String LIST = List.class.getName();
+    private static final String MAP = Map.class.getName();
+    private static final String STACK = Stack.class.getName();
+    private static final String VECTOR = Vector.class.getName();
+
+    private final ImmutableList<IncompatibleTypeMatcher> matchers = ImmutableList.of(
+            // Matched patterns are based error-prone CollectionIncompatibleType
+            // https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java
+            compatibleArgType(MAP, "containsKey(java.lang.Object)", 0, 0),
+            compatibleArgType(MAP, "containsValue(java.lang.Object)", 1, 0),
+            compatibleArgType(MAP, "get(java.lang.Object)", 0, 0),
+            compatibleArgType(MAP, "getOrDefault(java.lang.Object,V)", 0, 0),
+            compatibleArgType(MAP, "remove(java.lang.Object)", 0, 0),
+            compatibleArgType(COLLECTION, "contains(java.lang.Object)", 0, 0),
+            compatibleArgType(COLLECTION, "remove(java.lang.Object)", 0, 0),
+            compatibleArgType(DEQUE, "removeFirstOccurrence(java.lang.Object)", 0, 0),
+            compatibleArgType(DEQUE, "removeLastOccurrence(java.lang.Object)", 0, 0),
+            compatibleArgType(DICTIONARY, "get(java.lang.Object)", 0, 0),
+            compatibleArgType(DICTIONARY, "remove(java.lang.Object)", 0, 0),
+            compatibleArgType(LIST, "indexOf(java.lang.Object)", 0, 0),
+            compatibleArgType(LIST, "lastIndexOf(java.lang.Object)", 0, 0),
+            compatibleArgType(STACK, "search(java.lang.Object)", 0, 0),
+            compatibleArgType(VECTOR, "indexOf(java.lang.Object,int)", 0, 0),
+            compatibleArgType(VECTOR, "lastIndexOf(java.lang.Object,int)", 0, 0),
+            compatibleArgType(VECTOR, "removeElement(java.lang.Object)", 0, 0));
+
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+        // Return the description from the first matching IncompatibleTypeMatcher
+        for (int i = 0; i < matchers.size(); i++) {
+            IncompatibleTypeMatcher matcher = matchers.get(i);
+            Optional<Description> result = matcher.describe(tree, state);
+            if (result.isPresent()) {
+                return result.get();
+            }
+        }
+        return Description.NO_MATCH;
+    }
+
+    @Nullable
+    private static Type getBoxedResult(ExpressionTree expressionTree, VisitorState state) {
+        Type rawType = ASTHelpers.getResultType(expressionTree);
+        if (rawType == null) {
+            return null;
+        }
+        return state.getTypes().boxedTypeOrType(rawType);
+    }
+
+    @Nullable
+    private static Type getTargetTypeAsSuper(
+            MethodInvocationTree tree,
+            String superTarget,
+            VisitorState state) {
+        Type targetMapType = getTargetType(tree);
+        if (targetMapType == null) {
+            return null;
+        }
+        Symbol mapSymbol = state.getSymbolFromString(superTarget);
+        if (mapSymbol == null) {
+            return null;
+        }
+        return state.getTypes().asSuper(targetMapType, mapSymbol);
+    }
+
+    @Nullable
+    private static Type getTargetType(MethodInvocationTree tree) {
+        ExpressionTree methodSelect = tree.getMethodSelect();
+        if (methodSelect instanceof MemberSelectTree) {
+            MemberSelectTree memberSelectTree = (MemberSelectTree) methodSelect;
+            return ASTHelpers.getResultType(memberSelectTree.getExpression());
+        }
+        return null;
+    }
+
+    private IncompatibleTypeMatcher compatibleArgType(
+            String baseType,
+            String signature,
+            int typeArgumentIndex,
+            int argumentIndex) {
+        // Eagerly create the matcher to avoid allocation for each check
+        Matcher<ExpressionTree> methodMatcher = MethodMatchers.instanceMethod()
+                .onDescendantOf(baseType)
+                .withSignature(signature);
+        return (tree, state) -> {
+            if (!methodMatcher.matches(tree, state)) {
+                // This matcher does not apply
+                return Optional.empty();
+            }
+            if (tree.getArguments().size() <= argumentIndex) {
+                return IncompatibleTypeMatcher.NO_MATCH;
+            }
+            Type targetType = getTargetTypeAsSuper(tree, baseType, state);
+            if (targetType == null) {
+                return IncompatibleTypeMatcher.NO_MATCH;
+            }
+            if (targetType.getTypeArguments().size() <= typeArgumentIndex) {
+                return IncompatibleTypeMatcher.NO_MATCH;
+            }
+            Type typeArgumentType = targetType.getTypeArguments().get(typeArgumentIndex);
+            ExpressionTree argumentTree = tree.getArguments().get(argumentIndex);
+            Type argumentType = getBoxedResult(argumentTree, state);
+            if (argumentType == null) {
+                return IncompatibleTypeMatcher.NO_MATCH;
+            }
+            // Check erased types only to avoid more complex edge cases. This way we only warn when we
+            // have high confidence something isn't right.
+            // This tests that types are within the same (linear) inheritance hierarchy, but does not
+            // not accept types with a common ancestor.
+            if (ASTHelpers.isSubtype(argumentType, typeArgumentType, state)
+                    // Check the reverse direction as well, this allows 'Object' to succeed for
+                    // delegation, as well as most false positives without sacrificing many known
+                    // failure cases.
+                    || ASTHelpers.isSubtype(typeArgumentType, argumentType, state)) {
+                return IncompatibleTypeMatcher.NO_MATCH;
+            }
+            return Optional.of(buildDescription(argumentTree)
+                    .setMessage("Likely programming error due to using incompatible types as arguments for "
+                            + "a collection method that accepts Object. Value '"
+                            + state.getSourceForNode(argumentTree) + "' of type '" + prettyType(argumentType)
+                            + "' is not compatible with the expected type '" + prettyType(typeArgumentType) + '\'')
+                    .build());
+        };
+    }
+
+    /**
+     * Pretty prints the input type for use in description messages. This is not suitable for suggested fixes
+     * because unlike {@link SuggestedFixes#prettyType(VisitorState, SuggestedFix.Builder, Type)} with non-null
+     * state and builder, it doesn't add relevant imports.
+     */
+    private static String prettyType(Type type) {
+        return SuggestedFixes.prettyType(null, null, type);
+    }
+
+    private interface IncompatibleTypeMatcher {
+        /**
+         * Signals that a matcher applied to the input, but did not find any bugs. It is not necessary to check
+         * additional {@link IncompatibleTypeMatcher matchers}.
+         */
+        Optional<Description> NO_MATCH = Optional.of(Description.NO_MATCH);
+
+        /**
+         * Returns an empty optional if the provided {@link MethodInvocationTree} isn't matched.
+         * If the method is matched, an {@link Optional} of {@link Description#NO_MATCH} is
+         * returned for valid use.
+         */
+        Optional<Description> describe(MethodInvocationTree tree, VisitorState state);
+    }
+}

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleTypeTest.java

@@ -0,0 +1,145 @@
+/*
+ * (c) Copyright 2017 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+
+@Execution(ExecutionMode.CONCURRENT)
+class StrictCollectionIncompatibleTypeTest {
+
+    @Test
+    void testUnexpectedType_map() {
+        helper().addSourceLines(
+                "Test.java",
+                "import java.util.Map;",
+                "class Test {",
+                "   String f0(Map<Integer, String> map, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return map.get(key);",
+                "   }",
+                "   boolean f1(Map<Integer, String> map, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return map.containsKey(key);",
+                "   }",
+                "   String f2(CustomMap map, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return map.get(key);",
+                "   }",
+                "   boolean f3(CustomMap map, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return map.containsKey(key);",
+                "   }",
+                "   String f4(CustomMap map, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return map.remove(key);",
+                "   }",
+                "   interface CustomMap extends Map<Integer, String> {}",
+                "}"
+        ).doTest();
+    }
+
+    @Test
+    void testUnexpectedType_collection() {
+        helper().addSourceLines(
+                "Test.java",
+                "import java.util.Collection;",
+                "import java.util.List;",
+                "import com.google.common.collect.ImmutableMap;",
+                "class Test {",
+                "   boolean f0(Collection<Integer> in, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.contains(key);",
+                "   }",
+                "   boolean f1(List<Integer> in, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.contains(key);",
+                "   }",
+                "   boolean f2(Custom in, Integer key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.contains(key);",
+                "   }",
+                "   boolean f3(ImmutableMap<Integer, String> in, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.keySet().contains(key);",
+                "   }",
+                "   boolean f4(List<Integer> in, String key) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.remove(key);",
+                "   }",
+                "   boolean f4(List<Integer> in) {",
+                "       // BUG: Diagnostic contains: incompatible types",
+                "       return in.remove(5L);",
+                "   }",
+                "   interface Custom extends List<CharSequence> {}",
+                "}"
+        ).doTest();
+    }
+
+    @Test
+    void testCollectionContains_edges() {
+        helper().addSourceLines(
+                "Test.java",
+                "import java.util.Collection;",
+                "class Test {",
+                "   boolean f0(Collection<Integer> in) {",
+                "     return in.contains(null);",
+                "   }",
+                "   boolean f1(Collection<Integer> in) {",
+                "     return in.contains(3);",
+                "   }",
+                "   boolean f2(Collection<Class<? extends CharSequence>> in, Class<?> clazz) {",
+                "     return in.contains(clazz);",
+                "   }",
+                "}"
+        ).doTest();
+    }
+
+    @Test
+    void testAmbiguousMapKey_allowed() {
+        helper().addSourceLines(
+                "Test.java",
+                "import java.util.Map;",
+                "class Test {",
+                // Not recommended, but should not fail
+                "   Object f1(Map map, String key) {",
+                "       return map.get(key);",
+                "   }",
+                "}"
+        ).doTest();
+    }
+
+    @Test
+    void testIdeaSuppression() {
+        helper().addSourceLines(
+                "Test.java",
+                "import java.util.Map;",
+                "class Test {",
+                "   @SuppressWarnings(\"SuspiciousMethodCalls\")",
+                "   Object f1(Map<Integer, Integer> map, String key) {",
+                "       return map.get(key);",
+                "   }",
+                "}"
+        ).doTest();
+    }
+
+    private CompilationTestHelper helper() {
+        return CompilationTestHelper.newInstance(StrictCollectionIncompatibleType.class, getClass());
+    }
+}