Skip to content

SONARJAVA-6722 Implement S9142: Expensive compilation or preparation operations should not be performed inside loops - #5893

Open
NoemieBenard wants to merge 10 commits into
masterfrom
nb/sonarjava-6722-implement-S9142
Open

SONARJAVA-6722 Implement S9142: Expensive compilation or preparation operations should not be performed inside loops#5893
NoemieBenard wants to merge 10 commits into
masterfrom
nb/sonarjava-6722-implement-S9142

Conversation

@NoemieBenard

@NoemieBenard NoemieBenard commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • New checks:
    • Implemented CompilationOrPreparationInLoopCheck (S9142) to detect regex compilation and database preparation inside loops
    • Added sample test sources and unit tests for S9142 rule verification
  • Rule metadata:
    • Added rule definition and localization documentation for S9142

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6722

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Ruling needs updating. A fix PR has been created: #5894

Please review and merge it into your branch.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Ruling Diff Summary

Detected changes in 2 rule files: 0 issues removed, 11 issues added.

S9142 (java) on eclipse-jetty - 0 issues removed, 8 issues added - new ruling file

Added jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartCaptureTest.java (line 220)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartCaptureTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java (line 91)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java (line 98)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java (line 105)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/handler/InetAccessHandlerTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java (line 806)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java (line 808)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java)
S9142 (java) on sonar-server - 0 issues removed, 3 issues added - new ruling file

Added src/main/java/org/sonar/server/source/DecorationDataHolder.java (line 48)

(source file not found at this revision: src/main/java/org/sonar/server/source/DecorationDataHolder.java)

Added src/main/java/org/sonar/server/source/DecorationDataHolder.java (line 60)

(source file not found at this revision: src/main/java/org/sonar/server/source/DecorationDataHolder.java)

Added src/main/java/org/sonar/server/source/DecorationDataHolder.java (line 73)

(source file not found at this revision: src/main/java/org/sonar/server/source/DecorationDataHolder.java)

@sonarqube-next

sonarqube-next Bot commented Aug 6, 2026

Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
86.1% Coverage on New Code (required ≥ 90%)

See analysis details on SonarQube

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Ruling needs updating. A fix PR has been created: #5894

Please review and merge it into your branch.

@NoemieBenard
NoemieBenard force-pushed the nb/sonarjava-6722-implement-S9142 branch from 0765dd5 to a3a2611 Compare August 14, 2026 12:34
@gitar-bot

gitar-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements rule S9142 to detect expensive compilation or preparation operations inside loops, addressing misleading regex messages, test coverage gaps, and loop-invariant false positives.

✅ 3 resolved
Quality: Misleading message for String instance regex methods

📄 java-checks/src/main/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheck.java:82-84
For String.matches/replaceAll/replaceFirst/split the message is "Move this "matches" call outside the loop.", but these are instance calls on the per-iteration loop variable and cannot simply be moved out — the correct remediation (per the rule HTML) is to precompile a Pattern once and reuse matcher() inside the loop. Consider a method-specific message advising precompilation for the String-regex matchers versus the Pattern.compile/prepareStatement cases.

Edge Case: Missing test coverage for key edge cases

📄 java-checks-test-sources/default/src/main/java/checks/CompilationOrPreparationInLoopCheckSample.java:1-15
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.

Edge Case: Invariance check misses field mutations, causing false positives

📄 java-checks/src/main/java/org/sonar/java/checks/CompilationOrPreparationInLoopCheck.java:87-101
isLoopInvariant decides invariance by matching variable names and only tracks assignments/increments whose target is(Tree.Kind.IDENTIFIER). A field reassigned inside the loop via a member select (this.field = ..., this.field++) or mutated through a method call is never added to names, so Pattern.compile(field) in the loop is wrongly flagged as loop-invariant. Prefer resolving the argument's Symbol (semantic model) and confirming it is a local variable/parameter that is not reassigned in the loop; at minimum handle MEMBER_SELECT assignment targets and treat fields conservatively (do not report).

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@NoemieBenard
NoemieBenard marked this pull request as ready for review August 14, 2026 12:44
@NoemieBenard
NoemieBenard requested a review from nathsou August 14, 2026 12:52
return false;
}

private static boolean isLoopInvariant(ExpressionTree arg, Tree loop) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for loop invariants sounds like something that would be nice to extract as a common helper, I checked and rule S6909 defines a very similar method:

private static boolean isLoopInvariant(Set<String> declaredOrAssignedLocals, Candidate candidate) {
but your new implementation is much more robust so it might be worth refactoring S6909 and share the method impl. (could be done in another PR as a refactoring or here if you prefer).

return String.format("Move this \"%s\" call outside the loop.", ExpressionUtils.methodName(mit).name());
}

private static boolean isInForInitializer(Tree tree, Tree loop) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FP found by an LLM:

The iterable expression in an enhanced for-loop (e.g. for (String s : text.split(";")) or for (String s : Pattern.compile(";").split(text))) is evaluated only once before the loop starts (JLS §14.14.2).

Because isInForInitializer only checks ForStatementTree.initializer(), calls inside ForEachStatement.expression() will be flagged as false positives.

@nathsou nathsou removed their assignment Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants