diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarySubstring.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarySubstring.java new file mode 100644 index 00000000000..95231f0daf3 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarySubstring.java @@ -0,0 +1,167 @@ +/* + * Copyright 2026 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.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static com.google.errorprone.util.ASTHelpers.matchingMethods; +import static java.lang.String.format; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.SideEffectAnalysis; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.List; + +/** + * Flags {@code Integer.parseInt(s.substring(begin, end))} and friends, which allocate a copy of the + * region only to throw it away after parsing. + */ +@BugPattern( + summary = + "Parsing a substring allocates a copy of the region; the (CharSequence, int, int, int)" + + " overload parses it in place", + explanation = + "`Integer.parseInt(String)` and its siblings force callers to materialize the region they" + + " want to parse. Since JDK 9 each has a `(CharSequence, int, int, int)` overload that" + + " reads the region directly out of the original sequence, so the copy is pure" + + " overhead.\n\n" + + "The rewrite preserves the parsed value in every case. It does change two things" + + " about failures: an out-of-range index throws `IndexOutOfBoundsException` rather" + + " than its subclass `StringIndexOutOfBoundsException`, and the" + + " `NumberFormatException` message describes the region rather than the copy.\n\n" + + "A null target keeps throwing `NullPointerException`, because the overload opens" + + " with `Objects.requireNonNull`. That is specific to the parse overloads: the" + + " analogous `Appendable.append(CharSequence, int, int)` substitutes the string" + + " `\"null\"` and then applies the region to it, so extending this check to appends" + + " would silently turn a null dereference into truncated output.", + severity = WARNING, + tags = StandardTags.PERFORMANCE) +public final class UnnecessarySubstring extends BugChecker implements MethodInvocationTreeMatcher { + + /** + * Parses that have a {@code (CharSequence, int, int, int)} counterpart. {@code Integer.valueOf} + * and {@code Double.parseDouble} deliberately absent: they have no such overload. + */ + private static final Matcher PARSE = + staticMethod() + .onClassAny("java.lang.Integer", "java.lang.Long") + .namedAnyOf("parseInt", "parseUnsignedInt", "parseLong", "parseUnsignedLong"); + + private static final Matcher SUBSTRING = + instanceMethod() + .onExactClassAny("java.lang.String", "java.lang.StringBuilder", "java.lang.StringBuffer") + .named("substring"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!PARSE.matches(tree, state)) { + return NO_MATCH; + } + // The (String) and (String, int) overloads; the region-parsing overload takes four arguments. + List args = tree.getArguments(); + if (args.size() != 1 && args.size() != 2) { + return NO_MATCH; + } + if (!(args.get(0) instanceof MethodInvocationTree substring) + || !SUBSTRING.matches(substring, state)) { + return NO_MATCH; + } + if (!hasRegionParsingOverload(getSymbol(tree), state)) { + return NO_MATCH; + } + ExpressionTree target = getReceiver(substring); + if (target == null) { + return NO_MATCH; + } + + List bounds = substring.getArguments(); + String begin = state.getSourceForNode(bounds.get(0)); + String end; + if (bounds.size() == 2) { + end = state.getSourceForNode(bounds.get(1)); + } else { + // substring(begin) runs to the end, which the region overload needs spelled out. That repeats + // the target expression, so only rewrite when repeating it is free of consequence. + if (!canRepeat(target)) { + return NO_MATCH; + } + end = format("%s.length()", state.getSourceForNode(target)); + } + String radix = args.size() == 2 ? state.getSourceForNode(args.get(1)) : "10"; + + // Reuse the method select verbatim so static imports and qualified names survive the rewrite. + return describeMatch( + tree, + SuggestedFix.replace( + tree, + format( + "%s(%s, %s, %s, %s)", + state.getSourceForNode(tree.getMethodSelect()), + state.getSourceForNode(target), + begin, + end, + radix))); + } + + /** + * Whether {@code target} can be written twice: it must be side-effect free, and it must bind + * tightly enough that appending {@code .length()} still applies to the whole expression. + */ + private static boolean canRepeat(ExpressionTree target) { + return switch (target.getKind()) { + case IDENTIFIER, STRING_LITERAL -> true; + // `a.b.c` and `a[i]` qualify, but `a.b().c` and `a[f()]` do not. + case MEMBER_SELECT, ARRAY_ACCESS -> !SideEffectAnalysis.hasSideEffect(target); + default -> false; + }; + } + + /** + * Whether the class being parsed into declares the region-parsing overload. It arrived in JDK 9, + * so this is absent when compiling against an older platform. + */ + private static boolean hasRegionParsingOverload(MethodSymbol parse, VisitorState state) { + Type charSequence = state.getTypeFromString("java.lang.CharSequence"); + if (charSequence == null) { + return false; + } + return matchingMethods( + parse.name, + candidate -> + candidate.isStatic() + && candidate.params().size() == 4 + && isSameType(candidate.params().get(0).type, charSequence, state), + parse.owner.type, + state.getTypes()) + .findAny() + .isPresent(); + } +} 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 78501e4a057..f704c152422 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -467,6 +467,7 @@ import com.google.errorprone.bugpatterns.UnnecessarySetDefault; import com.google.errorprone.bugpatterns.UnnecessaryStaticImport; import com.google.errorprone.bugpatterns.UnnecessaryStringBuilder; +import com.google.errorprone.bugpatterns.UnnecessarySubstring; import com.google.errorprone.bugpatterns.UnnecessaryTestMethodPrefix; import com.google.errorprone.bugpatterns.UnnecessaryTypeArgument; import com.google.errorprone.bugpatterns.UnsafeFinalization; @@ -1213,6 +1214,7 @@ public static ScannerSupplier warningChecks() { UnnecessaryParentheses.class, UnnecessaryQualifier.class, UnnecessaryStringBuilder.class, + UnnecessarySubstring.class, UnrecognisedJavadocTag.class, UnsafeFinalization.class, UnsafeReflectiveConstructionCast.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarySubstringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarySubstringTest.java new file mode 100644 index 00000000000..e7601f5817f --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarySubstringTest.java @@ -0,0 +1,385 @@ +/* + * Copyright 2026 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link UnnecessarySubstring}. */ +@RunWith(JUnit4.class) +public class UnnecessarySubstringTest { + private final BugCheckerRefactoringTestHelper refactoring = + BugCheckerRefactoringTestHelper.newInstance(UnnecessarySubstring.class, getClass()); + private final CompilationTestHelper compilation = + CompilationTestHelper.newInstance(UnnecessarySubstring.class, getClass()); + + @Test + public void twoArgSubstring() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + int f(String s, int begin, int end) { + return Integer.parseInt(s.substring(begin, end)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + int f(String s, int begin, int end) { + return Integer.parseInt(s, begin, end, 10); + } + } + """) + .doTest(); + } + + @Test + public void oneArgSubstring() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + int f(String s) { + return Integer.parseInt(s.substring(1)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + int f(String s) { + return Integer.parseInt(s, 1, s.length(), 10); + } + } + """) + .doTest(); + } + + @Test + public void explicitRadixIsPreserved() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + int f(String s, int radix) { + return Integer.parseInt(s.substring(2, 4), radix); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + int f(String s, int radix) { + return Integer.parseInt(s, 2, 4, radix); + } + } + """) + .doTest(); + } + + @Test + public void allParseFlavours() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + void f(String s) { + Integer.parseInt(s.substring(1, 2)); + Integer.parseUnsignedInt(s.substring(1, 2)); + Long.parseLong(s.substring(1, 2)); + Long.parseUnsignedLong(s.substring(1, 2)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + void f(String s) { + Integer.parseInt(s, 1, 2, 10); + Integer.parseUnsignedInt(s, 1, 2, 10); + Long.parseLong(s, 1, 2, 10); + Long.parseUnsignedLong(s, 1, 2, 10); + } + } + """) + .doTest(); + } + + @Test + public void staticImportIsPreserved() { + refactoring + .addInputLines( + "Test.java", + """ + import static java.lang.Integer.parseInt; + + class Test { + int f(String s) { + return parseInt(s.substring(1, 3)); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.lang.Integer.parseInt; + + class Test { + int f(String s) { + return parseInt(s, 1, 3, 10); + } + } + """) + .doTest(); + } + + @Test + public void charSequenceReceivers() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + void f(StringBuilder sb, StringBuffer buf) { + Integer.parseInt(sb.substring(1, 2)); + Integer.parseInt(buf.substring(1, 2)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + void f(StringBuilder sb, StringBuffer buf) { + Integer.parseInt(sb, 1, 2, 10); + Integer.parseInt(buf, 1, 2, 10); + } + } + """) + .doTest(); + } + + @Test + public void compoundBoundExpressions() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + int f(String s, int i) { + return Integer.parseInt(s.substring(i + 1, s.indexOf(',')), i > 0 ? 16 : 10); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + int f(String s, int i) { + return Integer.parseInt(s, i + 1, s.indexOf(','), i > 0 ? 16 : 10); + } + } + """) + .doTest(); + } + + @Test + public void oneArgSubstringOnFieldOrArray() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + String field = ""; + + void f(String[] parts, int i) { + Integer.parseInt(this.field.substring(1)); + Integer.parseInt(parts[i].substring(1)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String field = ""; + + void f(String[] parts, int i) { + Integer.parseInt(this.field, 1, this.field.length(), 10); + Integer.parseInt(parts[i], 1, parts[i].length(), 10); + } + } + """) + .doTest(); + } + + /** Repeating the target would call {@code g()} twice, so the one-arg form is left alone. */ + @Test + public void negativeOneArgSubstringOnImpureTarget() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + String g() { + return ""; + } + + int f() { + return Integer.parseInt(g().substring(1)); + } + } + """) + .doTest(); + } + + /** Two-arg substring never repeats the target, so an impure target is still fine. */ + @Test + public void twoArgSubstringOnImpureTarget() { + refactoring + .addInputLines( + "Test.java", + """ + class Test { + String g() { + return ""; + } + + int f() { + return Integer.parseInt(g().substring(1, 3)); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + String g() { + return ""; + } + + int f() { + return Integer.parseInt(g(), 1, 3, 10); + } + } + """) + .doTest(); + } + + /** Appending {@code .length()} to a ternary would bind to the wrong operand. */ + @Test + public void negativeOneArgSubstringOnTernary() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + int f(boolean b, String x, String y) { + return Integer.parseInt((b ? x : y).substring(1)); + } + } + """) + .doTest(); + } + + /** These have no {@code (CharSequence, int, int, int)} overload. */ + @Test + public void negativeParsesWithoutRegionOverload() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + void f(String s) { + Double.parseDouble(s.substring(1, 3)); + Float.parseFloat(s.substring(1, 3)); + Boolean.parseBoolean(s.substring(1, 3)); + Integer.valueOf(s.substring(1, 3)); + Integer.decode(s.substring(1, 3)); + Short.parseShort(s.substring(1, 3)); + Byte.parseByte(s.substring(1, 3)); + } + } + """) + .doTest(); + } + + @Test + public void negativeAlreadyUsesRegionOverload() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + int f(String s) { + return Integer.parseInt(s, 1, 3, 10); + } + } + """) + .doTest(); + } + + @Test + public void negativeArgumentIsNotASubstring() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + int f(String s) { + return Integer.parseInt(s.trim()); + } + } + """) + .doTest(); + } + + /** {@code substring} on an unrelated type returns something the overload cannot consume. */ + @Test + public void negativeUnrelatedSubstringMethod() { + compilation + .addSourceLines( + "Test.java", + """ + class Test { + static class Rope { + String substring(int begin, int end) { + return ""; + } + } + + int f(Rope rope) { + return Integer.parseInt(rope.substring(1, 3)); + } + } + """) + .doTest(); + } +}