From d8a721b26c34433a9f7b6d75c53855e164aca81c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Feb 2021 17:20:50 +0100 Subject: [PATCH] PRP-10968 Suggestions --- .../bugpatterns/TimeZoneUsageCheck.java | 19 +-- .../bugpatterns/TimeZoneUsageCheckTest.java | 118 ++++++++---------- 2 files changed, 65 insertions(+), 72 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java index 30e6de37822..a71488bcdb2 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java @@ -25,14 +25,15 @@ /** A {@link BugChecker} which flags illegal time-zone related operations. */ @AutoService(BugChecker.class) @BugPattern( - name = "TimeZoneUsageCheck", - summary = "Avoid illegal operations on assorted time related objects", + name = "TimeZoneUsage", + summary = + "Derive the current time from a `Clock` Spring bean, and don't rely on a `Clock`'s time zone", linkType = LinkType.NONE, severity = SeverityLevel.WARNING, tags = StandardTags.FRAGILE_CODE) public final class TimeZoneUsageCheck extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher ILLEGAL_CLOCK_USAGES = + private static final Matcher IS_BANNED_TIME_METHOD = anyOf( instanceMethod().onDescendantOf(Clock.class.getName()).namedAnyOf("getZone", "withZone"), staticMethod() @@ -43,24 +44,24 @@ public final class TimeZoneUsageCheck extends BugChecker implements MethodInvoca "systemUTC", "tickMillis", "tickMinutes", - "tickSeconds")); - private static final Matcher NOW_USAGES = - anyOf( + "tickSeconds"), staticMethod() .onClassAny( Instant.class.getName(), LocalDate.class.getName(), LocalDateTime.class.getName(), LocalTime.class.getName()) - .namedAnyOf("now")); + .named("now")); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!ILLEGAL_CLOCK_USAGES.matches(tree, state) && !NOW_USAGES.matches(tree, state)) { + if (!IS_BANNED_TIME_METHOD.matches(tree, state)) { return Description.NO_MATCH; } + return buildDescription(tree) - .setMessage("This operation is not recommended, please refactor it") + .setMessage( + "Derive the current time from an existing `Clock` Spring bean, and don't rely on a `Clock`'s time zone") .build(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java index 2fa92ff9ff9..c798ac2ac7a 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java @@ -1,14 +1,19 @@ package tech.picnic.errorprone.bugpatterns; +import com.google.common.base.Predicates; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; public final class TimeZoneUsageCheckTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(TimeZoneUsageCheck.class, getClass()); + CompilationTestHelper.newInstance(TimeZoneUsageCheck.class, getClass()) + .expectErrorMessage( + "X", + Predicates.containsPattern( + "Derive the current time from an existing `Clock` Spring bean, and don't rely on a `Clock`'s time zone")); @Test - public void testNoIdentificationFoundCases() { + public void testIdentification() { compilationHelper .addSourceLines( "A.java", @@ -17,73 +22,60 @@ public void testNoIdentificationFoundCases() { "import java.time.Clock;", "import java.time.Duration;", "import java.time.Instant;", - "", - "class A {", - "void m() {", - " Clock clock = Clock.fixed(Instant.EPOCH, UTC);", - "clock.instant();", - "clock.millis();", - "Clock.offset(clock, Duration.ZERO);", - "Clock.tick(clock, Duration.ZERO);", - " }", - "}") - .doTest(); - } - - @Test - public void testIdentifyCases() { - compilationHelper - .addSourceLines( - "A.java", - "import static java.time.ZoneOffset.UTC;", - "", - "import java.time.Clock;", - "import java.time.Instant;", "import java.time.LocalDate;", "import java.time.LocalDateTime;", "import java.time.LocalTime;", "", "class A {", - "void m() {", - " // BUG: Diagnostic contains:", - "Clock.systemUTC();", - " // BUG: Diagnostic contains:", - "Clock.systemDefaultZone();", - " // BUG: Diagnostic contains:", - "Clock.system(UTC);", - " // BUG: Diagnostic contains:", - "Clock.tickMillis(UTC);", - " // BUG: Diagnostic contains:", - "Clock.tickMinutes(UTC);", - " // BUG: Diagnostic contains:", - "Clock.tickSeconds(UTC);", + " void m() {", " Clock clock = Clock.fixed(Instant.EPOCH, UTC);", - " // BUG: Diagnostic contains:", - "clock.getZone();", - " // BUG: Diagnostic contains:", - "clock.withZone(UTC);", - " // BUG: Diagnostic contains:", - "Instant.now();", - " // BUG: Diagnostic contains:", - "Instant.now(clock);", - " // BUG: Diagnostic contains:", - "LocalDate.now();", - " // BUG: Diagnostic contains:", - "LocalDate.now(clock);", - " // BUG: Diagnostic contains:", - "LocalDate.now(UTC);", - " // BUG: Diagnostic contains:", - "LocalDateTime.now();", - " // BUG: Diagnostic contains:", - "LocalDateTime.now(clock);", - " // BUG: Diagnostic contains:", - "LocalDateTime.now(UTC);", - " // BUG: Diagnostic contains:", - "LocalTime.now();", - " // BUG: Diagnostic contains:", - "LocalTime.now(clock);", - " // BUG: Diagnostic contains:", - "LocalTime.now(UTC);", + " clock.instant();", + " clock.millis();", + " Clock.offset(clock, Duration.ZERO);", + " Clock.tick(clock, Duration.ZERO);", + "", + " // BUG: Diagnostic matches: X", + " Clock.systemUTC();", + " // BUG: Diagnostic matches: X", + " Clock.systemDefaultZone();", + " // BUG: Diagnostic matches: X", + " Clock.system(UTC);", + " // BUG: Diagnostic matches: X", + " Clock.tickMillis(UTC);", + " // BUG: Diagnostic matches: X", + " Clock.tickMinutes(UTC);", + " // BUG: Diagnostic matches: X", + " Clock.tickSeconds(UTC);", + " // BUG: Diagnostic matches: X", + " clock.getZone();", + " // BUG: Diagnostic matches: X", + " clock.withZone(UTC);", + "", + " // BUG: Diagnostic matches: X", + " Instant.now();", + " // BUG: Diagnostic matches: X", + " Instant.now(clock);", + "", + " // BUG: Diagnostic matches: X", + " LocalDate.now();", + " // BUG: Diagnostic matches: X", + " LocalDate.now(clock);", + " // BUG: Diagnostic matches: X", + " LocalDate.now(UTC);", + "", + " // BUG: Diagnostic matches: X", + " LocalDateTime.now();", + " // BUG: Diagnostic matches: X", + " LocalDateTime.now(clock);", + " // BUG: Diagnostic matches: X", + " LocalDateTime.now(UTC);", + "", + " // BUG: Diagnostic matches: X", + " LocalTime.now();", + " // BUG: Diagnostic matches: X", + " LocalTime.now(clock);", + " // BUG: Diagnostic matches: X", + " LocalTime.now(UTC);", " }", "}") .doTest();