Skip to content

Commit

Permalink
Discourage looping over the result of toCharArray()
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 387181359
  • Loading branch information
cushon authored and Error Prone Team committed Jul 27, 2021
1 parent 4e91cb4 commit 6a63b57
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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];
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -852,6 +853,7 @@ public static ScannerSupplier errorChecks() {
LockOnBoxedPrimitive.class,
LogicalAssignment.class,
LongFloatConversion.class,
LoopOverCharArray.class,
MathAbsoluteRandom.class,
MissingCasesInEnumSwitch.class,
MissingFail.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
43 changes: 43 additions & 0 deletions docs/bugpattern/LoopOverCharArray.md
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit 6a63b57

Please sign in to comment.