-
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 CanonicalConstantFieldName
bug checker
#794
base: master
Are you sure you want to change the base?
Introduce CanonicalConstantFieldName
bug checker
#794
Conversation
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.
In addition to class variables within the same compilation unit, should we also match on constants imported from other classes ?
Ideally, the checker will suggest the fix in that class 🤔
Which raises the other question, do we really need to suggest a fix for the other classes referencing the constants in this
compilation unit ?
@Stephan202 suggested the use of TreeScanner
as in this example but I could not seem to make this work 😓
Help appreciated 🙏
31a9e4d
to
7fff49f
Compare
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.
Tnx for working on this @mohamedsamehsalah; it'll allow for some nice cleanup I expect. Did leave some comments based on a quick skim :)
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java
Outdated
Show resolved
Hide resolved
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
(I didn't check the comments before reviewing; sorry.)
Error Prone doesn't support this; for cases like that OpenRewrite would be better suited. (But for now, let's just focus on the private fields.)
Anywhere we can see the code that doesn't work? Perhaps I can take a look. |
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. |
4acc2bc
to
ecdccc3
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
ecdccc3
to
40a2a6f
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
@Stephan202 I covered your first pass comments, however, I still could not make use of the AFAIU, the suggested fix builder used in this PR will execute what you are suggesting in this comment:
Happy to hear your thoughts (again? 😅) 🙏 |
40a2a6f
to
0c94940
Compare
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.
@Stephan202 I covered your first pass comments, however, I still could not make use of the
TreeScanner
. Or in other words, I couldn't find what advantage does theTreeScanner
give that theTreeMatcher
does not.
Actually, you are using a TreeScanner
now, just through a method I didn't know existed: have a close look at SuggestedFixes.renameVariable
👀 😄 This should work!
Before I dive into a more thorough review: would you like to have a crack at resolving the surviving mutants?
if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) { | ||
String replacement = toUpperSnakeCase(variableName); | ||
|
||
if (!classVariables.contains(replacement)) { |
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.
IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit
could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner
may be employed.
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.
would you like to have a crack at resolving the surviving mutants?
Yes, sir 🖖
IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner may be employed.
Will look into this 👀
Thanks.
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. |
1c55636
to
12e51ab
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.
Sorry for the slow progress on this @Stephan202 , PTAL 🙏
12e51ab
to
a7f652e
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.
As a result of dropping, we can simplify the code, pushing a suggestion!
...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
Outdated
Show resolved
Hide resolved
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
/integration-test |
Thanks @rickie , changes LGMT 🚀 |
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.
Hey @mohamedsamehsalah , thanks a lot for pushing this! It looks really nice and am curious to see this getting rolled out on Picnic's codebase 🔥 !!!
linkType = CUSTOM, | ||
severity = WARNING, | ||
tags = LIKELY_ERROR) | ||
public final class CanonicalConstantFieldName extends BugChecker implements VariableTreeMatcher { |
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 we can omit the word "field" from most things in this class. As a constant is actually always a field right? 🤔
private static final Matcher<Tree> IS_CONSTANT = | ||
allOf( | ||
hasModifier(Modifier.STATIC), hasModifier(Modifier.PRIVATE), hasModifier(Modifier.FINAL)); | ||
private static final Pattern TO_SNAKE_CASE = Pattern.compile("([a-z])([A-Z])"); |
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.
This is actually not the TO_
snake case, it's the pattern for SNAKE_CASE
, right?
private static final Pattern TO_SNAKE_CASE = Pattern.compile("([a-z])([A-Z])"); | ||
private static final ImmutableSet<String> DEFAULT_EXCLUDED_CONSTANT_FIELD_NAMES = | ||
ImmutableSet.of("serialVersionUID"); | ||
private static final String EXCLUDED_CONSTANT_FIELD_NAMES = |
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.
private static final String EXCLUDED_CONSTANT_FIELD_NAMES = | |
private static final String ALLOWED_CONSTANT_NAMES_FLAG = |
Let's add the FLAG
suffix and maybe instead of excluded maybe we should use the word "allowed", WDYT?
@Override | ||
public Description matchVariable(VariableTree tree, VisitorState state) { | ||
String variableName = tree.getName().toString(); | ||
if (IS_CONSTANT.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.
If we flip the conditions here and do an early return, we have one level of indentation less. Additionally, we generally try to have early returns if possible :).
return Description.NO_MATCH; | ||
} | ||
|
||
private static ImmutableList<VariableTree> getVariablesInCompilationUnit( |
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.
Changed the order of static methods in the class based on which one is used first :).
ImmutableList<VariableTree> variablesInCompilationUnit = | ||
getVariablesInCompilationUnit(state.getPath().getCompilationUnit()); | ||
String replacement = toUpperSnakeCase(variableName); | ||
if (variablesInCompilationUnit.stream() |
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.
Just a suggestion but what if we extract this to a method, I think it reads nicely. Additionally if we use anyMatch
instead of noneMatch
it reads as "isVariableNameInUse`, instead of the negated variant, could be a bit easier to understand.
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.
More readable, I agree 💯
void identification() { | ||
CompilationTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) | ||
.addSourceLines( | ||
"A.java", |
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.
Added a few more tests :).
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
9605dfe
to
0728480
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Quality Gate passedIssues Measures |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Thanks for suggestions @rickie . LGTM 🚀 |
Suggested commit message: