PRP-11004 Introduce bug checker to flag Instant.now()
#10
Conversation
There was a problem hiding this comment.
Didn't check the tests yet. I wonder whether it'd be useful to extend UseTimeInScope: if there's a Clock in scope, then Instant.now() should become clock.instant().
| public final class InstantNowCheck extends BugChecker implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Matcher<ExpressionTree> NOW_USAGES = | ||
| anyOf(staticMethod().onClassAny(Instant.class.getName()).namedAnyOf("now")); |
There was a problem hiding this comment.
| anyOf(staticMethod().onClassAny(Instant.class.getName()).namedAnyOf("now")); | |
| staticMethod().onClassAny(Instant.class.getName().namedAnyOf("now")); |
That said, there are a few other classes that derive time from an implicit time source that we should flag.
| } | ||
|
|
||
| SuggestedFix.Builder builder = SuggestedFix.builder(); | ||
| builder.replace(tree, Util.treeToString(tree, state).replace(".now()", ".EPOCH")); |
There was a problem hiding this comment.
As mentioned during our last meeting, I don't think we should do this replacement: it's not correct. Instead developers should manually first the issue in a way that is most appropriate.
There was a problem hiding this comment.
In case we refactor this plugin to run in tests only, does it worth introducing more logic to that and not just replacing with InstantEpoch? if the test will fail - then should be fixed manually.
There was a problem hiding this comment.
If we can reliably identify tests (something that's also necessary for #5 (comment)) then sure, Instant.EPOCH (and LocalDate.EPOCH, etc.) works for me.
Stephan202
force-pushed
the
anna8712/PRP-11004
branch
from
e131122 to
bb952b0
Compare
There was a problem hiding this comment.
Rebased; did not attempt to fix the build. Given that we now also have #9, are you still interested in this PR? :)
| } | ||
|
|
||
| SuggestedFix.Builder builder = SuggestedFix.builder(); | ||
| builder.replace(tree, Util.treeToString(tree, state).replace(".now()", ".EPOCH")); |
There was a problem hiding this comment.
If we can reliably identify tests (something that's also necessary for #5 (comment)) then sure, Instant.EPOCH (and LocalDate.EPOCH, etc.) works for me.
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| name = "InstantNow", | ||
| summary = "Avoid using {@link Instant#now()} when possible", |
There was a problem hiding this comment.
Javadoc doesn't work here :)
Stephan202
force-pushed
the
anna8712/PRP-11004
branch
from
bb952b0 to
ae15923
Compare
|
@Stephan202 Thanks for your comments, I agree to close this PR, since https://github.com/PicnicSupermarket/error-prone-support/pull/9/files covers |
|
Sure; update the ticket :) |
No description provided.