diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S9142.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9142.json new file mode 100644 index 00000000000..c7d0a63969c --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9142.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S9142", + "hasTruePositives": true, + "falseNegatives": 0, + "falsePositives": 0 +} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json new file mode 100644 index 00000000000..9e26dfeeb6e --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S9142.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/its/ruling/src/test/resources/sonar-server/java-S9142.json b/its/ruling/src/test/resources/sonar-server/java-S9142.json new file mode 100644 index 00000000000..9e26dfeeb6e --- /dev/null +++ b/its/ruling/src/test/resources/sonar-server/java-S9142.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/CompilationOrPreparationInLoopCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/CompilationOrPreparationInLoopCheckSample.java new file mode 100644 index 00000000000..4696432851c --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/CompilationOrPreparationInLoopCheckSample.java @@ -0,0 +1,12 @@ +package checks; + +import java.util.List; +import java.util.regex.Pattern; + +class CompilationOrPreparationInLoopCheckSampleNonCompiling { + void test(List inputs) { + for (String input : inputs) { + Pattern.compile(unknownVar).matcher(input).find(); // Compliant - unresolved symbol is not a variable symbol + } + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/CompilationOrPreparationInLoopCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/CompilationOrPreparationInLoopCheckSample.java new file mode 100644 index 00000000000..05fc87c75ca --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/CompilationOrPreparationInLoopCheckSample.java @@ -0,0 +1,140 @@ +package checks; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.SQLException; +import java.util.List; +import java.util.regex.Pattern; + +class CompilationOrPreparationInLoopCheckSample { + + private static final String CONSTANT_PATTERN = "[a-z]+"; + private String mutablePattern = "[a-z]+"; + + void patternCompileNoncompliant(List inputs) { + for (String input : inputs) { + Pattern.compile("[a-z]+").matcher(input).find(); // Noncompliant {{Move this "compile" call outside the loop.}} + //^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + int i = 0; + while (i++ < inputs.size()) { + Pattern.compile("[a-z]+"); // Noncompliant + } + + for (String input : inputs) { + Pattern.compile(CONSTANT_PATTERN).matcher(input).find(); // Noncompliant + } + + String invariantPattern = "[a-z]+"; + for (String input : inputs) { + Pattern.compile(invariantPattern).matcher(input).find(); // Noncompliant + } + + for (String input : inputs) { + Pattern.compile("[a-z]+", Pattern.CASE_INSENSITIVE); // Noncompliant + int flags = input.isEmpty() ? 0 : Pattern.CASE_INSENSITIVE; + Pattern.compile("[a-z]+", flags); // Compliant: flags vary + } + } + + void stringMethodsNoncompliant(List inputs) { + for (String input : inputs) { + input.matches("[a-z]+"); // Noncompliant {{Extract this regular expression to a Pattern compiled outside the loop.}} + input.replaceAll("[a-z]+", "X"); // Noncompliant + input.replaceFirst("[a-z]+", "X"); // Noncompliant + input.split("[,;]"); // Noncompliant + input.split("."); // Noncompliant + input.split("\\a"); // Noncompliant + input.split(","); // Compliant: single non-metacharacter fast path + input.split("\\."); // Compliant: escaped non-alphanumeric fast path + } + } + + void prepareStatementNoncompliant(Connection conn, List ids) throws SQLException { + for (int id : ids) { + PreparedStatement ps = conn.prepareStatement("SELECT * FROM t WHERE id = ?"); // Noncompliant + ps.setInt(1, id); + ps.execute(); + ps.close(); + } + } + + void forInitializer(String s) { + for (Pattern p = Pattern.compile("[a-z]+"); p.matcher(s).find(); ) { // Compliant: initializer runs once + break; + } + for (; Pattern.compile("[a-z]+").matcher(s).find(); ) { // Noncompliant + break; + } + } + + void compliant(List inputs, Connection conn, List ids) throws SQLException { + Pattern p = Pattern.compile("[a-z]+"); + for (String input : inputs) { + p.matcher(input).find(); + } + + for (String input : inputs) { + input.toLowerCase(); // not a regex method + } + + PreparedStatement ps = conn.prepareStatement("SELECT * FROM t WHERE id = ?"); + for (int id : ids) { + ps.setInt(1, id); + ps.execute(); + } + } + + void patternVariesPerIteration(List patterns, List inputs) { + for (int i = 0; i < inputs.size(); i++) { + String pattern = patterns.get(i); + Pattern.compile(pattern).matcher(inputs.get(i)).find(); // Compliant - pattern changes per iteration + } + } + + void mutableFieldPattern(List inputs) { + for (String input : inputs) { + Pattern.compile(mutablePattern).matcher(input).find(); // Compliant - non-final field may be mutated via member select or method call + } + } + + void localReassignedInLoop(List inputs) { + String pattern = "[a-z]+"; + for (String input : inputs) { + pattern = input; // reassigned each iteration + Pattern.compile(pattern).matcher(input).find(); // Compliant - pattern changes per iteration + } + } + + void doWhileLoop(List inputs) { + int i = 0; + do { + Pattern.compile("[a-z]+").matcher(inputs.get(i)).find(); // Noncompliant + } while (i++ < inputs.size()); + } + + void nonConstantArg(List inputs) { + for (String input : inputs) { + Pattern.compile(input.trim()).matcher(input).find(); // Compliant - method call result is not a constant + } + } + + void nonIncrementUnaryInLoop(List inputs) { + boolean inverse = false; + for (String input : inputs) { + if (!inverse) { // non-increment unary expression + Pattern.compile("[a-z]+").matcher(input).find(); // Noncompliant + } + } + } + + void nonIdentifierMutationsInLoop(List inputs) { + int[] counters = new int[2]; + for (String input : inputs) { + counters[0] = input.length(); // assignment to non-identifier target + counters[1]++; // increment on non-identifier target + Pattern.compile("[a-z]+").matcher(input).find(); // Noncompliant + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheck.java b/java-checks/src/main/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheck.java new file mode 100644 index 00000000000..a997dd8776c --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheck.java @@ -0,0 +1,208 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.Collections; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import org.apache.commons.lang3.StringEscapeUtils; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.TreeHelper; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.ForEachStatement; +import org.sonar.plugins.java.api.tree.ForStatementTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.UnaryExpressionTree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S9142") +public class CompilationOrPreparationInLoopCheck extends IssuableSubscriptionVisitor { + + private static final String STRING_REGEX_MESSAGE = + "Extract this regular expression to a Pattern compiled outside the loop."; + + private static final Set LOOP_KINDS = EnumSet.of( + Tree.Kind.FOR_STATEMENT, Tree.Kind.FOR_EACH_STATEMENT, + Tree.Kind.WHILE_STATEMENT, Tree.Kind.DO_STATEMENT + ); + + private static final MethodMatchers PATTERN_COMPILE = MethodMatchers.create() + .ofTypes("java.util.regex.Pattern") + .names("compile") + .withAnyParameters() + .build(); + + private static final MethodMatchers STRING_REGEX_METHODS = MethodMatchers.create() + .ofTypes("java.lang.String") + .names("matches", "replaceAll", "replaceFirst", "split") + .withAnyParameters() + .build(); + + private static final MethodMatchers SPLIT = MethodMatchers.create() + .ofTypes("java.lang.String") + .names("split") + .withAnyParameters() + .build(); + + private static final MethodMatchers MATCHERS = MethodMatchers.or( + PATTERN_COMPILE, + STRING_REGEX_METHODS, + MethodMatchers.create() + .ofSubTypes("java.sql.Connection") + .names("prepareStatement") + .withAnyParameters() + .build() + ); + + @Override + public List nodesToVisit() { + return Collections.singletonList(Tree.Kind.METHOD_INVOCATION); + } + + @Override + public void visitNode(Tree tree) { + MethodInvocationTree mit = (MethodInvocationTree) tree; + if (!MATCHERS.matches(mit) || mit.arguments().isEmpty()) { + return; + } + Tree loop = TreeHelper.findClosestParentOfKind(mit, LOOP_KINDS); + if (loop == null || isInForInitializer(mit, loop)) { + return; + } + if (SPLIT.matches(mit) && isSplitFastPath(mit.arguments().get(0))) { + return; + } + List argsToCheck = PATTERN_COMPILE.matches(mit) ? mit.arguments() : List.of(mit.arguments().get(0)); + if (argsToCheck.stream().allMatch(arg -> isLoopInvariant(arg, loop))) { + reportIssue(mit, message(mit)); + } + } + + private static String message(MethodInvocationTree mit) { + if (STRING_REGEX_METHODS.matches(mit)) { + return STRING_REGEX_MESSAGE; + } + return String.format("Move this \"%s\" call outside the loop.", ExpressionUtils.methodName(mit).name()); + } + + private static boolean isInForInitializer(Tree tree, Tree loop) { + if (!loop.is(Tree.Kind.FOR_STATEMENT)) { + return false; + } + Tree initializer = ((ForStatementTree) loop).initializer(); + for (Tree current = tree; current != null && current != loop; current = current.parent()) { + if (current == initializer) { + return true; + } + } + return false; + } + + private static boolean isSplitFastPath(ExpressionTree arg) { + return ExpressionUtils.skipParentheses(arg).asConstant(String.class) + .filter(CompilationOrPreparationInLoopCheck::exceptionSplitMethod) + .isPresent(); + } + + /** + * Copy of {@link java.lang.String#split(String, int)} fast-path, matching {@link RegexPatternsNeedlesslyCheck}. + */ + private static boolean exceptionSplitMethod(String argValue) { + String regex = StringEscapeUtils.unescapeJava(argValue); + char ch; + if (regex.length() == 1) { + ch = regex.charAt(0); + return ".$|()[{^?*+\\".indexOf(ch) == -1 && + (ch < Character.MIN_HIGH_SURROGATE || ch > Character.MAX_LOW_SURROGATE); + } + if (regex.length() == 2 && regex.charAt(0) == '\\') { + ch = regex.charAt(1); + return (((ch - '0') | ('9' - ch)) < 0 && + ((ch - 'a') | ('z' - ch)) < 0 && + ((ch - 'A') | ('Z' - ch)) < 0) && + (ch < Character.MIN_HIGH_SURROGATE || ch > Character.MAX_LOW_SURROGATE); + } + return false; + } + + private static boolean isLoopInvariant(ExpressionTree arg, Tree loop) { + ExpressionTree expression = ExpressionUtils.skipParentheses(arg); + if (expression.is(Tree.Kind.IDENTIFIER)) { + Symbol symbol = ((IdentifierTree) expression).symbol(); + if (!symbol.isVariableSymbol()) { + return false; + } + if (symbol.owner().isTypeSymbol()) { + return symbol.isFinal(); + } + var collector = new DeclaredOrAssignedLocalsCollector(); + loop.accept(collector); + return !collector.names.contains(((IdentifierTree) expression).name()); + } + return ExpressionUtils.resolveAsConstant(expression) != null; + } + + private static class DeclaredOrAssignedLocalsCollector extends BaseTreeVisitor { + + final Set names = new HashSet<>(); + + @Override + public void visitVariable(VariableTree tree) { + super.visitVariable(tree); + names.add(tree.simpleName().name()); + } + + @Override + public void visitAssignmentExpression(AssignmentExpressionTree tree) { + super.visitAssignmentExpression(tree); + if (tree.variable().is(Tree.Kind.IDENTIFIER)) { + names.add(((IdentifierTree) tree.variable()).name()); + } + } + + @Override + public void visitUnaryExpression(UnaryExpressionTree tree) { + super.visitUnaryExpression(tree); + switch (tree.kind()) { + case POSTFIX_INCREMENT, POSTFIX_DECREMENT, PREFIX_INCREMENT, PREFIX_DECREMENT -> { + if (tree.expression().is(Tree.Kind.IDENTIFIER)) { + names.add(((IdentifierTree) tree.expression()).name()); + } + } + default -> { + // not a mutation + } + } + } + + @Override + public void visitForEachStatement(ForEachStatement tree) { + super.visitForEachStatement(tree); + names.add(tree.variable().simpleName().name()); + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheckTest.java new file mode 100644 index 00000000000..6067d2eca1d --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheckTest.java @@ -0,0 +1,52 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; + +class CompilationOrPreparationInLoopCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/CompilationOrPreparationInLoopCheckSample.java")) + .withCheck(new CompilationOrPreparationInLoopCheck()) + .verifyIssues(); + } + + @Test + void testNonCompiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/CompilationOrPreparationInLoopCheckSample.java")) + .withCheck(new CompilationOrPreparationInLoopCheck()) + .verifyNoIssues(); + } + + @Test + void testWithoutSemantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/CompilationOrPreparationInLoopCheckSample.java")) + .withCheck(new CompilationOrPreparationInLoopCheck()) + .withoutSemantic() + .verifyIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.html new file mode 100644 index 00000000000..1197214413b --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.html @@ -0,0 +1,134 @@ +

This is an issue when compilation or preparation methods are called inside loop bodies with constant or loop-invariant arguments. This includes +pattern compilation methods for regular expressions, string methods that accept regular expression patterns (such as match, replace, and split +operations), and database statement preparation methods.

+

In Java, this specifically refers to Pattern.compile(), String regex methods (matches(), replaceAll(), +replaceFirst(), split()), and Connection.prepareStatement().

+

Why is this an issue?

+

Compilation and preparation operations are expensive because they involve parsing, validation, and internal representation building. When these +operations are performed inside loops with constant or loop-invariant arguments, the same work is repeated unnecessarily on every iteration.

+

Regular expression compilation

+

When you call functions that compile regular expressions from strings or use string methods that accept regex patterns, the language runtime +must:

+
    +
  • Parse the regular expression syntax
    • +
  • Validate the pattern
    • +
  • Build an internal finite automaton (state machine)
    • +
  • Allocate memory for the compiled pattern
    • +
+

These steps happen every time, even when the pattern string is identical. For example, calling a string matching method with a pattern like +"\d+" inside a loop that processes 1,000 items means compiling the same pattern 1,000 times.

+

Database prepared statement preparation

+

When you call methods that create prepared statements from SQL strings, the database driver must:

+
    +
  • Send the SQL string to the database server
    • +
  • Parse and validate the SQL syntax
    • +
  • Create an execution plan
    • +
  • Allocate server-side resources (cursors, statement handles)
    • +
  • Return a client-side prepared statement object
    • +
+

Prepared statements exist specifically to avoid this overhead by allowing you to compile once and execute many times with different parameters. +Calling statement preparation methods inside a loop with the same SQL string defeats this purpose entirely.

+

The performance cost

+

The repeated compilation/preparation causes:

+
    +
  • CPU waste: Parsing and compilation happen repeatedly instead of once
    • +
  • Memory churn: Temporary objects are created and discarded on each iteration
    • +
  • Network overhead: For database operations, each preparation call may involve network round-trips
    • +
  • Slower execution: A loop that should take milliseconds might take seconds when processing large datasets
    • +
+

What is the potential impact?

+

The application may experience:

+
    +
  • Degraded performance: Operations that should be fast become noticeably slow, especially when processing large collections or + datasets
    • +
  • Resource exhaustion: For database operations, repeatedly creating parameterized query objects can exhaust server-side cursor or + statement handle limits, causing connection failures
    • +
  • Poor scalability: The performance penalty multiplies as data volume increases, making the application unable to handle + production workloads efficiently
    • +
  • Increased costs: Higher CPU usage and longer execution times can lead to increased infrastructure costs in cloud + environments
    • +
+

Exceptions

+

String.split does not compile a regular expression when the argument meets either of these conditions:

+
    +
  • It is a one-char String and this character is not one of the regex metacharacters ".$|()[{^?*+\"
    • +
  • It is a two-char String and the first char is the backslash and the second is not an ASCII digit or letter.
    • +
+

In these cases, no issue is raised.

+

How to fix it

+

For regular expression operations, compile the Pattern once before the loop and reuse it inside the loop. Do not move +String.matches(), replaceAll(), replaceFirst(), or split() outside the loop: the receiver still +changes on each iteration. Replace those calls with Pattern.matcher() or Pattern.split() applied to each input.

+

For database operations, prepare the statement once before the loop and reuse it with different parameters.

+

Code examples

+

Noncompliant code example

+
+for (String input : inputs) {
+    Pattern p = Pattern.compile("[a-z]+");  // Noncompliant
+    Matcher m = p.matcher(input);
+    if (m.find()) {
+        handle(m.group());
+    }
+}
+
+

Compliant solution

+
+Pattern LOWER = Pattern.compile("[a-z]+");
+for (String input : inputs) {
+    Matcher m = LOWER.matcher(input);
+    if (m.find()) {
+        handle(m.group());
+    }
+}
+
+

Noncompliant code example

+
+for (String input : inputs) {
+    if (input.matches("[a-z]+")) {  // Noncompliant
+        handle(input);
+    }
+}
+
+

Compliant solution

+
+Pattern LOWER = Pattern.compile("[a-z]+");
+for (String input : inputs) {
+    if (LOWER.matcher(input).matches()) {
+        handle(input);
+    }
+}
+
+

Noncompliant code example

+
+for (int id : ids) {
+    PreparedStatement ps = connection.prepareStatement("SELECT * FROM t WHERE id = ?");  // Noncompliant
+    ps.setInt(1, id);
+    ps.execute();
+    ps.close();
+}
+
+

Compliant solution

+
+PreparedStatement ps = connection.prepareStatement("SELECT * FROM t WHERE id = ?");
+for (int id : ids) {
+    ps.setInt(1, id);
+    ps.execute();
+}
+
+

Resources

+

Documentation

+ +

Articles & blog posts

+ +

Related rules

+
    +
  • {rule:java:S4248} - Regex patterns should not be created needlessly
    • +
  • {rule:java:S6909} - Constant parameters in a "PreparedStatement" should not be set more than once
    • +
diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.json new file mode 100644 index 00000000000..344210924c4 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9142.json @@ -0,0 +1,25 @@ +{ + "title": "Expensive compilation or preparation operations should not be performed inside loops", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "performance", + "regex", + "sql" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9142", + "sqKey": "S9142", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "EFFICIENT" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9142 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9142 new file mode 100644 index 00000000000..e69de29bb2d