Skip to content
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.

Sign up for GitHub

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

Fix suggestions emitted by the StringCaseLocaleUsage check #400

Merged
merged 4 commits into from
Dec 9, 2022
Merged

Fix suggestions emitted by the StringCaseLocaleUsage check #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c8552e
Improve `SuggestedFix` for `StringCaseLocaleUsage` check
rickie Dec 8, 2022
6a88ffa
Apply GFJ
rickie Dec 8, 2022
7e01f11
Suggestions
Stephan202 Dec 8, 2022
edf19f5
Suggestions
rickie Dec 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions ...prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE;
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.sun.tools.javac.parser.Tokens.TokenKind.RPAREN;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
import com.google.common.collect.Streams;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
Expand All @@ -15,8 +17,11 @@
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.ErrorProneTokens;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.util.Position;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
Expand Down Expand Up @@ -51,20 +56,34 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}

int closingParenPosition = getClosingParenPosition(tree, state);
if (closingParenPosition == Position.NOPOS) {
return describeMatch(tree);
}

return buildDescription(tree)
.addFix(suggestLocale(tree, "Locale.ROOT", state))
.addFix(suggestLocale(tree, "Locale.getDefault()", state))
.addFix(suggestLocale(closingParenPosition, "Locale.ROOT"))
.addFix(suggestLocale(closingParenPosition, "Locale.getDefault()"))
.build();
}

private static Fix suggestLocale(MethodInvocationTree tree, String locale, VisitorState state) {
// 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
// case the comment, rather than the method invocation arguments, will be modified. Implement a
// generic solution for this.
private static Fix suggestLocale(int insertPosition, String locale) {
return SuggestedFix.builder()
.addImport("java.util.Locale")
.replace(tree, SourceCode.treeToString(tree, state).replaceFirst("\\(", '(' + locale))
.replace(insertPosition, insertPosition, locale)
.build();
}

private static int getClosingParenPosition(MethodInvocationTree tree, VisitorState state) {
int startPosition = ASTHelpers.getStartPosition(tree);
if (startPosition == Position.NOPOS) {
return Position.NOPOS;
}

return Streams.findLast(
ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context).stream()
.filter(t -> t.kind() == RPAREN))
.map(token -> startPosition + token.pos())
.orElse(Position.NOPOS);
}
}
32 changes: 30 additions & 2 deletions ...e-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ void replacementFirstSuggestedFix() {
" void m() {",
" \"a\".toLowerCase(/* Comment with parens: (). */ );",
" \"b\".toUpperCase();",
" \"c\".toLowerCase().toString();",
"",
" toString().toLowerCase();",
" toString().toUpperCase /* Comment with parens: (). */();",
"",
" this.toString().toLowerCase() /* Comment with parens: (). */;",
" this.toString().toUpperCase();",
" }",
Comment on lines +71 to 72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
" this.toString().toUpperCase();",
" }",
" this.toString().toUpperCase();",
"",
" "a".toUpperCase().toString();"
" }",

But this is rather testing that the Matcher defined is not matching too broadly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why not. Will push something.

"}")
.addOutputLines(
Expand All @@ -70,8 +77,15 @@ void replacementFirstSuggestedFix() {
"",
"class A {",
" void m() {",
" \"a\".toLowerCase(Locale.ROOT /* Comment with parens: (). */);",
" \"a\".toLowerCase(/* Comment with parens: (). */ Locale.ROOT);",
" \"b\".toUpperCase(Locale.ROOT);",
" \"c\".toLowerCase(Locale.ROOT).toString();",
"",
" toString().toLowerCase(Locale.ROOT);",
" toString().toUpperCase /* Comment with parens: (). */(Locale.ROOT);",
"",
" this.toString().toLowerCase(Locale.ROOT) /* Comment with parens: (). */;",
" this.toString().toUpperCase(Locale.ROOT);",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
Expand All @@ -87,6 +101,13 @@ void replacementSecondSuggestedFix() {
" void m() {",
" \"a\".toLowerCase();",
" \"b\".toUpperCase(/* Comment with parens: (). */ );",
" \"c\".toLowerCase().toString();",
"",
" toString().toLowerCase();",
" toString().toUpperCase /* Comment with parens: (). */();",
"",
" this.toString().toLowerCase() /* Comment with parens: (). */;",
" this.toString().toUpperCase();",
" }",
"}")
.addOutputLines(
Expand All @@ -96,7 +117,14 @@ void replacementSecondSuggestedFix() {
"class A {",
" void m() {",
" \"a\".toLowerCase(Locale.getDefault());",
" \"b\".toUpperCase(Locale.getDefault() /* Comment with parens: (). */);",
" \"b\".toUpperCase(/* Comment with parens: (). */ Locale.getDefault());",
" \"c\".toLowerCase(Locale.getDefault()).toString();",
"",
" toString().toLowerCase(Locale.getDefault());",
" toString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());",
"",
" this.toString().toLowerCase(Locale.getDefault()) /* Comment with parens: (). */;",
" this.toString().toUpperCase(Locale.getDefault());",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
Expand Down
2 changes: 2 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,7 @@
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
<arg>-Xmaxerrs</arg>
Expand Down Expand Up @@ -997,6 +998,7 @@
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</additionalJOption>
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</additionalJOption>
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</additionalJOption>
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</additionalJOption>
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</additionalJOption>
<additionalJOption>--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</additionalJOption>
</additionalJOptions>
Expand Down