Merge StringSplit and StringSplitter

In StringSplitter, warn about all uses of split(String) (not just ones that can be trivially migrated to Splitter), and only emit a suffested fix if Splitter is already on the classpath. Fixes #899 RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=186067129
google · Feb 20, 2018 · 4de34eb · 4de34eb
1 parent d7cc532
commit 4de34eb
Show file tree

Hide file tree

Showing 6 changed files with 62 additions and 121 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringSplit.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringSplit.java
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringSplitter.java
@@ -27,6 +27,7 @@
 import com.google.errorprone.BugPattern.ProvidesFix;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
@@ -67,6 +68,20 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     if (!MATCHER.matches(tree, state)) {
       return NO_MATCH;
     }
+    Description.Builder description = buildDescription(tree);
+    Optional<Fix> fix = buildFix(tree, state);
+    if (!fix.isPresent()) {
+      return NO_MATCH;
+    }
+    if (state.getTypeFromString("com.google.common.base.Splitter") != null) {
+      description.addFix(fix.get());
+    } else {
+      description.addFix(SuggestedFix.postfixWith(getOnlyElement(tree.getArguments()), ", -1"));
+    }
+    return description.build();
+  }
+
+  public Optional<Fix> buildFix(MethodInvocationTree tree, VisitorState state) {
     Tree arg = getOnlyElement(tree.getArguments());
     String value = ASTHelpers.constValue(arg, String.class);
     boolean maybeRegex = false;
@@ -90,8 +105,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     if (parent instanceof EnhancedForLoopTree
         && ((EnhancedForLoopTree) parent).getExpression().equals(tree)) {
       // fix for `for (... : s.split(...)) {}` -> `for (... : Splitter.on(...).split(s)) {}`
-      return describeMatch(
-          tree,
+      return Optional.of(
           replaceWithSplitter(
                   SuggestedFix.builder(),
                   tree,
@@ -105,7 +119,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     if (parent instanceof ArrayAccessTree) {
       ArrayAccessTree arrayAccessTree = (ArrayAccessTree) parent;
       if (!arrayAccessTree.getExpression().equals(tree)) {
-        return NO_MATCH;
+        return Optional.empty();
       }
       SuggestedFix.Builder fix =
           SuggestedFix.builder()
@@ -115,32 +129,31 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
                   ((JCTree) arrayAccessTree).getStartPosition(),
                   "Iterables.get(")
               .replace(
-                  state.getEndPosition(arrayAccessTree.getExpression()),
-                  ((JCTree) arrayAccessTree.getIndex()).getStartPosition(),
+                  /* startPos= */ state.getEndPosition(arrayAccessTree.getExpression()),
+                  /* endPos= */ ((JCTree) arrayAccessTree.getIndex()).getStartPosition(),
                   String.format(", "))
               .replace(
                   state.getEndPosition(arrayAccessTree.getIndex()),
                   state.getEndPosition(arrayAccessTree),
                   ")");
-      return describeMatch(
-          tree,
+      return Optional.of(
           replaceWithSplitter(
                   fix, tree, value, state, "split", maybeRegex, /* 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.
     if (!(parent instanceof VariableTree)) {
-      return NO_MATCH;
+      return Optional.empty();
     }
     VariableTree varTree = (VariableTree) parent;
     if (!varTree.getInitializer().equals(tree)) {
-      return NO_MATCH;
+      return Optional.empty();
     }
     VarSymbol sym = ASTHelpers.getSymbol(varTree);
     TreePath enclosing = findEnclosing(state);
     if (enclosing == null) {
-      return NO_MATCH;
+      return Optional.empty();
     }
     // find all uses of the variable in the enclosing method
     List<TreePath> uses = new ArrayList<>();
@@ -214,7 +227,7 @@ public Boolean visitMemberSelect(MemberSelectTree tree, Void aVoid) {
         }
       }
       if (!firstNonNull(new UseFixer().scan(path.getParentPath(), null), false)) {
-        return NO_MATCH;
+        return Optional.empty();
       }
     }
     if (needsList[0]) {
@@ -224,7 +237,7 @@ public Boolean visitMemberSelect(MemberSelectTree tree, Void aVoid) {
       fix.replace((varTree).getType(), "Iterable<String>");
       replaceWithSplitter(fix, tree, value, state, "split", maybeRegex, needsMutableList[0]);
     }
-    return describeMatch(tree, fix.build());
+    return Optional.of(fix.build());
   }
 
   private SuggestedFix.Builder replaceWithSplitter(

diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -198,7 +198,6 @@
 import com.google.errorprone.bugpatterns.StreamToString;
 import com.google.errorprone.bugpatterns.StringBuilderInitWithChar;
 import com.google.errorprone.bugpatterns.StringEquality;
-import com.google.errorprone.bugpatterns.StringSplit;
 import com.google.errorprone.bugpatterns.StringSplitter;
 import com.google.errorprone.bugpatterns.SuppressWarningsDeprecated;
 import com.google.errorprone.bugpatterns.SwitchDefault;
@@ -605,7 +604,6 @@ public static ScannerSupplier errorChecks() {
           StaticQualifiedUsingExpression.class,
           StaticOrDefaultInterfaceMethod.class,
           StringEquality.class,
-          StringSplit.class,
           SwitchDefault.class,
           TestExceptionChecker.class,
           ThrowsUncheckedException.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitTest.java
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitterTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringSplitterTest.java
@@ -278,4 +278,39 @@ public void immediateArrayAccess() throws IOException {
             "}")
         .doTest();
   }
+
+  @Test
+  public void testStringSplitPositive() throws Exception {
+    CompilationTestHelper.newInstance(StringSplitter.class, getClass())
+        .addSourceFile("StringSplitPositiveCases.java")
+        .doTest();
+  }
+
+  @Test
+  public void testStringSplitNegative() throws Exception {
+    CompilationTestHelper.newInstance(StringSplitter.class, getClass())
+        .addSourceFile("StringSplitNegativeCases.java")
+        .doTest();
+  }
+
+  @Test
+  public void noSplitterOnClassPath() throws IOException {
+    testHelper
+        .addInputLines(
+            "in/Test.java",
+            "class Test {",
+            "  void f() {",
+            "    for (String s : \"\".split(\":\")) {}",
+            "  }",
+            "}")
+        .addOutputLines(
+            "out/Test.java",
+            "class Test {",
+            "  void f() {",
+            "    for (String s : \"\".split(\":\", -1)) {}",
+            "  }",
+            "}")
+        .setArgs("-cp", ":")
+        .doTest(TestMode.TEXT_MATCH);
+  }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/StringSplitPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/StringSplitPositiveCases.java
@@ -25,7 +25,7 @@ public class StringSplitPositiveCases {
 
   public void StringSplitOneArg() {
     String foo = "a:b";
-    // BUG: Diagnostic contains: String.split
-    foo.split(":");
+    // BUG: Diagnostic contains:
+    String[] xs = foo.split(":");
   }
 }