From 9c8552e785efe34725e53cfd78338d5724d0ff2c Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 8 Dec 2022 11:26:00 +0100 Subject: [PATCH 1/4] Improve `SuggestedFix` for `StringCaseLocaleUsage` check --- .../bugpatterns/StringCaseLocaleUsage.java | 15 +++++++-- .../StringCaseLocaleUsageTest.java | 32 +++++++++++++++++-- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java index 11b441229c..bde10ce05f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java @@ -58,13 +58,22 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } 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 + // 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(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java index f47b0c434c..c8c910190f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java @@ -62,6 +62,13 @@ void replacementFirstSuggestedFix() { " void m() {", " \"a\".toLowerCase(/* Comment with parens: (). */ );", " \"b\".toUpperCase();", + "", + " getString().toLowerCase();", + " getString().toUpperCase/* Comment with parens: (). */();", + " }", + "", + " private String getString() {", + " return \"\";", " }", "}") .addOutputLines( @@ -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);", + "", + " getString().toLowerCase(Locale.ROOT);", + " getString().toUpperCase /* Comment with parens: (). */(Locale.ROOT);", + " }", + "", + " private String getString() {", + " return \"\";", " }", "}") .doTest(TestMode.TEXT_MATCH); @@ -87,6 +101,13 @@ void replacementSecondSuggestedFix() { " void m() {", " \"a\".toLowerCase();", " \"b\".toUpperCase(/* Comment with parens: (). */ );", + "", + " getString().toLowerCase();", + " getString().toUpperCase/* Comment with parens: (). */();", + " }", + "", + " private String getString() {", + " return \"\";", " }", "}") .addOutputLines( @@ -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()). */ );", + "", + " getString().toLowerCase(Locale.getDefault());", + " getString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());", + " }", + "", + " private String getString() {", + " return \"\";", " }", "}") .doTest(TestMode.TEXT_MATCH); From 6a88ffae15519d5965b5b3429a01d2fde073fa9f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 8 Dec 2022 20:07:39 +0100 Subject: [PATCH 2/4] Apply GFJ --- .../errorprone/bugpatterns/StringCaseLocaleUsageTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java index c8c910190f..8d547c1224 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java @@ -64,7 +64,7 @@ void replacementFirstSuggestedFix() { " \"b\".toUpperCase();", "", " getString().toLowerCase();", - " getString().toUpperCase/* Comment with parens: (). */();", + " getString().toUpperCase /* Comment with parens: (). */();", " }", "", " private String getString() {", @@ -103,7 +103,7 @@ void replacementSecondSuggestedFix() { " \"b\".toUpperCase(/* Comment with parens: (). */ );", "", " getString().toLowerCase();", - " getString().toUpperCase/* Comment with parens: (). */();", + " getString().toUpperCase /* Comment with parens: (). */();", " }", "", " private String getString() {", From 7e01f11f9825e5c1f7fa8539e6274e8ac6874f21 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 8 Dec 2022 20:26:04 +0100 Subject: [PATCH 3/4] Suggestions --- .../bugpatterns/StringCaseLocaleUsage.java | 44 ++++++++++++------- .../StringCaseLocaleUsageTest.java | 40 ++++++++--------- pom.xml | 2 + 3 files changed, 47 insertions(+), 39 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java index bde10ce05f..9aa40e0130 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java @@ -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; @@ -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; /** @@ -51,29 +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 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(')'); + private static Fix suggestLocale(int insertPosition, String locale) { return SuggestedFix.builder() .addImport("java.util.Locale") - .replace( - tree, - source.substring(0, indexOfLastOpeningBracket) - + "(" - + locale - + sourceAfterLastOpeningBracket.substring(indexOfClosingBracket)) + .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); + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java index 8d547c1224..b19262dd4d 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java @@ -63,12 +63,11 @@ void replacementFirstSuggestedFix() { " \"a\".toLowerCase(/* Comment with parens: (). */ );", " \"b\".toUpperCase();", "", - " getString().toLowerCase();", - " getString().toUpperCase /* Comment with parens: (). */();", - " }", + " toString().toLowerCase();", + " toString().toUpperCase /* Comment with parens: (). */();", "", - " private String getString() {", - " return \"\";", + " this.toString().toLowerCase() /* Comment with parens: (). */;", + " this.toString().toUpperCase();", " }", "}") .addOutputLines( @@ -77,15 +76,14 @@ void replacementFirstSuggestedFix() { "", "class A {", " void m() {", - " \"a\".toLowerCase(/* Comment with parens: (Locale.ROOT). */ );", + " \"a\".toLowerCase(/* Comment with parens: (). */ Locale.ROOT);", " \"b\".toUpperCase(Locale.ROOT);", "", - " getString().toLowerCase(Locale.ROOT);", - " getString().toUpperCase /* Comment with parens: (). */(Locale.ROOT);", - " }", + " toString().toLowerCase(Locale.ROOT);", + " toString().toUpperCase /* Comment with parens: (). */(Locale.ROOT);", "", - " private String getString() {", - " return \"\";", + " this.toString().toLowerCase(Locale.ROOT) /* Comment with parens: (). */;", + " this.toString().toUpperCase(Locale.ROOT);", " }", "}") .doTest(TestMode.TEXT_MATCH); @@ -102,12 +100,11 @@ void replacementSecondSuggestedFix() { " \"a\".toLowerCase();", " \"b\".toUpperCase(/* Comment with parens: (). */ );", "", - " getString().toLowerCase();", - " getString().toUpperCase /* Comment with parens: (). */();", - " }", + " toString().toLowerCase();", + " toString().toUpperCase /* Comment with parens: (). */();", "", - " private String getString() {", - " return \"\";", + " this.toString().toLowerCase() /* Comment with parens: (). */;", + " this.toString().toUpperCase();", " }", "}") .addOutputLines( @@ -117,14 +114,13 @@ void replacementSecondSuggestedFix() { "class A {", " void m() {", " \"a\".toLowerCase(Locale.getDefault());", - " \"b\".toUpperCase(/* Comment with parens: (Locale.getDefault()). */ );", + " \"b\".toUpperCase(/* Comment with parens: (). */ Locale.getDefault());", "", - " getString().toLowerCase(Locale.getDefault());", - " getString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());", - " }", + " toString().toLowerCase(Locale.getDefault());", + " toString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());", "", - " private String getString() {", - " return \"\";", + " this.toString().toLowerCase(Locale.getDefault()) /* Comment with parens: (). */;", + " this.toString().toUpperCase(Locale.getDefault());", " }", "}") .doTest(TestMode.TEXT_MATCH); diff --git a/pom.xml b/pom.xml index a62b53dd0c..82cc841a76 100644 --- a/pom.xml +++ b/pom.xml @@ -852,6 +852,7 @@ --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED + --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED -Xmaxerrs @@ -997,6 +998,7 @@ --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED + --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED From edf19f54a6fed778f4cd55ff30a30fb847967309 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 9 Dec 2022 14:28:17 +0100 Subject: [PATCH 4/4] Suggestions --- .../errorprone/bugpatterns/StringCaseLocaleUsageTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java index b19262dd4d..caec7843c3 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java @@ -62,6 +62,7 @@ void replacementFirstSuggestedFix() { " void m() {", " \"a\".toLowerCase(/* Comment with parens: (). */ );", " \"b\".toUpperCase();", + " \"c\".toLowerCase().toString();", "", " toString().toLowerCase();", " toString().toUpperCase /* Comment with parens: (). */();", @@ -78,6 +79,7 @@ void replacementFirstSuggestedFix() { " void m() {", " \"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);", @@ -99,6 +101,7 @@ void replacementSecondSuggestedFix() { " void m() {", " \"a\".toLowerCase();", " \"b\".toUpperCase(/* Comment with parens: (). */ );", + " \"c\".toLowerCase().toString();", "", " toString().toLowerCase();", " toString().toUpperCase /* Comment with parens: (). */();", @@ -115,6 +118,7 @@ void replacementSecondSuggestedFix() { " void m() {", " \"a\".toLowerCase(Locale.getDefault());", " \"b\".toUpperCase(/* Comment with parens: (). */ Locale.getDefault());", + " \"c\".toLowerCase(Locale.getDefault()).toString();", "", " toString().toLowerCase(Locale.getDefault());", " toString().toUpperCase /* Comment with parens: (). */(Locale.getDefault());",