Suggestions

PicnicSupermarket · Oct 26, 2024 · fd415f5 · fd415f5
1 parent 7a794d1
commit fd415f5
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 55 deletions.
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
@@ -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.
-   *
-   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
-    @BeforeTemplate
-    ImmutableSet<T> before(Stream<T> stream) {
-      return stream.collect(toImmutableSet());
-    }
-
-    @AfterTemplate
-    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
-    ImmutableSet<T> after(Stream<T> stream) {
-      return stream.collect(toImmutableEnumSet());
-    }
-  }
-
   /** Prefer {@link Iterators#getNext(Iterator, Object)} over more contrived alternatives. */
   static final class IteratorGetNextOrDefault<T> {
     @BeforeTemplate

diff --git a/...one-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java b/...one-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java
@@ -1,45 +1,70 @@
 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() {}
 
   /**
    * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the
    * internally used {@link EnumSet}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
+    @BeforeTemplate
+    ImmutableSet<T> before(Iterable<T> elements) {
+      return ImmutableSet.copyOf(elements);
+    }
+
     @BeforeTemplate
     ImmutableSet<T> before(Collection<T> elements) {
       return ImmutableSet.copyOf(elements);
     }
 
     @AfterTemplate
-    ImmutableSet<T> after(Collection<T> elements) {
+    ImmutableSet<T> after(Iterable<T> elements) {
       return Sets.immutableEnumSet(elements);
     }
   }
 
   /**
    * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the
    * internally used {@link EnumSet}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
+  static final class SetsImmutableEnumSetArraysAsList<T extends Enum<T>> {
     @BeforeTemplate
     ImmutableSet<T> before(T[] elements) {
       return ImmutableSet.copyOf(elements);
@@ -72,6 +97,10 @@ ImmutableSet<T> after(T e1) {
   /**
    * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
    * the internally used {@link EnumSet}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
     @BeforeTemplate
@@ -90,6 +119,10 @@ ImmutableSet<T> after(T e1, T e2) {
   /**
    * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
    * the internally used {@link EnumSet}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
     @BeforeTemplate
@@ -109,6 +142,10 @@ ImmutableSet<T> 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}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
     @BeforeTemplate
@@ -128,6 +165,10 @@ ImmutableSet<T> 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}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
     @BeforeTemplate
@@ -147,6 +188,10 @@ ImmutableSet<T> 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}.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
     @BeforeTemplate
@@ -165,7 +210,7 @@ ImmutableSet<T> 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<T extends Enum<T>> {
+  static final class SetsImmutableEnumSetVarArgs<T extends Enum<T>> {
     @BeforeTemplate
     @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
     ImmutableSet<T> before(T e1, @Repeated T elements) {
@@ -177,4 +222,25 @@ ImmutableSet<T> 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.
+   *
+   * <p><strong>Warning:</strong> 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<T extends Enum<T>> {
+    @BeforeTemplate
+    ImmutableSet<T> before(Stream<T> stream) {
+      return stream.collect(toImmutableSet());
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    ImmutableSet<T> after(Stream<T> stream) {
+      return stream.collect(toImmutableEnumSet());
+    }
+  }
 }
diff --git a/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestInput.java b/...ntrib/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<Object> elidedTypesAndStaticImports() {
         Preconditions.class,
         Sets.class,
         Splitter.class,
-        Streams.class,
-        toImmutableSet());
+        Streams.class);
   }
 
   int testCheckIndex() {
@@ -38,10 +34,6 @@ void testCheckIndexConditional() {
     }
   }
 
-  ImmutableSet<BoundType> testStreamToImmutableEnumSet() {
-    return Stream.of(BoundType.OPEN).collect(toImmutableSet());
-  }
-
   ImmutableSet<String> testIteratorGetNextOrDefault() {
     return ImmutableSet.of(
         ImmutableList.of("a").iterator().hasNext()

diff --git a/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/AssortedRulesTestOutput.java b/...trib/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<Object> elidedTypesAndStaticImports() {
         Preconditions.class,
         Sets.class,
         Splitter.class,
-        Streams.class,
-        toImmutableSet());
+        Streams.class);
   }
 
   int testCheckIndex() {
@@ -39,10 +35,6 @@ void testCheckIndexConditional() {
     checkIndex(1, 2);
   }
 
-  ImmutableSet<BoundType> testStreamToImmutableEnumSet() {
-    return Stream.of(BoundType.OPEN).collect(toImmutableEnumSet());
-  }
-
   ImmutableSet<String> testIteratorGetNextOrDefault() {
     return ImmutableSet.of(
         Iterators.getNext(ImmutableList.of("a").iterator(), "foo"),

diff --git a/...c/test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestInput.java b/...c/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<Object> elidedTypesAndStaticImports() {
-    return ImmutableSet.of(EnumSet.class);
+    return ImmutableSet.of(EnumSet.class, toImmutableSet());
   }
 
-  ImmutableSet<RoundingMode> testSetsImmutableEnumSetIterable() {
-    return ImmutableSet.copyOf(EnumSet.range(RoundingMode.UP, RoundingMode.UNNECESSARY));
+  ImmutableSet<ImmutableSet<RoundingMode>> testSetsImmutableEnumSetIterable() {
+    return ImmutableSet.of(
+        ImmutableSet.copyOf(Iterables.cycle(RoundingMode.UP)),
+        ImmutableSet.copyOf(EnumSet.allOf(RoundingMode.class)));
   }
 
-  ImmutableSet<RoundingMode> testSetsImmutableEnumSetIterableArray() {
+  ImmutableSet<RoundingMode> testSetsImmutableEnumSetArraysAsList() {
     return ImmutableSet.copyOf(RoundingMode.values());
   }
 
@@ -72,7 +79,7 @@ ImmutableSet<RoundingMode> testSetsImmutableEnumSet6() {
         RoundingMode.HALF_EVEN);
   }
 
-  ImmutableSet<RoundingMode> testImmutableEnumSetVarArgs() {
+  ImmutableSet<RoundingMode> testSetsImmutableEnumSetVarArgs() {
     return ImmutableSet.copyOf(
         EnumSet.of(
             RoundingMode.UP,
@@ -82,4 +89,8 @@ ImmutableSet<RoundingMode> testImmutableEnumSetVarArgs() {
             RoundingMode.UNNECESSARY,
             RoundingMode.HALF_EVEN));
   }
+
+  ImmutableSet<BoundType> testStreamToImmutableEnumSet() {
+    return Stream.of(BoundType.OPEN).collect(toImmutableSet());
+  }
 }
diff --git a/.../test/resources/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRulesTestOutput.java b/.../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<Object> elidedTypesAndStaticImports() {
-    return ImmutableSet.of(EnumSet.class);
+    return ImmutableSet.of(EnumSet.class, toImmutableSet());
   }
 
-  ImmutableSet<RoundingMode> testSetsImmutableEnumSetIterable() {
-    return Sets.immutableEnumSet(EnumSet.range(RoundingMode.UP, RoundingMode.UNNECESSARY));
+  ImmutableSet<ImmutableSet<RoundingMode>> testSetsImmutableEnumSetIterable() {
+    return ImmutableSet.of(
+        Sets.immutableEnumSet(Iterables.cycle(RoundingMode.UP)),
+        Sets.immutableEnumSet(EnumSet.allOf(RoundingMode.class)));
   }
 
-  ImmutableSet<RoundingMode> testSetsImmutableEnumSetIterableArray() {
+  ImmutableSet<RoundingMode> testSetsImmutableEnumSetArraysAsList() {
     return Sets.immutableEnumSet(Arrays.asList(RoundingMode.values()));
   }
 
@@ -72,7 +80,7 @@ ImmutableSet<RoundingMode> testSetsImmutableEnumSet6() {
         RoundingMode.HALF_EVEN);
   }
 
-  ImmutableSet<RoundingMode> testImmutableEnumSetVarArgs() {
+  ImmutableSet<RoundingMode> testSetsImmutableEnumSetVarArgs() {
     return Sets.immutableEnumSet(
         RoundingMode.UP,
         RoundingMode.DOWN,
@@ -81,4 +89,8 @@ ImmutableSet<RoundingMode> testImmutableEnumSetVarArgs() {
         RoundingMode.UNNECESSARY,
         RoundingMode.HALF_EVEN);
   }
+
+  ImmutableSet<BoundType> testStreamToImmutableEnumSet() {
+    return Stream.of(BoundType.OPEN).collect(toImmutableEnumSet());
+  }
 }