-
Notifications
You must be signed in to change notification settings - Fork 742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent "<no match>" reports by StronglyTypeTime
#1609
Prevent "<no match>" reports by StronglyTypeTime
#1609
Conversation
This fix prevents reports such as the following, which are hard to debug for users: ``` [INFO] /path/to/some/File.java: [<no match>] <no match> (see <no match>) ```
@@ -133,7 +133,10 @@ private void handle(Tree tree) { | |||
}.scan(tree, null); | |||
|
|||
for (Map.Entry<VarSymbol, TreePath> entry : fields.entrySet()) { | |||
state.reportMatch(describeMatch(entry.getValue(), usages.get(entry.getKey()), state)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. I think this is a more general problem with the handling of NO_MATCH
that affects other checks, and that we should handle it here:
public void reportMatch(Description description) { |
instead of here:
if (description == null || description == Description.NO_MATCH) { |
Any concerns with that change? (I can make the edit when importing, I just want to check if you see any reasons not do that.) Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good suggestion, and indeed I wondered about this. But then I noticed that several existing plugins follow the current pattern, so I assumed that there was a reason for not handling NO_MATCH
at such a "deep" level.
But now that I think about it a bit more, the suggested change makes a lot of sense: NO_MATCH
is owned by Description
, so in a sense NO_MATCH
is to Description
what empty
is to Optional
. Since VisitorState
explicitly handles Description
it is thus reasonable that it also handles NO_MATCH
.
For reference, if you make that change then the following callers can also be simplified:
error-prone/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java
Lines 112 to 115 in 704e8a1
Description description = scanBlock(tree, block, state); | |
if (description != NO_MATCH) { | |
state.reportMatch(description); | |
} |
error-prone/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java
Lines 206 to 209 in 3c0c609
if (description == null || description == NO_MATCH) { | |
return; | |
} | |
state.reportMatch(description); |
error-prone/core/src/main/java/com/google/errorprone/bugpatterns/MockitoCast.java
Lines 180 to 183 in a91042e
Description description = matchMethodInvocation(node, state.withPath(getCurrentPath())); | |
if (description != Description.NO_MATCH) { | |
state.reportMatch(description); | |
} |
This fix prevents reports such as the following, which are hard to debug for users: ``` [INFO] /path/to/some/File.java: [<no match>] <no match> (see <no match>) Fixes #1609 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=311378177
This fix prevents reports such as the following, which are hard to debug for users: