Suppresses RLC non-final field overwrite warning for safe constructor field initialization

[iamsanjaymalakar]

No description provided.

[kelloggm]

Overall looks good, just a few minor comments. I'd like to take one more look before we merge, though.

[mernst]

@iamsanjaymalakar The typecheck job is not passing.

[kelloggm]

LGTM, I don't need to review again. Please address these comments and then request a review from Mike.

[mernst]

@iamsanjaymalakar Martin asked you to assign me when you had addressed his comments. Have you addressed them?

[iamsanjaymalakar]

@iamsanjaymalakar Martin asked you to assign me when you had addressed his comments. Have you addressed them?

Yes, I have. You can take a look.

[mernst]

Please clarify some documentation, and update the code if necessary. (I didn't read the code because there is no point in doing so until the documentation is clear.)

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (1)

2780-2780: Use reference equality (==) instead of equals() for Tree comparison.

The comparison should be return node == assignment; because you're checking for the exact same Tree object by identity, not structural equality. This is reinforced by the @FindDistinct annotation on the assignment parameter at line 1683, which specifically requires identity-based comparisons.

Apply this diff:

       if (field.equals(lhsEl)) {
         // Found an assignment to the same field:
         //   - current assignment → first write → true
         //   - earlier assignment → not first → false
-        return node.equals(assignment) ? Boolean.TRUE : Boolean.FALSE;
+        return node == assignment ? Boolean.TRUE : Boolean.FALSE;
       }

Based on past review comments.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d55afd1 and 04d72ba.

📒 Files selected for processing (1)

• checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)

javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)

• TreeUtils (116-2967)

javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)

• TreePathUtil (33-505)

🔇 Additional comments (5)

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5)

7-19: LGTM!

The new imports are all properly used by the constructor first-write analysis logic.

Also applies to: 37-37, 43-43, 49-49

---

1524-1540: LGTM!

The integration properly guards the first-write suppression by checking that we're in a constructor, the receiver matches the enclosing class, the field is private, and the helper confirms it's the first write. The early return prevents false positives for safe constructor initialization patterns.

---

1660-1760: LGTM on the overall logic!

The method correctly implements the five conservative checks documented in the Javadoc:

• (1) No inline initializer
• (2) No instance initializer block assignment or side-effecting call
• (3) No this(...) delegation
• (4) & (5) Checked via ConstructorFirstWriteScanner

The use of AtomicBoolean with the explanatory comment is good practice, and the scan of instance initializer blocks properly checks both assignments and method invocations for side effects.

---

1762-1784: LGTM!

The helper method correctly identifies this(...) constructor delegation by checking the first statement. The comment about JEP 482 is a helpful reminder for future maintenance when flexible constructor bodies are finalized.

---

2720-2818: Scanner design and logic are solid.

The TreeScanner properly:

• Visits assignments, method invocations, and object creations
• Filters out side-effect-free methods and super() constructor calls
• Implements correct short-circuit reduction (FALSE → TRUE → null precedence)

The modular design as a named inner class with a static entry point is clean and testable.

[msridhar]

@mernst a ping on this one; it'd be great if we could get this merged

[mernst]

@iamsanjaymalakar Please resolve the CodeRabbit comments (addressing the ones that are useful and disregarding the ones that are not), and then re-assign the PR to me.

[mernst]

Thanks! I improved the documentation. I have just a couple questions, then this can be merged.

[mernst]

How can this test evaluate to false?

[iamsanjaymalakar]

You’re right. For an attributed MethodTree (constructor), TreeUtils.elementFromDeclaration(enclosingMethodTree) shouldn’t be null, so the check is redundant. I copied the pattern from elsewhere defensively, but it’s dead here and inconsistent with the earlier branch; I’ll remove it.

[mernst]

I don't understand when null is called.

[iamsanjaymalakar]

null is returned when scanning a subtree that contains neither (a) an assignment to the target field nor (b) a disqualifying call/new before the target assignment. In the constructor loop, null means “no evidence in this statement; keep scanning”. TRUE/FALSE are returned as soon as a decisive event is encountered.

The null return is used in a loop to decide wheather to keep scanning:

for (StatementTree st : stmts) {
  Boolean scanResult = ConstructorFirstWriteScanner.isFirstWrite(st, assignment, field, cmAtf);
  if (scanResult != null) {
    return scanResult;
  }
}

[iamsanjaymalakar]

Thanks! I improved the documentation. I have just a couple questions, then this can be merged.

Let me know if you have any furthur questions.

[mernst]

Looks good, modulo a few changes for robustness, simplicity, and documentation.

[mernst]

Where is the logic that stops the scan as soon as true/false is encountered? (The return statements in visit* methods do not do that.)

[iamsanjaymalakar]

It's in the caller:

List<? extends StatementTree> stmts = constructor.getBody().getStatements();
    for (StatementTree st : stmts) {
      Boolean scanResult = ConstructorFirstWriteScanner.isFirstWrite(st, assignment, field, cmAtf);
      if (scanResult != null) {
        return scanResult;
      }

[mernst]

Since this is the first assignment, I would expect that it is permitted even though there would be an error reported at the other assignment. This is based on the comment above "checks if {@code assignment} is the first write to {@code field}", which is true for this assignment. Maybe that comment needs to be clarified.

[mernst]

It would work to just iterate over the constructor.getBody(). Is this where the short-circuiting is supposed to happen? If the first statement in the body is an if statement with 100 lines of code in its then and else branches, then all 200 lines of code will be processed, I think. If you want to exit early, then throw an exception from within a visit* method and have isFirstWrite catch it. Then there is no need to override reduce, either. And, then the code will only issue a warning at the second assignment in

if (p) {
  r = ... first assignment ...
  ...
  r = ... second assignment ...
}

[mernst]

Does "definitely the first write" mean the statement is necessarily executed? Or might there exist executions in which this assignment is not executed?

[mernst]

It would be straightforward to handle this case. Override visitIf to compute a value for both sides. If either one is true, return true. Otherwise, if either one is false, return false`. Otherwise, return null. (And first process the condition, of course.)