diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java index 4841481ee8..a38a823ea4 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java @@ -1,8 +1,6 @@ package tech.picnic.errorprone.refasterrules; import static com.google.common.base.Preconditions.checkElementIndex; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.Sets.toImmutableEnumSet; import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.Collections.disjoint; import static java.util.Objects.checkIndex; @@ -70,28 +68,6 @@ void after(int index, int size) { } } - /** - * Use {@link Sets#toImmutableEnumSet()} when possible, as it is more efficient than {@link - * ImmutableSet#toImmutableSet()} and produces a more compact object. - * - *

Warning: this rewrite rule is not completely behavior preserving: while the - * original code produces a set that iterates over the elements in encounter order, the - * replacement code iterates over the elements in enum definition order. - */ - // XXX: ^ Consider emitting a comment warning about this fact? - static final class StreamToImmutableEnumSet> { - @BeforeTemplate - ImmutableSet before(Stream stream) { - return stream.collect(toImmutableSet()); - } - - @AfterTemplate - @UseImportPolicy(STATIC_IMPORT_ALWAYS) - ImmutableSet after(Stream stream) { - return stream.collect(toImmutableEnumSet()); - } - } - /** Prefer {@link Iterators#getNext(Iterator, Object)} over more contrived alternatives. */ static final class IteratorGetNextOrDefault { @BeforeTemplate diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java index 8d38be3b92..b7b40af650 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java @@ -1,20 +1,32 @@ package tech.picnic.errorprone.refasterrules; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.Sets.toImmutableEnumSet; +import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; + import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; 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.Repeated; +import com.google.errorprone.refaster.annotation.UseImportPolicy; import java.util.Arrays; import java.util.Collection; import java.util.EnumSet; +import java.util.stream.Stream; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; /** * Refaster rules related to expressions dealing with {@code * com.google.common.collect.ImmutableEnumSet}s. */ +// XXX: Some of the rules defined here impact iteration order. That's a rather subtle change. Should +// we emit a comment warning about this fact? (This may produce a lot of noise. A bug checker could +// in some cases determine whether iteration order is important.) +// XXX: Consider replacing the `SetsImmutableEnumSet[N]` Refaster rules with a bug checker, such +// that call to `ImmutableSet#of(Object, Object, Object, Object, Object, Object, Object[])` with +// enum-typed values can also be rewritten. @OnlineDocumentation final class ImmutableEnumSetRules { private ImmutableEnumSetRules() {} @@ -22,15 +34,24 @@ private ImmutableEnumSetRules() {} /** * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the * internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the + * original code produces a set that iterates over its elements in the same order as the input + * {@link Iterable}, the replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSetIterable> { + @BeforeTemplate + ImmutableSet before(Iterable elements) { + return ImmutableSet.copyOf(elements); + } + @BeforeTemplate ImmutableSet before(Collection elements) { return ImmutableSet.copyOf(elements); } @AfterTemplate - ImmutableSet after(Collection elements) { + ImmutableSet after(Iterable elements) { return Sets.immutableEnumSet(elements); } } @@ -38,8 +59,12 @@ ImmutableSet after(Collection elements) { /** * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the * internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the + * original code produces a set that iterates over its elements in the same order as defined in + * the array, the replacement code iterates over the elements in enum definition order. */ - static final class SetsImmutableEnumSetIterableArray> { + static final class SetsImmutableEnumSetArraysAsList> { @BeforeTemplate ImmutableSet before(T[] elements) { return ImmutableSet.copyOf(elements); @@ -72,6 +97,10 @@ ImmutableSet after(T e1) { /** * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the {@link + * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order, + * the replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSet2> { @BeforeTemplate @@ -90,6 +119,10 @@ ImmutableSet after(T e1, T e2) { /** * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the {@link + * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order, + * the replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSet3> { @BeforeTemplate @@ -109,6 +142,10 @@ ImmutableSet after(T e1, T e2, T e3) { /** * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the {@link + * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order, + * the replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSet4> { @BeforeTemplate @@ -128,6 +165,10 @@ ImmutableSet after(T e1, T e2, T e3, T e4) { /** * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the {@link + * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order, + * the replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSet5> { @BeforeTemplate @@ -147,6 +188,10 @@ ImmutableSet after(T e1, T e2, T e3, T e4, T e5) { /** * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. + * + *

