From afb5064042c2209b8f585eb36050e4bab22c9f30 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 8 Oct 2023 12:10:54 +0200 Subject: [PATCH] Cover and simplify more cases --- .../refasterrules/AssertJMapRules.java | 91 +++------ .../refasterrules/AssertJRules.java | 73 ++----- .../AssertJMapRulesTestInput.java | 47 ++--- .../AssertJMapRulesTestOutput.java | 33 +--- .../errorprone/refaster/matchers/IsEmpty.java | 23 ++- .../refaster/matchers/IsEmptyTest.java | 187 ++++++++++++++---- 6 files changed, 222 insertions(+), 232 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java index f2067917604..53288f7611a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJMapRules.java @@ -3,80 +3,40 @@ import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static org.assertj.core.api.Assertions.assertThat; -import com.google.common.collect.ImmutableBiMap; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableMultiset; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.ImmutableSortedMultiset; -import com.google.common.collect.ImmutableSortedSet; import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.UseImportPolicy; -import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; -import java.util.TreeMap; -import java.util.TreeSet; import org.assertj.core.api.AbstractAssert; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.AbstractCollectionAssert; import org.assertj.core.api.AbstractMapAssert; import org.assertj.core.api.MapAssert; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsEmpty; @OnlineDocumentation final class AssertJMapRules { private AssertJMapRules() {} - // XXX: Reduce boilerplate using a `Matcher` that identifies "empty" instances. static final class AbstractMapAssertIsEmpty { @BeforeTemplate - @SuppressWarnings("unchecked") - void before(AbstractMapAssert mapAssert) { + void before( + AbstractMapAssert mapAssert, + @Matches(IsEmpty.class) Map wellTypedMap, + @Matches(IsEmpty.class) Map arbitraryMap, + @Matches(IsEmpty.class) Iterable keys) { Refaster.anyOf( - mapAssert.containsExactlyEntriesOf( - Refaster.anyOf( - ImmutableMap.of(), - ImmutableBiMap.of(), - ImmutableSortedMap.of(), - new HashMap<>(), - new LinkedHashMap<>(), - new TreeMap<>())), - mapAssert.hasSameSizeAs( - Refaster.anyOf( - ImmutableMap.of(), - ImmutableBiMap.of(), - ImmutableSortedMap.of(), - new HashMap<>(), - new LinkedHashMap<>(), - new TreeMap<>())), - mapAssert.isEqualTo( - Refaster.anyOf( - ImmutableMap.of(), - ImmutableBiMap.of(), - ImmutableSortedMap.of(), - new HashMap<>(), - new LinkedHashMap<>(), - new TreeMap<>())), - mapAssert.containsOnlyKeys( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), + mapAssert.containsExactlyEntriesOf(wellTypedMap), + mapAssert.containsExactlyInAnyOrderEntriesOf(wellTypedMap), + mapAssert.hasSameSizeAs(arbitraryMap), + mapAssert.isEqualTo(arbitraryMap), + mapAssert.containsOnlyKeys(keys), mapAssert.containsExactly(), mapAssert.containsOnly(), mapAssert.containsOnlyKeys()); @@ -112,15 +72,9 @@ void after(Map map) { static final class AbstractMapAssertIsNotEmpty { @BeforeTemplate - AbstractMapAssert before(AbstractMapAssert mapAssert) { - return mapAssert.isNotEqualTo( - Refaster.anyOf( - ImmutableMap.of(), - ImmutableBiMap.of(), - ImmutableSortedMap.of(), - new HashMap<>(), - new LinkedHashMap<>(), - new TreeMap<>())); + AbstractMapAssert before( + AbstractMapAssert mapAssert, @Matches(IsEmpty.class) Map map) { + return mapAssert.isNotEqualTo(map); } @AfterTemplate @@ -148,12 +102,14 @@ MapAssert after(Map map) { static final class AbstractMapAssertContainsExactlyInAnyOrderEntriesOf { @BeforeTemplate - AbstractMapAssert before(AbstractMapAssert mapAssert, Map map) { + AbstractMapAssert before( + AbstractMapAssert mapAssert, Map map) { return mapAssert.isEqualTo(map); } @AfterTemplate - AbstractMapAssert after(AbstractMapAssert mapAssert, Map map) { + AbstractMapAssert after( + AbstractMapAssert mapAssert, Map map) { return mapAssert.containsExactlyInAnyOrderEntriesOf(map); } } @@ -187,12 +143,12 @@ MapAssert after(Map map, int length) { static final class AbstractMapAssertHasSameSizeAs { @BeforeTemplate - AbstractMapAssert before(AbstractMapAssert mapAssert, Map map) { + AbstractMapAssert before(AbstractMapAssert mapAssert, Map map) { return mapAssert.hasSize(map.size()); } @AfterTemplate - AbstractMapAssert after(AbstractMapAssert mapAssert, Map map) { + AbstractMapAssert after(AbstractMapAssert mapAssert, Map map) { return mapAssert.hasSameSizeAs(map); } } @@ -225,13 +181,14 @@ MapAssert after(Map map, K key) { static final class AssertThatMapContainsOnlyKeys { @BeforeTemplate - AbstractCollectionAssert, K, ?> before(Map map, Set keys) { + AbstractCollectionAssert, K, ?> before( + Map map, Set keys) { return assertThat(map.keySet()).hasSameElementsAs(keys); } @AfterTemplate @UseImportPolicy(STATIC_IMPORT_ALWAYS) - MapAssert after(Map map, Set keys) { + MapAssert after(Map map, Set keys) { return assertThat(map).containsOnlyKeys(keys); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java index 49b92a4e15c..51ddd80a886 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssertJRules.java @@ -6,28 +6,23 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultiset; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMultiset; -import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Multiset; import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.NotMatches; import com.google.errorprone.refaster.annotation.Repeated; import com.google.errorprone.refaster.annotation.UseImportPolicy; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.OptionalDouble; import java.util.OptionalInt; import java.util.OptionalLong; import java.util.Set; -import java.util.TreeSet; import java.util.function.Predicate; import java.util.stream.Collector; import java.util.stream.Stream; @@ -47,6 +42,7 @@ import org.assertj.core.api.OptionalLongAssert; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; import tech.picnic.errorprone.refaster.matchers.IsArray; +import tech.picnic.errorprone.refaster.matchers.IsEmpty; /** Refaster rules related to AssertJ expressions and statements. */ // XXX: Most `AbstractIntegerAssert` rules can also be applied for other primitive types. Generate @@ -181,63 +177,16 @@ OptionalLongAssert after(OptionalLong optional, long expected) { static final class AssertThatObjectEnumerableIsEmpty { @BeforeTemplate @SuppressWarnings("unchecked") - void before(ObjectEnumerableAssert enumAssert) { + void before( + ObjectEnumerableAssert enumAssert, + @Matches(IsEmpty.class) Iterable wellTypedIterable, + @Matches(IsEmpty.class) Iterable arbitraryIterable) { Refaster.anyOf( - enumAssert.containsExactlyElementsOf( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), - enumAssert.containsExactlyInAnyOrderElementsOf( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), - enumAssert.hasSameElementsAs( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), - enumAssert.hasSameSizeAs( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), - enumAssert.isSubsetOf( - Refaster.anyOf( - ImmutableList.of(), - new ArrayList<>(), - ImmutableSet.of(), - new HashSet<>(), - new LinkedHashSet<>(), - ImmutableSortedSet.of(), - new TreeSet<>(), - ImmutableMultiset.of(), - ImmutableSortedMultiset.of())), + enumAssert.containsExactlyElementsOf(wellTypedIterable), + enumAssert.containsExactlyInAnyOrderElementsOf(wellTypedIterable), + enumAssert.hasSameElementsAs(wellTypedIterable), + enumAssert.hasSameSizeAs(arbitraryIterable), + enumAssert.isSubsetOf(wellTypedIterable), enumAssert.containsExactly(), enumAssert.containsExactlyInAnyOrder(), enumAssert.containsOnly(), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java index 7c41571ef71..ac54a5c5dd1 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java @@ -41,34 +41,19 @@ public ImmutableSet elidedTypesAndStaticImports() { void testAbstractMapAssertIsEmpty() { assertThat(ImmutableMap.of(1, 0)).containsExactlyEntriesOf(ImmutableMap.of()); - assertThat(ImmutableMap.of(2, 0)).containsExactlyEntriesOf(ImmutableBiMap.of()); - assertThat(ImmutableMap.of(3, 0)).containsExactlyEntriesOf(ImmutableSortedMap.of()); - assertThat(ImmutableMap.of(4, 0)).containsExactlyEntriesOf(new HashMap<>()); - assertThat(ImmutableMap.of(5, 0)).containsExactlyEntriesOf(new LinkedHashMap<>()); - assertThat(ImmutableMap.of(6, 0)).containsExactlyEntriesOf(new TreeMap<>()); - assertThat(ImmutableMap.of(7, 0)).hasSameSizeAs(ImmutableMap.of()); - assertThat(ImmutableMap.of(8, 0)).hasSameSizeAs(ImmutableBiMap.of()); - assertThat(ImmutableMap.of(9, 0)).hasSameSizeAs(ImmutableSortedMap.of()); - assertThat(ImmutableMap.of(10, 0)).hasSameSizeAs(new HashMap<>()); - assertThat(ImmutableMap.of(11, 0)).hasSameSizeAs(new LinkedHashMap<>()); - assertThat(ImmutableMap.of(12, 0)).hasSameSizeAs(new TreeMap<>()); - assertThat(ImmutableMap.of(13, 0)).isEqualTo(ImmutableMap.of()); - assertThat(ImmutableMap.of(14, 0)).isEqualTo(ImmutableBiMap.of()); - assertThat(ImmutableMap.of(15, 0)).isEqualTo(ImmutableSortedMap.of()); - assertThat(ImmutableMap.of(16, 0)).isEqualTo(new HashMap<>()); - assertThat(ImmutableMap.of(17, 0)).isEqualTo(new LinkedHashMap<>()); - assertThat(ImmutableMap.of(18, 0)).isEqualTo(new TreeMap<>()); - assertThat(ImmutableMap.of(19, 0)).containsOnlyKeys(ImmutableList.of()); - assertThat(ImmutableMap.of(20, 0)).containsOnlyKeys(new ArrayList<>()); - assertThat(ImmutableMap.of(21, 0)).containsOnlyKeys(ImmutableSet.of()); - assertThat(ImmutableMap.of(22, 0)).containsOnlyKeys(new HashSet<>()); - assertThat(ImmutableMap.of(23, 0)).containsOnlyKeys(ImmutableSortedSet.of()); - assertThat(ImmutableMap.of(24, 0)).containsOnlyKeys(new TreeSet<>()); - assertThat(ImmutableMap.of(25, 0)).containsOnlyKeys(ImmutableMultiset.of()); - assertThat(ImmutableMap.of(26, 0)).containsOnlyKeys(ImmutableSortedMultiset.of()); - assertThat(ImmutableMap.of(27, 0)).containsExactly(); - assertThat(ImmutableMap.of(28, 0)).containsOnly(); - assertThat(ImmutableMap.of(29, 0)).containsOnlyKeys(); + assertThat(ImmutableMap.of(2, 0)).containsExactlyEntriesOf(ImmutableMap.of(1, 2)); + assertThat(ImmutableMap.of(3, 0)).containsExactlyInAnyOrderEntriesOf(ImmutableMap.of()); + assertThat(ImmutableMap.of(4, 0)) + .containsExactlyInAnyOrderEntriesOf(ImmutableMap.of(1, 2, 3, 4)); + assertThat(ImmutableMap.of(5, 0)).hasSameSizeAs(ImmutableMap.of()); + assertThat(ImmutableMap.of(6, 0)).hasSameSizeAs(ImmutableMap.of(1, 2)); + assertThat(ImmutableMap.of(7, 0)).isEqualTo(ImmutableMap.of()); + assertThat(ImmutableMap.of(8, 0)).isEqualTo(ImmutableMap.of("foo", "bar")); + assertThat(ImmutableMap.of(9, 0)).containsOnlyKeys(ImmutableList.of()); + assertThat(ImmutableMap.of(10, 0)).containsOnlyKeys(ImmutableList.of(1)); + assertThat(ImmutableMap.of(11, 0)).containsExactly(); + assertThat(ImmutableMap.of(12, 0)).containsOnly(); + assertThat(ImmutableMap.of(13, 0)).containsOnlyKeys(); } void testAssertThatMapIsEmpty() { @@ -84,11 +69,7 @@ void testAssertThatMapIsEmpty() { ImmutableSet> testAbstractMapAssertIsNotEmpty() { return ImmutableSet.of( assertThat(ImmutableMap.of(1, 0)).isNotEqualTo(ImmutableMap.of()), - assertThat(ImmutableMap.of(2, 0)).isNotEqualTo(ImmutableBiMap.of()), - assertThat(ImmutableMap.of(3, 0)).isNotEqualTo(ImmutableSortedMap.of()), - assertThat(ImmutableMap.of(4, 0)).isNotEqualTo(new HashMap<>()), - assertThat(ImmutableMap.of(5, 0)).isNotEqualTo(new LinkedHashMap<>()), - assertThat(ImmutableMap.of(6, 0)).isNotEqualTo(new TreeMap<>())); + assertThat(ImmutableMap.of(2, 0)).isNotEqualTo(ImmutableMap.of("foo", "bar"))); } ImmutableSet> testAssertThatMapIsNotEmpty() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestOutput.java index c4483415625..5630c9aa72b 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestOutput.java @@ -41,34 +41,19 @@ public ImmutableSet elidedTypesAndStaticImports() { void testAbstractMapAssertIsEmpty() { assertThat(ImmutableMap.of(1, 0)).isEmpty(); - assertThat(ImmutableMap.of(2, 0)).isEmpty(); + assertThat(ImmutableMap.of(2, 0)).containsExactlyEntriesOf(ImmutableMap.of(1, 2)); assertThat(ImmutableMap.of(3, 0)).isEmpty(); - assertThat(ImmutableMap.of(4, 0)).isEmpty(); + assertThat(ImmutableMap.of(4, 0)) + .containsExactlyInAnyOrderEntriesOf(ImmutableMap.of(1, 2, 3, 4)); assertThat(ImmutableMap.of(5, 0)).isEmpty(); - assertThat(ImmutableMap.of(6, 0)).isEmpty(); + assertThat(ImmutableMap.of(6, 0)).hasSameSizeAs(ImmutableMap.of(1, 2)); assertThat(ImmutableMap.of(7, 0)).isEmpty(); - assertThat(ImmutableMap.of(8, 0)).isEmpty(); + assertThat(ImmutableMap.of(8, 0)).isEqualTo(ImmutableMap.of("foo", "bar")); assertThat(ImmutableMap.of(9, 0)).isEmpty(); - assertThat(ImmutableMap.of(10, 0)).isEmpty(); + assertThat(ImmutableMap.of(10, 0)).containsOnlyKeys(ImmutableList.of(1)); assertThat(ImmutableMap.of(11, 0)).isEmpty(); assertThat(ImmutableMap.of(12, 0)).isEmpty(); assertThat(ImmutableMap.of(13, 0)).isEmpty(); - assertThat(ImmutableMap.of(14, 0)).isEmpty(); - assertThat(ImmutableMap.of(15, 0)).isEmpty(); - assertThat(ImmutableMap.of(16, 0)).isEmpty(); - assertThat(ImmutableMap.of(17, 0)).isEmpty(); - assertThat(ImmutableMap.of(18, 0)).isEmpty(); - assertThat(ImmutableMap.of(19, 0)).isEmpty(); - assertThat(ImmutableMap.of(20, 0)).isEmpty(); - assertThat(ImmutableMap.of(21, 0)).isEmpty(); - assertThat(ImmutableMap.of(22, 0)).isEmpty(); - assertThat(ImmutableMap.of(23, 0)).isEmpty(); - assertThat(ImmutableMap.of(24, 0)).isEmpty(); - assertThat(ImmutableMap.of(25, 0)).isEmpty(); - assertThat(ImmutableMap.of(26, 0)).isEmpty(); - assertThat(ImmutableMap.of(27, 0)).isEmpty(); - assertThat(ImmutableMap.of(28, 0)).isEmpty(); - assertThat(ImmutableMap.of(29, 0)).isEmpty(); } void testAssertThatMapIsEmpty() { @@ -84,11 +69,7 @@ void testAssertThatMapIsEmpty() { ImmutableSet> testAbstractMapAssertIsNotEmpty() { return ImmutableSet.of( assertThat(ImmutableMap.of(1, 0)).isNotEmpty(), - assertThat(ImmutableMap.of(2, 0)).isNotEmpty(), - assertThat(ImmutableMap.of(3, 0)).isNotEmpty(), - assertThat(ImmutableMap.of(4, 0)).isNotEmpty(), - assertThat(ImmutableMap.of(5, 0)).isNotEmpty(), - assertThat(ImmutableMap.of(6, 0)).isNotEmpty()); + assertThat(ImmutableMap.of(2, 0)).isNotEqualTo(ImmutableMap.of("foo", "bar"))); } ImmutableSet> testAssertThatMapIsNotEmpty() { diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsEmpty.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsEmpty.java index c21a6fd204c..bbe9ee104ec 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsEmpty.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsEmpty.java @@ -7,6 +7,7 @@ import static com.google.errorprone.matchers.Matchers.argumentCount; import static com.google.errorprone.matchers.Matchers.isPrimitiveType; import static com.google.errorprone.matchers.Matchers.isSameType; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; @@ -23,13 +24,18 @@ import com.sun.source.tree.Tree; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; import java.util.Stack; +import java.util.TreeMap; +import java.util.TreeSet; import java.util.Vector; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -41,21 +47,30 @@ // XXX: Also match (effectively) final variables that reference provably-empty objects. // XXX: Also handle `#copyOf(someEmptyInstance)`, `#sortedCopyOf(someEmptyInstance)`, // `Sets.immutableEnumSet(emptyIterable)` (and other `Sets` methods), `EnumSet.noneOf(...)`, -// `emptyCollection.stream()`, `emptyMap.{keySet,values,entrySet}()`, etc. +// `emptyCollection.stream()`, `emptyStream.collect(...)`, `emptyMap.{keySet,values,entrySet}()`, +// etc. // XXX: Also recognize null-hostile "container" expression types that can only reference empty // instances, such as `ImmutableCollection` and `Flux`. +// XXX: Also recognize empty instances of `Optional`, `OptionalInt`, `OptionalLong`, and +// `OptionalDouble`. +// XXX: Also recognize empty builders and `emptyBuilder.build()` invocations. public final class IsEmpty implements Matcher { private static final long serialVersionUID = 1L; private static final Pattern EMPTY_INSTANCE_FACTORY_METHOD_PATTERN = Pattern.compile("empty.*"); - private static final Matcher PRIMITIVE_TYPE = isPrimitiveType(); + private static final Matcher EMPTY_COLLECTION_CONSTRUCTOR_ARGUMENT = + anyOf(isPrimitiveType(), isSubtypeOf(Comparator.class)); // XXX: Extend this list to include additional JDK collection types with a public constructor. private static final Matcher MUTABLE_COLLECTION_TYPE = anyOf( isSameType(ArrayList.class), isSameType(HashMap.class), + isSameType(HashSet.class), isSameType(LinkedHashMap.class), + isSameType(LinkedHashSet.class), isSameType(LinkedList.class), isSameType(Stack.class), + isSameType(TreeMap.class), + isSameType(TreeSet.class), isSameType(Vector.class)); private static final Matcher EMPTY_INSTANCE_FACTORY = anyOf( @@ -107,10 +122,10 @@ private boolean isEmptyCollectionConstructor(ExpressionTree tree, VisitorState s } List arguments = ((NewClassTree) tree).getArguments(); - if (arguments.stream().allMatch(a -> PRIMITIVE_TYPE.matches(a, state))) { + if (arguments.stream().allMatch(a -> EMPTY_COLLECTION_CONSTRUCTOR_ARGUMENT.matches(a, state))) { /* * This is a default constructor, or a constructor that creates an empty collection using - * custom (re)size/load factor parameters. + * custom (re)size/load factor parameters and/or a custom `Comparator`. */ return true; } diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java index 705d7b5f994..318328cbec3 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java @@ -19,14 +19,19 @@ void matches() { "import com.google.common.collect.ImmutableSetMultimap;", "import java.util.ArrayList;", "import java.util.Collections;", + "import java.util.Comparator;", "import java.util.HashMap;", + "import java.util.HashSet;", "import java.util.LinkedHashMap;", + "import java.util.LinkedHashSet;", "import java.util.LinkedList;", "import java.util.List;", "import java.util.Map;", "import java.util.Random;", "import java.util.Set;", "import java.util.Stack;", + "import java.util.TreeMap;", + "import java.util.TreeSet;", "import java.util.Vector;", "import java.util.stream.Stream;", "import reactor.core.publisher.Flux;", @@ -62,39 +67,63 @@ void matches() { " return new HashMap<>(ImmutableMap.of(1, 2));", " }", "", - " Map negative8() {", + " Set negative8() {", + " return new HashSet<>(ImmutableList.of(1));", + " }", + "", + " Map negative9() {", " return new LinkedHashMap<>(ImmutableMap.of(1, 2));", " }", "", - " ImmutableList negative9() {", + " Set negative10() {", + " return new LinkedHashSet<>(ImmutableList.of(1));", + " }", + "", + " List negative11() {", + " return new LinkedList<>(ImmutableList.of(1));", + " }", + "", + " Map negative12() {", + " return new HashMap<>(ImmutableMap.of(1, 2));", + " }", + "", + " Set negative13() {", + " return new HashSet<>(ImmutableList.of(1));", + " }", + "", + " List negative14() {", + " return new Vector<>(ImmutableList.of(1));", + " }", + "", + " ImmutableList negative15() {", " return ImmutableList.of(1);", " }", "", - " ImmutableSet negative10() {", + " ImmutableSet negative16() {", " return ImmutableSet.of(1);", " }", "", - " ImmutableMap negative11() {", + " ImmutableMap negative17() {", " return ImmutableMap.of(1, 2);", " }", "", - " ImmutableSetMultimap negative12() {", + " ImmutableSetMultimap negative18() {", " return ImmutableSetMultimap.of(1, 2);", " }", "", - " List negative13() {", + " List negative19() {", " return List.of(1);", " }", "", - " Map negative14() {", + " Map negative20() {", " return Map.of(1, 2);", " }", "", - " Set negative15() {", + " Set negative21() {", " return Set.of(1);", " }", "", - " Stream negative16() {", + " Stream negative22() {", " return Stream.of(1);", " }", "", @@ -157,163 +186,241 @@ void matches() { " ImmutableMap.of());", " }", "", - " Map positive12() {", + " Set positive12() {", + " // BUG: Diagnostic contains:", + " return new HashSet<>();", + " }", + "", + " Set positive13() {", + " // BUG: Diagnostic contains:", + " return new HashSet<>(1);", + " }", + "", + " Set positive14() {", + " // BUG: Diagnostic contains:", + " return new HashSet<>(1, 1.0F);", + " }", + "", + " Set positive15() {", + " // BUG: Diagnostic contains:", + " return new HashSet<>(", + " // BUG: Diagnostic contains:", + " ImmutableList.of());", + " }", + "", + " Map positive16() {", " // BUG: Diagnostic contains:", " return new LinkedHashMap<>();", " }", "", - " Map positive13() {", + " Map positive17() {", " // BUG: Diagnostic contains:", " return new LinkedHashMap<>(1);", " }", "", - " Map positive14() {", + " Map positive18() {", " // BUG: Diagnostic contains:", " return new LinkedHashMap<>(1, 1.0F);", " }", "", - " Map positive15() {", + " Map positive19() {", " // BUG: Diagnostic contains:", " return new LinkedHashMap<>(1, 1.0F, false);", " }", "", - " Map positive16() {", + " Map positive20() {", " // BUG: Diagnostic contains:", " return new LinkedHashMap<>(", " // BUG: Diagnostic contains:", " ImmutableMap.of());", " }", "", - " List positive17() {", + " Set positive21() {", + " // BUG: Diagnostic contains:", + " return new LinkedHashSet<>();", + " }", + "", + " Set positive22() {", + " // BUG: Diagnostic contains:", + " return new LinkedHashSet<>(1);", + " }", + "", + " Set positive23() {", + " // BUG: Diagnostic contains:", + " return new LinkedHashSet<>(1, 1.0F);", + " }", + "", + " Set positive24() {", + " // BUG: Diagnostic contains:", + " return new LinkedHashSet<>(", + " // BUG: Diagnostic contains:", + " ImmutableList.of());", + " }", + "", + " List positive25() {", " // BUG: Diagnostic contains:", " return new LinkedList<>();", " }", "", - " List positive18() {", + " List positive26() {", " // BUG: Diagnostic contains:", " return new LinkedList<>(", " // BUG: Diagnostic contains:", " ImmutableList.of());", " }", "", - " List positive19() {", + " List positive27() {", " // BUG: Diagnostic contains:", " return new Stack<>();", " }", "", - " List positive20() {", + " Map positive28() {", + " // BUG: Diagnostic contains:", + " return new TreeMap<>();", + " }", + "", + " Map positive29() {", + " // BUG: Diagnostic contains:", + " return new TreeMap<>(Comparator.naturalOrder());", + " }", + "", + " Map positive30() {", + " // BUG: Diagnostic contains:", + " return new TreeMap<>(", + " // BUG: Diagnostic contains:", + " ImmutableMap.of());", + " }", + "", + " Set positive31() {", + " // BUG: Diagnostic contains:", + " return new TreeSet<>();", + " }", + "", + " Set positive32() {", + " // BUG: Diagnostic contains:", + " return new TreeSet<>(Comparator.naturalOrder());", + " }", + "", + " Set positive33() {", + " // BUG: Diagnostic contains:", + " return new TreeSet<>(", + " // BUG: Diagnostic contains:", + " ImmutableList.of());", + " }", + "", + " List positive34() {", " // BUG: Diagnostic contains:", " return new Vector<>();", " }", "", - " List positive21() {", + " List positive35() {", " // BUG: Diagnostic contains:", " return new Vector<>(1);", " }", "", - " List positive22() {", + " List positive36() {", " // BUG: Diagnostic contains:", " return new Vector<>(1, 2);", " }", "", - " List positive23() {", + " List positive37() {", " // BUG: Diagnostic contains:", " return new Vector<>(", " // BUG: Diagnostic contains:", " ImmutableList.of());", " }", "", - " List positive24() {", + " List positive38() {", " // BUG: Diagnostic contains:", " return Collections.EMPTY_LIST;", " }", "", - " Map positive25s() {", + " Map positive39() {", " // BUG: Diagnostic contains:", " return Collections.EMPTY_MAP;", " }", "", - " Set positive26() {", + " Set positive40() {", " // BUG: Diagnostic contains:", " return Collections.EMPTY_SET;", " }", "", - " List positive27() {", + " List positive41() {", " // BUG: Diagnostic contains:", " return Collections.emptyList();", " }", "", - " Map positive28() {", + " Map positive42() {", " // BUG: Diagnostic contains:", " return Collections.emptyMap();", " }", "", - " Set positive29() {", + " Set positive43() {", " // BUG: Diagnostic contains:", " return Collections.emptySet();", " }", "", - " ImmutableList positive30() {", + " ImmutableList positive44() {", " // BUG: Diagnostic contains:", " return ImmutableList.of();", " }", "", - " ImmutableSet positive31() {", + " ImmutableSet positive45() {", " // BUG: Diagnostic contains:", " return ImmutableSet.of();", " }", "", - " ImmutableMap positive32() {", + " ImmutableMap positive46() {", " // BUG: Diagnostic contains:", " return ImmutableMap.of();", " }", "", - " ImmutableSetMultimap positive33() {", + " ImmutableSetMultimap positive47() {", " // BUG: Diagnostic contains:", " return ImmutableSetMultimap.of();", " }", "", - " List positive34() {", + " List positive48() {", " // BUG: Diagnostic contains:", " return List.of();", " }", "", - " Map positive35() {", + " Map positive49() {", " // BUG: Diagnostic contains:", " return Map.of();", " }", "", - " Set positive36() {", + " Set positive50() {", " // BUG: Diagnostic contains:", " return Set.of();", " }", "", - " Stream positive37() {", + " Stream positive51() {", " // BUG: Diagnostic contains:", " return Stream.of();", " }", "", - " Stream positive38() {", + " Stream positive52() {", " // BUG: Diagnostic contains:", " return Stream.empty();", " }", "", - " Flux positive39() {", + " Flux positive53() {", " // BUG: Diagnostic contains:", " return Flux.empty();", " }", "", - " Mono positive40() {", + " Mono positive54() {", " // BUG: Diagnostic contains:", " return Mono.empty();", " }", "", - " Context positive41() {", + " Context positive55() {", " // BUG: Diagnostic contains:", " return Context.empty();", " }", "", - " Flux positive42() {", + " Flux positive56() {", " // BUG: Diagnostic contains:", " return Flux.just();", " }",