From 769779cf219d719f70dd35d47cf2e2e836e6da94 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Wed, 27 Mar 2024 15:44:19 +0100 Subject: [PATCH] Introduce Refaster rules that resolve `EnumOrdinal` violations (#1104) --- .../refasterrules/ComparatorRules.java | 51 +++++++++++++++++++ .../refasterrules/EqualityRules.java | 3 +- .../ComparatorRulesTestInput.java | 17 +++++++ .../ComparatorRulesTestOutput.java | 18 +++++++ .../refasterrules/EqualityRulesTestInput.java | 4 +- .../EqualityRulesTestOutput.java | 2 + 6 files changed, 93 insertions(+), 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 index c6d537c0c3..01494a3fe2 100644 --- 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 after(Comparator cmp) { } } + /** Don't explicitly compare enums by their ordinal. */ + abstract static class ComparingEnum, T> { + @Placeholder(allowsIdentity = true) + abstract E toEnumFunction(@MayOptionallyUse T value); + + @BeforeTemplate + @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */) + Comparator before() { + return comparingInt(v -> toEnumFunction(v).ordinal()); + } + + @AfterTemplate + @UseImportPolicy(STATIC_IMPORT_ALWAYS) + Comparator after() { + return comparing(v -> toEnumFunction(v)); + } + } + /** Don't explicitly create {@link Comparator}s unnecessarily. */ static final class ThenComparing> { @BeforeTemplate @@ -419,4 +440,34 @@ static final class MaxByNaturalOrder> { return maxBy(naturalOrder()); } } + + /** Don't explicitly compare enums by their ordinal. */ + static final class IsLessThan> { + @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> { + @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 index e47918d4bc..7d5f57c234 100644 --- 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> { // 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/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java index 2d92ba5def..0fca14f8d4 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java +++ b/error-prone-contrib/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> testCustomComparator() { Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length))); } + Comparator testComparingEnum() { + return Comparator.comparingInt(s -> RoundingMode.valueOf(s).ordinal()); + } + Comparator testThenComparing() { return Comparator.naturalOrder().thenComparing(Comparator.comparing(String::isEmpty)); } @@ -173,4 +178,16 @@ BinaryOperator testComparatorsMax() { Collector> testMaxByNaturalOrder() { return minBy(reverseOrder()); } + + ImmutableSet testIsLessThan() { + return ImmutableSet.of( + RoundingMode.UP.ordinal() < RoundingMode.DOWN.ordinal(), + RoundingMode.UP.ordinal() >= RoundingMode.DOWN.ordinal()); + } + + ImmutableSet testIsLessThanOrEqualTo() { + return ImmutableSet.of( + RoundingMode.UP.ordinal() <= RoundingMode.DOWN.ordinal(), + RoundingMode.UP.ordinal() > RoundingMode.DOWN.ordinal()); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java index ff2058552b..9c34e00831 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java +++ b/error-prone-contrib/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> testCustomComparator() { Comparator.comparing(s -> "foo", Comparator.comparingInt(String::length))); } + Comparator testComparingEnum() { + return comparing(s -> RoundingMode.valueOf(s)); + } + Comparator testThenComparing() { return Comparator.naturalOrder().thenComparing(String::isEmpty); } @@ -163,4 +169,16 @@ BinaryOperator testComparatorsMax() { Collector> testMaxByNaturalOrder() { return maxBy(naturalOrder()); } + + ImmutableSet testIsLessThan() { + return ImmutableSet.of( + RoundingMode.UP.compareTo(RoundingMode.DOWN) < 0, + RoundingMode.UP.compareTo(RoundingMode.DOWN) >= 0); + } + + ImmutableSet testIsLessThanOrEqualTo() { + return ImmutableSet.of( + RoundingMode.UP.compareTo(RoundingMode.DOWN) <= 0, + RoundingMode.UP.compareTo(RoundingMode.DOWN) > 0); + } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java index 016580a647..7cd4e3bfb4 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestInput.java @@ -21,8 +21,10 @@ ImmutableSet 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/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java index 39bab876fa..0b8af555a7 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/EqualityRulesTestOutput.java @@ -21,6 +21,8 @@ ImmutableSet 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); }