Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimeUnitMismatch: consider trees like fooSeconds * 1000 to have units of millis. #4632

Merged
merged 1 commit into from
Oct 22, 2024
Merged

TimeUnitMismatch: consider trees like fooSeconds * 1000 to have units of millis. #4632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 142 additions & 28 deletions core/src/main/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
import static com.google.errorprone.suppliers.Suppliers.DOUBLE_TYPE;
import static com.google.errorprone.suppliers.Suppliers.INT_TYPE;
import static com.google.errorprone.suppliers.Suppliers.LONG_TYPE;
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.isSameType;
import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT;
import static java.util.EnumSet.allOf;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.concurrent.TimeUnit.MICROSECONDS;
Expand All @@ -37,13 +39,15 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;

import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.bugpatterns.BugChecker;
Expand All @@ -57,12 +61,14 @@
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.TypeCastTree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Symbol;
Expand All @@ -73,6 +79,7 @@
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import javax.inject.Inject;
import org.jspecify.annotations.Nullable;

/** Checker that detects likely time-unit mismatches by looking at identifier names. */
Expand All @@ -86,6 +93,13 @@ public final class TimeUnitMismatch extends BugChecker
MethodInvocationTreeMatcher,
NewClassTreeMatcher,
VariableTreeMatcher {
private final boolean improvements;

@Inject
TimeUnitMismatch(ErrorProneFlags flags) {
this.improvements = flags.getBoolean("TimeUnitMismatch:improvements").orElse(true);
}

@Override
public Description matchAssignment(AssignmentTree tree, VisitorState state) {
String formalName = extractArgumentName(tree.getVariable());
Expand Down Expand Up @@ -242,33 +256,22 @@ private boolean check(String formalName, ExpressionTree actualTree, VisitorState
* seconds?
*/

String actualName = extractArgumentName(actualTree);
if (actualName == null) {
/*
* TODO(cpovirk): Look for other assignments to a variable in the method to guess its type.
* (Maybe even guess the type returned by a method by looking at other calls in the file?) Of
* course, that may be slow.
*/
// TODO(cpovirk): Look for multiplication/division operations that are meant to change units.
// TODO(cpovirk): ...even if they include casts!
return false;
}

TimeUnit formalUnit = unitSuggestedByName(formalName);
TimeUnit actualUnit = unitSuggestedByName(actualName);
if (formalUnit == null || actualUnit == null || formalUnit == actualUnit) {
TimeUnit targetUnit = unitSuggestedByName(formalName);
TreeAndTimeUnit provided = unitSuggestedByTree(actualTree);
if (targetUnit == null || provided == null || targetUnit.equals(provided.outermostUnit())) {
return false;
}
TimeUnit providedUnit = provided.outermostUnit();

String message =
String.format(
"Possible unit mismatch: expected %s but was %s. Before accepting this change, make "
+ "sure that there is a true unit mismatch and not just an identifier whose name "
+ "contains the wrong unit. (If there is, correct that instead!)",
formalUnit.toString().toLowerCase(Locale.ROOT),
actualUnit.toString().toLowerCase(Locale.ROOT));
if ((actualUnit == MICROSECONDS || actualUnit == MILLISECONDS)
&& (formalUnit == MICROSECONDS || formalUnit == MILLISECONDS)) {
targetUnit.toString().toLowerCase(Locale.ROOT),
providedUnit.toString().toLowerCase(Locale.ROOT));
if ((providedUnit == MICROSECONDS || providedUnit == MILLISECONDS)
&& (targetUnit == MICROSECONDS || targetUnit == MILLISECONDS)) {
// TODO(cpovirk): Display this only if the code contained one of the ambiguous terms.
message +=
" WARNING: This checker considers \"ms\" and \"msec\" to always refer to *milli*seconds. "
Expand All @@ -277,7 +280,7 @@ private boolean check(String formalName, ExpressionTree actualTree, VisitorState
+ "before accepting this fix. If it instead means microseconds, consider renaming to "
+ "\"us\" or \"usec\" (or just \"micros\").";
// TODO(cpovirk): More ambitiously, suggest an edit to rename the identifier to "micros," etc.
} else if (formalUnit == SECONDS && (actualUnit != HOURS && actualUnit != DAYS)) {
} else if (targetUnit == SECONDS && (providedUnit != HOURS && providedUnit != DAYS)) {
message +=
" WARNING: The suggested replacement truncates fractional seconds, so a value "
+ "like 999ms becomes 0.";
Expand All @@ -291,19 +294,32 @@ 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.Builder fix = SuggestedFix.builder();
// TODO(cpovirk): This can conflict with constants with names like "SECONDS."
fix.addStaticImport(TimeUnit.class.getName() + "." + actualUnit);
// TODO(cpovirk): This won't work for `double` and won't work if the output needs to be `int`.
fix.prefixWith(
actualTree, String.format("%s.%s(", actualUnit, TIME_UNIT_TO_UNIT_METHODS.get(formalUnit)));
fix.postfixWith(actualTree, ")");
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();
}
/*
* 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
* utility methods in the case of micros). Maybe we should suggest a number of possible fixes?
*/
state.reportMatch(buildDescription(actualTree).setMessage(message).addFix(fix.build()).build());
state.reportMatch(buildDescription(actualTree).setMessage(message).addFix(fix).build());
/*
* TODO(cpovirk): Supply a different fix in the matchTimeUnitToUnit case (or the similar case in
* which someone is calling, say, toMillis() but should be calling toDays(). The current fix
Expand Down Expand Up @@ -374,6 +390,104 @@ private boolean check(String formalName, ExpressionTree actualTree, VisitorState
isSameType("java.lang.Long"),
isSameType("java.lang.Double"));

private @Nullable TreeAndTimeUnit unitSuggestedByTree(ExpressionTree tree) {
if (improvements && tree.getKind().equals(Kind.MULTIPLY)) {
var lhs = ((BinaryTree) tree).getLeftOperand();
var rhs = ((BinaryTree) tree).getRightOperand();
var lhsConversion = conversionFactor(lhs);
var rhsConversion = conversionFactor(rhs);
if (lhsConversion != null) {
return unitSuggestedWithConversion(lhsConversion, rhs);
}
if (rhsConversion != null) {
return unitSuggestedWithConversion(rhsConversion, lhs);
}
}
if (improvements && tree.getKind().equals(Kind.DIVIDE)) {
var lhs = ((BinaryTree) tree).getLeftOperand();
var rhs = ((BinaryTree) tree).getRightOperand();
var rhsConversion = conversionFactor(rhs);
if (rhsConversion != null) {
return unitSuggestedWithReciprocalConversion(rhsConversion, lhs);
}
}
String name = extractArgumentName(tree);
if (name == null) {
/*
* TODO(cpovirk): Look for other assignments to a variable in the method to guess its type.
* (Maybe even guess the type returned by a method by looking at other calls in the file?) Of
* course, that may be slow.
*/
// TODO(cpovirk): ...even if they include casts!
return null;
}
var unit = unitSuggestedByName(name);
return unit == null ? null : TreeAndTimeUnit.of(tree, unit, unit);
}

/**
* The result of inspecting a tree. Given {@code getFooSeconds() * 1000}, {@link
* TreeAndTimeUnit#innermostTree()} refers to {@code getFooSeconds()}, {@link
* TreeAndTimeUnit#outermostUnit()} is MILLISECONDS, and {@link TreeAndTimeUnit#innermostUnit()}
* is SECONDS.
*/
@AutoValue
abstract static class TreeAndTimeUnit {
public static TreeAndTimeUnit of(
ExpressionTree tree, TimeUnit timeUnit, TimeUnit underlyingUnit) {
return new AutoValue_TimeUnitMismatch_TreeAndTimeUnit(tree, timeUnit, underlyingUnit);
}

/** The innermost tree expressing a unit, ignoring any conversions around it. */
abstract ExpressionTree innermostTree();

/** The effective unit of the expression we started from. */
abstract TimeUnit outermostUnit();

/** The underlying unit of {@link #innermostTree()}. */
abstract TimeUnit innermostUnit();
}

private @Nullable TreeAndTimeUnit unitSuggestedWithConversion(
long conversionFactor, ExpressionTree tree) {
TreeAndTimeUnit underlying = unitSuggestedByTree(tree);
if (underlying == null) {
return null;
}
return allOf(TimeUnit.class).stream()
.filter(unit -> unit.convert(1, underlying.outermostUnit()) == conversionFactor)
.findFirst()
.map(u -> TreeAndTimeUnit.of(underlying.innermostTree(), u, underlying.innermostUnit()))
.orElse(null);
}

private @Nullable TreeAndTimeUnit unitSuggestedWithReciprocalConversion(
long conversionFactor, ExpressionTree tree) {
TreeAndTimeUnit underlying = unitSuggestedByTree(tree);
if (underlying == null) {
return null;
}
return allOf(TimeUnit.class).stream()
.filter(unit -> underlying.outermostUnit().convert(1, unit) == conversionFactor)
.findFirst()
.map(u -> TreeAndTimeUnit.of(underlying.innermostTree(), u, underlying.innermostUnit()))
.orElse(null);
}

private static @Nullable Long conversionFactor(ExpressionTree tree) {
var constValue = constValue(tree);
if (constValue instanceof Long) {
// Don't count 0 to be a valid conversion factor, because it _does_ show up as a conversion
// factor if you're doing integer division (i.e. 1 millisecond = 0 seconds, so the conversion
// factor naively looks like 0).
return (Long) constValue == 0L ? null : (Long) constValue;
}
if (constValue instanceof Integer) {
return (Integer) constValue == 0 ? null : ((Integer) constValue).longValue();
}
return null;
}

@VisibleForTesting
static @Nullable TimeUnit unitSuggestedByName(String name) {
// Tuple types, especially Pair, trip us up. Skip APIs that might be from them.
Expand Down
99 changes: 99 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/time/TimeUnitMismatchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -36,6 +37,9 @@ public class TimeUnitMismatchTest {
private final CompilationTestHelper compilationHelper =
CompilationTestHelper.newInstance(TimeUnitMismatch.class, getClass());

private final BugCheckerRefactoringTestHelper refactoringHelper =
BugCheckerRefactoringTestHelper.newInstance(TimeUnitMismatch.class, getClass());

@Test
public void testPositiveCase() {
compilationHelper
Expand Down Expand Up @@ -218,6 +222,101 @@ void optionalGet() {
.doTest();
}

@Test
public void mismatchedTypeAfterManualConversion() {
compilationHelper
.addSourceLines(
"Test.java",
"""
class Test {
// BUG: Diagnostic contains: MICROSECONDS.toMillis(getMicros())
long fooMillis = getMicros() * 1000;
// BUG: Diagnostic contains: MICROSECONDS.toMillis(getMicros())
long barMillis = 1000 * getMicros();
// BUG: Diagnostic contains:
long fooNanos = getMicros() / 1000;
// BUG: Diagnostic contains: SECONDS.toNanos(getSeconds())
long barNanos = getSeconds() * 1000 * 1000;

long getMicros() {
return 1;
}

long getSeconds() {
return 1;
}

void setMillis(long x) {}

void test(int timeMicros) {
// BUG: Diagnostic contains:
setMillis(timeMicros * 1000);
}
}
""")
.doTest();
}

@Test
public void noopConversion_isRemoved() {
refactoringHelper
.addInputLines(
"Test.java",
"""
class Test {
long fooMicros = getMicros() * 1000;

long getMicros() {
return 1;
}
}
""")
.addOutputLines(
"Test.java",
"""
class Test {
long fooMicros = getMicros();

long getMicros() {
return 1;
}
}
""")
.doTest();
}

@Test
public void zeroMultiplier_noComplaint() {
compilationHelper
.addSourceLines(
"Test.java",
"""
class Test {
static int MILLIS_PER_MINUTE = 42;
long fooMicros = 0 * MILLIS_PER_MINUTE;
}
""")
.doTest();
}

@Test
public void matchedTypeAfterManualConversion() {
compilationHelper
.addSourceLines(
"Test.java",
"""
class Test {
long fooNanos = getMicros() * 1000;
long fooMillis = getMicros() / 1000;

long getMicros() {
return 1;
}
}
""")
.doTest();
}

@Test
public void testUnitSuggestedByName() {
assertSeconds("sleepSec", "deadlineSeconds", "secondsTimeout", "msToS");
Expand Down
Loading