diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java b/core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java index 476d18cec8c..a4ce2502111 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java +++ b/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 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 OTHER_STRANGE_OPERATIONS = - ImmutableSet.of(Kind.PLUS, Kind.MINUS); + private static final ImmutableSet ARITHMETIC_OPERATORS = + ImmutableSet.of(Kind.PLUS, Kind.MINUS, Kind.MULTIPLY, Kind.DIVIDE); private static final Matcher 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; } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java index bb5ca2ba65c..4b25ff83819 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java +++ b/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); } } """)