Skip to content

SONARJAVA-6721 Extend rule S2442 to more "java.util.concurrent" objects - #5890

Open
NoemieBenard wants to merge 9 commits into
masterfrom
nb/sonarjava-6721-implement-S9141
Open

SONARJAVA-6721 Extend rule S2442 to more "java.util.concurrent" objects#5890
NoemieBenard wants to merge 9 commits into
masterfrom
nb/sonarjava-6721-implement-S9141

Conversation

@NoemieBenard

@NoemieBenard NoemieBenard commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • New checks:
    • Implemented SynchronizedOnConcurrentObjectCheck (S9141) to detect synchronized blocks on java.util.concurrent objects
  • Tests and documentation:
    • Added rule metadata, HTML documentation, JSON definition, test samples, and verifier tests for S9141

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

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

Copy link
Copy Markdown
Contributor

SONARJAVA-6721

@NoemieBenard
NoemieBenard marked this pull request as ready for review August 5, 2026 15:16
.onFile(mainCodeSourcesPath("checks/SynchronizedOnConcurrentObjectCheckSample.java"))
.withCheck(new SynchronizedOnConcurrentObjectCheck())
.verifyIssues();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also test withoutSemantic? In preparation for getting rid of autoscan.

gitar-bot[bot]

This comment was marked as resolved.

@gitar-bot
gitar-bot Bot dismissed their stale review August 6, 2026 10:17

✅ All code review findings resolved.

Configure merge blocking

@NoemieBenard NoemieBenard changed the title SONARJAVA-6721 Implement S9141: Intrinsic locks should not be used on "java.util.concurrent" objects SONARJAVA-6721 Extend rule S2442 to all "java.util.concurrent" objects Aug 6, 2026
@NoemieBenard
NoemieBenard marked this pull request as draft August 6, 2026 15:02
@sonarqube-next

sonarqube-next Bot commented Aug 6, 2026

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 7 days with no activity. If there is no activity in the next 7 days it will be closed automatically

@github-actions github-actions Bot added the stale label Aug 14, 2026
@NoemieBenard
NoemieBenard force-pushed the nb/sonarjava-6721-implement-S9141 branch from 6443de8 to bcdde13 Compare August 14, 2026 09:25
@gitar-bot

gitar-bot Bot commented Aug 14, 2026

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

Extends rule S2442 to detect synchronized blocks across all java.util.concurrent objects and updates the rule documentation and metadata accordingly. No issues found.

✅ 3 resolved
Edge Case: Rule flags all java.util.concurrent types, not just sync primitives

📄 java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java:48-54
isFromConcurrentPackage matches any type whose FQN starts with java.util.concurrent., so synchronized on concurrent collections (e.g. ConcurrentHashMap, CopyOnWriteArrayList, ConcurrentLinkedQueue) or utility types (Future, TimeUnit, ThreadLocalRandom) is also reported. Externally locking such objects is occasionally a legitimate pattern, so this may produce false positives beyond the synchronization primitives the RSPEC emphasizes. If the intent is truly the whole package this is fine; otherwise consider restricting to the lock/semaphore/latch/barrier/queue subtypes and adding compliant test cases for concurrent collections to document the intended scope.

Quality: S2442 rule metadata not updated for extended scope

📄 java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java:30 📄 java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java:60-64
This PR re-points the check to rule S2442 and extends it to flag Semaphore, CountDownLatch, CyclicBarrier, atomics, blocking queues, etc., but S2442.json/html still only describe synchronizing on a "Lock" object (title: "Synchronizing on a "Lock" object should be avoided", HTML shows only a Lock example). Users now get issues on non-Lock concurrent types with documentation that doesn't cover them. Update the S2442 title and HTML (why/noncompliant/compliant examples) to reflect the full set of java.util.concurrent synchronization primitives the check reports.

Bug: without-semantic test expects issues the check cannot raise

📄 java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java:34-41 📄 java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java:49-63
SynchronizedOnConcurrentObjectCheck decides entirely on expression.symbolType() / fullyQualifiedName() / superTypes(), which resolve to the unknown type when semantics are disabled, so the check raises zero issues in this mode. But test_without_semantic calls .verifyIssues() on the sample file, whose 12 // Noncompliant comments are still parsed as expected issues, so verification fails with "No issue raised. At least one issue expected". Change the assertion to .verifyNoIssues(), which is the correct idiom for verifying a semantics-dependent check behaves gracefully without semantics.

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

@NoemieBenard NoemieBenard changed the title SONARJAVA-6721 Extend rule S2442 to all "java.util.concurrent" objects SONARJAVA-6721 Extend rule S2442 to more "java.util.concurrent" objects Aug 14, 2026
@sonarqube-next

Copy link
Copy Markdown

@NoemieBenard
NoemieBenard marked this pull request as ready for review August 14, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants