SONARJAVA-4943 FP on S1144 if private method is referenced by name in annotations#4776
SONARJAVA-4943 FP on S1144 if private method is referenced by name in annotations#4776
Conversation
leonardo-pilastri-sonarsource
left a comment
leonardo-pilastri-sonarsource
left a comment
There was a problem hiding this comment.
Nice job! There is just one kinda of a corner case where we would encounter false negatives, other than that the logic looks nice to me!
| this.methodNames = methodNames; | ||
| } | ||
|
|
||
| private Set<String> methodNames; |
There was a problem hiding this comment.
Using a Set here is not sufficient: if in the same file you have two different classes like so
@MethodProvided(value = "foo")
static class TN {
private void foo() {} // Compliant, method is referenced in annotation
}
static class FN {
private void foo() {} // False negative, this method has same name but no references
}
the CheckAnnotationsForMethodNames will remove the name foo once and for all and the second encounter will not trigger an issue.
Maybe you could use the whole method signature instead of the simple name? Or a different data structure?
| } else if (arg instanceof AssignmentExpressionTree asgn && asgn.expression().is(Tree.Kind.STRING_LITERAL) && ( | ||
| isMethodAnnotation || isNameIndicatingMethod(((IdentifierTree) asgn.variable()).name()) | ||
| )) { | ||
| removeMethodName((LiteralTree) asgn.expression()); |
There was a problem hiding this comment.
Not a super strong opinion here, but maybe it could be more comprehensible like
| } else if (arg instanceof AssignmentExpressionTree asgn && asgn.expression().is(Tree.Kind.STRING_LITERAL) && ( | |
| isMethodAnnotation || isNameIndicatingMethod(((IdentifierTree) asgn.variable()).name()) | |
| )) { | |
| removeMethodName((LiteralTree) asgn.expression()); | |
| else if (isAnnotationArgumentIndicatingMethod(arg, isMethodAnnotation)) { | |
| removeMethodName((LiteralTree) ((AssignmentExpressionTree)arg).expression()); | |
| } |
with the logic separated in another boolean method
private static boolean isAnnotationArgumentIndicatingMethod(ExpressionTree arg, boolean isMethodAnnotation) {
return arg instanceof AssignmentExpressionTree asgn && asgn.expression().is(Tree.Kind.STRING_LITERAL) && (
isMethodAnnotation || isNameIndicatingMethod(((IdentifierTree) asgn.variable()).name())
);
}
leonardo-pilastri-sonarsource
left a comment
leonardo-pilastri-sonarsource
left a comment
There was a problem hiding this comment.
Look good to me! Nice job
Just a couple of comments, but it's up to you
| private void reportUnusedPrivateMethods() { | ||
| unusedPrivateMethods.stream() | ||
| .filter(methodTree -> !unresolvedMethodNames.contains(methodTree.simpleName().name())) | ||
| Stream<MethodTree> findUnusedPrivateMethods(ClassTree tree) { |
There was a problem hiding this comment.
Can we make this private static?
| this.filteredNames = methodNames; | ||
| } | ||
|
|
||
| private Set<String> filteredNames; |
There was a problem hiding this comment.
This could be final
|
2 participants
No description provided.