Introduce Refaster rules that resolve EnumOrdinal violations (#1104)

PicnicSupermarket · Mar 27, 2024 · 769779c · 769779c
1 parent 9d8a5af
commit 769779c
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 2 deletions.
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
@@ -16,8 +16,11 @@
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.refaster.Refaster;
 import com.google.errorprone.refaster.annotation.AfterTemplate;
+import com.google.errorprone.refaster.annotation.AlsoNegation;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import com.google.errorprone.refaster.annotation.Matches;
+import com.google.errorprone.refaster.annotation.MayOptionallyUse;
+import com.google.errorprone.refaster.annotation.Placeholder;
 import com.google.errorprone.refaster.annotation.Repeated;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.util.Arrays;
@@ -92,6 +95,24 @@ Comparator<T> after(Comparator<T> cmp) {
     }
   }
 
+  /** Don't explicitly compare enums by their ordinal. */
+  abstract static class ComparingEnum<E extends Enum<E>, T> {
+    @Placeholder(allowsIdentity = true)
+    abstract E toEnumFunction(@MayOptionallyUse T value);
+
+    @BeforeTemplate
+    @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+    Comparator<T> before() {
+      return comparingInt(v -> toEnumFunction(v).ordinal());
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    Comparator<T> after() {
+      return comparing(v -> toEnumFunction(v));
+    }
+  }
+
   /** Don't explicitly create {@link Comparator}s unnecessarily. */
   static final class ThenComparing<S, T extends Comparable<? super T>> {
     @BeforeTemplate
@@ -419,4 +440,34 @@ static final class MaxByNaturalOrder<T extends Comparable<? super T>> {
       return maxBy(naturalOrder());
     }
   }
+
+  /** Don't explicitly compare enums by their ordinal. */
+  static final class IsLessThan<E extends Enum<E>> {
+    @BeforeTemplate
+    @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+    boolean before(E value1, E value2) {
+      return value1.ordinal() < value2.ordinal();
+    }
+
+    @AfterTemplate
+    @AlsoNegation
+    boolean after(E value1, E value2) {
+      return value1.compareTo(value2) < 0;
+    }
+  }
+
+  /** Don't explicitly compare enums by their ordinal. */
+  static final class IsLessThanOrEqualTo<E extends Enum<E>> {
+    @BeforeTemplate
+    @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+    boolean before(E value1, E value2) {
+      return value1.ordinal() <= value2.ordinal();
+    }
+
+    @AfterTemplate
+    @AlsoNegation
+    boolean after(E value1, E value2) {
+      return value1.compareTo(value2) <= 0;
+    }
+  }
 }
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java
@@ -30,8 +30,9 @@ static final class PrimitiveOrReferenceEquality<T extends Enum<T>> {
     // XXX: This Refaster rule is the topic of https://github.com/google/error-prone/issues/559. We
     // work around the issue by selecting the "largest replacements". See the `Refaster` check.
     @BeforeTemplate
+    @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
     boolean before(T a, T b) {
-      return Refaster.anyOf(a.equals(b), Objects.equals(a, b));
+      return Refaster.anyOf(a.equals(b), Objects.equals(a, b), a.ordinal() == b.ordinal());
     }
 
     @AfterTemplate

diff --git a/...rib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java b/...rib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java
@@ -9,6 +9,7 @@
 import com.google.common.collect.Comparators;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import java.math.RoundingMode;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
@@ -54,6 +55,10 @@ ImmutableSet<Comparator<String>> testCustomComparator() {
         Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length)));
   }
 
+  Comparator<String> testComparingEnum() {
+    return Comparator.comparingInt(s -> RoundingMode.valueOf(s).ordinal());
+  }
+
   Comparator<String> testThenComparing() {
     return Comparator.<String>naturalOrder().thenComparing(Comparator.comparing(String::isEmpty));
   }
@@ -173,4 +178,16 @@ BinaryOperator<String> testComparatorsMax() {
   Collector<Integer, ?, Optional<Integer>> testMaxByNaturalOrder() {
     return minBy(reverseOrder());
   }
+
+  ImmutableSet<Boolean> testIsLessThan() {
+    return ImmutableSet.of(
+        RoundingMode.UP.ordinal() < RoundingMode.DOWN.ordinal(),
+        RoundingMode.UP.ordinal() >= RoundingMode.DOWN.ordinal());
+  }
+
+  ImmutableSet<Boolean> testIsLessThanOrEqualTo() {
+    return ImmutableSet.of(
+        RoundingMode.UP.ordinal() <= RoundingMode.DOWN.ordinal(),
+        RoundingMode.UP.ordinal() > RoundingMode.DOWN.ordinal());
+  }
 }
diff --git a/...ib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java b/...ib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java
@@ -1,5 +1,6 @@
 package tech.picnic.errorprone.refasterrules;
 
+import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
 import static java.util.Comparator.reverseOrder;
 import static java.util.function.Function.identity;
@@ -9,6 +10,7 @@
 import com.google.common.collect.Comparators;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import java.math.RoundingMode;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
@@ -52,6 +54,10 @@ ImmutableSet<Comparator<String>> testCustomComparator() {
         Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length)));
   }
 
+  Comparator<String> testComparingEnum() {
+    return comparing(s -> RoundingMode.valueOf(s));
+  }
+
   Comparator<String> testThenComparing() {
     return Comparator.<String>naturalOrder().thenComparing(String::isEmpty);
   }
@@ -163,4 +169,16 @@ BinaryOperator<String> testComparatorsMax() {
   Collector<Integer, ?, Optional<Integer>> testMaxByNaturalOrder() {
     return maxBy(naturalOrder());
   }
+
+  ImmutableSet<Boolean> testIsLessThan() {
+    return ImmutableSet.of(
+        RoundingMode.UP.compareTo(RoundingMode.DOWN) < 0,
+        RoundingMode.UP.compareTo(RoundingMode.DOWN) >= 0);
+  }
+
+  ImmutableSet<Boolean> testIsLessThanOrEqualTo() {
+    return ImmutableSet.of(
+        RoundingMode.UP.compareTo(RoundingMode.DOWN) <= 0,
+        RoundingMode.UP.compareTo(RoundingMode.DOWN) > 0);
+  }
 }
diff --git a/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java b/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java
@@ -21,8 +21,10 @@ ImmutableSet<Boolean> testPrimitiveOrReferenceEquality() {
     return ImmutableSet.of(
         RoundingMode.UP.equals(RoundingMode.DOWN),
         Objects.equals(RoundingMode.UP, RoundingMode.DOWN),
+        RoundingMode.UP.ordinal() == RoundingMode.DOWN.ordinal(),
         !RoundingMode.UP.equals(RoundingMode.DOWN),
-        !Objects.equals(RoundingMode.UP, RoundingMode.DOWN));
+        !Objects.equals(RoundingMode.UP, RoundingMode.DOWN),
+        RoundingMode.UP.ordinal() != RoundingMode.DOWN.ordinal());
   }
 
   boolean testEqualsPredicate() {

diff --git a/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java b/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java
@@ -21,6 +21,8 @@ ImmutableSet<Boolean> testPrimitiveOrReferenceEquality() {
     return ImmutableSet.of(
         RoundingMode.UP == RoundingMode.DOWN,
         RoundingMode.UP == RoundingMode.DOWN,
+        RoundingMode.UP == RoundingMode.DOWN,
+        RoundingMode.UP != RoundingMode.DOWN,
         RoundingMode.UP != RoundingMode.DOWN,
         RoundingMode.UP != RoundingMode.DOWN);
   }