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..f033571ca --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java @@ -0,0 +1,321 @@ +/* + * (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.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; +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.Types; +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.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.Map; +import java.util.Set; +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.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 { + + private static final long serialVersionUID = 1L; + + private static final Matcher NEW_ARRAY_LIST = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .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") + .named("newArrayListWithCapacity") + .withParameters("int"); + + private static final Matcher NEW_LINKED_LIST = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newLinkedList") + .withParameters(); + + private static final Matcher NEW_LINKED_LIST_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Lists") + .named("newLinkedList") + .withParameters("java.lang.Iterable"); + + 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_COPY_ON_WRITE_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() + .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") + .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_TREE_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .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_COPY_ON_WRITE_ARRAY_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newCopyOnWriteArraySet") + .withParameters(); + + private static final Matcher NEW_COPY_ON_WRITE_ARRAY_SET_WITH_ITERABLE = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .named("newCopyOnWriteArraySet") + .withParameters("java.lang.Iterable"); + + private static final Matcher NEW_LINKED_HASH_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .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_TREE_SET = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Sets") + .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") + .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_LINKED_HASH_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .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_ENUM_MAP = + MethodMatchers.staticMethod() + .onClass("com.google.common.collect.Maps") + .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() + .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_HASH_SET, HashSet.class) + .put(NEW_HASH_MAP, HashMap.class) + .put(NEW_ARRAY_LIST_WITH_CAPACITY, ArrayList.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_HASH_MAP_WITH_MAP, 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_TREE_SET, TreeSet.class) + .put(NEW_TREE_SET_WITH_COMPARATOR, TreeSet.class) + .put(NEW_TREE_SET_WITH_ITERABLE, TreeSet.class) + .put(NEW_HASH_SET_WITH_ITERABLE, HashSet.class) + .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_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) { + + Class collectionClass = findCollectionClassToUse(state, tree); + if (collectionClass == null) { + return Description.NO_MATCH; + } + + 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 = 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://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(); + } + + private Class findCollectionClassToUse(VisitorState state, ExpressionTree tree) { + for (Map.Entry, Class> entry : classMap.entrySet()) { + Matcher matcher = entry.getKey(); + 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; + } + + 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(args.get(0).type), + types.erasure(state.getTypeFromString("java.util.Collection"))); + } +} 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..cd1b723b7 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCollectionConstructorsTest.java @@ -0,0 +1,297 @@ +/* + * (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 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 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 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 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 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 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); + } +} 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