From c49e6c9a8296d5f9b38db7932c542fa79ab3befc Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 12 Jul 2024 15:30:20 -0700 Subject: [PATCH] Improve XorPower * handle longs as well as ints * handle e.g. `x * 10 ^ 3`, which evalutes as `(x * 10) ^ 3`, but where the intent may have been `x * 1000` PiperOrigin-RevId: 651903955 --- .../errorprone/bugpatterns/XorPower.java | 34 ++++++++++--- .../errorprone/bugpatterns/XorPowerTest.java | 50 ++++++++++++++++++- 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/XorPower.java b/core/src/main/java/com/google/errorprone/bugpatterns/XorPower.java index d118b3ebe344..5333881012ce 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/XorPower.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/XorPower.java @@ -18,8 +18,8 @@ import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; -import com.google.common.base.Strings; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; @@ -37,10 +37,17 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { if (!tree.getKind().equals(Tree.Kind.XOR)) { return NO_MATCH; } - Integer lhs = ASTHelpers.constValue(tree.getLeftOperand(), Integer.class); + Tree lhsTree = tree.getLeftOperand(); + while (lhsTree instanceof BinaryTree) { + lhsTree = ((BinaryTree) lhsTree).getRightOperand(); + } + Number lhs = ASTHelpers.constValue(lhsTree, Number.class); if (lhs == null) { return NO_MATCH; } + if (lhs.longValue() != lhs.intValue()) { + return NO_MATCH; + } switch (lhs.intValue()) { case 2: case 10: @@ -48,10 +55,13 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { default: return NO_MATCH; } - Integer rhs = ASTHelpers.constValue(tree.getRightOperand(), Integer.class); + Number rhs = ASTHelpers.constValue(tree.getRightOperand(), Number.class); if (rhs == null) { return NO_MATCH; } + if (rhs.longValue() != rhs.intValue()) { + return NO_MATCH; + } if (state.getSourceForNode(tree.getRightOperand()).startsWith("0")) { // hex and octal literals return NO_MATCH; @@ -62,16 +72,24 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { String.format( "The ^ operator is binary XOR, not a power operator, so '%s' will always" + " evaluate to %d.", - state.getSourceForNode(tree), lhs ^ rhs)); + state.getSourceForNode(tree), lhs.intValue() ^ rhs.intValue())); + String suffix = lhs instanceof Long ? "L" : ""; + int start = getStartPosition(lhsTree); + int end = state.getEndPosition(tree); switch (lhs.intValue()) { case 2: - if (rhs <= 31) { - description.addFix(SuggestedFix.replace(tree, String.format("1 << %d", rhs))); + if (rhs.intValue() <= (lhs instanceof Long ? 63 : 31)) { + String replacement = String.format("1%s << %d", suffix, rhs); + if (start != getStartPosition(tree)) { + replacement = "(" + replacement + ")"; + } + description.addFix(SuggestedFix.replace(start, end, replacement)); } break; case 10: - if (rhs <= 9) { - description.addFix(SuggestedFix.replace(tree, "1" + Strings.repeat("0", rhs))); + if (rhs.intValue() <= (lhs instanceof Long ? 18 : 9)) { + description.addFix( + SuggestedFix.replace(start, end, "1" + "0".repeat(rhs.intValue()) + suffix)); } break; default: diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/XorPowerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/XorPowerTest.java index d5e954c333ab..51936faf33ce 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/XorPowerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/XorPowerTest.java @@ -32,7 +32,7 @@ public void positive() { "Test.java", "class Test {", " static final int X = 2 ^ 16;", - " static final int Y = 2 ^ 32;", + " static final int TOO_BIG = 2 ^ 32;", " static final int Z = 2 ^ 31;", " static final int P = 10 ^ 6;", "}") @@ -40,7 +40,7 @@ public void positive() { "Test.java", "class Test {", " static final int X = 1 << 16;", - " static final int Y = 2 ^ 32;", + " static final int TOO_BIG = 2 ^ 32;", " static final int Z = 1 << 31;", " static final int P = 1000000;", "}") @@ -59,4 +59,50 @@ public void negative() { "}") .doTest(); } + + @Test + public void positiveLongs() { + BugCheckerRefactoringTestHelper.newInstance(XorPower.class, getClass()) + .addInputLines( + "Test.java", + "class Test {", + " static final long X = 2L ^ 16;", + " static final long Y = 2L ^ 32;", + " static final long Z = 2L ^ 31;", + " static final long TOO_BIG = 2L ^ 64;", + " static final long P = 10L ^ 6;", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " static final long X = 1L << 16;", + " static final long Y = 1L << 32;", + " static final long Z = 1L << 31;", + " static final long TOO_BIG = 2L ^ 64;", + " static final long P = 1000000L;", + "}") + .doTest(); + } + + @Test + public void precedence() { + BugCheckerRefactoringTestHelper.newInstance(XorPower.class, getClass()) + .addInputLines( + "Test.java", // + "class Test {", + " void f(int i) {", + " int x = i * 2 ^ 16;", + " int y = i * 10 ^ 3;", + " }", + "}") + .addOutputLines( + "Test.java", // + "class Test {", + " void f(int i) {", + " int x = i * (1 << 16);", + " int y = i * 1000;", + " }", + "}") + .doTest(); + } }