Flag non-relational operations and unary negation on compareTo results in CompareToZero.

core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java

@@ -24,6 +24,8 @@
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
 import static com.google.errorprone.util.ASTHelpers.constValue;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isSameType;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -33,28 +35,55 @@
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
-import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.BinaryTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.ParenthesizedTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
+import com.sun.source.tree.UnaryTree;
 import com.sun.source.util.SimpleTreeVisitor;
 import com.sun.source.util.TreePath;
 
 /** Suggests comparing the result of {@code compareTo} to only {@code 0}. */
 @BugPattern(
     summary =
-        "The result of #compareTo or #compare should only be compared to 0. It is an "
-            + "implementation detail whether a given type returns strictly the values {-1, 0, +1} "
-            + "or others.",
+        """
+        The result of #compareTo or #compare should only be compared to 0. It is an \
+        implementation detail whether a given type returns strictly the values {-1, 0, +1} \
+        or others.\
+        """,
     severity = ERROR)
 public final class CompareToZero extends BugChecker implements MethodInvocationTreeMatcher {
   private static final String SUGGEST_IMPROVEMENT =
-      "It is generally more robust (and readable) to compare the result of #compareTo/#compare to "
-          + "0. Although the suggested replacement is identical in this case, we'd suggest it for "
-          + "consistency.";
+      """
+      It is generally more robust (and readable) to compare the result of #compareTo/#compare to \
+      0. Although the suggested replacement is identical in this case, we'd suggest it for \
+      consistency.\
+      """;
+
+  private static final String NON_RELATIONAL_OPERATOR =
+      """
+      It is unsafe to perform arithmetic or logical operations on the result of #compareTo or \
+      #compare, as the return value is not guaranteed to be strictly in the range {-1, 0, \
+      +1}. Consider using Integer.signum to normalize the result before performing \
+      operations.\
+      """;
+
+  private static final String ARITHMETIC_OPERATOR =
+      """
+      It is unsafe to perform arithmetic operations on the result of #compareTo or #compare, as \
+      the return value is not guaranteed to be strictly in the range {-1, 0, +1}, and operations \
+      (especially multiplication) can also overflow. Consider using Integer.signum on the result \
+      of #compareTo or #compare.\
+      """;
+
+  private static final String NEGATION_OPERATOR =
+      """
+      It is unsafe to negate the result of #compareTo or #compare, as negating the result can \
+      overflow if compareTo returns Integer.MIN_VALUE. Consider using Integer.signum on \
+      the result of #compareTo or #compare.\
+      """;
 
   private static final ImmutableSet<Kind> COMPARISONS =
       ImmutableSet.of(
@@ -72,8 +101,8 @@ public final class CompareToZero extends BugChecker implements MethodInvocationT
           Kind.GREATER_THAN, Kind.LESS_THAN,
           Kind.GREATER_THAN_EQUAL, Kind.LESS_THAN_EQUAL);
 
-  private static final ImmutableSet<Kind> OTHER_STRANGE_OPERATIONS =
-      ImmutableSet.of(Kind.PLUS, Kind.MINUS);
+  private static final ImmutableSet<Kind> ARITHMETIC_OPERATORS =
+      ImmutableSet.of(Kind.PLUS, Kind.MINUS, Kind.MULTIPLY, Kind.DIVIDE);
 
   private static final Matcher<ExpressionTree> COMPARE_TO =
       anyOf(
@@ -121,6 +150,16 @@ public Void visitParenthesized(ParenthesizedTree parenthesizedTree, VisitorState
       return visitParent(state);
     }
 
+    @Override
+    public Void visitUnary(UnaryTree unaryTree, VisitorState state) {
+      if (unaryTree.getKind() == Kind.UNARY_MINUS) {
+        state.reportMatch(buildDescription(unaryTree).setMessage(NEGATION_OPERATOR).build());
+      } else if (unaryTree.getKind() == Kind.BITWISE_COMPLEMENT) {
+        state.reportMatch(buildDescription(unaryTree).setMessage(NON_RELATIONAL_OPERATOR).build());
+      }
+      return null;
+    }
+
     @Override
     public Void visitBinary(BinaryTree binaryTree, VisitorState state) {
       Kind kind = binaryTree.getKind();
@@ -133,14 +172,14 @@ public Void visitBinary(BinaryTree binaryTree, VisitorState state) {
       ExpressionTree otherSide =
           reversed ? binaryTree.getLeftOperand() : binaryTree.getRightOperand();
 
-      if (OTHER_STRANGE_OPERATIONS.contains(kind)) {
-        // Consider string concatenation with the result of compareTo to be OK, but otherwise
-        // raise the alarm.
-        if (!(kind.equals(Kind.PLUS)
-            && ASTHelpers.isSameType(
-                ASTHelpers.getType(otherSide), state.getSymtab().stringType, state))) {
-          state.reportMatch(describeMatch(binaryTree));
-        }
+      if (!COMPARISONS.contains(kind)) {
+        boolean isStringConcat =
+            isSameType(getType(binaryTree), state.getSymtab().stringType, state);
+        String message =
+            ARITHMETIC_OPERATORS.contains(kind) && !isStringConcat
+                ? ARITHMETIC_OPERATOR
+                : NON_RELATIONAL_OPERATOR;
+        state.reportMatch(buildDescription(binaryTree).setMessage(message).build());
         return null;
       }
 
@@ -185,9 +224,7 @@ public Void visitBinary(BinaryTree binaryTree, VisitorState state) {
             buildDescription(binaryTree).setMessage(SUGGEST_IMPROVEMENT).addFix(fix).build());
         return null;
       }
-      if (COMPARISONS.contains(binaryTree.getKind())) {
-        state.reportMatch(describeMatch(binaryTree));
-      }
+      state.reportMatch(describeMatch(binaryTree));
       return null;
     }
 

core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java

@@ -104,7 +104,7 @@ public void positiveAddition() {
             """
             class Test {
               int test(Integer i) {
-                // BUG: Diagnostic contains:
+                // BUG: Diagnostic contains: unsafe to perform arithmetic operations
                 return i.compareTo(2) + i.compareTo(3);
               }
             }
@@ -113,13 +113,70 @@ int test(Integer i) {
   }
 
   @Test
-  public void stringConcat_ignored() {
+  public void positiveArithmetic() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Integer i) {
+                // BUG: Diagnostic contains: unsafe to perform arithmetic operations
+                int a = i.compareTo(2) * 2;
+                // BUG: Diagnostic contains: unsafe to perform arithmetic operations
+                int c = i.compareTo(2) / 2;
+                // BUG: Diagnostic contains: unsafe to perform arithmetic or logical operations
+                int b = i.compareTo(2) % -1;
+                // BUG: Diagnostic contains: unsafe to perform arithmetic operations
+                int d = i.compareTo(2) - 1;
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void positiveUnaryOperators() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Integer i) {
+                // BUG: Diagnostic contains: unsafe to negate the result
+                int a = -i.compareTo(2);
+                // BUG: Diagnostic contains: unsafe to perform arithmetic or logical operations
+                int c = ~i.compareTo(2);
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void positiveBitwise() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              int test(Integer i) {
+                // BUG: Diagnostic contains: unsafe to perform arithmetic or logical operations
+                return i.compareTo(2) & 1;
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void stringConcat() {
     compilationHelper
         .addSourceLines(
             "Test.java",
             """
             class Test {
               String test(Integer i) {
+                // BUG: Diagnostic contains: unsafe to perform arithmetic or logical operations
                 return "" + i.compareTo(3);
               }
             }
@@ -176,6 +233,7 @@ void test(Integer i) {
                 boolean b1 = i.compareTo(2) < 0;
                 boolean b2 = i.compareTo(2) > 0;
                 boolean b3 = i.compareTo(2) == 0;
+                int b4 = +i.compareTo(2);
               }
             }
             """)