Suppresses RLC non-final field overwrite warning for safe constructor field initialization #7050
Conversation
… field initialization
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
Overall looks good, just a few minor comments. I'd like to take one more look before we merge, though.
| private @Owning FileInputStream s; | ||
|
|
||
| public ConstructorChainingLeak() throws Exception { | ||
| this(42); |
There was a problem hiding this comment.
Can you add a test like this one but where the this(42) call is replaced by a call to a super constructor? I'd like to at least codify the behavior in that scenario (I think it would be safe not to warn in that constructor, but it's okay if there's an FP now).
There was a problem hiding this comment.
Added a test case for a constructor that calls super().
The suppression logic already handles super constructor calls correctly, so no false positive is reported solely due to the super() call.
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
mernst
changed the title
|
@iamsanjaymalakar The typecheck job is not passing. |
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
LGTM, I don't need to review again. Please address these comments and then request a review from Mike.
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
|
@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
left a comment
mernst
left a comment
There was a problem hiding this comment.
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.)
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
1706-1719: Initializer scanners must flag method calls tooThe instance-initializer scan only notices explicit assignments. A prior helper like
init();that writesthis.fieldinternally slips through, so we wrongly deem a later constructor write “first” and suppress a real leak. OverridevisitMethodInvocation(and any other executable nodes you care about) to flipisInitializedas soon as we see them.initBlock.accept( new TreeScanner<Void, Void>() { @Override public Void visitAssignment(AssignmentTree assignmentTree, Void unused) { ExpressionTree lhs = assignmentTree.getVariable(); Element lhsElement = TreeUtils.elementFromTree(lhs); if (field.equals(lhsElement)) { isInitialized.set(true); return null; } return super.visitAssignment(assignmentTree, unused); } + + @Override + public Void visitMethodInvocation(MethodInvocationTree node, Void unused) { + isInitialized.set(true); + return super.visitMethodInvocation(node, unused); + } }, null);
1744-1764: Keep scanning the current assignment’s childrenWhen the visitor hits the assignment we’re analyzing, it returns
TRUEimmediately and never looks at its variable or RHS. Any method call insidethis.f = helper();is therefore ignored, so we suppress exactly the bad pattern we must diagnose. Walk the child trees first; only returnTRUEonce you’ve confirmed neither side reports an earlier write/call.public Boolean visitAssignment(AssignmentTree node, Void p) { Element lhsEl = TreeUtils.elementFromUse(node.getVariable()); if (field.equals(lhsEl)) { - return (node == assignment) - ? Boolean.TRUE - : Boolean.FALSE; // (5) Earlier assignment to same field + if (node != assignment) { + return Boolean.FALSE; // (5) earlier assignment to same field + } + Boolean fromLhs = scan(node.getVariable(), p); + if (fromLhs != null) { + return fromLhs; + } + Boolean fromRhs = scan(node.getExpression(), p); + if (fromRhs != null) { + return fromRhs; + } + return Boolean.TRUE; } return super.visitAssignment(node, p); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(4 hunks)checker/tests/resourceleak-firstinitconstructor/FinalField.java(1 hunks)checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java(1 hunks)checker/tests/resourceleak-firstinitconstructor/SideEffectFreeMethodCall.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (4)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java (1)
ElementUtils(51-1158)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)
checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java (1)
checker/tests/resourceleak-firstinitconstructor/FinalField.java (1)
InheritableMustCall(5-27)
checker/tests/resourceleak-firstinitconstructor/FinalField.java (1)
checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java (1)
InheritableMustCall(5-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
🔇 Additional comments (5)
checker/tests/resourceleak-firstinitconstructor/SideEffectFreeMethodCall.java (3)
1-3: LGTM!The imports are appropriate for testing resource-leak checking with Checker Framework annotations.
5-8: LGTM!The field declarations correctly set up the test scenario with private, uninitialized fields including an
@Owningresource field.
14-15: Empty catch is intentional; annotate the resource assignment with the expected must-call error
Add an error comment on thes = new FileInputStream("test.txt");line, for example:s = new FileInputStream("test.txt"); // :: error: (expected.mustcall.message)Likely an incorrect or invalid review comment.
checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java (2)
7-7: LGTM!The field is correctly declared as private and
@Owningto test must-call semantics for non-final fields initialized in constructors.
9-18: Empty catch is intentional for this test
This file lives under checker/tests/resourceleak-firstinitconstructor and is designed to exercise the static analysis of conditional resource initialization. Swallowing exceptions here ensures only the checker’s control-flow logic is tested—no runtime fix is needed.Likely an incorrect or invalid review comment.
checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java
Show resolved
Hide resolved
checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConditional.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (1)
2764-2795: Scan the RHS before declaring “first write”.
visitAssignmentreturnsTRUEimmediately when it encounters the target assignment, so any method calls in the RHS never get visited. Forthis.f = helper();wherehelper()writesthis.f(or calls another method that does), we incorrectly treat this write as the first one and suppress the warning. The scanner must continue traversing the RHS before returning success.public Boolean visitAssignment(AssignmentTree node, Void p) { Element lhsEl = TreeUtils.elementFromUse(node.getVariable()); if (field.equals(lhsEl)) { - return node.equals(assignment) ? Boolean.TRUE : Boolean.FALSE; + if (!node.equals(assignment)) { + return Boolean.FALSE; + } + Boolean rhsResult = scan(node.getExpression(), p); + return rhsResult != null ? rhsResult : Boolean.TRUE; } return super.visitAssignment(node, p); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 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 (4)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java (1)
ElementUtils(51-1158)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (1)
2774-2783: Keep scanning the RHS before declaring a “first write”.Line 2778 returns
TRUEimmediately, sohelper()calls on the RHS (e.g.,this.f = helper();) are never visited. Ifhelper()mutates the same field first, we suppress a real leak. Please scan the children before concluding.@Override public Boolean visitAssignment(AssignmentTree node, Void p) { Element lhsEl = TreeUtils.elementFromUse(node.getVariable()); 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; + if (!node.equals(assignment)) { + return Boolean.FALSE; + } + Boolean childResult = super.visitAssignment(node, p); + return childResult != null ? childResult : Boolean.TRUE; } return super.visitAssignment(node, p); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(5 hunks)checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java(1 hunks)
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java
Outdated
Show resolved
Hide resolved
…lizerMethodInvocation.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java (1)
checker/tests/resourceleak/MultipleIdenticalReturns.java (1)
org(29-36)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java(1 hunks)checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java(1 hunks)checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (4)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
InheritableMustCall(8-48)checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConstructor.java (1)
InheritableMustCall(8-35)checker/tests/resourceleak-firstinitconstructor/ConstructorChainingLeak.java (1)
InheritableMustCall(8-38)checker/tests/resourceleak-firstinitconstructor/PrivateMethodAssignmentLeak.java (1)
InheritableMustCall(8-41)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (1)
InheritableMustCall(8-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
🔇 Additional comments (11)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerMethodInvocation.java (1)
1-50: LGTM! Test correctly verifies resource leak warnings for field reassignments.The test properly validates that warnings are not suppressed when a constructor reassigns a field that was already initialized in an instance initializer block. The execution order (instance initializer → constructor body) means the constructor assignment at line 19 overwrites the value set by
init()at line 28, correctly triggering therequired.method.not.callederror. All expected error annotations align with the PR's goal of suppressing warnings only for safe first-time constructor field initialization, not for subsequent overwrites. The previous class/constructor name typos have been addressed.checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (4)
16-22: Verify the intended visibility ofassignF().The
assignF()method is package-private but appears to be called only from the instance initializer block within this class.Verify whether this method should be marked
privatefor consistency with similar test patterns, or if the package-private visibility is intentional for testing purposes.
24-30: LGTM!The constructor correctly tests the scenario where a field is reassigned after being initialized in an instance initializer block, and the expected error annotation properly validates that the resource leak is detected.
32-39: LGTM!The
open()method correctly tests post-construction reassignment of the@Owningfield, and both expected error annotations are appropriate:
missing.creates.mustcall.for: Documents the must-call obligation creationrequired.method.not.called: Detects the resource leak from overwriting the field
41-47: LGTM!The
close()method implementation is correct, with appropriate@EnsuresCalledMethodsannotation and proper exception handling.checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (6)
1-3: LGTM! Clear test documentation.The comments effectively explain the test's purpose and expected behavior, making it easy to understand why the suppressor should not apply in this scenario.
12-16: LGTM! Correctly tests method call before assignment.The test correctly expects the
required.method.not.callederror becausedoSomething()appears before the field assignment, preventing the first-write suppressor from applying. This aligns with the safe constructor initialization logic introduced by this PR.
24-26: LGTM! Appropriate placeholder method.The private
doSomething()method serves its purpose as a placeholder to test that method calls before field assignment prevent the suppressor from applying.
28-32: LGTM! Correctly models non-constructor field assignment.The
open()method correctly expects bothmissing.creates.mustcall.forandrequired.method.not.callederrors, as the first-write suppression only applies to constructors. This is consistent with similar test files in the directory.
34-40: LGTM! Proper close() method implementation.The
close()method correctly implements the must-call obligation with appropriate annotations and exception handling, consistent with the pattern established in other test files in this directory.
42-43: LGTM! Appropriate test helper class.The empty inner class
Cappropriately serves as a minimal helper for testing that constructor calls before field assignment prevent the first-write suppressor from applying.
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java
Show resolved
Hide resolved
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java
Show resolved
Hide resolved
mernst
left a comment
mernst
left a comment
There was a problem hiding this comment.
@iamsanjaymalakar I'm assigning this back to you because you haven't addressed all the CodeRabbit review feedback. Not all of it is good, but some of it is.
| // 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; |
There was a problem hiding this comment.
This looks wrong to me. I think it should be return node == assignment;, because we are looking for the exact assignment, not just one that is equal to it. Using == emphasizes this fact.
There was a problem hiding this comment.
I trired with the ==, but I was getting CF errors.
/__w/1/s/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:2776: error: [interning:not.interned] attempting to use a non-@Interned comparison operand
if (field == lhsEl) {
^
/__w/1/s/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:2780: error: [interning:not.interned] attempting to use a non-@Interned comparison operand
return node == assignment ? Boolean.TRUE : Boolean.FALSE;
^
I then switched to .equals which uses Objects.equals under the hood for reference comparison.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
1-3: Past review comment remains unaddressed.The previous review correctly noted that the comment on line 2 is imprecise. The warning related to the initializer context actually occurs in
assignF()(line 19), which is invoked from the instance initializer block, not directly in the block itself (lines 12-14).checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (1)
18-22: Mirror the explanatory comment for constructor call.Add a short note beside
new C(this)(like Line 13) to make the intent explicit.public MethodCallBeforeAssignment(boolean its_a_constructor_call) throws Exception { - new C(this); + new C(this); // Constructor call before assignment — suppressor should bail // :: error: (required.method.not.called) s = new FileInputStream("test.txt"); }checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
1705-1735: IIB scan misses side‑effecting object creation (new Helper(this)).Instance initializer scanning flags assignments and side‑effecting method calls, but not
NewClassTree. Constructors can mutate fields via helper constructors before the target assignment. AddvisitNewClasshere (mirroring the constructor scanner).initBlock.accept( new TreeScanner<Void, Void>() { @Override public Void visitAssignment(AssignmentTree assignmentTree, Void unused) { ExpressionTree lhs = assignmentTree.getVariable(); Element lhsElement = TreeUtils.elementFromTree(lhs); if (field.equals(lhsElement)) { isInitialized.set(true); return null; } return super.visitAssignment(assignmentTree, unused); } @Override public Void visitMethodInvocation(MethodInvocationTree node, Void unused) { // Any side-effecting method call in an initializer block could write to the field. if (!cmAtf.isSideEffectFree(TreeUtils.elementFromUse(node))) { isInitialized.set(true); return null; } return super.visitMethodInvocation(node, unused); } + @Override + public Void visitNewClass(NewClassTree node, Void unused) { + if (!cmAtf.isSideEffectFree(TreeUtils.elementFromUse(node))) { + isInitialized.set(true); + return null; + } + return super.visitNewClass(node, unused); + } }, null);
2774-2783: Don’t return before scanning RHS of the current assignment.Early TRUE short‑circuits skip any side‑effecting calls in the RHS that execute before the store (e.g.,
this.f = helper();). Scan children first, then declare success.public Boolean visitAssignment(AssignmentTree node, Void p) { Element lhsEl = TreeUtils.elementFromUse(node.getVariable()); if (field == lhsEl) { - // Found an assignment to the same field: - // - current assignment → first write → true - // - earlier assignment → not first → false - return node == assignment ? Boolean.TRUE : Boolean.FALSE; + // Found an assignment to the same field. + if (node != assignment) { + // Earlier assignment to the same field → not first. + return Boolean.FALSE; + } + // For the current assignment, still scan its children to detect any side-effecting + // evaluation that happens before the store (e.g., RHS calls). + scan(node.getVariable(), p); + Boolean rhs = scan(node.getExpression(), p); + return rhs != null ? rhs : Boolean.TRUE; } return super.visitAssignment(node, p); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(5 hunks)checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java(1 hunks)checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (4)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
InheritableMustCall(8-49)checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConstructor.java (1)
InheritableMustCall(8-35)checker/tests/resourceleak-firstinitconstructor/ConstructorChainingLeak.java (1)
InheritableMustCall(8-38)checker/tests/resourceleak-firstinitconstructor/PrivateMethodAssignmentLeak.java (1)
InheritableMustCall(8-41)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (4)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java (1)
ElementUtils(51-1158)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java (1)
InheritableMustCall(8-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
🔇 Additional comments (2)
checker/tests/resourceleak-firstinitconstructor/InstanceInitializerBlockLeak2.java (1)
4-49: Test structure and expected warnings are correct.The test appropriately verifies that constructor-first-write suppression does NOT apply when the first assignment occurs in an instance initializer block rather than directly in the constructor. The expected warnings correctly flag resource leaks at each reassignment point where the previous
FileInputStreamis not closed.checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (1)
1524-1541: Constructor “first write” suppression hook looks correct.The guard checks same‑receiver, private field, and defers to the first‑write analysis. Integration point and early return are appropriate.
Please confirm the “private” restriction is intended policy; if broader visibility is acceptable (e.g., package‑private where
thiscan’t escape pre‑super), update tests accordingly.
| /** | ||
| * Returns true if the given assignment is the first write to a field on its path in the | ||
| * constructor. This method is conservative: it returns {@code false} unless it can prove that the | ||
| * write is the first. This check runs only for non-final fields because the Java compiler already | ||
| * forbids reassignment of final fields. | ||
| * | ||
| * <p>The result is {@code true} only if all the following hold: | ||
| * | ||
| * <ul> | ||
| * <li>(1) The field has no non-null inline initializer at its declaration. | ||
| * <li>(2) The field is not assigned in any instance initializer block. | ||
| * <li>(3) The constructor does not delegate via {@code this(...)}. | ||
| * <li>(4) No earlier assignment to the same field appears before the current statement. | ||
| * <li>(5) No earlier method call appears before the current statement (except {@code | ||
| * super(...)}). | ||
| * </ul> | ||
| * | ||
| * @param assignment the actual assignment tree being analyzed, which is a statement in | ||
| * @param field the field being assigned | ||
| * @param constructor the constructor where the assignment appears {@code constructor} | ||
| * @return true if this assignment is the first write during construction | ||
| */ |
There was a problem hiding this comment.
Fix Javadoc params (incomplete/typo).
Tighten the param docs to avoid confusion.
- * @param assignment the actual assignment tree being analyzed, which is a statement in
+ * @param assignment the actual assignment tree being analyzed, which is a statement in the
+ * body of {@code constructor}
* @param field the field being assigned
- * @param constructor the constructor where the assignment appears {@code constructor}
+ * @param constructor the constructor where the assignment appears
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
| public MethodCallBeforeAssignment(int call_method_on_rhs_of_assignment) throws Exception { | ||
| s = helper(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify RHS method-call intent in the constructor.
A brief comment helps readers understand this variant exercises “method call on RHS of first assignment.”
public MethodCallBeforeAssignment(int call_method_on_rhs_of_assignment) throws Exception {
- s = helper();
+ // Method call on RHS of first assignment in ctor — distinct path under test
+ s = helper();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public MethodCallBeforeAssignment(int call_method_on_rhs_of_assignment) throws Exception { | |
| s = helper(); | |
| } | |
| public MethodCallBeforeAssignment(int call_method_on_rhs_of_assignment) throws Exception { | |
| // Method call on RHS of first assignment in ctor — distinct path under test | |
| s = helper(); | |
| } |
🤖 Prompt for AI Agents
In
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java
around lines 24 to 26, add a brief inline comment above the constructor that
clarifies this test variant intentionally exercises a method call on the
right-hand side of the first assignment (i.e., s = helper()) so readers
understand the purpose; keep the comment short and explicit (e.g., "This
constructor tests method call on RHS of first assignment").
| @Owning | ||
| // :: error: (missing.creates.mustcall.for) | ||
| FileInputStream helper() throws Exception { | ||
| // :: error: (required.method.not.called) | ||
| s = new FileInputStream("test.txt"); | ||
| return new FileInputStream("test.txt"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make dual-allocation in helper explicit.
Helper intentionally both assigns to the field and returns a fresh resource to trigger diagnostics. Add a one-liner to avoid confusion for future readers.
@Owning
// :: error: (missing.creates.mustcall.for)
FileInputStream helper() throws Exception {
- // :: error: (required.method.not.called)
+ // Intentionally assign to field and also return a new resource to exercise checks
+ // :: error: (required.method.not.called)
s = new FileInputStream("test.txt");
return new FileInputStream("test.txt");
}🤖 Prompt for AI Agents
In
checker/tests/resourceleak-firstinitconstructor/MethodCallBeforeAssignment.java
around lines 34 to 40, the helper method both assigns a FileInputStream to the
field and returns a fresh FileInputStream which is intentional to trigger
diagnostics; add a concise one-line comment above the assignment or return
(e.g., "// intentionally assign to field and return a separate fresh stream to
exercise diagnostics") to make the dual-allocation behavior explicit for future
readers.
There was a problem hiding this comment.
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@FindDistinctannotation on theassignmentparameter 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
📒 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
AtomicBooleanwith 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.
| // The variables accessed from within the anonymous class need to be effectively final, so | ||
| // AtomicBoolean is used here. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Update comment wording for precision.
The comment mentions "anonymous class" but the code uses a TreeScanner. Consider updating to "The variables accessed from within the TreeScanner need to be effectively final".
Based on past review comments.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 1705-1706, update the comment that currently references an
"anonymous class" to accurately refer to the TreeScanner being used; change it
to something like: "The variables accessed from within the TreeScanner need to
be effectively final, so AtomicBoolean is used here." Make the edit only to the
comment text to improve precision.
3 participants
No description provided.