diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java index 509cc83fdb2..a61708b79a8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java @@ -28,6 +28,7 @@ import static com.google.errorprone.util.ASTHelpers.constValue; import static com.google.errorprone.util.ASTHelpers.enclosingClass; import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.ASTHelpers.isSameType; import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; import static java.util.EnumSet.allOf; @@ -52,6 +53,7 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; @@ -90,6 +92,7 @@ severity = WARNING) public final class TimeUnitMismatch extends BugChecker implements AssignmentTreeMatcher, + BinaryTreeMatcher, MethodInvocationTreeMatcher, NewClassTreeMatcher, VariableTreeMatcher { @@ -109,6 +112,84 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { return ANY_MATCHES_WERE_ALREADY_REPORTED; } + @Override + public Description matchBinary(BinaryTree tree, VisitorState state) { + if (!improvements + || !NUMERIC_TIME_TYPE.matches(tree.getLeftOperand(), state) + || !NUMERIC_TIME_TYPE.matches(tree.getRightOperand(), state)) { + return Description.NO_MATCH; + } + switch (tree.getKind()) { + case PLUS: + case MINUS: + case LESS_THAN: + case GREATER_THAN: + case LESS_THAN_EQUAL: + case GREATER_THAN_EQUAL: + case EQUAL_TO: + case NOT_EQUAL_TO: + case PLUS_ASSIGNMENT: + case MINUS_ASSIGNMENT: + break; + default: + return Description.NO_MATCH; + } + + TreeAndTimeUnit lhs = unitSuggestedByTree(tree.getLeftOperand()); + TreeAndTimeUnit rhs = unitSuggestedByTree(tree.getRightOperand()); + + if (lhs == null || rhs == null) { + return Description.NO_MATCH; + } + if (lhs.outermostUnit().equals(rhs.outermostUnit())) { + return Description.NO_MATCH; + } + + StringBuilder message = + new StringBuilder( + String.format( + "This operation seems to mix up time units: %s and %s. The generated fix uses the" + + " smaller unit to preserve precision.", + lhs.outermostUnit(), rhs.outermostUnit())); + + if (isSameType(getType(tree), state.getSymtab().booleanType, state)) { + message.append( + " We picked the smaller unit (so larger result) to avoid truncation errors, but this may" + + " result in overflow."); + } else { + message.append( + " We picked the smaller unit to preserve truncation, but this may not be the right unit" + + " for the result. Please review carefully!"); + } + + // To create a fix, pick the smaller unit to retain precision. A better idea would be to look + // at the target of the expression and try to work out the likely target unit, but that's a lot + // harder. + if (lhs.outermostUnit().convert(1, rhs.outermostUnit()) != 0) { + return buildDescription(tree) + .setMessage(message.toString()) + .addFix( + convertTree( + tree.getRightOperand(), + rhs.innermostTree(), + lhs.outermostUnit(), + rhs.innermostUnit(), + state)) + .build(); + } else { + return buildDescription(tree) + .setMessage(message.toString()) + .addFix( + convertTree( + tree.getLeftOperand(), + lhs.innermostTree(), + rhs.outermostUnit(), + lhs.innermostUnit(), + state)) + .build(); + } + } + @Override public Description matchVariable(VariableTree tree, VisitorState state) { if (tree.getInitializer() != null) { @@ -294,26 +375,9 @@ private boolean check(String formalName, ExpressionTree actualTree, VisitorState * to _multiply_ by 1000, rather than divide as we would if we were converting seconds to * milliseconds. */ - SuggestedFix fix; - - if (provided.innermostUnit().equals(targetUnit)) { - fix = SuggestedFix.replace(actualTree, state.getSourceForNode(provided.innermostTree())); - } else { - fix = - SuggestedFix.builder() - // TODO(cpovirk): This can conflict with constants with names like "SECONDS." - .addStaticImport(TimeUnit.class.getName() + "." + provided.innermostUnit()) - // TODO(cpovirk): This won't work for `double` and won't work if the output needs to - // be `int`. - .replace( - actualTree, - String.format( - "%s.%s(%s)", - provided.innermostUnit(), - TIME_UNIT_TO_UNIT_METHODS.get(targetUnit), - state.getSourceForNode(provided.innermostTree()))) - .build(); - } + SuggestedFix fix = + convertTree( + actualTree, provided.innermostTree(), targetUnit, provided.innermostUnit(), state); /* * TODO(cpovirk): Often a better fix would be Duration.ofMillis(...).toNanos(). However, that * implies that the values are durations rather than instants, and it requires Java 8 (and some @@ -329,6 +393,28 @@ private boolean check(String formalName, ExpressionTree actualTree, VisitorState return true; } + private static SuggestedFix convertTree( + ExpressionTree actualTree, + ExpressionTree innerTree, + TimeUnit to, + TimeUnit from, + VisitorState state) { + if (to.equals(from)) { + return SuggestedFix.replace(actualTree, state.getSourceForNode(innerTree)); + } + return SuggestedFix.builder() + // TODO(cpovirk): This can conflict with constants with names like "SECONDS." + .addStaticImport(TimeUnit.class.getName() + "." + from) + // TODO(cpovirk): This won't work for `double` and won't work if the output needs to + // be `int`. + .replace( + actualTree, + String.format( + "%s.%s(%s)", + from, TIME_UNIT_TO_UNIT_METHODS.get(to), state.getSourceForNode(innerTree))) + .build(); + } + /** * Extracts the "argument name," as defined in section 2.1 of "Nomen est Omen," from the * expression. This translates a potentially complex expression into a simple name that can be diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatchTest.java index 6595d6028f0..09588927822 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatchTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatchTest.java @@ -317,6 +317,37 @@ long getMicros() { .doTest(); } + @Test + public void binaryTree() { + compilationHelper + .addSourceLines( + "Test.java", + """ + abstract class Test { + abstract long getStartMillis(); + + abstract long getEndMillis(); + + abstract long getStartNanos(); + + abstract long getEndNanos(); + + void test() { + var b1 = getStartMillis() < getEndMillis(); + var b2 = getStartNanos() + getEndNanos(); + // BUG: Diagnostic contains: MILLISECONDS.toNanos(getStartMillis()) < getEndNanos() + var b3 = getStartMillis() < getEndNanos(); + // BUG: Diagnostic contains: MILLISECONDS.toNanos(getStartMillis()) + getEndNanos() + var b4 = getStartMillis() + getEndNanos(); + // BUG: Diagnostic contains: MILLISECONDS.toNanos(getStartMillis()) + getEndNanos() + var b5 = getStartMillis() * 1000 + getEndNanos(); + var b6 = getStartMillis() * 1_000_000 + getEndNanos(); + } + } + """) + .doTest(); + } + @Test public void testUnitSuggestedByName() { assertSeconds("sleepSec", "deadlineSeconds", "secondsTimeout", "msToS");