From 6aae9b76ee592b7da70d8f858d30327fec2dacc1 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Mon, 8 Jul 2024 16:30:29 +0200 Subject: [PATCH 1/4] Introduce `PredicateIsEqualEnums` --- .../errorprone/refasterrules/EqualityRules.java | 16 ++++++++++++++++ .../refasterrules/EqualityRulesTestInput.java | 7 ++++++- .../refasterrules/EqualityRulesTestOutput.java | 7 ++++++- 3 files changed, 28 insertions(+), 2 deletions(-) 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 7d5f57c234..80f5bcc2f0 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 @@ -1,5 +1,6 @@ package tech.picnic.errorprone.refasterrules; +import static java.util.function.Predicate.isEqual; import static java.util.function.Predicate.not; import com.google.errorprone.annotations.CanIgnoreReturnValue; @@ -43,6 +44,21 @@ boolean after(T a, T b) { } } + /** + * Prefer reference-based equality for enums over {@link Predicate#isEqual(Object)} comparison. + */ + static final class PredicateIsEqualEnums> { + @BeforeTemplate + Predicate before(T a) { + return isEqual(a); + } + + @AfterTemplate + Predicate after(T a) { + return v -> v == a; + } + } + /** Prefer {@link Object#equals(Object)} over the equivalent lambda function. */ // XXX: As it stands, this rule is a special case of what `MethodReferenceUsage` tries to achieve. // If/when `MethodReferenceUsage` becomes production ready, we should simply drop this check. 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 7cd4e3bfb4..af0ffcf305 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 @@ -1,5 +1,6 @@ package tech.picnic.errorprone.refasterrules; +import static java.util.function.Predicate.isEqual; import static java.util.function.Predicate.not; import com.google.common.collect.BoundType; @@ -14,7 +15,7 @@ final class EqualityRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Objects.class, Optional.class, not(null)); + return ImmutableSet.of(Objects.class, Optional.class, isEqual(null), not(null)); } ImmutableSet testPrimitiveOrReferenceEquality() { @@ -33,6 +34,10 @@ boolean testEqualsPredicate() { return Stream.of("foo").anyMatch(s -> "bar".equals(s)); } + boolean testPredicateIsEqualEnums() { + return Stream.of(RoundingMode.UP).anyMatch(isEqual(RoundingMode.DOWN)); + } + boolean testDoubleNegation() { return !!Boolean.TRUE; } 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 0b8af555a7..c56e21a4f0 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 @@ -1,5 +1,6 @@ package tech.picnic.errorprone.refasterrules; +import static java.util.function.Predicate.isEqual; import static java.util.function.Predicate.not; import com.google.common.collect.BoundType; @@ -14,7 +15,7 @@ final class EqualityRulesTest implements RefasterRuleCollectionTestCase { @Override public ImmutableSet elidedTypesAndStaticImports() { - return ImmutableSet.of(Objects.class, Optional.class, not(null)); + return ImmutableSet.of(Objects.class, Optional.class, isEqual(null), not(null)); } ImmutableSet testPrimitiveOrReferenceEquality() { @@ -33,6 +34,10 @@ boolean testEqualsPredicate() { return Stream.of("foo").anyMatch("bar"::equals); } + boolean testPredicateIsEqualEnums() { + return Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN); + } + boolean testDoubleNegation() { return Boolean.TRUE; } From 62481aa342b0c84b58da8beb08d992ee3e25678f Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Wed, 10 Jul 2024 20:53:52 +0200 Subject: [PATCH 2/4] Missed a spot --- .../picnic/errorprone/refasterrules/EqualityRules.java | 8 +++----- .../errorprone/refasterrules/EqualityRulesTestInput.java | 7 +++++-- .../errorprone/refasterrules/EqualityRulesTestOutput.java | 7 +++++-- 3 files changed, 13 insertions(+), 9 deletions(-) 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 80f5bcc2f0..ce1ae28f95 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 @@ -44,13 +44,11 @@ boolean after(T a, T b) { } } - /** - * Prefer reference-based equality for enums over {@link Predicate#isEqual(Object)} comparison. - */ - static final class PredicateIsEqualEnums> { + /** Prefer reference-based equality for enums over more contrived equality checks. */ + static final class EnumsReferenceEqualityLambda> { @BeforeTemplate Predicate before(T a) { - return isEqual(a); + return Refaster.anyOf(isEqual(a), a::equals); } @AfterTemplate 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 af0ffcf305..fe50095996 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 @@ -34,8 +34,11 @@ boolean testEqualsPredicate() { return Stream.of("foo").anyMatch(s -> "bar".equals(s)); } - boolean testPredicateIsEqualEnums() { - return Stream.of(RoundingMode.UP).anyMatch(isEqual(RoundingMode.DOWN)); + ImmutableSet testEnumsReferenceEqualityLambda() { + + return ImmutableSet.of( + Stream.of(RoundingMode.UP).anyMatch(isEqual(RoundingMode.DOWN)), + Stream.of(RoundingMode.UP).anyMatch(RoundingMode.DOWN::equals)); } boolean testDoubleNegation() { 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 c56e21a4f0..9884f8efb7 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 @@ -34,8 +34,11 @@ boolean testEqualsPredicate() { return Stream.of("foo").anyMatch("bar"::equals); } - boolean testPredicateIsEqualEnums() { - return Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN); + ImmutableSet testEnumsReferenceEqualityLambda() { + + return ImmutableSet.of( + Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN), + Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN)); } boolean testDoubleNegation() { From 982837e798eb732ab48114bfbdb02a95b99599c1 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Thu, 11 Jul 2024 08:56:06 +0200 Subject: [PATCH 3/4] Typo? --- .../tech/picnic/errorprone/refasterrules/EqualityRules.java | 2 +- .../picnic/errorprone/refasterrules/EqualityRulesTestInput.java | 1 - .../errorprone/refasterrules/EqualityRulesTestOutput.java | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) 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 ce1ae28f95..f96c489164 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 @@ -20,7 +20,7 @@ final class EqualityRules { private EqualityRules() {} - /** Prefer reference-based quality for enums. */ + /** Prefer reference-based equality for enums. */ // Primitive value comparisons are not listed, because Error Prone flags those out of the box. static final class PrimitiveOrReferenceEquality> { /** 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 fe50095996..c2cd7335bf 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 @@ -35,7 +35,6 @@ boolean testEqualsPredicate() { } ImmutableSet testEnumsReferenceEqualityLambda() { - return ImmutableSet.of( Stream.of(RoundingMode.UP).anyMatch(isEqual(RoundingMode.DOWN)), Stream.of(RoundingMode.UP).anyMatch(RoundingMode.DOWN::equals)); 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 9884f8efb7..32af82fbe8 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 @@ -35,7 +35,6 @@ boolean testEqualsPredicate() { } ImmutableSet testEnumsReferenceEqualityLambda() { - return ImmutableSet.of( Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN), Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN)); From 3227949e69acd25e6e657068e148ed87b8489c30 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Jul 2024 10:44:51 +0200 Subject: [PATCH 4/4] Suggestions --- .../errorprone/refasterrules/EqualityRules.java | 16 ++++++++-------- .../refasterrules/EqualityRulesTestInput.java | 12 +++++------- .../refasterrules/EqualityRulesTestOutput.java | 12 +++++------- 3 files changed, 18 insertions(+), 22 deletions(-) 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 f96c489164..6f5da60794 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 @@ -21,8 +21,8 @@ final class EqualityRules { private EqualityRules() {} /** Prefer reference-based equality for enums. */ - // Primitive value comparisons are not listed, because Error Prone flags those out of the box. - static final class PrimitiveOrReferenceEquality> { + // Primitive value comparisons are not matched, because Error Prone flags those out of the box. + static final class EnumReferenceEquality> { /** * Enums can be compared by reference. It is safe to do so even in the face of refactorings, * because if the type is ever converted to a non-enum, then Error-Prone will complain about any @@ -44,16 +44,16 @@ boolean after(T a, T b) { } } - /** Prefer reference-based equality for enums over more contrived equality checks. */ - static final class EnumsReferenceEqualityLambda> { + /** Prefer reference-based equality for enums. */ + static final class EnumReferenceEqualityLambda> { @BeforeTemplate - Predicate before(T a) { - return Refaster.anyOf(isEqual(a), a::equals); + Predicate before(T e) { + return Refaster.anyOf(isEqual(e), e::equals); } @AfterTemplate - Predicate after(T a) { - return v -> v == a; + Predicate after(T e) { + return v -> v == e; } } 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 c2cd7335bf..986dabad17 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 @@ -18,7 +18,7 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Objects.class, Optional.class, isEqual(null), not(null)); } - ImmutableSet testPrimitiveOrReferenceEquality() { + ImmutableSet testEnumReferenceEquality() { return ImmutableSet.of( RoundingMode.UP.equals(RoundingMode.DOWN), Objects.equals(RoundingMode.UP, RoundingMode.DOWN), @@ -28,18 +28,16 @@ ImmutableSet testPrimitiveOrReferenceEquality() { RoundingMode.UP.ordinal() != RoundingMode.DOWN.ordinal()); } + ImmutableSet> testEnumReferenceEqualityLambda() { + return ImmutableSet.of(isEqual(RoundingMode.DOWN), RoundingMode.UP::equals); + } + boolean testEqualsPredicate() { // XXX: When boxing is involved this rule seems to break. Example: // Stream.of(1).anyMatch(e -> Integer.MIN_VALUE.equals(e)); return Stream.of("foo").anyMatch(s -> "bar".equals(s)); } - ImmutableSet testEnumsReferenceEqualityLambda() { - return ImmutableSet.of( - Stream.of(RoundingMode.UP).anyMatch(isEqual(RoundingMode.DOWN)), - Stream.of(RoundingMode.UP).anyMatch(RoundingMode.DOWN::equals)); - } - boolean testDoubleNegation() { return !!Boolean.TRUE; } 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 32af82fbe8..b89decfb0d 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 @@ -18,7 +18,7 @@ public ImmutableSet elidedTypesAndStaticImports() { return ImmutableSet.of(Objects.class, Optional.class, isEqual(null), not(null)); } - ImmutableSet testPrimitiveOrReferenceEquality() { + ImmutableSet testEnumReferenceEquality() { return ImmutableSet.of( RoundingMode.UP == RoundingMode.DOWN, RoundingMode.UP == RoundingMode.DOWN, @@ -28,18 +28,16 @@ ImmutableSet testPrimitiveOrReferenceEquality() { RoundingMode.UP != RoundingMode.DOWN); } + ImmutableSet> testEnumReferenceEqualityLambda() { + return ImmutableSet.of(v -> v == RoundingMode.DOWN, v -> v == RoundingMode.UP); + } + boolean testEqualsPredicate() { // XXX: When boxing is involved this rule seems to break. Example: // Stream.of(1).anyMatch(e -> Integer.MIN_VALUE.equals(e)); return Stream.of("foo").anyMatch("bar"::equals); } - ImmutableSet testEnumsReferenceEqualityLambda() { - return ImmutableSet.of( - Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN), - Stream.of(RoundingMode.UP).anyMatch(v -> v == RoundingMode.DOWN)); - } - boolean testDoubleNegation() { return Boolean.TRUE; }