Discourage looping over the result of toCharArray()

PiperOrigin-RevId: 387181359

Author: cushon

core/src/main/java/com/google/errorprone/bugpatterns/LoopOverCharArray.java

@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+import static com.google.errorprone.util.ASTHelpers.getStartPosition;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+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.sun.source.tree.BlockTree;
+import com.sun.source.tree.EnhancedForLoopTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.util.TreeScanner;
+import java.util.List;
+
+/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
+@BugPattern(
+    name = "LoopOverCharArray",
+    summary = "toCharArray allocates a new array, using charAt is more efficient",
+    severity = WARNING)
+public class LoopOverCharArray extends BugChecker implements BugChecker.EnhancedForLoopTreeMatcher {
+
+  private static final Matcher<ExpressionTree> TO_CHAR_ARRAY =
+      instanceMethod().onExactClass("java.lang.String").named("toCharArray");
+
+  @Override
+  public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState state) {
+    if (!TO_CHAR_ARRAY.matches(tree.getExpression(), state)) {
+      return NO_MATCH;
+    }
+    ExpressionTree receiver = ASTHelpers.getReceiver(tree.getExpression());
+    if (!(receiver instanceof IdentifierTree)) {
+      return NO_MATCH;
+    }
+    StatementTree body = tree.getStatement();
+    if (!(body instanceof BlockTree)) {
+      return NO_MATCH;
+    }
+    List<? extends StatementTree> statements = ((BlockTree) body).getStatements();
+    if (statements.isEmpty()) {
+      return NO_MATCH;
+    }
+    Description.Builder description = buildDescription(tree);
+    // The fix uses `i` as the loop index variable, so give up if there's already an `i` in scope
+    if (!alreadyDefinesIdentifier(tree)) {
+      description.addFix(
+          SuggestedFix.replace(
+              getStartPosition(tree),
+              getStartPosition(statements.get(0)),
+              String.format(
+                  "for (int i = 0; i < %s.length(); i++) { char %s = %s.charAt(i);",
+                  state.getSourceForNode(receiver),
+                  tree.getVariable().getName(),
+                  state.getSourceForNode(receiver))));
+    }
+    return description.build();
+  }
+
+  private static boolean alreadyDefinesIdentifier(Tree tree) {
+    boolean[] result = {false};
+    new TreeScanner<Void, Void>() {
+      @Override
+      public Void visitIdentifier(IdentifierTree node, Void unused) {
+        if (node.getName().contentEquals("i")) {
+          result[0] = true;
+        }
+        return super.visitIdentifier(node, unused);
+      }
+    }.scan(tree, null);
+    return result[0];
+  }
+}

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -195,6 +195,7 @@
 import com.google.errorprone.bugpatterns.LongFloatConversion;
 import com.google.errorprone.bugpatterns.LongLiteralLowerCaseSuffix;
 import com.google.errorprone.bugpatterns.LoopConditionChecker;
+import com.google.errorprone.bugpatterns.LoopOverCharArray;
 import com.google.errorprone.bugpatterns.LossyPrimitiveCompare;
 import com.google.errorprone.bugpatterns.MathAbsoluteRandom;
 import com.google.errorprone.bugpatterns.MathRoundIntLong;
@@ -852,6 +853,7 @@ public static ScannerSupplier errorChecks() {
           LockOnBoxedPrimitive.class,
           LogicalAssignment.class,
           LongFloatConversion.class,
+          LoopOverCharArray.class,
           MathAbsoluteRandom.class,
           MissingCasesInEnumSwitch.class,
           MissingFail.class,

core/src/test/java/com/google/errorprone/bugpatterns/LoopOverCharArrayTest.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2021 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** {@link LoopOverCharArray}Test */
+@RunWith(JUnit4.class)
+public class LoopOverCharArrayTest {
+  @Test
+  public void refactor() {
+    BugCheckerRefactoringTestHelper.newInstance(LoopOverCharArray.class, getClass())
+        .addInputLines(
+            "Test.java",
+            "class T {",
+            "  void f(String s) {",
+            "    for (char c : s.toCharArray()) {",
+            "      System.err.print(c);",
+            "    }",
+            "    for (char i : s.toCharArray()) {",
+            "      System.err.print(i);",
+            "    }",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "class T {",
+            "  void f(String s) {",
+            "    for (int i = 0; i < s.length(); i++) {",
+            "      char c = s.charAt(i);",
+            "      System.err.print(c);",
+            "    }",
+            "    for (char i : s.toCharArray()) {",
+            "      System.err.print(i);",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
+}

docs/bugpattern/LoopOverCharArray.md

@@ -0,0 +1,43 @@
+[`String#toCharArray`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#toCharArray\(\))
+allocates a new array. Calling `charAt` is more efficient, because it avoids
+creating a new array with a copy of the character data.
+
+That is, prefer this:
+
+```java
+boolean isDigits(String string) {
+  for (int i = 0; i < s.length(); i++) {
+    char c = s.charAt(i);
+    if (!Character.isDigit(c)) {
+      return false;
+    }
+  }
+  return true;
+}
+```
+
+to this:
+
+```java
+boolean isDigits(String string) {
+  // this allocates a new char[]
+  for (char c : string.toCharArray()) {
+    if (!Character.isDigit(c)) {
+      return false;
+    }
+  }
+  return true;
+}
+```
+
+Note that many loops over characters can be expressed using streams with
+[`String#chars`][chars] or [`String#codePoints`][codePoints], for example:
+
+```java
+boolean isDigits(String string) {
+  string.codePoints().allMatch(Character::isDigit);
+}
+```
+
+[chars]: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#chars()
+[codePoints]: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#codePoints()