From ecc4ebea48389582b532e42146a413e6bb3298d0 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 3 Oct 2019 17:07:07 -0700 Subject: [PATCH 01/14] Added errorprone checks for Guavas static factory methods. --- .../PreferCollectionConstructors.java | 293 +++++++++++++++ .../PreferCollectionConstructorsTest.java | 339 ++++++++++++++++++ 2 files changed, 632 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java new file mode 100644 index 000000000..023f0ab9a --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -0,0 +1,293 @@ +/* + * (c) Copyright 2019 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.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.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 com.sun.tools.javac.tree.JCTree; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Comparator; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CopyOnWriteArraySet; +import java.util.stream.Collectors; + +@AutoService(BugChecker.class) +@BugPattern( + name = "PreferCollectionConstructors", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Since Java 7 the standard collection constructors should be used instead of the static factory " + + "methods provided by Guava.") +public final class PreferCollectionConstructors extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final long serialVersionUID = 1L; + + private static final Matcher NEW_ARRAY_LIST = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newArrayList"); + + private static final Matcher NEW_LINKED_LIST = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newLinkedList"); + + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_LIST = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newCopyOnWriteArrayList"); + + private static final Matcher NEW_ARRAY_LIST_WITH_CAPACITY = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .namedAnyOf("newArrayListWithCapacity", "newArrayListWithExpectedSize") + .withParameters("int"); + + private static final Matcher NEW_CONCURRENT_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newConcurrentMap") + .withParameters(); + + private static final Matcher NEW_HASH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newHashMap"); + + private static final Matcher NEW_HASH_MAP_WITH_EXPECTED_SIZE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newHashMapWithExpectedSize") + .withParameters("int"); + + private static final Matcher NEW_TREE_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newTreeMap"); + + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newCopyOnWriteArraySet"); + + private static final Matcher NEW_LINKED_HASH_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newLinkedHashSet"); + + private static final Matcher NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newLinkedHashSetWithExpectedSize") + .withParameters("int"); + + private static final Matcher NEW_TREE_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newTreeSet"); + + private static final Matcher NEW_HASH_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newHashSet"); + + private static final Matcher NEW_HASH_SET_WITH_EXPECTED_SIZE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newHashSetWithExpectedSize") + .withParameters("int"); + + private static final Matcher NEW_LINKED_HASH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newLinkedHashMap"); + + private static final Matcher NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newLinkedHashMapWithExpectedSize") + .withParameters("int"); + + private static final Matcher NEW_ENUM_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newEnumMap"); + + private static final Matcher NEW_IDENTITY_HASH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newIdentityHashMap"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + // Param types used to figure out which overload is called + List paramTypes = ((JCTree.JCExpression) tree.getMethodSelect()).type.asMethodType().argtypes; + + // Argument types used to differentiate between an Iterable and a Collection + List argTypes = ((JCTree.JCMethodInvocation) tree).args + .stream().map((JCTree.JCExpression e) -> e.type).collect(Collectors.toList()); + + // For convenience + ExpressionTree arg = tree.getArguments().size() == 1 ? tree.getArguments().get(0) : null; + + if (NEW_ARRAY_LIST.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, ArrayList.class, arg); + } + if (NEW_ARRAY_LIST_WITH_CAPACITY.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, ArrayList.class, arg); + } + if (NEW_LINKED_LIST.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, LinkedList.class, arg); + } + if (NEW_COPY_ON_WRITE_ARRAY_LIST.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, CopyOnWriteArrayList.class, arg); + } + if (NEW_CONCURRENT_MAP.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, ConcurrentHashMap.class, arg); + } + if (NEW_HASH_MAP.matches(tree, state) + && (paramTypes.isEmpty() || checkParamTypes(state, paramTypes, Map.class))) { + return buildSingleArgFixSuggestion(tree, state, HashMap.class, arg); + } + if (NEW_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, HashMap.class, arg); + } + if (NEW_COPY_ON_WRITE_ARRAY_SET.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, CopyOnWriteArraySet.class, arg); + } + if (NEW_LINKED_HASH_SET.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, LinkedHashSet.class, arg); + } + if (NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, LinkedHashSet.class, arg); + } + if (NEW_TREE_SET.matches(tree, state) + && (paramTypes.isEmpty() + || checkParamTypes(state, paramTypes, Comparator.class) + || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, TreeSet.class, arg); + } + if (NEW_HASH_SET.matches(tree, state) + && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { + return buildSingleArgFixSuggestion(tree, state, HashSet.class, arg); + } + if (NEW_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, HashSet.class, arg); + } + if (NEW_TREE_MAP.matches(tree, state) + && (paramTypes.isEmpty() + || checkParamTypes(state, paramTypes, SortedMap.class) + || checkParamTypes(state, paramTypes, Comparator.class))) { + return buildSingleArgFixSuggestion(tree, state, TreeMap.class, arg); + } + if (NEW_LINKED_HASH_MAP.matches(tree, state) + && (paramTypes.isEmpty() || checkParamTypes(state, paramTypes, Map.class))) { + return buildSingleArgFixSuggestion(tree, state, LinkedHashMap.class, arg); + } + if (NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, LinkedHashMap.class, arg); + } + if (NEW_ENUM_MAP.matches(tree, state) + && (paramTypes.isEmpty() + || checkParamTypes(state, paramTypes, Class.class) + || checkParamTypes(state, paramTypes, Map.class))) { + return buildSingleArgFixSuggestion(tree, state, EnumMap.class, arg); + } + if (NEW_IDENTITY_HASH_MAP.matches(tree, state)) { + return buildSingleArgFixSuggestion(tree, state, IdentityHashMap.class, arg); + } + return Description.NO_MATCH; + } + + private Description buildSingleArgFixSuggestion( + MethodInvocationTree tree, + VisitorState state, + Class collectionClass, + ExpressionTree argTree) { + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + String collectionType = SuggestedFixes.qualifyType(state, fixBuilder, collectionClass.getName()); + String typeArgs = tree.getTypeArguments() + .stream() + .map(state::getSourceForNode) + .collect(Collectors.joining(", ")); + String arg = argTree == null ? "" : state.getSourceForNode(argTree); + String replacement = "new " + collectionType + "<" + typeArgs + ">(" + arg + ")"; + return buildDescription(tree) + .setMessage("The factory method call should be replaced with a constructor call.") + .addFix(fixBuilder.replace(tree, replacement).build()) + .build(); + } + + private boolean isIterableParamCollectionArg(VisitorState state, List params, List args) { + return checkParamTypes(state, params, Iterable.class) + && checkArgTypes(state, args, Collection.class); + } + + private boolean checkParamTypes(VisitorState state, List params, Class... expected) { + return checkTypes(state, false, params, expected); + } + + private boolean checkArgTypes(VisitorState state, List args, Class... expected) { + return checkTypes(state, true, args, expected); + } + + private boolean checkTypes(VisitorState state, boolean subtypes, List typeList1, Class... typeList2) { + Types types = state.getTypes(); + if (typeList1.size() != typeList2.length) { + return false; + } + for (int i = 0; i < typeList1.size(); i++) { + Type t1 = types.erasure(typeList1.get(i)); + Type t2 = types.erasure(state.getTypeFromString(typeList2[i].getName())); + if ((!subtypes && !types.isSameType(t1, t2)) || (subtypes && !types.isSubtype(t1, t2))) { + return false; + } + } + return true; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java new file mode 100644 index 000000000..1f9407951 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java @@ -0,0 +1,339 @@ +/* + * (c) Copyright 2019 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.BugCheckerRefactoringTestHelper; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; + +@Execution(ExecutionMode.CONCURRENT) +public final class PreferCollectionConstructorsTest { + + @Test + public void testNewArrayListRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newArrayList()", + "new ArrayList<>()", + "java.util.ArrayList"); + } + + @Test + public void testNewArrayListWithCollectionsRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newArrayList(new java.util.ArrayList<>())", + "new ArrayList<>(new java.util.ArrayList<>())", + "java.util.ArrayList"); + } + + @Test + public void testNewArrayListWithCapacityRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newArrayListWithCapacity(123)", + "new ArrayList<>(123)", + "java.util.ArrayList"); + } + + @Test + public void testNewArrayListWithExpectedSizeRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newArrayListWithExpectedSize(123)", + "new ArrayList<>(123)", + "java.util.ArrayList"); + } + + @Test + public void testNewLinkedListRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newLinkedList()", + "new LinkedList<>()", + "java.util.LinkedList"); + } + + @Test + public void testNewLinkedListWithCollectionsRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newLinkedList(new java.util.ArrayList<>())", + "new LinkedList<>(new java.util.ArrayList<>())", + "java.util.LinkedList"); + } + + @Test + public void testNewCopyOnWriteListRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newCopyOnWriteArrayList()", + "new CopyOnWriteArrayList<>()", + "java.util.concurrent.CopyOnWriteArrayList"); + } + + @Test + public void testNewCopyOnWriteArrayListWithCollectionsRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newCopyOnWriteArrayList(new java.util.ArrayList<>())", + "new CopyOnWriteArrayList<>(new java.util.ArrayList<>())", + "java.util.concurrent.CopyOnWriteArrayList"); + } + + @Test + public void testNewConcurrentMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newConcurrentMap()", + "new ConcurrentHashMap<>()", + "java.util.concurrent.ConcurrentHashMap"); + } + + @Test + public void testNewEnumMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newEnumMap(java.util.concurrent.TimeUnit.class)", + "new EnumMap<>(java.util.concurrent.TimeUnit.class)", + "java.util.EnumMap"); + } + + @Test + public void testNewEnumMapWithMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newEnumMap(new java.util.HashMap())", + "new EnumMap<>(new java.util.HashMap())", + "java.util.EnumMap"); + } + + @Test + public void testNewHashMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newHashMap()", + "new HashMap<>()", + "java.util.HashMap"); + } + + @Test + public void testNewHashMapWithMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newHashMap(new java.util.HashMap<>())", + "new HashMap<>(new java.util.HashMap<>())", + "java.util.HashMap"); + } + + @Test + public void testNewHashMapWithExpectedSizeRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newHashMapWithExpectedSize(123)", + "new HashMap<>(123)", + "java.util.HashMap"); + } + + @Test + public void testNewIdentityHashMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newIdentityHashMap()", + "new IdentityHashMap<>()", + "java.util.IdentityHashMap"); + } + + @Test + public void testNewLinkedHashMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newLinkedHashMap()", + "new LinkedHashMap<>()", + "java.util.LinkedHashMap"); + } + + @Test + public void testNewLinkedHashMapWithMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newLinkedHashMap(new java.util.HashMap<>())", + "new LinkedHashMap<>(new java.util.HashMap<>())", + "java.util.LinkedHashMap"); + } + + @Test + public void testNewLinkedHashMapWithExpectedSizeRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newLinkedHashMapWithExpectedSize(123)", + "new LinkedHashMap<>(123)", + "java.util.LinkedHashMap"); + } + + @Test + public void testNewTreeMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newTreeMap()", + "new TreeMap<>()", + "java.util.TreeMap"); + } + + @Test + public void testNewTreeMapWithMapRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newTreeMap(new java.util.TreeMap<>())", + "new TreeMap<>(new java.util.TreeMap<>())", + "java.util.TreeMap"); + } + + @Test + public void testNewTreeMapWithComparatorRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newTreeMap(java.util.Comparator.naturalOrder())", + "new TreeMap<>(java.util.Comparator.naturalOrder())", + "java.util.TreeMap"); + } + + @Test + public void testNewCopyOnWriteArraySetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newCopyOnWriteArraySet()", + "new CopyOnWriteArraySet<>()", + "java.util.concurrent.CopyOnWriteArraySet"); + } + + @Test + public void testNewCopyOnWriteArraySetWithSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newCopyOnWriteArraySet(new java.util.HashSet<>())", + "new CopyOnWriteArraySet<>(new java.util.HashSet<>())", + "java.util.concurrent.CopyOnWriteArraySet"); + } + + @Test + public void testNewHashSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newHashSet()", + "new HashSet<>()", + "java.util.HashSet"); + } + + @Test + public void testNewHashSetWithSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newHashSet(new java.util.HashSet<>())", + "new HashSet<>(new java.util.HashSet<>())", + "java.util.HashSet"); + } + + @Test + public void testNewHashSetWithExpectedSizeRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newHashSetWithExpectedSize(123)", + "new HashSet<>(123)", + "java.util.HashSet"); + } + + @Test + public void testNewLinkedHashSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newLinkedHashSet()", + "new LinkedHashSet<>()", + "java.util.LinkedHashSet"); + } + + @Test + public void testNewLinkedHashSetWithSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newLinkedHashSet(new java.util.HashSet<>())", + "new LinkedHashSet<>(new java.util.HashSet<>())", + "java.util.LinkedHashSet"); + } + + @Test + public void testNewLinkedHashSetWithExpectedSizeRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newLinkedHashSetWithExpectedSize(123)", + "new LinkedHashSet<>(123)", + "java.util.LinkedHashSet"); + } + + @Test + public void testNewTreeSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newTreeSet()", + "new TreeSet<>()", + "java.util.TreeSet"); + } + + @Test + public void testNewTreeSetWithSetRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newTreeSet(new java.util.HashSet<>())", + "new TreeSet<>(new java.util.HashSet<>())", + "java.util.TreeSet"); + } + + @Test + public void testNewTreeSetWithComparatorRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newTreeSet(java.util.Comparator.naturalOrder())", + "new TreeSet<>(java.util.Comparator.naturalOrder())", + "java.util.TreeSet"); + } + + @Test + public void testWithOneTypeArgRewrite() { + testStaticFactoryMethodRewrite( + "Sets.newHashSet()", + "new HashSet()", + "java.util.HashSet"); + } + + @Test + public void testWithTwoTypeArgsRewrite() { + testStaticFactoryMethodRewrite( + "Maps.newTreeMap()", + "new TreeMap()", + "java.util.TreeMap"); + } + + @Test + public void testWithVarargRewrite() { + testStaticFactoryMethodRewrite( + "Lists.newArrayList(\"a\", \"b\", \"c\")", + "Lists.newArrayList(\"a\", \"b\", \"c\")"); + } + + @Test + public void testWithExplicitVarargInvocationRewrite() { + testStaticFactoryMethodRewrite( + "Sets.>newHashSet(new java.util.HashSet())", + "Sets.>newHashSet(new java.util.HashSet())"); + } + + private void testStaticFactoryMethodRewrite(String before, String after, String... addedImports) { + List imports = Arrays.asList( + "import com.google.common.collect.Iterables;", + "import com.google.common.collect.Lists;", + "import com.google.common.collect.Maps;", + "import com.google.common.collect.Sets;"); + + List inputLines = new ArrayList<>(imports); + inputLines.add("class Test {{ " + before + "; }}"); + + List outputLines = new ArrayList<>(imports); + for (String addedImport : addedImports) { + outputLines.add("import " + addedImport + ";"); + } + outputLines.add("class Test {{ " + after + "; }}"); + + BugCheckerRefactoringTestHelper.newInstance(new PreferCollectionConstructors(), getClass()) + .addInputLines("Test.java", inputLines.toArray(new String[0])) + .addOutputLines("Test.java", outputLines.toArray(new String[0])) + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +} From 5bdded847b423e5d8af1812782f99d3690d022e7 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 3 Oct 2019 20:05:44 -0700 Subject: [PATCH 02/14] Changed from SeverityLevel.WARNING to SeverityLevel.SUGGESTION. --- .../baseline/errorprone/PreferCollectionConstructors.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 023f0ab9a..2dafa71fb 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -55,7 +55,7 @@ name = "PreferCollectionConstructors", link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, - severity = BugPattern.SeverityLevel.WARNING, + severity = BugPattern.SeverityLevel.SUGGESTION, summary = "Since Java 7 the standard collection constructors should be used instead of the static factory " + "methods provided by Guava.") public final class PreferCollectionConstructors extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { From dc575f0a487f24d4b587c9145f8d4542b6a27c8d Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 3 Oct 2019 20:10:06 -0700 Subject: [PATCH 03/14] Added changelog entry. --- changelog/@unreleased/pr-941.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-941.v2.yml diff --git a/changelog/@unreleased/pr-941.v2.yml b/changelog/@unreleased/pr-941.v2.yml new file mode 100644 index 000000000..992263c37 --- /dev/null +++ b/changelog/@unreleased/pr-941.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Errorprone rules for usage of Guava static factory methods + links: + - https://github.com/palantir/gradle-baseline/pull/941 From 627d86040760db3a5748ed0b111f60ebf3fa78d4 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 3 Oct 2019 20:16:15 -0700 Subject: [PATCH 04/14] Organized imports. --- .../errorprone/PreferCollectionConstructorsTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java index 1f9407951..67359cd19 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java @@ -17,15 +17,14 @@ package com.palantir.baseline.errorprone; import com.google.errorprone.BugCheckerRefactoringTestHelper; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + @Execution(ExecutionMode.CONCURRENT) public final class PreferCollectionConstructorsTest { From 5b23e758eb07e0830ca39fa6acce178f91cf0e16 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 3 Oct 2019 22:26:17 -0700 Subject: [PATCH 05/14] Applied spotless. --- .../errorprone/PreferCollectionConstructorsTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java index 67359cd19..ca6df0511 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java @@ -17,13 +17,12 @@ package com.palantir.baseline.errorprone; import com.google.errorprone.BugCheckerRefactoringTestHelper; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.parallel.Execution; -import org.junit.jupiter.api.parallel.ExecutionMode; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; @Execution(ExecutionMode.CONCURRENT) public final class PreferCollectionConstructorsTest { From f1e8730593751cf3bfaec18fbb1a619b0378a9f9 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Fri, 4 Oct 2019 21:10:07 -0700 Subject: [PATCH 06/14] Check parameter types using withParameters instead. --- .../PreferCollectionConstructors.java | 297 ++++++++++++------ 1 file changed, 195 insertions(+), 102 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 2dafa71fb..0c02fa606 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -17,6 +17,7 @@ package com.palantir.baseline.errorprone; import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -29,10 +30,10 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; -import com.sun.tools.javac.tree.JCTree; +import com.sun.tools.javac.tree.JCTree.JCExpression; +import com.sun.tools.javac.tree.JCTree.JCMethodInvocation; +import com.sun.tools.javac.util.List; import java.util.ArrayList; -import java.util.Collection; -import java.util.Comparator; import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; @@ -40,9 +41,6 @@ import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.LinkedList; -import java.util.List; -import java.util.Map; -import java.util.SortedMap; import java.util.TreeMap; import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; @@ -65,23 +63,44 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_ARRAY_LIST = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") - .named("newArrayList"); + .named("newArrayList") + .withParameters(); + + private static final Matcher NEW_ARRAY_LIST_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newArrayList") + .withParameters("java.lang.Iterable"); + + private static final Matcher NEW_ARRAY_LIST_WITH_CAPACITY = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .namedAnyOf("newArrayListWithCapacity", "newArrayListWithExpectedSize") + .withParameters("int"); private static final Matcher NEW_LINKED_LIST = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") - .named("newLinkedList"); + .named("newLinkedList") + .withParameters(); - private static final Matcher NEW_COPY_ON_WRITE_ARRAY_LIST = + private static final Matcher NEW_LINKED_LIST_WITH_ITERABLE = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") - .named("newCopyOnWriteArrayList"); + .named("newLinkedList") + .withParameters("java.lang.Iterable"); - private static final Matcher NEW_ARRAY_LIST_WITH_CAPACITY = + private static final Matcher NEW_COW_ARRAY_LIST = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") - .namedAnyOf("newArrayListWithCapacity", "newArrayListWithExpectedSize") - .withParameters("int"); + .named("newCopyOnWriteArrayList") + .withParameters(); + + private static final Matcher NEW_COW_ARRAY_LIST_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newCopyOnWriteArrayList") + .withParameters("java.lang.Iterable"); private static final Matcher NEW_CONCURRENT_MAP = MethodMatchers.staticMethod() @@ -92,7 +111,14 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_HASH_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") - .named("newHashMap"); + .named("newHashMap") + .withParameters(); + + private static final Matcher NEW_HASH_MAP_WITH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newHashMap") + .withParameters("java.util.Map"); private static final Matcher NEW_HASH_MAP_WITH_EXPECTED_SIZE = MethodMatchers.staticMethod() @@ -103,17 +129,44 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_TREE_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") - .named("newTreeMap"); + .named("newTreeMap") + .withParameters(); + + private static final Matcher NEW_TREE_MAP_WITH_COMPARATOR = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newTreeMap") + .withParameters("java.util.Comparator"); + + private static final Matcher NEW_TREE_MAP_WITH_SORTED_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newTreeMap") + .withParameters("java.util.SortedMap"); + + private static final Matcher NEW_COW_ARRAY_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newCopyOnWriteArraySet") + .withParameters(); - private static final Matcher NEW_COPY_ON_WRITE_ARRAY_SET = + private static final Matcher NEW_COW_ARRAY_SET_WITH_ITERABLE = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") - .named("newCopyOnWriteArraySet"); + .named("newCopyOnWriteArraySet") + .withParameters("java.lang.Iterable"); private static final Matcher NEW_LINKED_HASH_SET = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") - .named("newLinkedHashSet"); + .named("newLinkedHashSet") + .withParameters(); + + private static final Matcher NEW_LINKED_HASH_SET_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newLinkedHashSet") + .withParameters("java.lang.Iterable"); private static final Matcher NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE = MethodMatchers.staticMethod() @@ -124,12 +177,32 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_TREE_SET = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") - .named("newTreeSet"); + .named("newTreeSet") + .withParameters(); + + private static final Matcher NEW_TREE_SET_WITH_COMPARATOR = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newTreeSet") + .withParameters("java.util.Comparator"); + + private static final Matcher NEW_TREE_SET_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newTreeSet") + .withParameters("java.lang.Iterable"); private static final Matcher NEW_HASH_SET = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") - .named("newHashSet"); + .named("newHashSet") + .withParameters(); + + private static final Matcher NEW_HASH_SET_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newHashSet") + .withParameters("java.lang.Iterable"); private static final Matcher NEW_HASH_SET_WITH_EXPECTED_SIZE = MethodMatchers.staticMethod() @@ -140,7 +213,14 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_LINKED_HASH_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") - .named("newLinkedHashMap"); + .named("newLinkedHashMap") + .withParameters(); + + private static final Matcher NEW_LINKED_HASH_MAP_WITH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newLinkedHashMap") + .withParameters("java.util.Map"); private static final Matcher NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE = MethodMatchers.staticMethod() @@ -151,7 +231,20 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_ENUM_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") - .named("newEnumMap"); + .named("newEnumMap") + .withParameters(); + + private static final Matcher NEW_ENUM_MAP_WITH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newEnumMap") + .withParameters("java.util.Map"); + + private static final Matcher NEW_ENUM_MAP_WITH_CLASS = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .named("newEnumMap") + .withParameters("java.lang.Class"); private static final Matcher NEW_IDENTITY_HASH_MAP = MethodMatchers.staticMethod() @@ -160,91 +253,113 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - // Param types used to figure out which overload is called - List paramTypes = ((JCTree.JCExpression) tree.getMethodSelect()).type.asMethodType().argtypes; - - // Argument types used to differentiate between an Iterable and a Collection - List argTypes = ((JCTree.JCMethodInvocation) tree).args - .stream().map((JCTree.JCExpression e) -> e.type).collect(Collectors.toList()); + // Argument type used to differentiate between an Iterable and a Collection + List args = ((JCMethodInvocation) tree).args; + Type firstArgType = args.isEmpty() ? null : args.get(0).type; // For convenience - ExpressionTree arg = tree.getArguments().size() == 1 ? tree.getArguments().get(0) : null; + ExpressionTree firstArg = Iterables.getFirst(tree.getArguments(), null); - if (NEW_ARRAY_LIST.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, ArrayList.class, arg); + if (NEW_ARRAY_LIST.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); + } + if (NEW_ARRAY_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); } if (NEW_ARRAY_LIST_WITH_CAPACITY.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, ArrayList.class, arg); + return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); + } + if (NEW_LINKED_LIST.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, LinkedList.class, firstArg); } - if (NEW_LINKED_LIST.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, LinkedList.class, arg); + if (NEW_LINKED_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, LinkedList.class, firstArg); } - if (NEW_COPY_ON_WRITE_ARRAY_LIST.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, CopyOnWriteArrayList.class, arg); + if (NEW_COW_ARRAY_LIST.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, CopyOnWriteArrayList.class, firstArg); + } + if (NEW_COW_ARRAY_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, CopyOnWriteArrayList.class, firstArg); } if (NEW_CONCURRENT_MAP.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, ConcurrentHashMap.class, arg); + return buildConstructorFixSuggestion(tree, state, ConcurrentHashMap.class, firstArg); + } + if (NEW_HASH_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); } - if (NEW_HASH_MAP.matches(tree, state) - && (paramTypes.isEmpty() || checkParamTypes(state, paramTypes, Map.class))) { - return buildSingleArgFixSuggestion(tree, state, HashMap.class, arg); + if (NEW_HASH_MAP_WITH_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); } if (NEW_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, HashMap.class, arg); + return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); + } + if (NEW_COW_ARRAY_SET.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, CopyOnWriteArraySet.class, firstArg); } - if (NEW_COPY_ON_WRITE_ARRAY_SET.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, CopyOnWriteArraySet.class, arg); + if (NEW_COW_ARRAY_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, CopyOnWriteArraySet.class, firstArg); } - if (NEW_LINKED_HASH_SET.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, LinkedHashSet.class, arg); + if (NEW_LINKED_HASH_SET.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); + } + if (NEW_LINKED_HASH_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); } if (NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, LinkedHashSet.class, arg); + return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); + } + if (NEW_TREE_SET.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); } - if (NEW_TREE_SET.matches(tree, state) - && (paramTypes.isEmpty() - || checkParamTypes(state, paramTypes, Comparator.class) - || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, TreeSet.class, arg); + if (NEW_TREE_SET_WITH_COMPARATOR.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); } - if (NEW_HASH_SET.matches(tree, state) - && (paramTypes.isEmpty() || isIterableParamCollectionArg(state, paramTypes, argTypes))) { - return buildSingleArgFixSuggestion(tree, state, HashSet.class, arg); + if (NEW_TREE_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); + } + if (NEW_HASH_SET.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); + } + if (NEW_HASH_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { + return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); } if (NEW_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, HashSet.class, arg); + return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); + } + if (NEW_TREE_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); + } + if (NEW_TREE_MAP_WITH_SORTED_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); + } + if (NEW_TREE_MAP_WITH_COMPARATOR.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); } - if (NEW_TREE_MAP.matches(tree, state) - && (paramTypes.isEmpty() - || checkParamTypes(state, paramTypes, SortedMap.class) - || checkParamTypes(state, paramTypes, Comparator.class))) { - return buildSingleArgFixSuggestion(tree, state, TreeMap.class, arg); + if (NEW_LINKED_HASH_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); } - if (NEW_LINKED_HASH_MAP.matches(tree, state) - && (paramTypes.isEmpty() || checkParamTypes(state, paramTypes, Map.class))) { - return buildSingleArgFixSuggestion(tree, state, LinkedHashMap.class, arg); + if (NEW_LINKED_HASH_MAP_WITH_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); } if (NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, LinkedHashMap.class, arg); + return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); } - if (NEW_ENUM_MAP.matches(tree, state) - && (paramTypes.isEmpty() - || checkParamTypes(state, paramTypes, Class.class) - || checkParamTypes(state, paramTypes, Map.class))) { - return buildSingleArgFixSuggestion(tree, state, EnumMap.class, arg); + if (NEW_ENUM_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); + } + if (NEW_ENUM_MAP_WITH_CLASS.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); + } + if (NEW_ENUM_MAP_WITH_MAP.matches(tree, state)) { + return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); } if (NEW_IDENTITY_HASH_MAP.matches(tree, state)) { - return buildSingleArgFixSuggestion(tree, state, IdentityHashMap.class, arg); + return buildConstructorFixSuggestion(tree, state, IdentityHashMap.class, firstArg); } return Description.NO_MATCH; } - private Description buildSingleArgFixSuggestion( + private Description buildConstructorFixSuggestion( MethodInvocationTree tree, VisitorState state, Class collectionClass, @@ -258,36 +373,14 @@ private Description buildSingleArgFixSuggestion( String arg = argTree == null ? "" : state.getSourceForNode(argTree); String replacement = "new " + collectionType + "<" + typeArgs + ">(" + arg + ")"; return buildDescription(tree) - .setMessage("The factory method call should be replaced with a constructor call.") + .setMessage("The factory method call should be replaced with a constructor call. " + + "See https://git.io/JeCT6 for more information.") .addFix(fixBuilder.replace(tree, replacement).build()) .build(); } - private boolean isIterableParamCollectionArg(VisitorState state, List params, List args) { - return checkParamTypes(state, params, Iterable.class) - && checkArgTypes(state, args, Collection.class); - } - - private boolean checkParamTypes(VisitorState state, List params, Class... expected) { - return checkTypes(state, false, params, expected); - } - - private boolean checkArgTypes(VisitorState state, List args, Class... expected) { - return checkTypes(state, true, args, expected); - } - - private boolean checkTypes(VisitorState state, boolean subtypes, List typeList1, Class... typeList2) { + private boolean isCollectionArg(VisitorState state, Type type) { Types types = state.getTypes(); - if (typeList1.size() != typeList2.length) { - return false; - } - for (int i = 0; i < typeList1.size(); i++) { - Type t1 = types.erasure(typeList1.get(i)); - Type t2 = types.erasure(state.getTypeFromString(typeList2[i].getName())); - if ((!subtypes && !types.isSameType(t1, t2)) || (subtypes && !types.isSubtype(t1, t2))) { - return false; - } - } - return true; + return types.isSubtype(types.erasure(type), types.erasure(state.getTypeFromString("java.util.Collection"))); } } From 5bbb3669a990aa9a6f14278e46432422518b6704 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Sat, 5 Oct 2019 10:18:57 -0700 Subject: [PATCH 07/14] Reduced cyclomatic complexity by introducing a map. --- .../PreferCollectionConstructors.java | 197 +++++++----------- 1 file changed, 78 insertions(+), 119 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 0c02fa606..1ea0830ee 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -17,7 +17,8 @@ package com.palantir.baseline.errorprone; import com.google.auto.service.AutoService; -import com.google.common.collect.Iterables; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -28,7 +29,6 @@ import com.google.errorprone.matchers.method.MethodMatchers; 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 com.sun.tools.javac.tree.JCTree.JCExpression; import com.sun.tools.javac.tree.JCTree.JCMethodInvocation; @@ -41,6 +41,8 @@ import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.LinkedList; +import java.util.Map; +import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; @@ -90,13 +92,13 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newLinkedList") .withParameters("java.lang.Iterable"); - private static final Matcher NEW_COW_ARRAY_LIST = + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_LIST = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") .named("newCopyOnWriteArrayList") .withParameters(); - private static final Matcher NEW_COW_ARRAY_LIST_WITH_ITERABLE = + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_LIST_WITH_ITERABLE = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") .named("newCopyOnWriteArrayList") @@ -144,13 +146,13 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newTreeMap") .withParameters("java.util.SortedMap"); - private static final Matcher NEW_COW_ARRAY_SET = + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_SET = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") .named("newCopyOnWriteArraySet") .withParameters(); - private static final Matcher NEW_COW_ARRAY_SET_WITH_ITERABLE = + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") .named("newCopyOnWriteArraySet") @@ -251,136 +253,93 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .onClass("com.google.common.collect.Maps") .named("newIdentityHashMap"); + private static final Map, Class> classMap = + ImmutableMap., Class>builder() + .put(NEW_ARRAY_LIST, ArrayList.class) + .put(NEW_ARRAY_LIST_WITH_ITERABLE, ArrayList.class) + .put(NEW_ARRAY_LIST_WITH_CAPACITY, ArrayList.class) + .put(NEW_LINKED_LIST, LinkedList.class) + .put(NEW_LINKED_LIST_WITH_ITERABLE, LinkedList.class) + .put(NEW_COPY_ON_WRITE_ARRAY_LIST, CopyOnWriteArrayList.class) + .put(NEW_COPY_ON_WRITE_ARRAY_LIST_WITH_ITERABLE, CopyOnWriteArrayList.class) + .put(NEW_CONCURRENT_MAP, ConcurrentHashMap.class) + .put(NEW_HASH_MAP, HashMap.class) + .put(NEW_HASH_MAP_WITH_MAP, HashMap.class) + .put(NEW_HASH_MAP_WITH_EXPECTED_SIZE, HashMap.class) + .put(NEW_COPY_ON_WRITE_ARRAY_SET, CopyOnWriteArraySet.class) + .put(NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE, CopyOnWriteArraySet.class) + .put(NEW_LINKED_HASH_SET, LinkedHashSet.class) + .put(NEW_LINKED_HASH_SET_WITH_ITERABLE, LinkedHashSet.class) + .put(NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE, LinkedHashSet.class) + .put(NEW_TREE_SET, TreeSet.class) + .put(NEW_TREE_SET_WITH_COMPARATOR, TreeSet.class) + .put(NEW_TREE_SET_WITH_ITERABLE, TreeSet.class) + .put(NEW_HASH_SET, HashSet.class) + .put(NEW_HASH_SET_WITH_ITERABLE, HashSet.class) + .put(NEW_HASH_SET_WITH_EXPECTED_SIZE, HashSet.class) + .put(NEW_TREE_MAP, TreeMap.class) + .put(NEW_TREE_MAP_WITH_SORTED_MAP, TreeMap.class) + .put(NEW_TREE_MAP_WITH_COMPARATOR, TreeMap.class) + .put(NEW_LINKED_HASH_MAP, LinkedHashMap.class) + .put(NEW_LINKED_HASH_MAP_WITH_MAP, LinkedHashMap.class) + .put(NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE, LinkedHashMap.class) + .put(NEW_ENUM_MAP, EnumMap.class) + .put(NEW_ENUM_MAP_WITH_CLASS, EnumMap.class) + .put(NEW_ENUM_MAP_WITH_MAP, EnumMap.class) + .put(NEW_IDENTITY_HASH_MAP, IdentityHashMap.class) + .build(); + + private static final Set> requiresCollectionArg = ImmutableSet.of( + NEW_ARRAY_LIST_WITH_ITERABLE, + NEW_LINKED_LIST_WITH_ITERABLE, + NEW_COPY_ON_WRITE_ARRAY_LIST_WITH_ITERABLE, + NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE, + NEW_LINKED_HASH_SET_WITH_ITERABLE, + NEW_TREE_SET_WITH_ITERABLE, + NEW_HASH_SET_WITH_ITERABLE); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - // Argument type used to differentiate between an Iterable and a Collection - List args = ((JCMethodInvocation) tree).args; - Type firstArgType = args.isEmpty() ? null : args.get(0).type; - - // For convenience - ExpressionTree firstArg = Iterables.getFirst(tree.getArguments(), null); - if (NEW_ARRAY_LIST.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); - } - if (NEW_ARRAY_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); - } - if (NEW_ARRAY_LIST_WITH_CAPACITY.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, ArrayList.class, firstArg); - } - if (NEW_LINKED_LIST.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedList.class, firstArg); - } - if (NEW_LINKED_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, LinkedList.class, firstArg); - } - if (NEW_COW_ARRAY_LIST.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, CopyOnWriteArrayList.class, firstArg); - } - if (NEW_COW_ARRAY_LIST_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, CopyOnWriteArrayList.class, firstArg); - } - if (NEW_CONCURRENT_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, ConcurrentHashMap.class, firstArg); - } - if (NEW_HASH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); - } - if (NEW_HASH_MAP_WITH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); - } - if (NEW_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, HashMap.class, firstArg); - } - if (NEW_COW_ARRAY_SET.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, CopyOnWriteArraySet.class, firstArg); - } - if (NEW_COW_ARRAY_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, CopyOnWriteArraySet.class, firstArg); - } - if (NEW_LINKED_HASH_SET.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); - } - if (NEW_LINKED_HASH_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); - } - if (NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashSet.class, firstArg); - } - if (NEW_TREE_SET.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); - } - if (NEW_TREE_SET_WITH_COMPARATOR.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); + Class collectionClass = findCollectionClassToUse(state, tree); + if (collectionClass == null) { + return Description.NO_MATCH; } - if (NEW_TREE_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, TreeSet.class, firstArg); - } - if (NEW_HASH_SET.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); - } - if (NEW_HASH_SET_WITH_ITERABLE.matches(tree, state) && isCollectionArg(state, firstArgType)) { - return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); - } - if (NEW_HASH_SET_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, HashSet.class, firstArg); - } - if (NEW_TREE_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); - } - if (NEW_TREE_MAP_WITH_SORTED_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); - } - if (NEW_TREE_MAP_WITH_COMPARATOR.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, TreeMap.class, firstArg); - } - if (NEW_LINKED_HASH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); - } - if (NEW_LINKED_HASH_MAP_WITH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); - } - if (NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, LinkedHashMap.class, firstArg); - } - if (NEW_ENUM_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); - } - if (NEW_ENUM_MAP_WITH_CLASS.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); - } - if (NEW_ENUM_MAP_WITH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, EnumMap.class, firstArg); - } - if (NEW_IDENTITY_HASH_MAP.matches(tree, state)) { - return buildConstructorFixSuggestion(tree, state, IdentityHashMap.class, firstArg); - } - return Description.NO_MATCH; - } - private Description buildConstructorFixSuggestion( - MethodInvocationTree tree, - VisitorState state, - Class collectionClass, - ExpressionTree argTree) { SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); String collectionType = SuggestedFixes.qualifyType(state, fixBuilder, collectionClass.getName()); String typeArgs = tree.getTypeArguments() .stream() .map(state::getSourceForNode) .collect(Collectors.joining(", ")); - String arg = argTree == null ? "" : state.getSourceForNode(argTree); + String arg = tree.getArguments().isEmpty() ? "" : state.getSourceForNode(tree.getArguments().get(0)); String replacement = "new " + collectionType + "<" + typeArgs + ">(" + arg + ")"; return buildDescription(tree) - .setMessage("The factory method call should be replaced with a constructor call. " + - "See https://git.io/JeCT6 for more information.") + .setMessage("The factory method call should be replaced with a constructor call. " + + "See https://git.io/JeCT6 for more information.") .addFix(fixBuilder.replace(tree, replacement).build()) .build(); } - private boolean isCollectionArg(VisitorState state, Type type) { + private Class findCollectionClassToUse(VisitorState state, ExpressionTree tree) { + boolean firstArgIsCollection = isFirstArgCollection(state, tree); + for (Map.Entry, Class> entry : classMap.entrySet()) { + Matcher matcher = entry.getKey(); + if (matcher.matches(tree, state) && (!requiresCollectionArg.contains(matcher) || firstArgIsCollection)) { + return entry.getValue(); + } + } + return null; + } + + private boolean isFirstArgCollection(VisitorState state, ExpressionTree tree) { + List args = ((JCMethodInvocation) tree).args; + if (args.isEmpty()) { + return false; + } Types types = state.getTypes(); - return types.isSubtype(types.erasure(type), types.erasure(state.getTypeFromString("java.util.Collection"))); + return types.isSubtype( + types.erasure(args.get(0).type), + types.erasure(state.getTypeFromString("java.util.Collection"))); } } From 14265020434f8efb83def769c67f4f6ee4fe7984 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Mon, 7 Oct 2019 08:59:51 -0700 Subject: [PATCH 08/14] Always return a result after first match. --- .../errorprone/PreferCollectionConstructors.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 1ea0830ee..968f2be5b 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -322,11 +322,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } private Class findCollectionClassToUse(VisitorState state, ExpressionTree tree) { - boolean firstArgIsCollection = isFirstArgCollection(state, tree); for (Map.Entry, Class> entry : classMap.entrySet()) { Matcher matcher = entry.getKey(); - if (matcher.matches(tree, state) && (!requiresCollectionArg.contains(matcher) || firstArgIsCollection)) { - return entry.getValue(); + if (matcher.matches(tree, state)) { + if (!requiresCollectionArg.contains(matcher) || isFirstArgCollection(state, tree)) { + return entry.getValue(); + } + // All matchers are mutually exclusive, so no point in looking for another match. + break; } } return null; From 41101f217bd184ddd82f05b07ded2730cfff90ee Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Mon, 7 Oct 2019 09:08:35 -0700 Subject: [PATCH 09/14] Moved most common matchers (according to a big repo I happened to have checked out) to the top. --- .../errorprone/PreferCollectionConstructors.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 968f2be5b..d5fe67ee3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -256,16 +256,20 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Map, Class> classMap = ImmutableMap., Class>builder() .put(NEW_ARRAY_LIST, ArrayList.class) - .put(NEW_ARRAY_LIST_WITH_ITERABLE, ArrayList.class) + .put(NEW_HASH_SET, HashSet.class) + .put(NEW_HASH_MAP, HashMap.class) .put(NEW_ARRAY_LIST_WITH_CAPACITY, ArrayList.class) + .put(NEW_HASH_MAP_WITH_EXPECTED_SIZE, HashMap.class) + .put(NEW_HASH_SET_WITH_EXPECTED_SIZE, HashSet.class) + .put(NEW_LINKED_HASH_MAP, LinkedHashMap.class) + .put(NEW_TREE_MAP, TreeMap.class) + .put(NEW_CONCURRENT_MAP, ConcurrentHashMap.class) + .put(NEW_ARRAY_LIST_WITH_ITERABLE, ArrayList.class) .put(NEW_LINKED_LIST, LinkedList.class) .put(NEW_LINKED_LIST_WITH_ITERABLE, LinkedList.class) .put(NEW_COPY_ON_WRITE_ARRAY_LIST, CopyOnWriteArrayList.class) .put(NEW_COPY_ON_WRITE_ARRAY_LIST_WITH_ITERABLE, CopyOnWriteArrayList.class) - .put(NEW_CONCURRENT_MAP, ConcurrentHashMap.class) - .put(NEW_HASH_MAP, HashMap.class) .put(NEW_HASH_MAP_WITH_MAP, HashMap.class) - .put(NEW_HASH_MAP_WITH_EXPECTED_SIZE, HashMap.class) .put(NEW_COPY_ON_WRITE_ARRAY_SET, CopyOnWriteArraySet.class) .put(NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE, CopyOnWriteArraySet.class) .put(NEW_LINKED_HASH_SET, LinkedHashSet.class) @@ -274,13 +278,9 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .put(NEW_TREE_SET, TreeSet.class) .put(NEW_TREE_SET_WITH_COMPARATOR, TreeSet.class) .put(NEW_TREE_SET_WITH_ITERABLE, TreeSet.class) - .put(NEW_HASH_SET, HashSet.class) .put(NEW_HASH_SET_WITH_ITERABLE, HashSet.class) - .put(NEW_HASH_SET_WITH_EXPECTED_SIZE, HashSet.class) - .put(NEW_TREE_MAP, TreeMap.class) .put(NEW_TREE_MAP_WITH_SORTED_MAP, TreeMap.class) .put(NEW_TREE_MAP_WITH_COMPARATOR, TreeMap.class) - .put(NEW_LINKED_HASH_MAP, LinkedHashMap.class) .put(NEW_LINKED_HASH_MAP_WITH_MAP, LinkedHashMap.class) .put(NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE, LinkedHashMap.class) .put(NEW_ENUM_MAP, EnumMap.class) From 3899dac79ac0f87029aa24eabfcfca55bc3d2175 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Tue, 8 Oct 2019 20:13:16 -0700 Subject: [PATCH 10/14] Update baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java Co-Authored-By: David Schlosnagle --- .../baseline/errorprone/PreferCollectionConstructors.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index d5fe67ee3..55661b9ef 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -316,7 +316,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState String replacement = "new " + collectionType + "<" + typeArgs + ">(" + arg + ")"; return buildDescription(tree) .setMessage("The factory method call should be replaced with a constructor call. " - + "See https://git.io/JeCT6 for more information.") + + "See https://github.com/palantir/gradle-baseline/blob/develop/docs/best-practices/java-coding-guidelines/readme.md#avoid-generics-clutter-where-possible for more information.") .addFix(fixBuilder.replace(tree, replacement).build()) .build(); } From 0d78c96730230e035955e1ca08b16434f78dced1 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Wed, 9 Oct 2019 06:39:52 -0700 Subject: [PATCH 11/14] Added REQUIRES_HUMAN_ATTENTION in annotation. --- .../baseline/errorprone/PreferCollectionConstructors.java | 1 + 1 file changed, 1 insertion(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 55661b9ef..1624350cf 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -56,6 +56,7 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, severity = BugPattern.SeverityLevel.SUGGESTION, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, summary = "Since Java 7 the standard collection constructors should be used instead of the static factory " + "methods provided by Guava.") public final class PreferCollectionConstructors extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { From 73c534eb28f7fa13cd41cbda0738d690a81990db Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Wed, 9 Oct 2019 06:43:02 -0700 Subject: [PATCH 12/14] Added PreferCollectionConstructors to BaselineErrorProneExtension.DEFAULT_PATCH_CHECKS. --- .../baseline/extensions/BaselineErrorProneExtension.java | 1 + 1 file changed, 1 insertion(+) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 65b57e295..620f4c811 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -27,6 +27,7 @@ public class BaselineErrorProneExtension { "LambdaMethodReference", "OptionalOrElseMethodInvocation", "PreferBuiltInConcurrentKeySet", + "PreferCollectionConstructors", "PreferCollectionTransform", "PreferListsPartition", "PreferSafeLoggableExceptions", From 06f5f5c55ef938763438574f7d21581a35512e8a Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Thu, 10 Oct 2019 08:16:26 -0700 Subject: [PATCH 13/14] Dropped rerwrite rules for *WithExpectedSize methods. --- .../PreferCollectionConstructors.java | 30 +------------- .../PreferCollectionConstructorsTest.java | 40 ------------------- 2 files changed, 1 insertion(+), 69 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java index 1624350cf..f033571ca 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -78,7 +78,7 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu private static final Matcher NEW_ARRAY_LIST_WITH_CAPACITY = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Lists") - .namedAnyOf("newArrayListWithCapacity", "newArrayListWithExpectedSize") + .named("newArrayListWithCapacity") .withParameters("int"); private static final Matcher NEW_LINKED_LIST = @@ -123,12 +123,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newHashMap") .withParameters("java.util.Map"); - private static final Matcher NEW_HASH_MAP_WITH_EXPECTED_SIZE = - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Maps") - .named("newHashMapWithExpectedSize") - .withParameters("int"); - private static final Matcher NEW_TREE_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") @@ -171,12 +165,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newLinkedHashSet") .withParameters("java.lang.Iterable"); - private static final Matcher NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE = - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Sets") - .named("newLinkedHashSetWithExpectedSize") - .withParameters("int"); - private static final Matcher NEW_TREE_SET = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Sets") @@ -207,12 +195,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newHashSet") .withParameters("java.lang.Iterable"); - private static final Matcher NEW_HASH_SET_WITH_EXPECTED_SIZE = - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Sets") - .named("newHashSetWithExpectedSize") - .withParameters("int"); - private static final Matcher NEW_LINKED_HASH_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") @@ -225,12 +207,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .named("newLinkedHashMap") .withParameters("java.util.Map"); - private static final Matcher NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE = - MethodMatchers.staticMethod() - .onClass("com.google.common.collect.Maps") - .named("newLinkedHashMapWithExpectedSize") - .withParameters("int"); - private static final Matcher NEW_ENUM_MAP = MethodMatchers.staticMethod() .onClass("com.google.common.collect.Maps") @@ -260,8 +236,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .put(NEW_HASH_SET, HashSet.class) .put(NEW_HASH_MAP, HashMap.class) .put(NEW_ARRAY_LIST_WITH_CAPACITY, ArrayList.class) - .put(NEW_HASH_MAP_WITH_EXPECTED_SIZE, HashMap.class) - .put(NEW_HASH_SET_WITH_EXPECTED_SIZE, HashSet.class) .put(NEW_LINKED_HASH_MAP, LinkedHashMap.class) .put(NEW_TREE_MAP, TreeMap.class) .put(NEW_CONCURRENT_MAP, ConcurrentHashMap.class) @@ -275,7 +249,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .put(NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE, CopyOnWriteArraySet.class) .put(NEW_LINKED_HASH_SET, LinkedHashSet.class) .put(NEW_LINKED_HASH_SET_WITH_ITERABLE, LinkedHashSet.class) - .put(NEW_LINKED_HASH_SET_WITH_EXPECTED_SIZE, LinkedHashSet.class) .put(NEW_TREE_SET, TreeSet.class) .put(NEW_TREE_SET_WITH_COMPARATOR, TreeSet.class) .put(NEW_TREE_SET_WITH_ITERABLE, TreeSet.class) @@ -283,7 +256,6 @@ public final class PreferCollectionConstructors extends BugChecker implements Bu .put(NEW_TREE_MAP_WITH_SORTED_MAP, TreeMap.class) .put(NEW_TREE_MAP_WITH_COMPARATOR, TreeMap.class) .put(NEW_LINKED_HASH_MAP_WITH_MAP, LinkedHashMap.class) - .put(NEW_LINKED_HASH_MAP_WITH_EXPECTED_SIZE, LinkedHashMap.class) .put(NEW_ENUM_MAP, EnumMap.class) .put(NEW_ENUM_MAP_WITH_CLASS, EnumMap.class) .put(NEW_ENUM_MAP_WITH_MAP, EnumMap.class) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java index ca6df0511..cd1b723b7 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java @@ -51,14 +51,6 @@ public void testNewArrayListWithCapacityRewrite() { "java.util.ArrayList"); } - @Test - public void testNewArrayListWithExpectedSizeRewrite() { - testStaticFactoryMethodRewrite( - "Lists.newArrayListWithExpectedSize(123)", - "new ArrayList<>(123)", - "java.util.ArrayList"); - } - @Test public void testNewLinkedListRewrite() { testStaticFactoryMethodRewrite( @@ -131,14 +123,6 @@ public void testNewHashMapWithMapRewrite() { "java.util.HashMap"); } - @Test - public void testNewHashMapWithExpectedSizeRewrite() { - testStaticFactoryMethodRewrite( - "Maps.newHashMapWithExpectedSize(123)", - "new HashMap<>(123)", - "java.util.HashMap"); - } - @Test public void testNewIdentityHashMapRewrite() { testStaticFactoryMethodRewrite( @@ -163,14 +147,6 @@ public void testNewLinkedHashMapWithMapRewrite() { "java.util.LinkedHashMap"); } - @Test - public void testNewLinkedHashMapWithExpectedSizeRewrite() { - testStaticFactoryMethodRewrite( - "Maps.newLinkedHashMapWithExpectedSize(123)", - "new LinkedHashMap<>(123)", - "java.util.LinkedHashMap"); - } - @Test public void testNewTreeMapRewrite() { testStaticFactoryMethodRewrite( @@ -227,14 +203,6 @@ public void testNewHashSetWithSetRewrite() { "java.util.HashSet"); } - @Test - public void testNewHashSetWithExpectedSizeRewrite() { - testStaticFactoryMethodRewrite( - "Sets.newHashSetWithExpectedSize(123)", - "new HashSet<>(123)", - "java.util.HashSet"); - } - @Test public void testNewLinkedHashSetRewrite() { testStaticFactoryMethodRewrite( @@ -251,14 +219,6 @@ public void testNewLinkedHashSetWithSetRewrite() { "java.util.LinkedHashSet"); } - @Test - public void testNewLinkedHashSetWithExpectedSizeRewrite() { - testStaticFactoryMethodRewrite( - "Sets.newLinkedHashSetWithExpectedSize(123)", - "new LinkedHashSet<>(123)", - "java.util.LinkedHashSet"); - } - @Test public void testNewTreeSetRewrite() { testStaticFactoryMethodRewrite( From fa067266c4ee3241cd57949d84fc452759f4f4b2 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 14 Oct 2019 10:26:37 -0400 Subject: [PATCH 14/14] Update BaselineErrorProneExtension.java --- .../baseline/extensions/BaselineErrorProneExtension.java | 1 - 1 file changed, 1 deletion(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 620f4c811..65b57e295 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -27,7 +27,6 @@ public class BaselineErrorProneExtension { "LambdaMethodReference", "OptionalOrElseMethodInvocation", "PreferBuiltInConcurrentKeySet", - "PreferCollectionConstructors", "PreferCollectionTransform", "PreferListsPartition", "PreferSafeLoggableExceptions",