Fix suggestions emitted by the StringCaseLocaleUsage check (#400)

The suggested `Locale` arguments are now always located in the correct place.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsage.java

@@ -4,9 +4,11 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
 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.Fix;
 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,20 +56,34 @@ public final class StringCaseLocaleUsage extends BugChecker implements MethodInv
       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);
+  }
 }

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java

@@ -62,6 +62,13 @@ final class StringCaseLocaleUsageTest {
             "  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(
@@ -70,8 +77,15 @@ final class StringCaseLocaleUsageTest {
             "",
             "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);
@@ -87,6 +101,13 @@ final class StringCaseLocaleUsageTest {
             "  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(
@@ -96,7 +117,14 @@ final class StringCaseLocaleUsageTest {
             "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);

pom.xml

@@ -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>
@@ -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>