Skip to content

Commit

Permalink
Fix suggestions emitted by the StringCaseLocaleUsage check (#400)
Browse files Browse the repository at this point in the history 
The suggested `Locale` arguments are now always located in the correct place.
  • Loading branch information
@rickie
rickie authored Dec 9, 2022
1 parent 096acfb commit 8145028
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 10 deletions.
35 changes: 27 additions & 8 deletions ...prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java
View file
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
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();",
" }",
"}")
.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
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

0 comments on commit 8145028

Please sign in to comment.