Warning: this rule is not completely behavior preserving: while the + * original code produces a set that iterates over its elements in the listed order, the + * replacement code iterates over the elements in enum definition order. */ static final class SetsImmutableEnumSet6> { @BeforeTemplate @@ -165,7 +210,7 @@ ImmutableSet after(T e1, T e2, T e3, T e4, T e5, T e6) { * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of * the internally used {@link EnumSet}. */ - static final class ImmutableEnumSetVarArgs> { + static final class SetsImmutableEnumSetVarArgs> { @BeforeTemplate @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */) ImmutableSet before(T e1, @Repeated T elements) { @@ -177,4 +222,25 @@ ImmutableSet after(T e1, @Repeated T elements) { return Sets.immutableEnumSet(e1, Refaster.asVarargs(elements)); } } + + /** + * Use {@link Sets#toImmutableEnumSet()} when possible, as it is more efficient than {@link + * ImmutableSet#toImmutableSet()} and produces a more compact object. + * + *

Warning: this rule is not completely behavior preserving: while the + * original code produces a set that iterates over its elements in encounter order, the + * replacement code iterates over the elements in enum definition order. + */ + static final class StreamToImmutableEnumSet> { + @BeforeTemplate + ImmutableSet before(Stream stream) { + return stream.collect(toImmutableSet()); + } + + @AfterTemplate + @UseImportPolicy(STATIC_IMPORT_ALWAYS) + ImmutableSet after(Stream stream) { + return stream.collect(toImmutableEnumSet()); + } + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestInput.java index 1f785293b0..98174797e0 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestInput.java @@ -1,10 +1,7 @@ package tech.picnic.errorprone.refasterrules; -import static com.google.common.collect.ImmutableSet.toImmutableSet; - import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.BoundType; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -24,8 +21,7 @@ public ImmutableSet elidedTypesAndStaticImports() { Preconditions.class, Sets.class, Splitter.class, - Streams.class, - toImmutableSet()); + Streams.class); } int testCheckIndex() { @@ -38,10 +34,6 @@ void testCheckIndexConditional() { } } - ImmutableSet testStreamToImmutableEnumSet() { - return Stream.of(BoundType.OPEN).collect(toImmutableSet()); - } - ImmutableSet testIteratorGetNextOrDefault() { return ImmutableSet.of( ImmutableList.of("a").iterator().hasNext() diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java index 2fd9c408af..2e6794dbfd 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java @@ -1,12 +1,9 @@ package tech.picnic.errorprone.refasterrules; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.Sets.toImmutableEnumSet; import static java.util.Objects.checkIndex; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; -import com.google.common.collect.BoundType; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -27,8 +24,7 @@ public ImmutableSet elidedTypesAndStaticImports() { Preconditions.class, Sets.class, Splitter.class, - Streams.class, - toImmutableSet()); + Streams.class); } int testCheckIndex() { @@ -39,10 +35,6 @@ void testCheckIndexConditional() { checkIndex(1, 2); } - ImmutableSet testStreamToImmutableEnumSet() { - return Stream.of(BoundType.OPEN).collect(toImmutableEnumSet()); - } - ImmutableSet testIteratorGetNextOrDefault() { return ImmutableSet.of( Iterators.getNext(ImmutableList.of("a").iterator(), "foo"), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestInput.java index c3d1c5ac78..10662a9ab6 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestInput.java @@ -1,21 +1,28 @@ package tech.picnic.errorprone.refasterrules; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + +import com.google.common.collect.BoundType; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import java.math.RoundingMode; import java.util.EnumSet; +import java.util.stream.Stream; import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; final class ImmutableEnumSetRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(EnumSet.class); + return ImmutableSet.of(EnumSet.class, toImmutableSet()); } - ImmutableSet testSetsImmutableEnumSetIterable() { - return ImmutableSet.copyOf(EnumSet.range(RoundingMode.UP, RoundingMode.UNNECESSARY)); + ImmutableSet> testSetsImmutableEnumSetIterable() { + return ImmutableSet.of( + ImmutableSet.copyOf(Iterables.cycle(RoundingMode.UP)), + ImmutableSet.copyOf(EnumSet.allOf(RoundingMode.class))); } - ImmutableSet testSetsImmutableEnumSetIterableArray() { + ImmutableSet testSetsImmutableEnumSetArraysAsList() { return ImmutableSet.copyOf(RoundingMode.values()); } @@ -72,7 +79,7 @@ ImmutableSet testSetsImmutableEnumSet6() { RoundingMode.HALF_EVEN); } - ImmutableSet testImmutableEnumSetVarArgs() { + ImmutableSet testSetsImmutableEnumSetVarArgs() { return ImmutableSet.copyOf( EnumSet.of( RoundingMode.UP, @@ -82,4 +89,8 @@ ImmutableSet testImmutableEnumSetVarArgs() { RoundingMode.UNNECESSARY, RoundingMode.HALF_EVEN)); } + + ImmutableSet testStreamToImmutableEnumSet() { + return Stream.of(BoundType.OPEN).collect(toImmutableSet()); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestOutput.java index 45544b7fa6..c96b01db21 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestOutput.java @@ -1,23 +1,31 @@ package tech.picnic.errorprone.refasterrules; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.Sets.toImmutableEnumSet; + +import com.google.common.collect.BoundType; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import java.math.RoundingMode; import java.util.Arrays; import java.util.EnumSet; +import java.util.stream.Stream; import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase; final class ImmutableEnumSetRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(EnumSet.class); + return ImmutableSet.of(EnumSet.class, toImmutableSet()); } - ImmutableSet testSetsImmutableEnumSetIterable() { - return Sets.immutableEnumSet(EnumSet.range(RoundingMode.UP, RoundingMode.UNNECESSARY)); + ImmutableSet> testSetsImmutableEnumSetIterable() { + return ImmutableSet.of( + Sets.immutableEnumSet(Iterables.cycle(RoundingMode.UP)), + Sets.immutableEnumSet(EnumSet.allOf(RoundingMode.class))); } - ImmutableSet testSetsImmutableEnumSetIterableArray() { + ImmutableSet testSetsImmutableEnumSetArraysAsList() { return Sets.immutableEnumSet(Arrays.asList(RoundingMode.values())); } @@ -72,7 +80,7 @@ ImmutableSet testSetsImmutableEnumSet6() { RoundingMode.HALF_EVEN); } - ImmutableSet testImmutableEnumSetVarArgs() { + ImmutableSet testSetsImmutableEnumSetVarArgs() { return Sets.immutableEnumSet( RoundingMode.UP, RoundingMode.DOWN, @@ -81,4 +89,8 @@ ImmutableSet testImmutableEnumSetVarArgs() { RoundingMode.UNNECESSARY, RoundingMode.HALF_EVEN); } + + ImmutableSet testStreamToImmutableEnumSet() { + return Stream.of(BoundType.OPEN).collect(toImmutableEnumSet()); + } }