Require `Integer.signum()` when switching over the result of `compare()` or `compareTo()`. Extend `CompareToZero` to also match `switch` statements and `switch` expressions whose selector expression is a `Comparable.compareTo()` or `Comparator.compare()` (or static `compare()`) call, suggesting `signum(...)` around the selector expression. PiperOrigin-RevId: 983432744
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 a4ce250..6956266 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CompareToZero.java
@@ -26,6 +26,7 @@ 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 static com.google.errorprone.util.ASTHelpers.stripParentheses; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -39,6 +40,8 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.SwitchExpressionTree; +import com.sun.source.tree.SwitchTree; import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.UnaryTree; @@ -161,6 +164,30 @@ } @Override + public Void visitSwitch(SwitchTree switchTree, VisitorState state) { + return handleSwitch(switchTree, switchTree.getExpression(), state); + } + + @Override + public Void visitSwitchExpression( + SwitchExpressionTree switchExpressionTree, VisitorState state) { + return handleSwitch(switchExpressionTree, switchExpressionTree.getExpression(), state); + } + + private Void handleSwitch(Tree switchTree, ExpressionTree expression, VisitorState state) { + if (expression == child) { + ExpressionTree selector = stripParentheses(expression); + SuggestedFix fix = + SuggestedFix.builder() + .addStaticImport("java.lang.Integer.signum") + .replace(selector, String.format("signum(%s)", state.getSourceForNode(selector))) + .build(); + state.reportMatch(describeMatch(switchTree, fix)); + } + return null; + } + + @Override public Void visitBinary(BinaryTree binaryTree, VisitorState state) { Kind kind = binaryTree.getKind();
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 3acb45e..7d1591e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CompareToZeroTest.java
@@ -242,15 +242,14 @@ @Test public void switchStatements_positive() { - compilationHelper - .addSourceLines( + refactoringHelper + .addInputLines( "Test.java", """ import java.util.Comparator; class Test { boolean switchCompare(String left, String right, Comparator<String> comparator) { - // TODO(b/561674957): we should flag this! switch (comparator.compare(left, right)) { case 1 -> { return false; @@ -265,7 +264,6 @@ } boolean switchCompareTo(String left, String right) { - // TODO(b/561674957): we should flag this! switch (left.compareTo(right)) { case 1 -> { return false; @@ -278,6 +276,64 @@ } return false; } + boolean switchCompareToOnlyZero(String left, String right) { + switch (left.compareTo(right)) { + case 0 -> { + return true; + } + default -> { + return false; + } + } + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.lang.Integer.signum; + + import java.util.Comparator; + + class Test { + boolean switchCompare(String left, String right, Comparator<String> comparator) { + switch (signum(comparator.compare(left, right))) { + case 1 -> { + return false; + } + case -1 -> { + return true; + } + case 0 -> {} + default -> {} + } + return false; + } + + boolean switchCompareTo(String left, String right) { + switch (signum(left.compareTo(right))) { + case 1 -> { + return false; + } + case -1 -> { + return true; + } + case 0 -> {} + default -> {} + } + return false; + } + + boolean switchCompareToOnlyZero(String left, String right) { + switch (signum(left.compareTo(right))) { + case 0 -> { + return true; + } + default -> { + return false; + } + } + } } """) .doTest(); @@ -289,6 +345,8 @@ .addSourceLines( "Test.java", """ + import static java.lang.Integer.signum; + import java.util.Comparator; class Test { @@ -319,6 +377,27 @@ } return false; } + + boolean switchNegativeOneAndDefault(String left, String right) { + return switch (signum(left.compareTo(right))) { + case -1 -> true; + default -> false; + }; + } + + boolean switchNegativeOnePositiveOneAndDefault(String left, String right) { + return switch (signum(left.compareTo(right))) { + case -1, 1 -> true; + default -> false; + }; + } + + boolean switchZeroPositiveOneAndDefault(String left, String right) { + return switch (signum(left.compareTo(right))) { + case 0, 1 -> true; + default -> false; + }; + } } """) .doTest();
diff --git a/docs/bugpattern/CompareToZero.md b/docs/bugpattern/CompareToZero.md index f10d1a3..f2978e3 100644 --- a/docs/bugpattern/CompareToZero.md +++ b/docs/bugpattern/CompareToZero.md
@@ -1,13 +1,12 @@ The contract for `Comparator#compare` and `Comparable#compareTo` states that the result is an integer which is `< 0` for less than, `== 0` for equality and `> 0` for greater than. While most implementations return `-1`, `0` and `+1` for those -cases respectively, this is not guaranteed. Always comparing to `0` is the -safest use of the return value. +cases respectively, this is not guaranteed. Always comparing directly against +`0` is the safest use of the return value. ```java boolean <T> isLessThan(Comparator<T> comparator, T a, T b) { - // Fragile: it's not guaranteed that `comparator` returns -1 to mean - // "less than". + // Fragile: it's not guaranteed that `comparator` returns -1 to mean "less than". return comparator.compare(a, b) == -1; } ``` @@ -32,3 +31,39 @@ return comparator.compare(a, b) > 0; } ``` + +When comparing against `0`, `0` should always be on the right-hand side of the +operator so that `a.compareTo(b) <op> 0` mirrors the relationship `a <op> b`: + +```java + boolean <T> greaterThan(Comparable<T> a, T b) { + // Confusing: `<` is used to check that `a` is greater than `b`. + return 0 < a.compareTo(b); + } +``` + +```java + boolean <T> greaterThan(Comparable<T> a, T b) { + return a.compareTo(b) > 0; + } +``` + +Similarly, when switching on the result of `compare` or `compareTo` in a +`switch` statement or expression, the selector must be wrapped in +`Integer.signum()` to normalize the result to `-1`, `0`, or `1`: + +```java + switch (comparator.compare(a, b)) { + case -1 -> ... + case 0 -> ... + default -> ... + } +``` + +```java + switch (signum(comparator.compare(a, b))) { + case -1 -> ... + case 0 -> ... + default -> ... + } +```