Skip to content

Commit

Permalink
Rename DurationOfLongTemporalUnit to DurationTemporalUnit and cover d…
Browse files Browse the repository at this point in the history
…uration.plus(long, TemporalUnit) and duration.minus(long, TemporalUnit).

Also ban calls to Instant APIs where the TemporalUnit is not supported.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=250547375
  • Loading branch information
kluever authored and ronshapiro committed Jun 12, 2019
1 parent 254f3f2 commit beef3d8
Show file tree
Hide file tree
Showing 5 changed files with 370 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit.getInvalidChronoUnit;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;

import com.google.common.collect.ImmutableSet;
Expand All @@ -36,28 +38,36 @@
import java.util.Arrays;

/**
* Bans calls to {@code Duration.of(long, TemporalUnit)} where the {@link
* java.time.temporal.TemporalUnit} is not {@link ChronoUnit#DAYS} or it has an estimated duration
* (which is guaranteed to throw an {@code DateTimeException}).
* Bans calls to {@code Duration} APIs where the {@link java.time.temporal.TemporalUnit} is not
* {@link ChronoUnit#DAYS} or it has an estimated duration (which is guaranteed to throw an {@code
* DateTimeException}).
*/
@BugPattern(
name = "DurationOfLongTemporalUnit",
summary = "Duration.of(long, TemporalUnit) only works for DAYS or exact durations.",
name = "DurationTemporalUnit",
summary = "Duration APIs only work for DAYS or exact durations.",
explanation =
"Duration.of(long, TemporalUnit) only works for TemporalUnits with exact durations or"
"Duration APIs only work for TemporalUnits with exact durations or"
+ " ChronoUnit.DAYS. E.g., Duration.of(1, ChronoUnit.YEARS) is guaranteed to throw a"
+ " DateTimeException.",
severity = ERROR,
providesFix = NO_FIX)
public final class DurationOfLongTemporalUnit extends BugChecker
implements MethodInvocationTreeMatcher {
public final class DurationTemporalUnit extends BugChecker implements MethodInvocationTreeMatcher {

private static final String DURATION = "java.time.Duration";
private static final String TEMPORAL_UNIT = "java.time.temporal.TemporalUnit";

private static final Matcher<ExpressionTree> DURATION_OF_LONG_TEMPORAL_UNIT =
allOf(
staticMethod()
.onClass("java.time.Duration")
.named("of")
.withParameters("long", "java.time.temporal.TemporalUnit"),
anyOf(
staticMethod().onClass(DURATION).named("of").withParameters("long", TEMPORAL_UNIT),
instanceMethod()
.onExactClass(DURATION)
.named("minus")
.withParameters("long", TEMPORAL_UNIT),
instanceMethod()
.onExactClass(DURATION)
.named("plus")
.withParameters("long", TEMPORAL_UNIT)),
Matchers.not(Matchers.packageStartsWith("java.")));

private static final ImmutableSet<ChronoUnit> INVALID_TEMPORAL_UNITS =
Expand All @@ -69,15 +79,10 @@ public final class DurationOfLongTemporalUnit extends BugChecker
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (DURATION_OF_LONG_TEMPORAL_UNIT.matches(tree, state)) {
if (isSecondParamDaysOrExactDuration(tree)) {
if (getInvalidChronoUnit(tree.getArguments().get(1), INVALID_TEMPORAL_UNITS).isPresent()) {
return describeMatch(tree);
}
}
return Description.NO_MATCH;
}

// will be used by other checks (e.g., Instant.plus(long, TemporalUnit))
static boolean isSecondParamDaysOrExactDuration(MethodInvocationTree tree) {
return getInvalidChronoUnit(tree.getArguments().get(1), INVALID_TEMPORAL_UNITS).isPresent();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright 2019 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.errorprone.bugpatterns.time;

import static com.google.common.collect.Sets.toImmutableEnumSet;
import static com.google.errorprone.BugPattern.ProvidesFix.NO_FIX;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit.getInvalidChronoUnit;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.time.temporal.ChronoUnit;
import java.util.Arrays;

/**
* Bans calls to {@code Instant} APIs where the {@link java.time.temporal.TemporalUnit} is not one
* of: {@code NANOS}, {@code MICROS}, {@code MILLIS}, {@code SECONDS}, {@code MINUTES}, {@code
* HOURS}, {@code HALF_DAYS}, or {@code DAYS}.
*/
@BugPattern(
name = "InstantTemporalUnit",
summary =
"Instant APIs only work for NANOS, MICROS, MILLIS, SECONDS, MINUTES, HOURS, HALF_DAYS and"
+ " DAYS.",
severity = ERROR,
providesFix = NO_FIX)
public final class InstantTemporalUnit extends BugChecker implements MethodInvocationTreeMatcher {

private static final String INSTANT = "java.time.Instant";
private static final String TEMPORAL_UNIT = "java.time.temporal.TemporalUnit";

private static final Matcher<ExpressionTree> INSTANT_OF_LONG_TEMPORAL_UNIT =
allOf(
anyOf(
instanceMethod()
.onExactClass(INSTANT)
.named("minus")
.withParameters("long", TEMPORAL_UNIT),
instanceMethod()
.onExactClass(INSTANT)
.named("plus")
.withParameters("long", TEMPORAL_UNIT),
instanceMethod()
.onExactClass(INSTANT)
.named("until")
.withParameters("java.time.temporal.Temporal", TEMPORAL_UNIT)),
Matchers.not(Matchers.packageStartsWith("java.")));

// This definition comes from Instant.isSupported(TemporalUnit)
static final ImmutableSet<ChronoUnit> INVALID_TEMPORAL_UNITS =
Arrays.stream(ChronoUnit.values())
.filter(c -> !c.isTimeBased())
.filter(c -> !c.equals(ChronoUnit.DAYS)) // DAYS is explicitly allowed
.collect(toImmutableEnumSet());

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (INSTANT_OF_LONG_TEMPORAL_UNIT.matches(tree, state)) {
if (getInvalidChronoUnit(tree.getArguments().get(1), INVALID_TEMPORAL_UNITS).isPresent()) {
return describeMatch(tree);
}
}
return Description.NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,9 @@
import com.google.errorprone.bugpatterns.threadsafety.UnlockMethodChecker;
import com.google.errorprone.bugpatterns.time.DurationFrom;
import com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit;
import com.google.errorprone.bugpatterns.time.DurationOfLongTemporalUnit;
import com.google.errorprone.bugpatterns.time.DurationTemporalUnit;
import com.google.errorprone.bugpatterns.time.DurationToLongTimeUnit;
import com.google.errorprone.bugpatterns.time.InstantTemporalUnit;
import com.google.errorprone.bugpatterns.time.JavaDurationGetSecondsGetNano;
import com.google.errorprone.bugpatterns.time.JavaDurationWithNanos;
import com.google.errorprone.bugpatterns.time.JavaDurationWithSeconds;
Expand Down Expand Up @@ -492,7 +493,7 @@ public static ScannerSupplier errorChecks() {
DuplicateMapKeys.class,
DurationFrom.class,
DurationGetTemporalUnit.class,
DurationOfLongTemporalUnit.class,
DurationTemporalUnit.class,
DurationToLongTimeUnit.class,
EqualsHashCode.class,
EqualsNaN.class,
Expand All @@ -518,6 +519,7 @@ public static ScannerSupplier errorChecks() {
InfiniteRecursion.class,
InjectOnFinalField.class,
InjectOnMemberAndConstructor.class,
InstantTemporalUnit.class,
InvalidPatternSyntax.class,
InvalidTimeZoneID.class,
InvalidZoneId.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Tests for {@link DurationOfLongTemporalUnit}. */
/** Tests for {@link DurationTemporalUnit}. */
@RunWith(JUnit4.class)
public class DurationOfLongTemporalUnitTest {
public class DurationTemporalUnitTest {
private final CompilationTestHelper helper =
CompilationTestHelper.newInstance(DurationOfLongTemporalUnit.class, getClass());
CompilationTestHelper.newInstance(DurationTemporalUnit.class, getClass());

@Test
public void durationOf_good() {
Expand Down Expand Up @@ -54,26 +54,122 @@ public void durationOf_bad() {
"import java.time.Duration;",
"import java.time.temporal.ChronoUnit;",
"public class TestClass {",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D0 = Duration.of(1, ChronoUnit.CENTURIES);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D1 = Duration.of(1, ChronoUnit.DECADES);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D2 = Duration.of(1, ChronoUnit.ERAS);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D3 = Duration.of(1, ChronoUnit.FOREVER);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D4 = Duration.of(1, ChronoUnit.MILLENNIA);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D5 = Duration.of(1, ChronoUnit.MONTHS);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D6 = Duration.of(1, ChronoUnit.WEEKS);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D7 = Duration.of(1, ChronoUnit.YEARS);",
"}")
.doTest();
}

@Test
public void durationPlus_good() {
helper
.addSourceLines(
"TestClass.java",
"import java.time.Duration;",
"import java.time.temporal.ChronoUnit;",
"public class TestClass {",
" private static final Duration D0 = Duration.ZERO.plus(1, ChronoUnit.DAYS);",
" private static final Duration D1 = Duration.ZERO.plus(1, ChronoUnit.HALF_DAYS);",
" private static final Duration D2 = Duration.ZERO.plus(1, ChronoUnit.HOURS);",
" private static final Duration D3 = Duration.ZERO.plus(1, ChronoUnit.MICROS);",
" private static final Duration D4 = Duration.ZERO.plus(1, ChronoUnit.MILLIS);",
" private static final Duration D5 = Duration.ZERO.plus(1, ChronoUnit.MINUTES);",
" private static final Duration D6 = Duration.ZERO.plus(1, ChronoUnit.NANOS);",
" private static final Duration D7 = Duration.ZERO.plus(1, ChronoUnit.SECONDS);",
"}")
.doTest();
}

@Test
public void durationPlus_bad() {
helper
.addSourceLines(
"TestClass.java",
"import java.time.Duration;",
"import java.time.temporal.ChronoUnit;",
"public class TestClass {",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D0 = Duration.ZERO.plus(1, ChronoUnit.CENTURIES);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D1 = Duration.ZERO.plus(1, ChronoUnit.DECADES);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D2 = Duration.ZERO.plus(1, ChronoUnit.ERAS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D3 = Duration.ZERO.plus(1, ChronoUnit.FOREVER);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D4 = Duration.ZERO.plus(1, ChronoUnit.MILLENNIA);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D5 = Duration.ZERO.plus(1, ChronoUnit.MONTHS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D6 = Duration.ZERO.plus(1, ChronoUnit.WEEKS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D7 = Duration.ZERO.plus(1, ChronoUnit.YEARS);",
"}")
.doTest();
}

@Test
public void durationMinus_good() {
helper
.addSourceLines(
"TestClass.java",
"import java.time.Duration;",
"import java.time.temporal.ChronoUnit;",
"public class TestClass {",
" private static final Duration D0 = Duration.ZERO.minus(1, ChronoUnit.DAYS);",
" private static final Duration D1 = Duration.ZERO.minus(1, ChronoUnit.HALF_DAYS);",
" private static final Duration D2 = Duration.ZERO.minus(1, ChronoUnit.HOURS);",
" private static final Duration D3 = Duration.ZERO.minus(1, ChronoUnit.MICROS);",
" private static final Duration D4 = Duration.ZERO.minus(1, ChronoUnit.MILLIS);",
" private static final Duration D5 = Duration.ZERO.minus(1, ChronoUnit.MINUTES);",
" private static final Duration D6 = Duration.ZERO.minus(1, ChronoUnit.NANOS);",
" private static final Duration D7 = Duration.ZERO.minus(1, ChronoUnit.SECONDS);",
"}")
.doTest();
}

@Test
public void durationMinus_bad() {
helper
.addSourceLines(
"TestClass.java",
"import java.time.Duration;",
"import java.time.temporal.ChronoUnit;",
"public class TestClass {",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D0 = Duration.ZERO.minus(1, ChronoUnit.CENTURIES);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D1 = Duration.ZERO.minus(1, ChronoUnit.DECADES);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D2 = Duration.ZERO.minus(1, ChronoUnit.ERAS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D3 = Duration.ZERO.minus(1, ChronoUnit.FOREVER);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D4 = Duration.ZERO.minus(1, ChronoUnit.MILLENNIA);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D5 = Duration.ZERO.minus(1, ChronoUnit.MONTHS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D6 = Duration.ZERO.minus(1, ChronoUnit.WEEKS);",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D7 = Duration.ZERO.minus(1, ChronoUnit.YEARS);",
"}")
.doTest();
}

@Test
public void durationOfStaticImport() {
helper
Expand All @@ -86,7 +182,7 @@ public void durationOfStaticImport() {
"public class TestClass {",
" private static final Duration D1 = Duration.of(1, SECONDS);",
" private static final Duration D2 = Duration.of(1, NANOS);",
" // BUG: Diagnostic contains: DurationOfLongTemporalUnit",
" // BUG: Diagnostic contains: DurationTemporalUnit",
" private static final Duration D3 = Duration.of(1, YEARS);",
"}")
.doTest();
Expand Down
Loading

0 comments on commit beef3d8

Please sign in to comment.