-
Notifications
You must be signed in to change notification settings - Fork 724
SONARJAVA-6722 Implement S9142: Expensive compilation or preparation operations should not be performed inside loops #5893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S9142", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 0, | ||
| "falsePositives": 0 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartCaptureTest.java": [ | ||
| 220 | ||
| ], | ||
| "org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java": [ | ||
| 91, | ||
| 98, | ||
| 105, | ||
| 112, | ||
| 121 | ||
| ], | ||
| "org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java": [ | ||
| 806, | ||
| 808 | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/source/DecorationDataHolder.java": [ | ||
| 48, | ||
| 60, | ||
| 73 | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| 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]+"; | ||
|
|
||
| void patternCompileNoncompliant(List<String> 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 | ||
| } | ||
| } | ||
|
|
||
| void stringMethodsNoncompliant(List<String> inputs) { | ||
| for (String input : inputs) { | ||
| input.matches("[a-z]+"); // Noncompliant | ||
| input.replaceAll("[a-z]+", "X"); // Noncompliant | ||
| input.replaceFirst("[a-z]+", "X"); // Noncompliant | ||
| input.split("[,;]"); // Noncompliant | ||
| } | ||
| } | ||
|
|
||
| void prepareStatementNoncompliant(Connection conn, List<Integer> 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 compliant(List<String> inputs, Connection conn, List<Integer> 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<String> patterns, List<String> 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 | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| /* | ||
| * 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.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.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.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 Set<Tree.Kind> 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 MATCHERS = MethodMatchers.or( | ||
| MethodMatchers.create() | ||
| .ofTypes("java.util.regex.Pattern") | ||
| .names("compile") | ||
| .withAnyParameters() | ||
| .build(), | ||
| MethodMatchers.create() | ||
| .ofTypes("java.lang.String") | ||
| .names("matches", "replaceAll", "replaceFirst", "split") | ||
| .withAnyParameters() | ||
| .build(), | ||
| MethodMatchers.create() | ||
| .ofSubTypes("java.sql.Connection") | ||
| .names("prepareStatement") | ||
| .withAnyParameters() | ||
| .build() | ||
| ); | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> 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) { | ||
| return; | ||
| } | ||
| ExpressionTree patternArg = mit.arguments().get(0); | ||
| if (isLoopInvariant(patternArg, loop)) { | ||
| reportIssue(mit, String.format( | ||
| "Move this \"%s\" call outside the loop.", ExpressionUtils.methodName(mit).name())); | ||
| } | ||
|
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: Misleading message for String instance regex methodsFor Was this helpful? React with 👍 / 👎 |
||
| } | ||
|
|
||
| private static boolean isLoopInvariant(ExpressionTree arg, Tree loop) { | ||
| if (arg.is(Tree.Kind.IDENTIFIER)) { | ||
| var collector = new DeclaredOrAssignedLocalsCollector(); | ||
| loop.accept(collector); | ||
| return !collector.names.contains(((IdentifierTree) arg).name()); | ||
| } | ||
| return ExpressionUtils.resolveAsConstant(arg) != null; | ||
| } | ||
|
|
||
| private static class DeclaredOrAssignedLocalsCollector extends BaseTreeVisitor { | ||
|
|
||
| final Set<String> names = new HashSet<>(); | ||
|
|
||
| @Override | ||
| public void visitVariable(VariableTree tree) { | ||
|
Comment on lines
+87
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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()); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| class CompilationOrPreparationInLoopCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/CompilationOrPreparationInLoopCheckSample.java")) | ||
| .withCheck(new CompilationOrPreparationInLoopCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| <p>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.</p> | ||
| <p>In Java, this specifically refers to <code>Pattern.compile()</code>, String regex methods (<code>matches()</code>, <code>replaceAll()</code>, | ||
| <code>replaceFirst()</code>, <code>split()</code>), and <code>Connection.prepareStatement()</code>.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>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.</p> | ||
| <h2>Regular expression compilation</h2> | ||
| <p>When you call functions that compile regular expressions from strings or use string methods that accept regex patterns, the language runtime | ||
| must:</p> | ||
| <ul> | ||
| <li>Parse the regular expression syntax</li> | ||
| <li>Validate the pattern</li> | ||
| <li>Build an internal finite automaton (state machine)</li> | ||
| <li>Allocate memory for the compiled pattern</li> | ||
| </ul> | ||
| <p>These steps happen every time, even when the pattern string is identical. For example, calling a string matching method with a pattern like | ||
| <code>"\d+"</code> inside a loop that processes 1,000 items means compiling the same pattern 1,000 times.</p> | ||
| <h2>Database prepared statement preparation</h2> | ||
| <p>When you call methods that create prepared statements from SQL strings, the database driver must:</p> | ||
| <ul> | ||
| <li>Send the SQL string to the database server</li> | ||
| <li>Parse and validate the SQL syntax</li> | ||
| <li>Create an execution plan</li> | ||
| <li>Allocate server-side resources (cursors, statement handles)</li> | ||
| <li>Return a client-side prepared statement object</li> | ||
| </ul> | ||
| <p>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.</p> | ||
| <h2>The performance cost</h2> | ||
| <p>The repeated compilation/preparation causes:</p> | ||
| <ul> | ||
| <li><strong>CPU waste</strong>: Parsing and compilation happen repeatedly instead of once</li> | ||
| <li><strong>Memory churn</strong>: Temporary objects are created and discarded on each iteration</li> | ||
| <li><strong>Network overhead</strong>: For database operations, each preparation call may involve network round-trips</li> | ||
| <li><strong>Slower execution</strong>: A loop that should take milliseconds might take seconds when processing large datasets</li> | ||
| </ul> | ||
| <h3>What is the potential impact?</h3> | ||
| <p>The application may experience:</p> | ||
| <ul> | ||
| <li><strong>Degraded performance</strong>: Operations that should be fast become noticeably slow, especially when processing large collections or | ||
| datasets</li> | ||
| <li><strong>Resource exhaustion</strong>: For database operations, repeatedly creating parameterized query objects can exhaust server-side cursor or | ||
| statement handle limits, causing connection failures</li> | ||
| <li><strong>Poor scalability</strong>: The performance penalty multiplies as data volume increases, making the application unable to handle | ||
| production workloads efficiently</li> | ||
| <li><strong>Increased costs</strong>: Higher CPU usage and longer execution times can lead to increased infrastructure costs in cloud | ||
| environments</li> | ||
| </ul> | ||
| <h2>How to fix it</h2> | ||
| <p>For regular expression operations, compile the <code>Pattern</code> once before the loop and reuse it inside the loop. Use the | ||
| <code>Pattern.matcher()</code> method to apply the pattern to different input strings.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| // Direct Pattern.compile in loop | ||
| for (String input : inputs) { | ||
| Pattern p = Pattern.compile("[a-z]+"); // Noncompliant | ||
| Matcher m = p.matcher(input); | ||
| if (m.find()) { | ||
| handle(m.group()); | ||
| } | ||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| // Compile once, reuse Pattern | ||
| Pattern LOWER = Pattern.compile("[a-z]+"); | ||
| for (String input : inputs) { | ||
| Matcher m = LOWER.matcher(input); | ||
| if (m.find()) { | ||
| handle(m.group()); | ||
| } | ||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li>Oracle Java Documentation - <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/regex/Pattern.html">Pattern (Java SE | ||
| 17 & JDK 17)</a></li> | ||
| <li>Oracle Java Documentation - <a | ||
| href="https://docs.oracle.com/en/java/javase/17/docs/api/java.sql/java/sql/PreparedStatement.html">PreparedStatement (Java SE 17 & JDK | ||
| 17)</a></li> | ||
| <li>Baeldung - <a href="https://www.baeldung.com/regular-expressions-java">Guide to Java Regular Expressions API</a></li> | ||
| <li>SpotBugs Documentation - <a href="https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#IIL_PATTERN_COMPILE_IN_LOOP">Bug Descriptions - | ||
| Pattern compile in loop</a></li> | ||
| </ul> | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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": "5 min" | ||
| }, | ||
| "tags": [ | ||
| "performance", | ||
| "regex", | ||
| "sql" | ||
| ], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-9142", | ||
| "sqKey": "S9142", | ||
| "scope": "All", | ||
| "quickfix": "unknown", | ||
| "code": { | ||
| "impacts": { | ||
| "MAINTAINABILITY": "MEDIUM" | ||
| }, | ||
| "attribute": "EFFICIENT" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Edge Case: Missing test coverage for key edge cases
The sample omits several important scenarios: nested loops (invariant relative to inner loop), an argument that is a mutated field (
this.field = ...), lambda/anonymous-class bodies inside the loop, and compound assignments (pattern += ...). Adding these would pin down the intended behavior and guard against the false positives described above.Was this helpful? React with 👍 / 👎