Introduce CanonicalClassNameUsage check
#881
Conversation
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| if (!GET_NAME_INVOCATION.matches(tree, state) || !isPassedToCanonicalNameUsingType(state)) { |
There was a problem hiding this comment.
Acording to Class#getCanonicalName docs, the method can return null for local, anonymous & hidden classes.
I wonder do we need to add a CanonicalNameIsNotNull check here ?
But I guess the idea is that methods that accept canonical names are aware of the API...
There was a problem hiding this comment.
Oeh nice that you think about such edge cases 🚀 !
Interesting it will indeed not work if we rewrite new Object(){}.getClass().getName() as it will return null. However, it does not really make sense to write such code in one of the methods that we flag, which are specific methods from tech.picnic.errorprone or com.google.errorprone. So I think for now it's fine to not flag this 🤔.
There was a problem hiding this comment.
Nice catch @mohamedsamehsalah! In practice we're interested in matching .getName() on class literals I see, so we can add an additional constraint for that; then the result will never be null.
| public final class CanonicalClassNameUsage extends BugChecker | ||
| implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Matcher<ExpressionTree> GET_NAME_INVOCATION = |
There was a problem hiding this comment.
For these situations we use both _INVOCATION and _METHOD. I feel INVOCATION fits better here. Maybe we should apply this consistently 🤔.
There was a problem hiding this comment.
Indeed; I had the same feeling. Last I looked into this (won't do another pass now) there were more inconsistencies, so perhaps this is something to review in a separate PR.
rickie
force-pushed
the
sschroevers/canonical-class-name-usage
branch
from
2e67804 to
1fa3951
Compare
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/canonical-class-name-usage
branch
from
1fa3951 to
2695c4d
Compare
There was a problem hiding this comment.
Tnx for the reviews @mohamedsamehsalah and @rickie! I rebased and added a commit.
| public final class CanonicalClassNameUsage extends BugChecker | ||
| implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Matcher<ExpressionTree> GET_NAME_INVOCATION = |
There was a problem hiding this comment.
Indeed; I had the same feeling. Last I looked into this (won't do another pass now) there were more inconsistencies, so perhaps this is something to review in a separate PR.
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| if (!GET_NAME_INVOCATION.matches(tree, state) || !isPassedToCanonicalNameUsingType(state)) { |
There was a problem hiding this comment.
Nice catch @mohamedsamehsalah! In practice we're interested in matching .getName() on class literals I see, so we can add an additional constraint for that; then the result will never be null.
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Error Prone checks deal with source code and type matchers, both of which generally involve canonical type names, rather than the strings produced by `Class#getName()`. This distinction is particularly relevant when dealing with nested types.
rickie
force-pushed
the
sschroevers/canonical-class-name-usage
branch
from
2695c4d to
9307a2e
Compare
|
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Suggested commit message: