Rewrite foo(javaDuration.getSeconds(), SECONDS) -> foo(javaDuration).

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=259016019
google · Jul 24, 2019 · 9aa023d · 9aa023d
1 parent 73c063a
commit 9aa023d
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 6 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/PreferDurationOverload.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/PreferDurationOverload.java
@@ -19,6 +19,7 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.constructor;
+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.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.isSameType;
@@ -111,6 +112,15 @@ public final class PreferDurationOverload extends BugChecker
       anyOf(
           );
 
+  private static final Matcher<ExpressionTree> JAVA_DURATION_DECOMPOSITION_MATCHER =
+      anyOf(
+          instanceMethod().onExactClass(JAVA_DURATION).named("toNanos"),
+          instanceMethod().onExactClass(JAVA_DURATION).named("toMillis"),
+          instanceMethod().onExactClass(JAVA_DURATION).named("getSeconds"),
+          instanceMethod().onExactClass(JAVA_DURATION).named("toMinutes"),
+          instanceMethod().onExactClass(JAVA_DURATION).named("toHours"),
+          instanceMethod().onExactClass(JAVA_DURATION).named("toDays"));
+
   // TODO(kak): Add support for constructors that accept a <long, TimeUnit> or JodaTime Duration
 
   @Override
@@ -143,19 +153,34 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
             SuggestedFix.Builder fix = SuggestedFix.builder();
             String qualifiedDuration = SuggestedFixes.qualifyType(state, fix, JAVA_DURATION);
             String value = state.getSourceForNode(arguments.get(0));
-            String replacement;
-
-            // TODO(kak): Add support for:
-            //   foo(javaDuration.getSeconds(), SECONDS);
-            //   foo(javaDuration.toMillis(), MILLISECONDS);
+            String replacement = null;
+
+            // rewrite foo(javaDuration.getSeconds(), SECONDS) -> foo(javaDuration)
+            if (arguments.get(0) instanceof MethodInvocationTree) {
+              MethodInvocationTree maybeDurationDecomposition =
+                  (MethodInvocationTree) arguments.get(0);
+              if (JAVA_DURATION_DECOMPOSITION_MATCHER.matches(maybeDurationDecomposition, state)) {
+                if (ASTHelpers.isSameType(
+                    ASTHelpers.getReceiverType(maybeDurationDecomposition),
+                    state.getTypeFromString(JAVA_DURATION),
+                    state)) {
+                  replacement =
+                      state.getSourceForNode(ASTHelpers.getReceiver(maybeDurationDecomposition));
+                }
+              }
+            }
 
+            // handle microseconds separately, since there is no Duration factory for micros
             if (optionalTimeUnit.get() == MICROSECONDS) {
               String qualifiedChronoUnit =
                   SuggestedFixes.qualifyType(state, fix, "java.time.temporal.ChronoUnit");
               replacement =
                   String.format(
                       durationFactory, qualifiedDuration, value, qualifiedChronoUnit + ".MICROS");
-            } else {
+            }
+
+            // Otherwise, just use the normal replacement
+            if (replacement == null) {
               replacement = String.format(durationFactory, qualifiedDuration, value);
             }
 

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/time/PreferDurationOverloadTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/time/PreferDurationOverloadTest.java
@@ -59,6 +59,45 @@ public void callingLongTimeUnitMethodWithDurationOverload() {
         .doTest();
   }
 
+  @Test
+  public void callingLongTimeUnitMethodWithDurationOverload_durationDecompose() {
+    helper
+        .addSourceLines(
+            "TestClass.java",
+            "import com.google.common.cache.CacheBuilder;",
+            "import java.time.Duration;",
+            "import java.util.concurrent.TimeUnit;",
+            "public class TestClass {",
+            "  public CacheBuilder foo(CacheBuilder builder) {",
+            "    Duration duration = Duration.ofMillis(12345);",
+            "    // BUG: Diagnostic contains: builder.expireAfterAccess(duration);",
+            "    return builder.expireAfterAccess(duration.getSeconds(), TimeUnit.SECONDS);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void callingLongTimeUnitMethodWithDurationOverload_durationHashCode() {
+    // this is admittedly a _very_ weird case, but we should _not_ suggest re-writing to:
+    // builder.expireAfterAccess(duration)
+    helper
+        .addSourceLines(
+            "TestClass.java",
+            "import com.google.common.cache.CacheBuilder;",
+            "import java.time.Duration;",
+            "import java.util.concurrent.TimeUnit;",
+            "public class TestClass {",
+            "  public CacheBuilder foo(CacheBuilder builder) {",
+            "    Duration duration = Duration.ofMillis(12345);",
+            "    // BUG: Diagnostic contains: return"
+                + " builder.expireAfterAccess(Duration.ofSeconds(duration.hashCode()));",
+            "    return builder.expireAfterAccess(duration.hashCode(), TimeUnit.SECONDS);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void callingLongTimeUnitMethodWithDurationOverload_intParam() {
     helper