-
Notifications
You must be signed in to change notification settings - Fork 36
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
Introduce Slf4jLogDeclaration
to canonicalize slf4j's variable's declaration
#783
base: master
Are you sure you want to change the base?
Introduce Slf4jLogDeclaration
to canonicalize slf4j's variable's declaration
#783
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
536811a
to
fc4d717
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
fc4d717
to
3c70f11
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
3c70f11
to
5947396
Compare
Kudos, SonarCloud Quality Gate passed! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Quick skim; hope this helps :)
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public Description matchClass(ClassTree tree, VisitorState state) { | ||
if (tree.getKind() == Kind.INTERFACE || !TEST_CLASS_WITH_LOGGER.matches(tree, 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.
I don't think we need to skip interfaces: for those we can still suggest (but not auto-fix) the canonical field name.
(Separately from that we should drop redundant modifiers, but that should be a separate check, modeled after this Checkstyle rule.)
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 don't think we need to skip interfaces
The intention was to not apply modifiers to inteface variables but still suggest/fix field name.
: for those we can still suggest (but not auto-fix) the canonical field name.
Why only suggest and not auto-fix ?
(Separately from that we should drop redundant modifiers, but that should be a separate check, modeled after this Checkstyle rule.)
Should this check be part of this PR ?
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
@Override | ||
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
if (!GET_LOGGER_METHOD.matches(tree, 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.
We're now creating separate fixes for the field declaration (modifiers + name) and for the referenced class. I wonder whether this means that we should have two separate checks. I can imagine that some users will appreciate the second, while finding the first too conservative.
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.
Pushed a change using TreeScanner (Conceptually still two checks usingvisitClass
& visitMethodInvocationTree
).
What do you think about making the separation into an EP flag ?
...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
if (LOGGER.matches(member, state)) { | ||
VariableTree variable = (VariableTree) member; | ||
SuggestedFixes.addModifiers( | ||
member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) |
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.
Making the field final
or private
could make the code fail to compile. Likely this is still the right trade-off, but perhaps something worth calling out.
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.
Does it make sense to make the suggested modifiers EP flags instead ?
5947396
to
61626b7
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Kudos, SonarCloud Quality Gate passed! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Some comments we discussed offline :).
...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java
Outdated
Show resolved
Hide resolved
4142606
to
a54db09
Compare
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.
Sorry for the slow progress 🐢
...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java
Outdated
Show resolved
Hide resolved
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
7109c5d
to
b29f243
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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 @rickie , I resolved the comments from the pair programming sessions and tried to clean up some of the mutants 🐞. PTAL 🙏 .
} | ||
|
||
Symbol enclosingElement = ASTHelpers.getSymbol(tree).getEnclosingElement(); | ||
if (enclosingElement == null) { |
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.
Either we do this check or we suppress NullAway
warning
String argumentClassName; | ||
if (arg.getKind() == Kind.STRING_LITERAL) { | ||
java.util.regex.Matcher matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName); | ||
checkArgument(matcher.matches(), "Invalid argument name."); |
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.
Here and below, this check is used to invoke the matching.
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.
Did another pass, some small improvements here and there :).
ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); | ||
String argumentName = SourceCode.treeToString(arg, state); | ||
|
||
String argumentClassName; |
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.
We can simplify the code a little:
java.util.regex.Matcher matcher;
if (arg.getKind() == Kind.STRING_LITERAL) {
matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName);
} else {
matcher = CLASS_ARGUMENT.matcher(argumentName);
}
checkArgument(matcher.matches(), "Invalid argument name.");
String argumentClassName = matcher.group(1);
} | ||
} | ||
|
||
private static void updateGetLoggerArgument( |
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'm not seeing why we need a TreeScanner
to walk over a VariableTree
, I think we can improve this :).
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.
We do this because we need to match on the variable declaration and not in the class level (Which was flagged here). In addition, in case the variable is in a nested class, without this, the match will be on the outer class level .
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
/integration-test |
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 there is only one open question for me that we should answer before approving. Apart from that, I have almost nothing to add here!
Really cool stuff @mohamedsamehsalah 🚀 !
matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); | ||
} | ||
|
||
checkArgument(matcher.matches(), "Invalid argument name."); |
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.
Hmm interesting. Maybe a verify
or checkState
, would be better?
"", | ||
" static class WrongArgumentOverloadedGetLogger {", | ||
" // BUG: Diagnostic contains:", | ||
" private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", |
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.
On a second look, I'm not really sure how we should handle these string overloads... There are quite some different ways of using them, if we look outside of Picnic to usages. Within Picnic we have a few like Something.NameOfclass
, so not sure what we want to do there...
CC: @Stephan202 What do you think?
" }", | ||
"", | ||
" static class WrongArgumentOverloadedGetLogger {", | ||
" // BUG: Diagnostic contains:", |
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.
We can drop this one in a replacement test 😉.
"class A {", | ||
" private static final long serialVersionUID = 1L;", | ||
"", | ||
" private static final Logger LOG = LoggerFactory.getLogger(A.class);", |
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 put the "positive" examples at the top of the file :).
Quality Gate passedIssues Measures |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
#632
Suggested commit message: