From fcafc5c95041fc73514d692f1d3d2c8907ae95f2 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 5 Jan 2022 10:03:47 +0100 Subject: [PATCH] Add type arguments in `@AfterTemplate`s for builders --- .../ImmutableListMultimapRules.java | 4 +- .../refasterrules/ImmutableListRules.java | 4 +- .../refasterrules/ImmutableMapRules.java | 4 +- .../refasterrules/ImmutableMultisetRules.java | 4 +- .../ImmutableSetMultimapRules.java | 4 +- .../ImmutableSortedMapRules.java | 8 +- .../ImmutableSetTemplates.java | 155 ++++++++++++++++++ 7 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java index b68a7a16018..28b1241b349 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java @@ -35,8 +35,6 @@ private ImmutableListMultimapRules() {} * Prefer {@link ImmutableListMultimap#builder()} over the associated constructor on constructions * that produce a less-specific type. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableListMultimapBuilder { @BeforeTemplate ImmutableMultimap.Builder before() { @@ -48,7 +46,7 @@ ImmutableMultimap.Builder before() { @AfterTemplate ImmutableListMultimap.Builder after() { - return ImmutableListMultimap.builder(); + return ImmutableListMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java index 1848c268b9e..d48f697c6b4 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListRules.java @@ -28,8 +28,6 @@ final class ImmutableListRules { private ImmutableListRules() {} /** Prefer {@link ImmutableList#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableListBuilder { @BeforeTemplate ImmutableList.Builder before() { @@ -38,7 +36,7 @@ ImmutableList.Builder before() { @AfterTemplate ImmutableList.Builder after() { - return ImmutableList.builder(); + return ImmutableList.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java index aa68506b837..aa82b76f681 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMapRules.java @@ -29,8 +29,6 @@ final class ImmutableMapRules { private ImmutableMapRules() {} /** Prefer {@link ImmutableMap#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableMapBuilder { @BeforeTemplate ImmutableMap.Builder before() { @@ -39,7 +37,7 @@ ImmutableMap.Builder before() { @AfterTemplate ImmutableMap.Builder after() { - return ImmutableMap.builder(); + return ImmutableMap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java index fee76fd89d7..c70aa0f8d2a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableMultisetRules.java @@ -21,8 +21,6 @@ final class ImmutableMultisetRules { private ImmutableMultisetRules() {} /** Prefer {@link ImmutableMultiset#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableMultisetBuilder { @BeforeTemplate ImmutableMultiset.Builder before() { @@ -31,7 +29,7 @@ ImmutableMultiset.Builder before() { @AfterTemplate ImmutableMultiset.Builder after() { - return ImmutableMultiset.builder(); + return ImmutableMultiset.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java index 1c704943fcc..6e7eaa7a143 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java @@ -29,8 +29,6 @@ final class ImmutableSetMultimapRules { private ImmutableSetMultimapRules() {} /** Prefer {@link ImmutableSetMultimap#builder()} over the associated constructor. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSetMultimapBuilder { @BeforeTemplate ImmutableSetMultimap.Builder before() { @@ -39,7 +37,7 @@ ImmutableSetMultimap.Builder before() { @AfterTemplate ImmutableSetMultimap.Builder after() { - return ImmutableSetMultimap.builder(); + return ImmutableSetMultimap.builder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java index 36975c7025c..48a4e559fba 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSortedMapRules.java @@ -37,8 +37,6 @@ ImmutableSortedMap.Builder after(Comparator cmp) { * Prefer {@link ImmutableSortedMap#naturalOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSortedMapNaturalOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -47,7 +45,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.naturalOrder(); + return ImmutableSortedMap.naturalOrder(); } } @@ -55,8 +53,6 @@ ImmutableSortedMap.Builder after() { * Prefer {@link ImmutableSortedMap#reverseOrder()} over the alternative that requires explicitly * providing the {@link Comparator}. */ - // XXX: This drops generic type information, sometimes leading to non-compilable code. See - // https://github.com/google/error-prone/pull/2706. static final class ImmutableSortedMapReverseOrderBuilder, V> { @BeforeTemplate ImmutableSortedMap.Builder before() { @@ -65,7 +61,7 @@ ImmutableSortedMap.Builder before() { @AfterTemplate ImmutableSortedMap.Builder after() { - return ImmutableSortedMap.reverseOrder(); + return ImmutableSortedMap.reverseOrder(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java new file mode 100644 index 00000000000..30bfae29160 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java @@ -0,0 +1,155 @@ +package tech.picnic.errorprone.refastertemplates; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.stream.Collectors.collectingAndThen; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toSet; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets.SetView; +import com.google.common.collect.Streams; +import com.google.errorprone.refaster.ImportPolicy; +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.UseImportPolicy; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Iterator; +import java.util.Set; +import java.util.stream.Stream; + +/** Refaster templates related to expressions dealing with {@link ImmutableSet}s. */ +final class ImmutableSetTemplates { + private ImmutableSetTemplates() {} + + /** Prefer {@link ImmutableSet#builder()} over the associated constructor. */ + // XXX: Picnic's Error Prone fork supports method invocation type argument inlining in the + // `@AfterTemplate`. Without using the fork, the expression in the `@AfterTemplate` can result in + // non-compilable code. See: https://github.com/google/error-prone/pull/2706. + static final class ImmutableSetBuilder { + @BeforeTemplate + ImmutableSet.Builder before() { + return new ImmutableSet.Builder<>(); + } + + @AfterTemplate + ImmutableSet.Builder after() { + return ImmutableSet.builder(); + } + } + + /** Prefer {@link ImmutableSet#of()} over more contrived alternatives. */ + static final class EmptyImmutableSet { + @BeforeTemplate + ImmutableSet before() { + return Refaster.anyOf( + ImmutableSet.builder().build(), Stream.empty().collect(toImmutableSet())); + } + + @AfterTemplate + ImmutableSet after() { + return ImmutableSet.of(); + } + } + + /** + * Prefer {@link ImmutableSet#of(Object)} over alternatives that don't communicate the + * immutability of the resulting set at the type level. + */ + // XXX: Note that this rewrite rule is incorrect for nullable elements. + static final class SingletonImmutableSet { + @BeforeTemplate + Set before(T element) { + return Collections.singleton(element); + } + + @AfterTemplate + ImmutableSet after(T element) { + return ImmutableSet.of(element); + } + } + + /** Prefer {@link ImmutableSet#copyOf(Iterable)} and variants over more contrived alternatives. */ + static final class IterableToImmutableSet { + @BeforeTemplate + ImmutableSet before(T[] iterable) { + return Refaster.anyOf( + ImmutableSet.builder().add(iterable).build(), + Arrays.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Iterator iterable) { + return Refaster.anyOf( + ImmutableSet.builder().addAll(iterable).build(), + Streams.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Iterable iterable) { + return Refaster.anyOf( + ImmutableSet.builder().addAll(iterable).build(), + Streams.stream(iterable).collect(toImmutableSet())); + } + + @BeforeTemplate + ImmutableSet before(Collection iterable) { + return iterable.stream().collect(toImmutableSet()); + } + + @AfterTemplate + ImmutableSet after(Iterable iterable) { + return ImmutableSet.copyOf(iterable); + } + } + + /** Prefer {@link ImmutableSet#toImmutableSet()} over less idiomatic alternatives. */ + // XXX: Once the code base has been sufficiently cleaned up, we might want to also rewrite + // `Collectors.toSet(`), with the caveat that it allows mutation (though this cannot be relied + // upon) as well as nulls. Another option is to explicitly rewrite those variants to + // `Collectors.toSet(HashSet::new)`. + static final class StreamToImmutableSet { + @BeforeTemplate + ImmutableSet before(Stream stream) { + return Refaster.anyOf( + ImmutableSet.copyOf(stream.iterator()), + stream.distinct().collect(toImmutableSet()), + stream.collect(collectingAndThen(toList(), ImmutableSet::copyOf)), + stream.collect(collectingAndThen(toSet(), ImmutableSet::copyOf))); + } + + @AfterTemplate + @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS) + ImmutableSet after(Stream stream) { + return stream.collect(toImmutableSet()); + } + } + + /** Don't unnecessarily copy an {@link ImmutableSet}. */ + static final class ImmutableSetCopyOfImmutableSet { + @BeforeTemplate + ImmutableSet before(ImmutableSet set) { + return ImmutableSet.copyOf(set); + } + + @AfterTemplate + ImmutableSet after(ImmutableSet set) { + return set; + } + } + + /** Prefer {@link SetView#immutableCopy()} over the more verbose alternative. */ + static final class ImmutableSetCopyOfSetView { + @BeforeTemplate + ImmutableSet before(SetView set) { + return ImmutableSet.copyOf(set); + } + + @AfterTemplate + ImmutableSet after(SetView set) { + return set.immutableCopy(); + } + } +}