diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java index 27e62bdd8f4..10e2c939b3c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java @@ -6,6 +6,7 @@ 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.NotMatches; import com.google.errorprone.refaster.annotation.UseImportPolicy; import java.math.BigDecimal; import java.math.BigInteger; @@ -18,6 +19,7 @@ import org.assertj.core.api.AbstractLongAssert; import org.assertj.core.api.AbstractShortAssert; import org.assertj.core.api.NumberAssert; +import tech.picnic.errorprone.refaster.util.IsCharacter; final class AssertJNumberTemplates { private AssertJNumberTemplates() {} @@ -226,9 +228,14 @@ AbstractBigDecimalAssert before(AbstractBigDecimalAssert numberAssert) { } } + /** + * Prefer {@link AbstractLongAssert#isOdd()} over more contrived alternatives. + * + *

Note that for {@link Character}s this rewrite would lead to non-compilable code. + */ static final class AssertThatIsOdd { @BeforeTemplate - AbstractIntegerAssert before(int number) { + AbstractIntegerAssert before(@NotMatches(IsCharacter.class) int number) { return assertThat(number % 2).isEqualTo(1); } @@ -244,9 +251,14 @@ AbstractLongAssert before(long number) { } } + /** + * Prefer {@link AbstractLongAssert#isEven()} over more contrived alternatives. + * + *

Note that for {@link Character}s this rewrite would lead to non-compilable code. + */ static final class AssertThatIsEven { @BeforeTemplate - AbstractIntegerAssert before(int number) { + AbstractIntegerAssert before(@NotMatches(IsCharacter.class) int number) { return assertThat(number % 2).isEqualTo(0); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/EqualityTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/EqualityTemplates.java index 1f101d8e269..3a1e267c671 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/EqualityTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/EqualityTemplates.java @@ -68,17 +68,14 @@ boolean after(boolean b) { * Don't negate an equality test or use the ternary operator to compare two booleans; directly * test for inequality instead. */ + // XXX: Replacing `a ? !b : b` with `a != b` changes semantics if both `a` and `b` are boxed + // booleans. static final class Negation { @BeforeTemplate boolean before(boolean a, boolean b) { return Refaster.anyOf(!(a == b), a ? !b : b); } - @BeforeTemplate - boolean before(long a, long b) { - return !(a == b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a == b); @@ -99,17 +96,14 @@ boolean after(boolean a, boolean b) { * Don't negate an inequality test or use the ternary operator to compare two booleans; directly * test for equality instead. */ + // XXX: Replacing `a ? b : !b` with `a == b` changes semantics if both `a` and `b` are boxed + // booleans. static final class IndirectDoubleNegation { @BeforeTemplate boolean before(boolean a, boolean b) { return Refaster.anyOf(!(a != b), a ? b : !b); } - @BeforeTemplate - boolean before(long a, long b) { - return !(a != b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a != b); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/PrimitiveTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/PrimitiveTemplates.java index 259cea802b3..622d2538c87 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/PrimitiveTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/PrimitiveTemplates.java @@ -10,11 +10,6 @@ private PrimitiveTemplates() {} /** Avoid contrived ways of expressing the "less than" relationship. */ static final class LessThan { - @BeforeTemplate - boolean before(long a, long b) { - return !(a >= b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a >= b); @@ -28,11 +23,6 @@ boolean after(long a, long b) { /** Avoid contrived ways of expressing the "less than or equal to" relationship. */ static final class LessThanOrEqualTo { - @BeforeTemplate - boolean before(long a, long b) { - return !(a > b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a > b); @@ -46,11 +36,6 @@ boolean after(long a, long b) { /** Avoid contrived ways of expressing the "greater than" relationship. */ static final class GreaterThan { - @BeforeTemplate - boolean before(long a, long b) { - return !(a <= b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a <= b); @@ -64,11 +49,6 @@ boolean after(long a, long b) { /** Avoid contrived ways of expressing the "greater than or equal to" relationship. */ static final class GreaterThanOrEqualTo { - @BeforeTemplate - boolean before(long a, long b) { - return !(a < b); - } - @BeforeTemplate boolean before(double a, double b) { return !(a < b); diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestInput.java index d92b1cdf14d..ce5773aabe7 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestInput.java @@ -81,23 +81,27 @@ final class AssertJNumberTemplatesTest implements RefasterTemplateTestCase { return ImmutableSet.of( assertThat((byte) 1 % 2).isEqualTo(1), assertThat(Byte.valueOf((byte) 1) % 2).isEqualTo(1), + assertThat((char) 1 % 2).isEqualTo(1), + assertThat(Character.valueOf((char) 1) % 2).isEqualTo(1), + assertThat((short) 1 % 2).isEqualTo(1), + assertThat(Short.valueOf((short) 1) % 2).isEqualTo(1), assertThat(1 % 2).isEqualTo(1), assertThat(Integer.valueOf(1) % 2).isEqualTo(1), assertThat(1L % 2).isEqualTo(1), - assertThat(Long.valueOf(1) % 2).isEqualTo(1), - assertThat((short) 1 % 2).isEqualTo(1), - assertThat(Short.valueOf((short) 1) % 2).isEqualTo(1)); + assertThat(Long.valueOf(1) % 2).isEqualTo(1)); } ImmutableSet> testAssertThatIsEven() { return ImmutableSet.of( assertThat((byte) 1 % 2).isEqualTo(0), assertThat(Byte.valueOf((byte) 1) % 2).isEqualTo(0), + assertThat((char) 1 % 2).isEqualTo(0), + assertThat(Character.valueOf((char) 1) % 2).isEqualTo(0), + assertThat((short) 1 % 2).isEqualTo(0), + assertThat(Short.valueOf((short) 1) % 2).isEqualTo(0), assertThat(1 % 2).isEqualTo(0), assertThat(Integer.valueOf(1) % 2).isEqualTo(0), assertThat(1L % 2).isEqualTo(0), - assertThat(Long.valueOf(1) % 2).isEqualTo(0), - assertThat((short) 1 % 2).isEqualTo(0), - assertThat(Short.valueOf((short) 1) % 2).isEqualTo(0)); + assertThat(Long.valueOf(1) % 2).isEqualTo(0)); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestOutput.java index 72b9f27ce45..227fedf57e3 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplatesTestOutput.java @@ -81,23 +81,27 @@ final class AssertJNumberTemplatesTest implements RefasterTemplateTestCase { return ImmutableSet.of( assertThat((byte) 1).isOdd(), assertThat(Byte.valueOf((byte) 1)).isOdd(), + assertThat((char) 1 % 2).isEqualTo(1), + assertThat(Character.valueOf((char) 1) % 2).isEqualTo(1), + assertThat((short) 1).isOdd(), + assertThat(Short.valueOf((short) 1)).isOdd(), assertThat(1).isOdd(), assertThat(Integer.valueOf(1)).isOdd(), assertThat(1L).isOdd(), - assertThat(Long.valueOf(1)).isOdd(), - assertThat((short) 1).isOdd(), - assertThat(Short.valueOf((short) 1)).isOdd()); + assertThat(Long.valueOf(1)).isOdd()); } ImmutableSet> testAssertThatIsEven() { return ImmutableSet.of( assertThat((byte) 1).isEven(), assertThat(Byte.valueOf((byte) 1)).isEven(), + assertThat((char) 1 % 2).isEqualTo(0), + assertThat(Character.valueOf((char) 1) % 2).isEqualTo(0), + assertThat((short) 1).isEven(), + assertThat(Short.valueOf((short) 1)).isEven(), assertThat(1).isEven(), assertThat(Integer.valueOf(1)).isEven(), assertThat(1L).isEven(), - assertThat(Long.valueOf(1)).isEven(), - assertThat((short) 1).isEven(), - assertThat(Short.valueOf((short) 1)).isEven()); + assertThat(Long.valueOf(1)).isEven()); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestInput.java index f8299fc8adb..129fe4eaf1f 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestInput.java @@ -31,11 +31,13 @@ boolean testDoubleNegation() { return !!Boolean.TRUE; } + @SuppressWarnings("SimplifyBooleanExpression") ImmutableSet testNegation() { return ImmutableSet.of( - Boolean.TRUE ? !Boolean.FALSE : Boolean.FALSE, - !(Boolean.TRUE == Boolean.FALSE), + true ? !false : false, + !(true == false), !((byte) 3 == (byte) 4), + !((char) 3 == (char) 4), !((short) 3 == (short) 4), !(3 == 4), !(3L == 4L), @@ -44,11 +46,13 @@ ImmutableSet testNegation() { !(BoundType.OPEN == BoundType.CLOSED)); } + @SuppressWarnings("SimplifyBooleanExpression") ImmutableSet testIndirectDoubleNegation() { return ImmutableSet.of( - Boolean.TRUE ? Boolean.FALSE : !Boolean.FALSE, - !(Boolean.TRUE != Boolean.FALSE), + true ? false : !false, + !(true != false), !((byte) 3 != (byte) 4), + !((char) 3 != (char) 4), !((short) 3 != (short) 4), !(3 != 4), !(3L != 4L), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestOutput.java index f142f465f12..907e4768ce9 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/EqualityTemplatesTestOutput.java @@ -31,11 +31,13 @@ boolean testDoubleNegation() { return Boolean.TRUE; } + @SuppressWarnings("SimplifyBooleanExpression") ImmutableSet testNegation() { return ImmutableSet.of( - Boolean.TRUE != Boolean.FALSE, - Boolean.TRUE != Boolean.FALSE, + true != false, + true != false, (byte) 3 != (byte) 4, + (char) 3 != (char) 4, (short) 3 != (short) 4, 3 != 4, 3L != 4L, @@ -44,11 +46,13 @@ ImmutableSet testNegation() { BoundType.OPEN != BoundType.CLOSED); } + @SuppressWarnings("SimplifyBooleanExpression") ImmutableSet testIndirectDoubleNegation() { return ImmutableSet.of( - Boolean.TRUE == Boolean.FALSE, - Boolean.TRUE == Boolean.FALSE, + true == false, + true == false, (byte) 3 == (byte) 4, + (char) 3 == (char) 4, (short) 3 == (short) 4, 3 == 4, 3L == 4L, diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestInput.java index d41931b501a..c94bdc65a2e 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestInput.java @@ -13,6 +13,7 @@ public ImmutableSet elidedTypesAndStaticImports() { ImmutableSet testLessThan() { return ImmutableSet.of( !((byte) 3 >= (byte) 4), + !((char) 3 >= (char) 4), !((short) 3 >= (short) 4), !(3 >= 4), !(3L >= 4L), @@ -23,6 +24,7 @@ ImmutableSet testLessThan() { ImmutableSet testLessThanOrEqualTo() { return ImmutableSet.of( !((byte) 3 > (byte) 4), + !((char) 3 > (char) 4), !((short) 3 > (short) 4), !(3 > 4), !(3L > 4L), @@ -33,6 +35,7 @@ ImmutableSet testLessThanOrEqualTo() { ImmutableSet testGreaterThan() { return ImmutableSet.of( !((byte) 3 <= (byte) 4), + !((char) 3 <= (char) 4), !((short) 3 <= (short) 4), !(3 <= 4), !(3L <= 4L), @@ -43,6 +46,7 @@ ImmutableSet testGreaterThan() { ImmutableSet testGreaterThanOrEqualTo() { return ImmutableSet.of( !((byte) 3 < (byte) 4), + !((char) 3 < (char) 4), !((short) 3 < (short) 4), !(3 < 4), !(3L < 4L), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestOutput.java index ba2647ad838..0c7e1034c8a 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refastertemplates/PrimitiveTemplatesTestOutput.java @@ -12,22 +12,46 @@ public ImmutableSet elidedTypesAndStaticImports() { ImmutableSet testLessThan() { return ImmutableSet.of( - (byte) 3 < (byte) 4, (short) 3 < (short) 4, 3 < 4, 3L < 4L, 3F < 4F, 3.0 < 4.0); + (byte) 3 < (byte) 4, + (char) 3 < (char) 4, + (short) 3 < (short) 4, + 3 < 4, + 3L < 4L, + 3F < 4F, + 3.0 < 4.0); } ImmutableSet testLessThanOrEqualTo() { return ImmutableSet.of( - (byte) 3 <= (byte) 4, (short) 3 <= (short) 4, 3 <= 4, 3L <= 4L, 3F <= 4F, 3.0 <= 4.0); + (byte) 3 <= (byte) 4, + (char) 3 <= (char) 4, + (short) 3 <= (short) 4, + 3 <= 4, + 3L <= 4L, + 3F <= 4F, + 3.0 <= 4.0); } ImmutableSet testGreaterThan() { return ImmutableSet.of( - (byte) 3 > (byte) 4, (short) 3 > (short) 4, 3 > 4, 3L > 4L, 3F > 4F, 3.0 > 4.0); + (byte) 3 > (byte) 4, + (char) 3 > (char) 4, + (short) 3 > (short) 4, + 3 > 4, + 3L > 4L, + 3F > 4F, + 3.0 > 4.0); } ImmutableSet testGreaterThanOrEqualTo() { return ImmutableSet.of( - (byte) 3 >= (byte) 4, (short) 3 >= (short) 4, 3 >= 4, 3L >= 4L, 3F >= 4F, 3.0 >= 4.0); + (byte) 3 >= (byte) 4, + (char) 3 >= (char) 4, + (short) 3 >= (short) 4, + 3 >= 4, + 3L >= 4L, + 3F >= 4F, + 3.0 >= 4.0); } int testLongToIntExact() { diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java new file mode 100644 index 00000000000..2d8cc1b1b26 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/util/IsCharacter.java @@ -0,0 +1,20 @@ +package tech.picnic.errorprone.refaster.util; + +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.isSameType; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; + +/** A matcher of {@link Character}-typed expressions. */ +public final class IsCharacter implements Matcher { + private static final long serialVersionUID = 1L; + private static final Matcher DELEGATE = + anyOf(isSameType(Character.class), isSameType(s -> s.getSymtab().charType)); + + @Override + public boolean matches(ExpressionTree tree, VisitorState state) { + return DELEGATE.matches(tree, state); + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsCharacterTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsCharacterTest.java new file mode 100644 index 00000000000..01977513f25 --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/util/IsCharacterTest.java @@ -0,0 +1,65 @@ +package tech.picnic.errorprone.refaster.util; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.bugpatterns.BugChecker; +import org.junit.jupiter.api.Test; + +final class IsCharacterTest { + @Test + void matches() { + CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " String negative1() {", + " return \"a\";", + " }", + "", + " String negative2() {", + " return \"a\".toCharArray().toString();", + " }", + "", + " Character positive1() {", + " // BUG: Diagnostic contains:", + " return 'a';", + " }", + "", + " Character positive2() {", + " // BUG: Diagnostic contains:", + " return Character.valueOf('a');", + " }", + "", + " char positive3() {", + " // BUG: Diagnostic contains:", + " return \"a\".charAt(1);", + " }", + "", + " char positive4() {", + " // BUG: Diagnostic contains:", + " return Character.valueOf((char) 1);", + " }", + "", + " char positive5() {", + " // BUG: Diagnostic contains:", + " return (char) 1;", + " }", + "}") + .doTest(); + } + + /** A {@link BugChecker} which simply delegates to {@link IsCharacter}. */ + @BugPattern(summary = "Flags expressions matched by `IsCharacter`", severity = ERROR) + public static final class MatcherTestChecker extends AbstractMatcherTestChecker { + private static final long serialVersionUID = 1L; + + // XXX: This is a false positive reported by Checkstyle. See + // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120. + @SuppressWarnings("RedundantModifier") + public MatcherTestChecker() { + super(new IsCharacter()); + } + } +}