diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationGetTemporalUnit.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationGetTemporalUnit.java index ea53a7f4878..20fb2d84941 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationGetTemporalUnit.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationGetTemporalUnit.java @@ -79,7 +79,9 @@ public final class DurationGetTemporalUnit extends BugChecker @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (MATCHER.matches(tree, state)) { - Optional invalidUnit = getInvalidChronoUnit(tree, INVALID_TEMPORAL_UNITS); + Optional invalidUnit = + getInvalidChronoUnit( + Iterables.getOnlyElement(tree.getArguments()), INVALID_TEMPORAL_UNITS); if (invalidUnit.isPresent()) { if (SUGGESTIONS.containsKey(invalidUnit.get())) { SuggestedFix.Builder builder = SuggestedFix.builder(); @@ -95,10 +97,10 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - // used by PeriodGetTemporalUnit + // used by PeriodGetTemporalUnit and DurationOfLongTemporalUnit static Optional getInvalidChronoUnit( - MethodInvocationTree tree, EnumSet invalidUnits) { - Optional constant = getEnumName(Iterables.getOnlyElement(tree.getArguments())); + ExpressionTree tree, Iterable invalidUnits) { + Optional constant = getEnumName(tree); if (constant.isPresent()) { for (ChronoUnit invalidTemporalUnit : invalidUnits) { if (constant.get().equals(invalidTemporalUnit.name())) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnit.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnit.java new file mode 100644 index 00000000000..70672f51507 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnit.java @@ -0,0 +1,77 @@ +/* + * 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.method.MethodMatchers.staticMethod; + +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 Duration.of(long, TemporalUnit)} where the {@link + * java.time.temporal.TemporalUnit} has an estimated duration (which is guaranteed to throw an + * {@code DateTimeException}). + */ +@BugPattern( + name = "DurationOfLongTemporalUnit", + summary = "Duration.of(long, TemporalUnit) only works for TemporalUnits with exact durations.", + explanation = + "Duration.of(long, TemporalUnit) only works for TemporalUnits with exact durations. " + + "E.g., Duration.of(1, ChronoUnit.YEAR) is guaranteed to throw a DateTimeException.", + severity = ERROR, + providesFix = NO_FIX) +public final class DurationOfLongTemporalUnit extends BugChecker + implements MethodInvocationTreeMatcher { + + private static final Matcher DURATION_OF_LONG_TEMPORAL_UNIT = + allOf( + staticMethod() + .onClass("java.time.Duration") + .named("of") + .withParameters("long", "java.time.temporal.TemporalUnit"), + Matchers.not(Matchers.packageStartsWith("java."))); + + private static final ImmutableSet INVALID_TEMPORAL_UNITS = + Arrays.stream(ChronoUnit.values()) + .filter(c -> c.isDurationEstimated()) + .collect(toImmutableEnumSet()); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (DURATION_OF_LONG_TEMPORAL_UNIT.matches(tree, state)) { + if (getInvalidChronoUnit(tree.getArguments().get(1), INVALID_TEMPORAL_UNITS).isPresent()) { + // TODO(kak): do we want to include the name of the invalid ChronoUnit in the message? + return describeMatch(tree); + } + } + return Description.NO_MATCH; + } +} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/time/PeriodGetTemporalUnit.java b/core/src/main/java/com/google/errorprone/bugpatterns/time/PeriodGetTemporalUnit.java index c13ce96a3cb..e807ee5f040 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/time/PeriodGetTemporalUnit.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/time/PeriodGetTemporalUnit.java @@ -19,6 +19,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.bugpatterns.time.DurationGetTemporalUnit.getInvalidChronoUnit; +import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -57,7 +58,9 @@ public final class PeriodGetTemporalUnit extends BugChecker implements MethodInv @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { return MATCHER.matches(tree, state) - && getInvalidChronoUnit(tree, INVALID_TEMPORAL_UNITS).isPresent() + && getInvalidChronoUnit( + Iterables.getOnlyElement(tree.getArguments()), INVALID_TEMPORAL_UNITS) + .isPresent() ? describeMatch(tree) : Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 9618fc638a7..acfb58fdbad 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -391,6 +391,7 @@ 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.DurationToLongTimeUnit; import com.google.errorprone.bugpatterns.time.JavaDurationGetSecondsGetNano; import com.google.errorprone.bugpatterns.time.JavaDurationWithNanos; @@ -491,6 +492,7 @@ public static ScannerSupplier errorChecks() { DuplicateMapKeys.class, DurationFrom.class, DurationGetTemporalUnit.class, + DurationOfLongTemporalUnit.class, DurationToLongTimeUnit.class, EqualsHashCode.class, EqualsNaN.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnitTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnitTest.java new file mode 100644 index 00000000000..c5d9f554196 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/time/DurationOfLongTemporalUnitTest.java @@ -0,0 +1,142 @@ +/* + * 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link DurationOfLongTemporalUnit}. */ +@RunWith(JUnit4.class) +public class DurationOfLongTemporalUnitTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(DurationOfLongTemporalUnit.class, getClass()); + + @Test + public void durationOf() { + helper + .addSourceLines( + "TestClass.java", + "import java.time.Duration;", + "import java.time.temporal.ChronoUnit;", + "public class TestClass {", + " private static final Duration D1 = Duration.of(1, ChronoUnit.SECONDS);", + " private static final Duration D2 = Duration.of(1, ChronoUnit.NANOS);", + " // BUG: Diagnostic contains: DurationOfLongTemporalUnit", + " private static final Duration D3 = Duration.of(1, ChronoUnit.YEARS);", + "}") + .doTest(); + } + + @Test + public void durationOfStaticImport() { + helper + .addSourceLines( + "TestClass.java", + "import static java.time.temporal.ChronoUnit.NANOS;", + "import static java.time.temporal.ChronoUnit.SECONDS;", + "import static java.time.temporal.ChronoUnit.YEARS;", + "import java.time.Duration;", + "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", + " private static final Duration D3 = Duration.of(1, YEARS);", + "}") + .doTest(); + } + + @Test + public void durationOfWithRandomTemporalUnit() { + helper + .addSourceLines( + "TestClass.java", + "import static java.time.temporal.ChronoUnit.SECONDS;", + "import static java.time.temporal.ChronoUnit.YEARS;", + "import java.time.Duration;", + "import java.time.temporal.TemporalUnit;", + "import java.util.Random;", + "public class TestClass {", + " private static final TemporalUnit random = ", + " new Random().nextBoolean() ? YEARS : SECONDS;", + // Since we don't know at compile time what 'random' is, we can't flag this + " private static final Duration D1 = Duration.of(1, random);", + "}") + .doTest(); + } + + @Test + public void durationOfWithAliasedTemporalUnit() { + helper + .addSourceLines( + "TestClass.java", + "import static java.time.temporal.ChronoUnit.YEARS;", + "import java.time.Duration;", + "import java.time.temporal.Temporal;", + "import java.time.temporal.TemporalUnit;", + "public class TestClass {", + " private static final TemporalUnit SECONDS = YEARS;", + // This really should be flagged, but isn't :( + " private static final Duration D1 = Duration.of(1, SECONDS);", + "}") + .doTest(); + } + + @Test + public void durationOfWithCustomTemporalUnit() { + helper + .addSourceLines( + "TestClass.java", + "import java.time.Duration;", + "import java.time.temporal.Temporal;", + "import java.time.temporal.TemporalUnit;", + "public class TestClass {", + " private static class BadTemporalUnit implements TemporalUnit {", + " @Override", + " public long between(Temporal t1, Temporal t2) {", + " throw new AssertionError();", + " }", + " @Override", + " public R addTo(R temporal, long amount) {", + " throw new AssertionError();", + " }", + " @Override", + " public boolean isTimeBased() {", + " throw new AssertionError();", + " }", + " @Override", + " public boolean isDateBased() {", + " throw new AssertionError();", + " }", + " @Override", + " public boolean isDurationEstimated() {", + " throw new AssertionError();", + " }", + " @Override", + " public Duration getDuration() {", + " throw new AssertionError();", + " }", + " }", + " private static final TemporalUnit MINUTES = new BadTemporalUnit();", + " private static final TemporalUnit SECONDS = new BadTemporalUnit();", + // This really should be flagged, but isn't :( + " private static final Duration D1 = Duration.of(1, SECONDS);", + " private static final Duration D2 = Duration.of(1, MINUTES);", + "}") + .doTest(); + } +}