SONARJAVA-6945: Implemented rule S9385 - Copy constructors should initialize all fields - #6116
romainbrenguier wants to merge 1 commit into
Conversation
…tialize all fields This rule detects copy constructors that leave eligible instance fields uninitialized. A copy constructor is a constructor with a single parameter of the same type as the enclosing class. The rule reports one issue on the constructor's simple name, with secondary locations on each uninitialized field declaration. Eligible fields are instance fields that are not static, not transient, and not initialized at declaration time. The analysis follows assignment paths through: - Direct field assignments - Compound assignments and increment/decrement operations - Constructor delegation chains - Instance helper method calls Deferred assignments in lambdas, local classes, and anonymous classes are not followed.
|
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| import org.sonar.plugins.java.api.tree.VariableTree; | ||
|
|
||
| @Rule(key = "S9365") | ||
| @Rule(key = "S9385") |
There was a problem hiding this comment.
🚨 Bug: Rule key renamed S9365→S9385 but old rule resources kept
The base commit already shipped this check as @Rule(key = "S9365") with matching resources (S9365.html, S9365.json, profiles/Sonar_way/S9365). This commit changes the annotation to S9385 and adds S9385.html (byte-identical to S9365.html) plus S9385.json (differs from S9365.json only in sqKey) and profiles/Sonar_way/S9385, while leaving all three S9365 files in place. ProfileJsonGenerator.collectRuleKeys turns every marker file into a rule key, so the generated Sonar_way_profile.json now activates both java:S9365 and java:S9385, but no check class declares key S9365 any more — the built-in profile activates a rule that does not exist in the java repository, and the repository ends up with two identical rule descriptions. GeneratedCheckListTest only walks from check classes to metadata, so it will not catch the orphan. Either revert the key to S9365 (matching the untouched ruleSpecification: "RSPEC-9365") and drop the three new S9385 files, or complete the rename by deleting S9365.html, S9365.json and profiles/Sonar_way/S9365.
Fix 1: Keep the already-released key S9365 and delete the newly added S9385.html, S9385.json and profiles/Sonar_way/S9385 files.
@Rule(key = "S9365")
- Apply fix
Fix 2: Alternative: finish the rename by removing the S9365 html, json and Sonar_way marker so only S9385 remains.
// keep @Rule(key = "S9385") and remove the stale resources:
// git rm sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9365.html
// git rm sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9365.json
// git rm sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9365
- Apply fix
Check a box to apply a fix or reply for a change | Was this helpful? React with 👍 / 👎
| "ruleSpecification": "RSPEC-9365", | ||
| "sqKey": "S9385", |
There was a problem hiding this comment.
⚠️ Bug: S9385.json points at ruleSpecification RSPEC-9365
S9385.json was copied from S9365.json with only sqKey updated, so it still declares "ruleSpecification": "RSPEC-9365" while "sqKey": "S9385". Every other rule metadata file in this directory keeps the two in sync (e.g. S6912 → RSPEC-6912), so the new rule links its documentation and traceability to a different RSPEC id. If S9385 is the intended key, update the field; if RSPEC-9365 is correct, the key change itself is the mistake.
Align ruleSpecification with the new sqKey.:
"ruleSpecification": "RSPEC-9385",
"sqKey": "S9385",
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private boolean enabled; | ||
|
|
||
| Basic(Basic other) { // Noncompliant {{This copy constructor leaves eligible fields uninitialized; initialize them explicitly to distinguish omissions from intentional resets.}} [[secondary=10,11,12]] | ||
| Basic(Basic other) { // Noncompliant |
There was a problem hiding this comment.
💡 Quality: Sample drops message and secondary-location assertions
All 17 // Noncompliant comments lose their [[secondary=...]] assertions and the primary {{...}} message assertion. CheckVerifier.verifyIssues() only compares secondaries and messages when the comment declares them, so the tests still pass but nothing now covers the secondary locations the check builds in visitNode — the very behaviour the rule description advertises ("The declarations of the omitted fields are identified as secondary locations"). The previously asserted values were correct (e.g. Basic eligible fields are lines 9-12 with name assigned, giving secondaries 10,11,12), so restoring at least the message assertion plus secondaries on a couple of representative cases keeps that behaviour under test.
Restore the message and secondary assertions on at least the representative cases (Basic, Generic, DeferredAssignments).:
Basic(Basic other) { // Noncompliant {{This copy constructor leaves eligible fields uninitialized; initialize them explicitly to distinguish omissions from intentional resets.}} [[secondary=10,11,12]]
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private static AnalysisResult analyzeMethod(MethodTree method, Symbol.TypeSymbol owner, Set<Symbol> eligibleFields, | ||
| Set<Symbol.MethodSymbol> activeMethods) { |
There was a problem hiding this comment.
💡 Quality: Documentation removed from the check's complex methods
The commit strips the javadoc from analyzeMethod and from the AssignmentCollector class, which documented the non-obvious contracts of the analysis: what activeMethods is for (recursion detection in the current call chain) and what complete means (no issue is raised unless every followed initialization path resolved). Those contracts are not derivable from the signatures and are exactly the "documentation for complex functions" the project guidelines ask for, so keeping the javadoc on these two elements is preferable to removing it along with the inline comments.
Restore the javadoc on analyzeMethod (and equivalently on AssignmentCollector).:
/**
* Analyzes explicit field writes performed by a constructor or helper method, including writes reached through
* resolvable calls on the current instance. Active methods form the current call chain and prevent infinite recursion.
*/
private static AnalysisResult analyzeMethod(MethodTree method, Symbol.TypeSymbol owner, Set<Symbol> eligibleFields,
Set<Symbol.MethodSymbol> activeMethods) {
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
CI failed: Integration tests failed due to a missing rule definition for 'java:S9365', causing SonarQube startup failure, which subsequently caused artifact-dependent notification steps to fail.Overview1 unique failure pattern and 1 cascading notification failure found across 6 analyzed logs. The primary issue is a test failure caused by a missing rule key in SonarQube's built-in quality profiles loader during integration tests, which also prevented ruling artifacts from being generated for downstream notification jobs. FailuresSonarQube Startup Failure due to Missing Rule Definition (confidence: high)
Missing Ruling Artifacts in Notification Job (confidence: high)
Summary
Code Review 🚫 Blocked 0 resolved / 4 findingsImplements rule S9385 to detect copy constructors that fail to initialize eligible fields, with comprehensive test coverage. The change is blocked by three issues: the rule key was renamed from S9365 to S9385 but old S9365 resources remain in the repository, causing both rule keys to be activated in the generated profile while S9365 no longer exists in code; 🚨 Bug: Rule key renamed S9365→S9385 but old rule resources kept📄 java-checks/src/main/java/org/sonar/java/checks/CopyConstructorMissesFieldCheck.java:48 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9385.json:13-14 The base commit already shipped this check as Keep the already-released key S9365 and delete the newly added S9385.html, S9385.json and profiles/Sonar_way/S9385 files.Alternative: finish the rename by removing the S9365 html, json and Sonar_way marker so only S9385 remains.
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Comment gitar unblock to override this block and allow merging.
Configure merge blocking · Maintainers can dismiss this review.
1 participant
This rule detects copy constructors that leave eligible instance fields uninitialized.
A copy constructor is a constructor with a single parameter of the same type as the enclosing class. The rule reports issues for constructors that fail to explicitly initialize eligible fields (instance fields that are not static, not transient, and not initialized at declaration time).
Test cases cover: