Support Pattern.split in StringSplitter

cushon · Error Prone Team · commit ea7b968f7922 · 2021-01-08T13:37:12.000-08:00
Fixes https://github.com/google/error-prone/pull/1683/files PiperOrigin-RevId: 350827009
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java
@@ -20,8 +20,11 @@
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
 import static com.google.errorprone.util.Regexes.convertRegexToLiteral;
 import static java.lang.String.format;
 
@@ -46,6 +49,7 @@
 import com.sun.source.util.TreePath;
 import com.sun.source.util.TreePathScanner;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
+import com.sun.tools.javac.code.Type;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
@@ -59,10 +63,15 @@
 public class StringSplitter extends BugChecker implements MethodInvocationTreeMatcher {
 
   private static final Matcher<ExpressionTree> MATCHER =
-      instanceMethod()
-          .onExactClass("java.lang.String")
-          .named("split")
-          .withParameters("java.lang.String");
+      anyOf(
+          instanceMethod()
+              .onExactClass("java.lang.String")
+              .named("split")
+              .withParameters("java.lang.String"),
+          instanceMethod()
+              .onExactClass("java.util.regex.Pattern")
+              .named("split")
+              .withParameters("java.lang.CharSequence"));
 
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
@@ -78,20 +87,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
   }
 
   public Optional<Fix> buildFix(MethodInvocationTree tree, VisitorState state) {
-    Tree arg = getOnlyElement(tree.getArguments());
-    String methodAndArgument = getMethodAndArgument(arg, state);
+    ExpressionTree arg = getOnlyElement(tree.getArguments());
     Tree parent = state.getPath().getParentPath().getLeaf();
     if (parent instanceof EnhancedForLoopTree
         && ((EnhancedForLoopTree) parent).getExpression().equals(tree)) {
       // fix for `for (... : s.split(...)) {}` -> `for (... : Splitter.on(...).split(s)) {}`
       return Optional.of(
           replaceWithSplitter(
-                  SuggestedFix.builder(),
-                  tree,
-                  methodAndArgument,
-                  state,
-                  "split",
-                  /* mutableList= */ false)
+                  SuggestedFix.builder(), tree, arg, state, "split", /* mutableList= */ false)
               .build());
     }
     if (parent instanceof ArrayAccessTree) {
@@ -115,9 +118,7 @@ public Optional<Fix> buildFix(MethodInvocationTree tree, VisitorState state) {
                   state.getEndPosition(arrayAccessTree),
                   ")");
       return Optional.of(
-          replaceWithSplitter(
-                  fix, tree, methodAndArgument, state, "split", /* mutableList= */ false)
-              .build());
+          replaceWithSplitter(fix, tree, arg, state, "split", /* mutableList= */ false).build());
     }
     // If the result of split is assigned to a variable, try to fix all uses of the variable in the
     // enclosing method. If we don't know how to fix any of them, bail out.
@@ -218,12 +219,12 @@ public Boolean visitMemberSelect(MemberSelectTree tree, Void unused) {
       if (!isImplicitlyTyped) {
         fix.replace(varType, "List<String>").addImport("java.util.List");
       }
-      replaceWithSplitter(fix, tree, methodAndArgument, state, "splitToList", needsMutableList[0]);
+      replaceWithSplitter(fix, tree, arg, state, "splitToList", needsMutableList[0]);
     } else {
       if (!isImplicitlyTyped) {
         fix.replace(varType, "Iterable<String>");
       }
-      replaceWithSplitter(fix, tree, methodAndArgument, state, "split", needsMutableList[0]);
+      replaceWithSplitter(fix, tree, arg, state, "split", needsMutableList[0]);
     }
     return Optional.of(fix.build());
   }
@@ -258,24 +259,42 @@ private static String getMethodAndArgument(Tree origArg, VisitorState state) {
   private SuggestedFix.Builder replaceWithSplitter(
       SuggestedFix.Builder fix,
       MethodInvocationTree tree,
-      String methodAndArgument,
+      ExpressionTree arg,
       VisitorState state,
       String splitMethod,
       boolean mutableList) {
     ExpressionTree receiver = ASTHelpers.getReceiver(tree);
     if (mutableList) {
       fix.addImport("java.util.ArrayList");
     }
-    return fix.addImport("com.google.common.base.Splitter")
-        .prefixWith(
-            receiver,
-            String.format(
-                "%sSplitter.%s.%s(",
-                (mutableList ? "new ArrayList<>(" : ""), methodAndArgument, splitMethod))
-        .replace(
-            state.getEndPosition(receiver),
-            state.getEndPosition(tree),
-            (mutableList ? ")" : "") + ")");
+    fix.addImport("com.google.common.base.Splitter");
+    Type receiverType = getType(receiver);
+    if (isSubtype(receiverType, state.getSymtab().stringType, state)) {
+      String methodAndArgument = getMethodAndArgument(arg, state);
+      return fix.prefixWith(
+              receiver,
+              String.format(
+                  "%sSplitter.%s.%s(",
+                  (mutableList ? "new ArrayList<>(" : ""), methodAndArgument, splitMethod))
+          .replace(
+              state.getEndPosition(receiver),
+              state.getEndPosition(tree),
+              (mutableList ? ")" : "") + ")");
+    }
+    if (isSubtype(receiverType, state.getTypeFromString("java.util.regex.Pattern"), state)) {
+      return fix.prefixWith(
+              receiver, String.format("%sSplitter.on(", (mutableList ? "new ArrayList<>(" : "")))
+          .postfixWith(receiver, ")")
+          .replace(
+              /* startPos= */ state.getEndPosition(receiver),
+              /* endPos= */ getStartPosition(arg),
+              String.format(".%s(", splitMethod))
+          .replace(
+              state.getEndPosition(arg),
+              state.getEndPosition(tree),
+              (mutableList ? ")" : "") + ")");
+    }
+    throw new AssertionError(receiver);
   }
 
   private TreePath findEnclosing(VisitorState state) {
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitterTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitterTest.java
@@ -438,4 +438,44 @@ public void noSplitterOnClassPath() {
         .setArgs("-cp", ":")
         .doTest(TestMode.TEXT_MATCH);
   }
+
+  @Test
+  public void patternSplit() {
+    testHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.regex.Pattern;",
+            "class Test {",
+            "  void f() {",
+            "    String x = Pattern.compile(\"\").split(\"c\")[0];",
+            "    for (String s : Pattern.compile(\"\").split(\":\")) {}",
+            "    String[] xs = Pattern.compile(\"c\").split(\"\");",
+            "    xs[0] = null;",
+            "    System.err.println(xs[0]);",
+            "    String[] pieces = Pattern.compile(\":\").split(\"\");",
+            "    for (int i = 0; i < pieces.length; i++) {}",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "import com.google.common.base.Splitter;",
+            "import com.google.common.collect.Iterables;",
+            "import java.util.ArrayList;",
+            "import java.util.List;",
+            "import java.util.regex.Pattern;",
+            "",
+            "class Test {",
+            "  void f() {",
+            "    String x = Iterables.get(Splitter.on(Pattern.compile(\"\")).split(\"c\"), 0);",
+            "    for (String s : Splitter.on(Pattern.compile(\"\")).split(\":\")) {}",
+            "    List<String> xs ="
+                + " new ArrayList<>(Splitter.on(Pattern.compile(\"c\")).splitToList(\"\"));",
+            "    xs.set(0, null);",
+            "    System.err.println(xs.get(0));",
+            "    List<String> pieces = Splitter.on(Pattern.compile(\":\")).splitToList(\"\");",
+            "    for (int i = 0; i < pieces.size(); i++) {}",
+            "  }",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
 }
diff --git a/docs/bugpattern/StringSplitter.md b/docs/bugpattern/StringSplitter.md
@@ -1,5 +1,5 @@
-`String.split(String)` has surprising behaviour. For example, consider the
-following puzzler from
+`String.split(String)` and `Pattern.split(CharSequence) have surprising
+behaviour. For example, consider the following puzzler from
 http://konigsberg.blogspot.com/2009/11/final-thoughts-java-puzzler-splitting.html:
 
 ```java
@@ -11,13 +11,13 @@ The result is `[""]` and `[]`!
 
 More examples:
 
-input    | `input.split(":")`  | `Splitter.on(':').split(input)`
--------- | ------------------- | -------------------------------
-`""`     | `[""]`              | `[""]`
-`":"`    | `[]`                | `["", ""]`
-`":::"`  | `[]`                | `["", "", "", ""]`
-`"a:::"` | `["a"]`             | `["a", "", "", ""]`
-`":::b"` | `["", "", "", "b"]` | `["", "", "", "b"]`
+input    | `input.split(":")`  | `Pattern.compile(":").split(input) | `Splitter.on(':').split(input)`
+-------- | ------------------- | ---------------------------------- | -------------------------------
+`""`     | `[""]`              | `[""]`                             | `[""]`
+`":"`    | `[]`                | `[]`                               | `["", ""]`
+`":::"`  | `[]`                | `[]`                               | `["", "", "", ""]`
+`"a:::"` | `["a"]`             | `["a"]`                            | `["a", "", "", ""]`
+`":::b"` | `["", "", "", "b"]` | `["", "", "", "b"]`                | `["", "", "", "b"]`
 
 Prefer either:
 
@@ -27,7 +27,9 @@ Prefer either:
     handling of empty strings and the trimming of whitespace with `trimResults`
     and `omitEmptyStrings`.
 
-*   [`String.split(String, int)`](https://docs.oracle.com/javase/9/docs/api/java/lang/String.html#split-java.lang.String-int-)
+*   [`String.split(String, int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#split\(java.lang.String,int\))
+    or
+    [`Pattern.split(CharSequence, int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#split\(java.lang.String,int\))
     and setting an explicit 'limit' to `-1` to match the behaviour of
     `Splitter`.
 
