From 6a63b5713958c51e4e8f7c135720b9e84fca9309 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 27 Jul 2021 13:20:41 -0700 Subject: [PATCH] Discourage looping over the result of `toCharArray()` PiperOrigin-RevId: 387181359 --- .../bugpatterns/LoopOverCharArray.java | 95 +++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/LoopOverCharArrayTest.java | 57 +++++++++++ docs/bugpattern/LoopOverCharArray.md | 43 +++++++++ 4 files changed, 197 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/LoopOverCharArray.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/LoopOverCharArrayTest.java create mode 100644 docs/bugpattern/LoopOverCharArray.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LoopOverCharArray.java b/core/src/main/java/com/google/errorprone/bugpatterns/LoopOverCharArray.java new file mode 100644 index 00000000000..4d5ccf71ee0 --- /dev/null +++ b/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 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 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() { + @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]; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 91fc44a1ff3..f75ba7bb256 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/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, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/LoopOverCharArrayTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/LoopOverCharArrayTest.java new file mode 100644 index 00000000000..72e7f3d387a --- /dev/null +++ b/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(); + } +} diff --git a/docs/bugpattern/LoopOverCharArray.md b/docs/bugpattern/LoopOverCharArray.md new file mode 100644 index 00000000000..e0e8d64f448 --- /dev/null +++ b/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()