Fix suggestions emitted by the StringCaseLocaleUsage check
#400
Conversation
|
Looks good. All 5 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
|
Looks good. All 5 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| " getString().toLowerCase();", | ||
| " getString().toUpperCase /* Comment with parens: (). */();", | ||
| " }", | ||
| "", | ||
| " private String getString() {", | ||
| " return \"\";", | ||
| " }", |
There was a problem hiding this comment.
We can simply use toString() instead of a custom getString().
| // XXX: The logic that replaces the first parenthesis assumes that `tree` does not have a source | ||
| // code representation such as `str.toLowerCase/* Some comment with parens (). */()`. In such a | ||
| // XXX: The logic that replaces the last parenthesis assumes that `tree` does not have a source | ||
| // code representation such as `str.toLowerCase(/* Some comment with parens (). */)`. In such a | ||
| // case the comment, rather than the method invocation arguments, will be modified. Implement a | ||
| // generic solution for this. | ||
| String source = SourceCode.treeToString(tree, state); | ||
| int indexOfLastOpeningBracket = source.lastIndexOf('('); | ||
| String sourceAfterLastOpeningBracket = source.substring(indexOfLastOpeningBracket); | ||
| int indexOfClosingBracket = sourceAfterLastOpeningBracket.indexOf(')'); | ||
| return SuggestedFix.builder() | ||
| .addImport("java.util.Locale") | ||
| .replace(tree, SourceCode.treeToString(tree, state).replaceFirst("\\(", '(' + locale)) | ||
| .replace( | ||
| tree, | ||
| source.substring(0, indexOfLastOpeningBracket) | ||
| + "(" | ||
| + locale | ||
| + sourceAfterLastOpeningBracket.substring(indexOfClosingBracket)) | ||
| .build(); |
There was a problem hiding this comment.
If a known-hacky solution bit us once, then we shouldn't replace it with another hacky solution. It seem better to look only at the final closing paren. Additionally, we can take inspiration from Error Prone's (Java)doc-related checks to see how we can robustly avoid matching comments. Will push a proposal.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
^ The four surviving mutants are in code that handles the absence of source code; not sure how we can test those cases without jumping through lots of hoops. Not worth the effort :) |
rickie
force-pushed
the
rossendrijver/bugs/stringlocalecaseusage
branch
from
16c65de to
7e01f11
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| " this.toString().toUpperCase();", | ||
| " }", |
There was a problem hiding this comment.
As we match on last ), we could add a test to make sure it does not match when there is another chained method invocation afterwards:
| " this.toString().toUpperCase();", | |
| " }", | |
| " this.toString().toUpperCase();", | |
| "", | |
| " "a".toUpperCase().toString();" | |
| " }", |
But this is rather testing that the Matcher defined is not matching too broadly.
There was a problem hiding this comment.
I don't see why not. Will push something.
|
Added commit. Will merge once 🟢 . Added one word to the suggested commit message 👀. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
rickie
changed the title
SuggestedFix for StringCaseLocaleUsage checkStringCaseLocaleUsage check
Encountered this while testing the upcoming release on downstream code.
The following code:
Got rewritten to:
But should've been:
After some testing, it turns out that searching for the first occurrence of
(is much more error-prone than searching for the last occurrence. Therefore I propose to change the implementation accordingly.NB: To be (arguably) more correct, I changed the implementation to find the first closing bracket after the last opening bracket. There are cases where this can still cause problems. An example:
Yes we should tackle this, but I think it'd be better for now to leave this as is and create a ticket to properly fix this.
So this is indeed not the optimal solution and we should implement a more correct one. However, for now I'd argue this is a clear improvement.