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: 652520871
google · Jul 15, 2024 · 2b306b6 · 2b306b6
1 parent 008cfb0
commit 2b306b6
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 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
@@ -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,21 +37,31 @@ 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:
         break;
       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
@@ -32,15 +32,15 @@ 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;",
             "}")
         .addOutputLines(
             "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();
+  }
 }