From 1e2eff29af4c829349cb8e0515a6d1ebf709977b Mon Sep 17 00:00:00 2001 From: Anna Dvorkin Date: Sat, 27 Jun 2020 00:59:24 +0200 Subject: [PATCH 1/5] PRP-10968 Introduce `TimeZoneUsageCheck` --- .../bugpatterns/TimeZoneUsageCheck.java | 66 ++++++++++++++ .../bugpatterns/TimeZoneUsageCheckTest.java | 91 +++++++++++++++++++ 2 files changed, 157 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java 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 new file mode 100644 index 0000000000..30e6de3782 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java @@ -0,0 +1,66 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.staticMethod; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; +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.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.time.Clock; +import java.time.Instant; +import java.time.LocalDate; +import java.time.LocalDateTime; +import java.time.LocalTime; + +/** 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", + 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 = + anyOf( + instanceMethod().onDescendantOf(Clock.class.getName()).namedAnyOf("getZone", "withZone"), + staticMethod() + .onClass(Clock.class.getName()) + .namedAnyOf( + "system", + "systemDefaultZone", + "systemUTC", + "tickMillis", + "tickMinutes", + "tickSeconds")); + private static final Matcher NOW_USAGES = + anyOf( + staticMethod() + .onClassAny( + Instant.class.getName(), + LocalDate.class.getName(), + LocalDateTime.class.getName(), + LocalTime.class.getName()) + .namedAnyOf("now")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!ILLEGAL_CLOCK_USAGES.matches(tree, state) && !NOW_USAGES.matches(tree, state)) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .setMessage("This operation is not recommended, please refactor it") + .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 new file mode 100644 index 0000000000..2fa92ff9ff --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheckTest.java @@ -0,0 +1,91 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class TimeZoneUsageCheckTest { + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(TimeZoneUsageCheck.class, getClass()); + + @Test + public void testNoIdentificationFoundCases() { + compilationHelper + .addSourceLines( + "A.java", + "import static java.time.ZoneOffset.UTC;", + "", + "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);", + " 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);", + " }", + "}") + .doTest(); + } +} From bf541e46284dcc04fa6843976c2406b3abe51d3b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Feb 2021 17:20:50 +0100 Subject: [PATCH 2/5] 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 30e6de3782..a71488bcdb 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 2fa92ff9ff..c798ac2ac7 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(); From 77c588ce752e0d769246453e2df46fbad84f5f1f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 15 Feb 2021 10:53:37 +0100 Subject: [PATCH 3/5] Remove setMessage from descriptionbuilder because it returns the summary by default --- .../picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java | 7 ++----- 1 file changed, 2 insertions(+), 5 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 a71488bcdb..4cf1eade77 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 @@ -27,7 +27,7 @@ @BugPattern( name = "TimeZoneUsage", summary = - "Derive the current time from a `Clock` Spring bean, and don't rely on a `Clock`'s time zone", + "Derive the current time from an existing `Clock` Spring bean, and don't rely on a `Clock`'s time zone", linkType = LinkType.NONE, severity = SeverityLevel.WARNING, tags = StandardTags.FRAGILE_CODE) @@ -59,9 +59,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - return buildDescription(tree) - .setMessage( - "Derive the current time from an existing `Clock` Spring bean, and don't rely on a `Clock`'s time zone") - .build(); + return buildDescription(tree).build(); } } From 78fed64c87b0f153a6915af68ecc9015acf31a22 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 15 Feb 2021 10:57:08 +0100 Subject: [PATCH 4/5] Remove newline --- .../tech/picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java | 1 - 1 file changed, 1 deletion(-) 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 4cf1eade77..241072c6b1 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 @@ -58,7 +58,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (!IS_BANNED_TIME_METHOD.matches(tree, state)) { return Description.NO_MATCH; } - return buildDescription(tree).build(); } } From d540c393e5928ee0ed1aa05bf5c352ce77583f7b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 15 Feb 2021 17:15:12 +0100 Subject: [PATCH 5/5] Make 1 return statement with ternary --- .../picnic/errorprone/bugpatterns/TimeZoneUsageCheck.java | 7 +++---- 1 file changed, 3 insertions(+), 4 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 241072c6b1..16fbd77478 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 @@ -55,9 +55,8 @@ public final class TimeZoneUsageCheck extends BugChecker implements MethodInvoca @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!IS_BANNED_TIME_METHOD.matches(tree, state)) { - return Description.NO_MATCH; - } - return buildDescription(tree).build(); + return IS_BANNED_TIME_METHOD.matches(tree, state) + ? buildDescription(tree).build() + : Description.NO_MATCH; } }