Extend RLC to collections of resources #7166
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
checker/tests/resourceleak/IndexMode.java (1)
9-18: Add explanatory comments for consistency.For consistency with the updated methods (getMode2a, getMode3, getMode4, getModeIS), consider adding similar comments to these methods explaining that no error is reported because Map#get returns @NotOwning:
getMode(lines 9-18)getMode2(lines 21-37)getMode5(lines 67-69)Additionally, the TODO comment in
getMode2(lines 23-31) describes a complex interaction with type variable defaulting. Given the @NotOwning annotation change, please verify whether this TODO is still accurate or if it should be updated to reference the @NotOwning behavior instead.Also applies to: 21-37, 67-69
♻️ Duplicate comments (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (3)
3514-3515: Use object equality instead of reference equality for block comparison.Using reference equality (
==) to check if the successor is the loop update block is fragile. The CFG may reconstruct block objects, breaking reference equality. Use.equals()for robustness.As per past review comments (unresolved).
Apply this diff:
- @SuppressWarnings("interning:not.interned") - boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; + boolean isLastBlockOfBody = + Objects.equals(successorAndExceptionType.first, loopUpdateBlock);
3575-3576: Use object equality instead of reference equality for block comparison.Similar to line 3515, this uses reference equality (
==) where object equality would be more appropriate. CFG block reconstruction could break this comparison.As per past review comments (unresolved).
Apply this diff:
- @SuppressWarnings("interning:not.interned") - boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; + boolean thenSuccessor = + Objects.equals(conditionalBlock.getThenSuccessor(), loopUpdateBlock);
767-798: Handle null mustCallValues gracefully instead of throwing.The current code throws a
BugInCFwhengetMustCallValuesOfResourceCollectionComponentreturns null (line 783), but this can legitimately occur when the element type lacks a@MustCallannotation. Instead of crashing, the method should treat null as "no obligations to track" and skip adding theCollectionObligation.Based on learnings and past review comments.
Apply this diff to handle null gracefully:
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); - if (mustCallValues == null) { - throw new BugInCF( - "List of MustCall values of component type is null for OwningCollection return value:" - + " " - + node); + if (mustCallValues == null || mustCallValues.isEmpty()) { + // No must-call obligations for the element type; nothing to track. + return; } if (!ResourceLeakUtils.isIterator(node.getType())) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java(5 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(42 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java(3 hunks)checker/tests/resourceleak/EnhancedFor.java(1 hunks)checker/tests/resourceleak/IndexMode.java(3 hunks)checker/tests/resourceleak/Issue6030.java(2 hunks)checker/tests/resourceleak/SocketIntoList.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java (1)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java (1)
CollectionOwnershipChecker(14-26)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java (4)
AnnotatedTypeMirror(64-2780)AnnotatedDeclaredType(948-1237)AnnotatedExecutableType(1262-1608)AnnotatedTypeVariable(1715-1939)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(66-749)PotentiallyFulfillingLoop(630-707)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)dataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.java (1)
JavaExpression(77-920)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-3007)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)dataflow/src/main/java/org/checkerframework/dataflow/util/NodeUtils.java (1)
NodeUtils(22-132)
🪛 ast-grep (0.39.5)
checker/tests/resourceleak/SocketIntoList.java
[info] 19-19: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 28-28: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (15)
checker/tests/resourceleak/IndexMode.java (3)
54-54: LGTM!The comment updates consistently document that Map#get returns @NotOwning, which explains why no error is reported. This is consistent with the broader ownership annotation changes in this PR.
Also applies to: 62-62, 75-75
72-72: LGTM!The clarification that values can have any @MustCall type is helpful for understanding the test case.
41-41: Confirm Map#get has @NotOwning in the JDK stubs
All updated comments assume Map#get returns an @NotOwning reference; please verify this annotation in the JDK stub definitions.checker/tests/resourceleak/Issue6030.java (4)
4-5: LGTM: Imports support the new ownership annotations.These imports are necessary for the
@NotOwningCollectionand@NotOwningannotations used in the method signatures below.
28-30: LGTM: Consistent annotation pattern.The
@NotOwningCollectionannotation on the receiver is consistent with the other methods in this class (hasNext()andnext()), maintaining uniform ownership semantics for the test case.
21-24: Correct the comment regardingIterator#nextannotation. The superinterface’snext()return is unannotated, not@NotOwning; update or remove the misleading justification.Likely an incorrect or invalid review comment.
16-19: Ensure the test still triggers the expected error
The@NotOwningCollectionreceiver annotation is appropriate, but please confirm that running
./gradlew :checker:test --tests '*Issue6030*'
(or the equivalent test-cftests script) still produces themissing.creates.mustcall.forerror for Issue6030.java.checker/tests/resourceleak/SocketIntoList.java (3)
26-33: Comments successfully clarified per past review.The explanatory comments have been improved for clarity, addressing the past review feedback. The explanation now clearly states why no
required.method.not.callederror is reported at the declaration site (line 29) while anargumenterror appears at theaddcall (line 33):List#add(@Owning E)would assume the obligation, butlis typed asList<@MustCall({}) Socket>which cannot hold elements with non-empty MustCall values.
38-42: Clear explanation of ownership requirements.The comments accurately explain the ownership semantics:
List#getreturns@NotOwningelements, so the socketslacks ownership, buts.bind()attempts to reset the MustCall obligations, which requires ownership. The error annotation is correctly placed.
17-23: Return type annotation is unnecessary. The test intentionally omits@OwningCollectionon the return to model an unowned view; no change needed.Likely an incorrect or invalid review comment.
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5)
2827-2845: Well-structured null handling for collection parameter obligations.The code correctly checks for null
mustCallValues(line 2829) before iterating and only creates obligations for non-Iterator types (line 2830). This is the proper pattern for handling potential null returns fromgetMustCallValuesOfResourceCollectionComponent.
3335-3432: Pattern matching logic for enhanced for-loops is sound.The method systematically identifies enhanced for-loop structures by:
- Forward traversal to find loop body entry (lines 3352-3371)
- Backward traversal to find loop condition (lines 3377-3401)
- Defensive check ensuring
enhancedForLoopis non-null wheniterableExpressionis non-null (lines 3342-3346)The early return at line 3389 when multiple predecessors are encountered is conservative and sound. The method correctly handles complex CFG patterns.
3449-3552: Loop body analysis structure is well-designed.The method follows good practices:
- Upfront null checks with descriptive error messages (lines 3452-3462)
- Early return for trivial cases (lines 3468-3471)
- Consistent worklist pattern matching main
analyze()method- Proper null handling for
calledMethodsAfterBlock(lines 3523-3529)- Clear exception handling with abort logic (lines 3539-3541)
Note: Reference equality issue at line 3515 is flagged separately.
3161-3188: Clean utility methods for formatting must-call error messages.The
formatMissingMustCallMethodsandformatMustCallMethodhelper methods properly format method names for user-facing error messages, including handling theUNKNOWN_METHOD_NAMEsentinel value by replacing it with "Unknown". This improves error message clarity.
2962-2983: Loop body analysis guard prevents duplicate warnings.The early return at lines 2962-2965 correctly prevents warning reports during loop body analysis, which is intended for detection only. The subsequent
CollectionObligationerror reporting (lines 2967-2983) properly checksreportedErrorAliasesto avoid duplicates and formats the error message using theformatMustCallMethodhelper.
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
Handle wildcard super bounds to avoid dropping manual @MustCallUnknown.
When the type argument is a wildcard, you only inspect getExtendsBound(). If the wildcard was written with a lower bound (e.g. ? super @MustCallUnknown Foo), extendsBound is null and we immediately reset it to bottom, discarding the user-written @MustCallUnknown and making the type unsound. Please also consult the super bound before replacing the qualifier—if either bound carries a manual @MustCallUnknown, leave the argument intact.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java
around lines 202 to 215, the wildcard handling only checks getExtendsBound() and
can drop a user-written @MustCallUnknown on a ? super ... bound; change the
logic to consult both the extends and super bounds: obtain the extendsBound and
superBound from the WildcardType (guarding against nulls) and call
ResourceLeakUtils.hasManualMustCallUnknownAnno on each; only call
typeArg.replaceAnnotation(BOTTOM) if neither bound contains a manual
@MustCallUnknown.
There was a problem hiding this comment.
I think this is a false positive, because this code is only handling upper bounds. I'm not sure why anyone would ever even write ? super @MustCallUnknown Foo, since the only type that can fulfill that is the (useless) @MustCallUnknown Foo.
There was a problem hiding this comment.
(Whoever reviews this code next should double-check my reasoning, though.)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| try { | ||
| propagateObligationsToSuccessorBlock( | ||
| obligations, | ||
| currentBlock, | ||
| successorAndExceptionType.first, | ||
| successorAndExceptionType.second, | ||
| visited, | ||
| worklist); | ||
| } catch (InvalidLoopBodyAnalysisException e) { | ||
| // this exception is only thrown from the loop body analysis, which has another entry point. | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify exception handling comment for loop analysis abort.
The catch block comment (line 2486) is terse. The similar catch at line 3540 has a clearer explanation: "Expected when analyzing unreachable loop bodies. Safely abort the analysis." Consider aligning these comments for consistency.
Apply this diff:
} catch (InvalidLoopBodyAnalysisException e) {
- // this exception is only thrown from the loop body analysis, which has another entry point.
+ // Expected when analyzing unreachable loop bodies during loop-body analysis.
+ // Safely abort propagation since the block has no incoming store.
}📝 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.
| try { | |
| propagateObligationsToSuccessorBlock( | |
| obligations, | |
| currentBlock, | |
| successorAndExceptionType.first, | |
| successorAndExceptionType.second, | |
| visited, | |
| worklist); | |
| } catch (InvalidLoopBodyAnalysisException e) { | |
| // this exception is only thrown from the loop body analysis, which has another entry point. | |
| } | |
| } | |
| try { | |
| propagateObligationsToSuccessorBlock( | |
| obligations, | |
| currentBlock, | |
| successorAndExceptionType.first, | |
| successorAndExceptionType.second, | |
| visited, | |
| worklist); | |
| } catch (InvalidLoopBodyAnalysisException e) { | |
| // Expected when analyzing unreachable loop bodies during loop-body analysis. | |
| // Safely abort propagation since the block has no incoming store. | |
| } | |
| } |
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 2477 to 2488, the catch block for InvalidLoopBodyAnalysisException
has a terse comment; replace it with a clearer explanatory comment (matching the
style at line 3540) such as: "Expected when analyzing unreachable loop bodies.
Safely abort the analysis." to clarify that this exception is normal for
unreachable loop bodies and that the analysis should be aborted safely.
| public void findFulfillingForEachLoops(ControlFlowGraph cfg) { | ||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add any owning parameters to the initial set of variables to track. | ||
| BlockWithObligations entry = | ||
| new BlockWithObligations(cfg.getEntryBlock(), new HashSet<Obligation>()); | ||
| worklist.add(entry); | ||
| visited.add(entry); | ||
|
|
||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof MethodInvocationNode) { | ||
| patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg); | ||
| } | ||
| } | ||
| propagate( | ||
| new BlockWithObligations(successorAndExceptionType.first, new HashSet<Obligation>()), | ||
| visited, | ||
| worklist); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding debug logging for loop analysis metrics.
As noted in past review comments, this method analyzes loops but provides no visibility into how many loops were analyzed or marked as fulfilling. Adding debug logging would help with performance analysis and debugging when loops aren't detected as expected.
As per past review comments (unresolved).
Add logging at key points:
public void findFulfillingForEachLoops(ControlFlowGraph cfg) {
Set<BlockWithObligations> visited = new HashSet<>();
Deque<BlockWithObligations> worklist = new ArrayDeque<>();
int loopsAnalyzed = 0;
// ... existing code ...
while (!worklist.isEmpty()) {
// ... existing code ...
for (Node node : currentBlock.getNodes()) {
if (node instanceof MethodInvocationNode) {
loopsAnalyzed++;
patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg);
}
}
}
if (checker.hasOption("debug")) {
System.err.println("Analyzed " + loopsAnalyzed + " potential fulfilling loops in " + cfg);
}
}🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 3286 to 3315, add lightweight debug logging and metrics: introduce
a counter (e.g., loopsAnalyzed and loopsMarkedFulfilling) incremented when you
detect a potential enhanced-collection for-each (increment before calling
patternMatchEnhancedCollectionForLoop and increment a second counter
inside/after it if it reports a fulfilling loop), and after the worklist loop
emit a debug message (to System.err or the existing checker logger) only when
checker.hasOption("debug") is true that prints the counts and the cfg
identifier; ensure the additions are non-intrusive, thread-safe for this
analysis context, and do not change existing control flow or exception handling.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)
1218-1233: Critical: Null-safety violations can cause NPE (previously flagged).This method has the same null-safety issues flagged in the previous review:
- Line 1227:
flowResult.getInput(block)dereferencesflowResult, which can be null when analysis hasn't run (see line 1116 whereflowResultis declared as@MonotonicNonNull)- Lines 1227, 1229: Both
flowResult.getInput(block)andanalysis.getInput(block)can return null for unreachable blocks- Line 1232:
input.getRegularStore()will throw NPE ifinputis nullThe return type should be
@Nullable Storeto match similar methods likegetStoreBefore(Node)at lines 1241-1257, which properly handles null cases.Apply the fix suggested in the previous review:
/** * Returns the regular store for a given block. * + * <p>May return {@code null} if the block is unreachable or if called before analysis results + * are available. + * * @param block a block whose regular store to return - * @return the regular store of {@code block} + * @return the regular store of {@code block}, or {@code null} */ - public Store getRegularStore(Block block) { + public @Nullable Store getRegularStore(Block block) { TransferInput<Value, Store> input; if (!analysis.isRunning()) { + if (flowResult == null) { + throw new BugInCF("getRegularStore called before analysis result is available"); + } input = flowResult.getInput(block); } else { input = analysis.getInput(block); } - - return input.getRegularStore(); + return input == null ? null : input.getRegularStore(); }checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (1)
6-6: Duplicate: Remove unused CFStore import and update Javadoc reference.As noted in a previous review, the
CFStoreimport at line 6 is unused in code and only appears in the Javadoc. Either remove the import and fully-qualify the Javadoc reference, or reference it in code if needed.Based on past review comments.
Also applies to: 13-15
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
808-851: Add null check for mustCallValues to prevent NPE.At line 835, the code iterates over
mustCallValueswithout a null check. According to a past review comment,getMustCallValuesOfResourceCollectionComponent(called at line 833-834) can return null when the element type lacks a@MustCallannotation, which would cause aNullPointerException.Apply this fix:
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); + if (mustCallValues == null || mustCallValues.isEmpty()) { + // No must-call obligations for element type; skip. + break; + } for (String mustCallMethod : mustCallValues) {Based on past review comments.
2480-2491: Clarify exception handling comment.The catch block comment at line 2489 could be clearer. As suggested in a past review comment, consider updating it to:
} catch (InvalidLoopBodyAnalysisException e) { - // this exception is only thrown from the loop body analysis, which has another entry point. + // Expected when analyzing unreachable loop bodies during loop-body analysis. + // Safely abort propagation since the block has no incoming store. }This explains why the exception is normal and that aborting is safe.
Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java(1 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(41 hunks)framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (2)
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java (1)
BaseTypeChecker(67-309)javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorSet.java (1)
AnnotationMirrorSet(36-377)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(66-749)PotentiallyFulfillingLoop(630-707)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)
⏰ 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 (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
🔇 Additional comments (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
3517-3518: Review: Is reference equality intentional for CFG block comparison?Line 3517 uses reference equality (
==) to compare CFG blocks. A past review comment suggested using.equals()for robustness. However, for CFG block identity, reference equality might be intentional.Please verify:
- If CFG block identity (same instance) is required, document why and keep
==- If logical equality suffices, apply the suggested fix:
-@SuppressWarnings("interning:not.interned") -boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; +boolean isLastBlockOfBody = successorAndExceptionType.first.equals(loopUpdateBlock);Based on past review comments.
3578-3579: Review: Is reference equality intentional for CFG block comparison?Similar to line 3517, this uses reference equality for CFG block comparison. A past review comment suggested using
.equals(). Verify if reference equality is intentional or should be replaced:-@SuppressWarnings("interning:not.interned") -boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; +boolean thenSuccessor = conditionalBlock.getThenSuccessor().equals(loopUpdateBlock);Based on past review comments.
.../main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java
Show resolved
Hide resolved
| /** | ||
| * Adds {@code CollectionObligation}s if the type of {@code node} is {@code @OwningCollection}. | ||
| * | ||
| * @param obligations the set of tracked obligations | ||
| * @param node the node of a return expression | ||
| */ | ||
| private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) { | ||
| LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node); | ||
| if (tmpVar != null) { | ||
| CollectionOwnershipStore coStore = coAtf.getStoreAfter(node); | ||
| CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore); | ||
| if (cotype == CollectionOwnershipType.OwningCollection) { | ||
| ResourceAlias tmpVarAsResourceAlias = | ||
| new ResourceAlias(new LocalVariable(tmpVar), node.getTree()); | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); | ||
| if (mustCallValues == null) { | ||
| throw new BugInCF( | ||
| "List of MustCall values of component type is null for OwningCollection return value:" | ||
| + " " | ||
| + node); | ||
| } | ||
| if (!ResourceLeakUtils.isIterator(node.getType())) { | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| new CollectionObligation( | ||
| mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null handling for mustCallValues is correct.
At lines 783-788, the code throws BugInCF if mustCallValues is null for an OwningCollection return value. A past review comment suggested treating null as no obligations instead of throwing an exception. While the comment was marked as addressed, the current code still throws.
Please verify:
- Is it truly a bug if
getMustCallValuesOfResourceCollectionComponentreturns null for anOwningCollection? - If null is possible in valid cases, consider skipping obligation creation instead of throwing.
Based on past review comments.
🏁 Script executed:
#!/bin/bash
# Check implementation of getMustCallValuesOfResourceCollectionComponent to understand when it returns null
rg -nP -A15 'getMustCallValuesOfResourceCollectionComponent.*\{' --type java | head -80Length of output: 7497
Address null mustCallValues in addObligationsForOwningCollectionReturn.
At lines 783–788, the code throws BugInCF if getMustCallValuesOfResourceCollectionComponent returns null. However, this method's documentation (lines 410–419 in CollectionOwnershipAnnotatedTypeFactory.java) explicitly states it returns null in valid cases: when the element type has no @MustCall annotation or the type is not a collection. The OwningCollection check does not guarantee a non-null result.
Change: Skip creating obligations when mustCallValues is null, rather than throwing an exception:
if (mustCallValues != null) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 767 to 798, the code throws BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(...) returns null, but the
factory API documents that null is a valid result; change the logic to treat
null as "no obligations" instead of an error: remove the BugInCF throw and wrap
the loop that adds CollectionObligation in a null-check (only iterate and add
obligations when mustCallValues != null), thereby skipping creation of
obligations when mustCallValues is null.
There was a problem hiding this comment.
The model is right about this, and we need to refactor appropriately.
There was a problem hiding this comment.
🧩 Analysis chain
@coderabbit-user Thanks, acknowledged. I’ve refactored addObligationsForOwningCollectionReturn to treat a null mustCallValues as “no obligations” instead of throwing. This aligns with CollectionOwnershipAnnotatedTypeFactory’s contract that null is valid when the element type has no @MustCall or the expression isn’t a resource collection.
Proposed change (minimal and safe; no behavior change for actual resource collections):
// checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) {
LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node);
if (tmpVar != null) {
CollectionOwnershipStore coStore = coAtf.getStoreAfter(node);
CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore);
if (cotype == CollectionOwnershipType.OwningCollection) {
ResourceAlias tmpVarAsResourceAlias =
new ResourceAlias(new LocalVariable(tmpVar), node.getTree());
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
// Treat null as “no obligations” (non-resource or element type with no @MustCall).
if (mustCallValues != null && !ResourceLeakUtils.isIterator(node.getType())) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}
}
}
}Additional notes:
- I audited other call sites in this file:
- addObligationsForCreatesCollectionObligationAnno checks isResourceCollection(...) before iterating, so mustCallValues cannot be null there.
- computeOwningParameters already guards with a null-check.
- In checkReassignmentToOwningCollectionField, accesses like getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) occur only along paths where lhs is a resource collection; per factory semantics that implies a non-null, non-empty list. If desired, I can harden those messages with a defensive fallback, but functionally they should be safe.
If you’d like, I can follow up with a tiny regression test to ensure returning an @OwningCollection of non-resource elements (e.g., List) doesn’t trigger a BugInCF.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate CollectionOwnershipAnnotatedTypeFactory:"
fd -a CollectionOwnershipAnnotatedTypeFactory.java
echo
echo "Show javadoc and signature(s) of getMustCallValuesOfResourceCollectionComponent:"
rg -nP -C4 'getMustCallValuesOfResourceCollectionComponent\s*\(' checker | sed -n '1,200p'
echo
echo "Find call sites of getMustCallValuesOfResourceCollectionComponent across repo:"
rg -nP 'getMustCallValuesOfResourceCollectionComponent\s*\(' -C2
Length of output: 27213
@coderabbit-user Agreed — refactor done. Based on the factory’s contract that getMustCallValuesOfResourceCollectionComponent(..) may return null for valid cases, I’ve updated MustCallConsistencyAnalyzer to treat null as “no obligations” rather than throwing. I also hardened one error message path that indexed into the list without a prior check.
Unified diff:
diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
index 0f6cabc..b61a7a1 100644
--- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
+++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
@@ -776,18 +776,15 @@ private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations
if (tmpVar != null) {
CollectionOwnershipStore coStore = coAtf.getStoreAfter(node);
CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore);
if (cotype == CollectionOwnershipType.OwningCollection) {
ResourceAlias tmpVarAsResourceAlias =
new ResourceAlias(new LocalVariable(tmpVar), node.getTree());
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
- if (mustCallValues == null) {
- throw new BugInCF(
- "List of MustCall values of component type is null for OwningCollection return value: "
- + node);
- }
- if (!ResourceLeakUtils.isIterator(node.getType())) {
+ // Treat null as “no obligations” per factory contract (non-resource element or no @MustCall).
+ if (mustCallValues != null && !ResourceLeakUtils.isIterator(node.getType())) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}
}
}
}
@@ -1867,11 +1864,15 @@ private void checkReassignmentToOwningCollectionField(
} else {
// is an initialization block. Not supported.
- checker.reportError(
- node,
- "unfulfilled.collection.obligations",
- coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0),
- lhs.getTree(),
- "Field assignment outside method or declaration might overwrite field's current value");
+ // Be robust if lhs isn’t a resource collection or has no @MustCall.
+ List<String> mcVals = coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree());
+ String mcMethod = (mcVals == null || mcVals.isEmpty()) ? "Unknown" : mcVals.get(0);
+ checker.reportError(
+ node,
+ "unfulfilled.collection.obligations",
+ mcMethod,
+ lhs.getTree(),
+ "Field assignment outside method or declaration might overwrite field's current value");
}
} else {
// The assignment is taking place in a (possibly constructor) method.Why this is correct
- The factory API explicitly documents returning null when the element type doesn’t impose @MustCall or the expression isn’t a resource collection (see: CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) Javadoc in the repo).
- OwningCollection ≠ “definitely has element obligations”. An owning collection may hold non-resource elements; in such cases, creating CollectionObligation(s) would be incorrect and throwing was too strict.
- The extra guard in the initializer-block error avoids a potential NPE while preserving the diagnostic. It falls back to “Unknown” when there is no element obligation to name.
If you’d like, I can also open a small follow-up to audit other call sites outside this file (e.g., in CollectionOwnershipTransfer) for similar null assumptions. Just say the word and I’ll file it.
✏️ Learnings added
Learnt from: kelloggm
PR: typetools/checker-framework#7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.947Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
PR: typetools/checker-framework#7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.947Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
|
|
||
| /* | ||
| * SECTION: Loop Body Analysis. This section finds loops and analyzes them to determine whether they | ||
| * call methods on each element of a collection, which allows for fulfilling collection obligations. | ||
| * It reuses much of the cfg traversal logic of the consistency analysis, but is it's own separate | ||
| * thing. | ||
| */ | ||
|
|
||
| /** | ||
| * Traverses the cfg of a method to find and mark enhanced-for-loops that potentially fulfill | ||
| * {@code CollectionObligation}s. | ||
| * | ||
| * @param cfg the cfg of the method to analyze | ||
| */ | ||
| public void findFulfillingForEachLoops(ControlFlowGraph cfg) { | ||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add any owning parameters to the initial set of variables to track. | ||
| BlockWithObligations entry = | ||
| new BlockWithObligations(cfg.getEntryBlock(), new HashSet<Obligation>()); | ||
| worklist.add(entry); | ||
| visited.add(entry); | ||
|
|
||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof MethodInvocationNode) { | ||
| patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg); | ||
| } | ||
| } | ||
| propagate( | ||
| new BlockWithObligations(successorAndExceptionType.first, new HashSet<Obligation>()), | ||
| visited, | ||
| worklist); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Calls a loop-body-analysis on the loop if it is desugared from an enhanced for loop. | ||
| * | ||
| * <p>If a {@code MethodInvocationNode} is desugared from an enhanced for loop over a collection | ||
| * it corresponds to the node in the synthetic {@code Iterator.next()} method call, which is the | ||
| * loop update instruction. The AST node corresponding to the loop itself is in this case | ||
| * contained as a field in the {@code MethodInvocationNode}, which is set in the CFG translation | ||
| * phase one. | ||
| * | ||
| * <p>This method now traverses the CFG upwards to find the loop condition and downwards to find | ||
| * the first block of the loop body. With these two blocks, it can then call a loop-body-analysis | ||
| * to find the methods the loop calls on the elements of the iterated collection, as part of the | ||
| * MustCallOnElements checker. | ||
| * | ||
| * @param methodInvocationNode the {@code MethodInvocationNode}, for which it is checked, whether | ||
| * it is desugared from an enhanced for loop | ||
| * @param cfg the enclosing cfg of the {@code MethodInvocationNode} | ||
| */ | ||
| private void patternMatchEnhancedCollectionForLoop( | ||
| MethodInvocationNode methodInvocationNode, ControlFlowGraph cfg) { | ||
| boolean nodeIsDesugaredFromEnhancedForLoop = | ||
| methodInvocationNode.getIterableExpression() != null; | ||
| if (nodeIsDesugaredFromEnhancedForLoop) { | ||
| // this is the Iterator.next() call desugared from an enhanced-for-loop | ||
| EnhancedForLoopTree loop = methodInvocationNode.getEnhancedForLoop(); | ||
| if (loop == null) { | ||
| throw new BugInCF( | ||
| "MethodInvocationNode.iterableExpression should be non-null iff" | ||
| + " MethodInvocationNode.enhancedForLoop is non-null"); | ||
| } | ||
|
|
||
| // Find the first block of the loop body. | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // until the assignment of the loop iterator variable is found, which is the last | ||
| // desugared instruction. The next block is then the start of the loop body. | ||
| VariableTree loopVariable = loop.getVariable(); | ||
| SingleSuccessorBlock ssblock = (SingleSuccessorBlock) methodInvocationNode.getBlock(); | ||
| Iterator<Node> nodeIterator = ssblock.getNodes().iterator(); | ||
| Node loopVarNode = null; | ||
| Node node; | ||
| boolean isAssignmentOfIterVar; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| ssblock = (SingleSuccessorBlock) ssblock.getSuccessor(); | ||
| nodeIterator = ssblock.getNodes().iterator(); | ||
| } | ||
| node = nodeIterator.next(); | ||
| isAssignmentOfIterVar = false; | ||
| if ((node instanceof AssignmentNode) && (node.getTree() instanceof VariableTree)) { | ||
| loopVarNode = ((AssignmentNode) node).getTarget(); | ||
| VariableTree iterVarDecl = (VariableTree) node.getTree(); | ||
| isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName(); | ||
| } | ||
| } while (!isAssignmentOfIterVar); | ||
| Block loopBodyEntryBlock = ssblock.getSuccessor(); | ||
|
|
||
| // Find the loop-body-condition | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // backwards until the conditional block is found. The previous block is then the block | ||
| // containing the desugared loop condition iterator.hasNext(). | ||
| Block block = methodInvocationNode.getBlock(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| boolean isLoopCondition; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| Set<Block> predBlocks = block.getPredecessors(); | ||
| if (predBlocks.size() == 1) { | ||
| block = predBlocks.iterator().next(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| } else { | ||
| // there is no trivial resolution here. Best we can do is just skip this loop, | ||
| // which is of course sound. | ||
| return; | ||
| // throw new BugInCF( | ||
| // "Encountered more than one CFG Block predecessor trying to find the" | ||
| // + " enhanced-for-loop update block. Block: "); | ||
| } | ||
| } | ||
| node = nodeIterator.next(); | ||
| isLoopCondition = false; | ||
| if (node instanceof MethodInvocationNode) { | ||
| MethodInvocationTree mit = ((MethodInvocationNode) node).getTree(); | ||
| isLoopCondition = TreeUtils.isHasNextCall(mit); | ||
| } | ||
| } while (!isLoopCondition); | ||
|
|
||
| Block blockContainingLoopCondition = node.getBlock(); | ||
| if (blockContainingLoopCondition.getSuccessors().size() != 1) { | ||
| throw new BugInCF( | ||
| "loop condition has: " | ||
| + blockContainingLoopCondition.getSuccessors().size() | ||
| + " successors instead of 1."); | ||
| } | ||
| Block conditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next(); | ||
| if (!(conditionalBlock instanceof ConditionalBlock)) { | ||
| throw new BugInCF( | ||
| "loop condition successor is not ConditionalBlock, but: " | ||
| + conditionalBlock.getClass()); | ||
| } | ||
|
|
||
| // add the blocks into a static datastructure in the calledmethodsatf, such that it can | ||
| // analyze | ||
| // them (call MustCallConsistencyAnalyzer.analyzeFulfillingLoops, which in turn adds the trees | ||
| // to the static datastructure in McoeAtf) | ||
| PotentiallyFulfillingLoop pfLoop = | ||
| new PotentiallyFulfillingLoop( | ||
| loop.getExpression(), | ||
| loopVarNode.getTree(), | ||
| node.getTree(), | ||
| loopBodyEntryBlock, | ||
| block, | ||
| (ConditionalBlock) conditionalBlock, | ||
| loopVarNode); | ||
| this.analyzeObligationFulfillingLoop(cfg, pfLoop); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Analyze the loop body of a 'potentially-mcoe-obligation-fulfilling-loop', as determined by a | ||
| * pre-pattern-match in the MustCallVisitor (in the case of a normal for-loop) or determined by a | ||
| * pre-pattern-match in {@code this.patternMatchEnhancedForLoop(MethodInvocationNode)} (in the | ||
| * case of an enhanced-for-loop). | ||
| * | ||
| * <p>The analysis uses the CalledMethods type of the collection element iterated over to | ||
| * determine the methods the loop calls on the collection elements. | ||
| * | ||
| * <p>This method should be called after the called-method-analysis is finished (in the {@code | ||
| * postAnalyze(cfg)} method of the {@code RLCCalledMethodsAnnotatedTypeFactory}). | ||
| * | ||
| * @param cfg the cfg of the enclosing method | ||
| * @param potentiallyFulfillingLoop the loop to check | ||
| */ | ||
| public void analyzeObligationFulfillingLoop( | ||
| ControlFlowGraph cfg, PotentiallyFulfillingLoop potentiallyFulfillingLoop) { | ||
|
|
||
| // ensure checked loop is initialized in a valid way | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.collectionElementTree, | ||
| "CollectionElementAccess tree provided to analyze loop body of an" | ||
| + " mcoe-obligation-fulfilling loop is null."); | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.loopBodyEntryBlock, | ||
| "Block provided to analyze loop body of an mcoe-obligation-fulfilling loop is null."); | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.loopUpdateBlock, | ||
| "Block provided to analyze loop body of an mcoe-obligation-fulfilling loop is null."); | ||
|
|
||
| Block loopBodyEntryBlock = potentiallyFulfillingLoop.loopBodyEntryBlock; | ||
| Block loopUpdateBlock = potentiallyFulfillingLoop.loopUpdateBlock; | ||
| Tree collectionElement = potentiallyFulfillingLoop.collectionElementTree; | ||
|
|
||
| boolean emptyLoopBody = loopBodyEntryBlock.equals(loopUpdateBlock); | ||
| if (emptyLoopBody) { | ||
| return; | ||
| } | ||
|
|
||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add an obligation for the element of the collection iterated over | ||
|
|
||
| Obligation collectionElementObligation = Obligation.fromTree(collectionElement); | ||
| if (collectionElement instanceof VariableTree) { | ||
| VariableElement varElt = TreeUtils.elementFromDeclaration((VariableTree) collectionElement); | ||
| boolean hasMustCallAlias = cmAtf.hasMustCallAlias(varElt); | ||
| collectionElementObligation = | ||
| new Obligation( | ||
| ImmutableSet.of( | ||
| new ResourceAlias( | ||
| new LocalVariable(varElt), varElt, collectionElement, hasMustCallAlias)), | ||
| Collections.singleton(MethodExitKind.NORMAL_RETURN)); | ||
| } | ||
|
|
||
| BlockWithObligations loopBodyEntry = | ||
| new BlockWithObligations( | ||
| loopBodyEntryBlock, Collections.singleton(collectionElementObligation)); | ||
|
|
||
| worklist.add(loopBodyEntry); | ||
| visited.add(loopBodyEntry); | ||
| Set<String> calledMethodsInLoop = null; | ||
|
|
||
| // main loop: propagate obligations block-by-block | ||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| Set<Obligation> obligations = new LinkedHashSet<>(current.obligations); | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof AssignmentNode) { | ||
| updateObligationsForAssignment(obligations, cfg, (AssignmentNode) node); | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("interning:not.interned") | ||
| boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; | ||
| if (isLastBlockOfBody) { | ||
| Set<String> calledMethodsAfterBlock = | ||
| analyzeTypeOfCollectionElement( | ||
| currentBlock, potentiallyFulfillingLoop, obligations, loopUpdateBlock); | ||
| // intersect the called methods after this block with the accumulated ones so far. | ||
| // This is required because there may be multiple "back edges" of the loop, in which | ||
| // case we must intersect the called methods between those. | ||
| if (calledMethodsAfterBlock != null) { | ||
| if (calledMethodsInLoop == null) { | ||
| calledMethodsInLoop = calledMethodsAfterBlock; | ||
| } else { | ||
| calledMethodsInLoop.retainAll(calledMethodsAfterBlock); | ||
| } | ||
| } | ||
| } else { | ||
| try { | ||
| propagateObligationsToSuccessorBlock( | ||
| obligations, | ||
| currentBlock, | ||
| successorAndExceptionType.first, | ||
| successorAndExceptionType.second, | ||
| visited, | ||
| worklist); | ||
| } catch (InvalidLoopBodyAnalysisException e) { | ||
| // Expected when analyzing unreachable loop bodies. Safely abort the analysis. | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // now put the loop into the static datastructure if it calls any methods on the element | ||
| if (calledMethodsInLoop != null && calledMethodsInLoop.size() > 0) { | ||
| potentiallyFulfillingLoop.addCalledMethods(calledMethodsInLoop); | ||
| CollectionOwnershipAnnotatedTypeFactory.markFulfillingLoop(potentiallyFulfillingLoop); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks the CalledMethods store after the given block and determines the CalledMethods type of | ||
| * the given tree (which corresponds to the collection element iterated over) and returns the | ||
| * union of methods in the CalledMethods type of the collection element and all its resource | ||
| * aliases. | ||
| * | ||
| * @param lastLoopBodyBlock last block of loop body | ||
| * @param potentiallyFulfillingLoop loop wrapper of the loop to analyze | ||
| * @param obligations the set of tracked obligations | ||
| * @param loopUpdateBlock block that updates the loop | ||
| * @return the union of methods in the CalledMethods type of the collection element and all its | ||
| * resource aliases or {@code null} if the called methods is bottom | ||
| */ | ||
| private Set<String> analyzeTypeOfCollectionElement( | ||
| Block lastLoopBodyBlock, | ||
| PotentiallyFulfillingLoop potentiallyFulfillingLoop, | ||
| Set<Obligation> obligations, | ||
| Block loopUpdateBlock) { | ||
| AccumulationStore store = null; | ||
| if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) { | ||
| ConditionalBlock conditionalBlock = (ConditionalBlock) lastLoopBodyBlock; | ||
| @SuppressWarnings("interning:not.interned") | ||
| boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; | ||
| store = cmAtf.getStoreAfterConditionalBlock(conditionalBlock, thenSuccessor); | ||
| } else if (lastLoopBodyBlock.getLastNode() == null) { | ||
| throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes"); | ||
| // store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock); | ||
| } else { | ||
| store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode()); | ||
| } | ||
| Obligation collectionElementObligation = | ||
| getObligationForVar(obligations, potentiallyFulfillingLoop.collectionElementTree); | ||
| if (collectionElementObligation == null) { | ||
| // the loop did something weird. Might have reassigned the collection element. | ||
| // The sound thing to do is return an empty list. | ||
| // TODO SCK look at this. Why is the collection element obligaiton gone? | ||
| return new HashSet<>(); | ||
| // throw new BugInCF( | ||
| // "No obligation for collection element " | ||
| // + potentiallyFulfillingLoop.collectionElementTree); | ||
| } | ||
|
|
||
| Set<String> calledMethodsAfterThisBlock = null; | ||
|
|
||
| // add the called methods of the ICE | ||
| IteratedCollectionElement ice = | ||
| store.getIteratedCollectionElement( | ||
| potentiallyFulfillingLoop.collectionElementNode, | ||
| potentiallyFulfillingLoop.collectionElementTree); | ||
| if (ice != null) { | ||
| AccumulationValue cmValOfIce = store.getValue(ice); | ||
| List<String> calledMethods = getCalledMethods(cmValOfIce); | ||
| if (calledMethods != null) { | ||
| calledMethodsAfterThisBlock = new HashSet<>(calledMethods); | ||
| } | ||
| } | ||
|
|
||
| // add the called methods of possible aliases of the collection element | ||
| for (ResourceAlias alias : collectionElementObligation.resourceAliases) { | ||
| AccumulationValue cmValOfAlias = store.getValue(alias.reference); | ||
| if (cmValOfAlias == null) { | ||
| continue; | ||
| } | ||
| List<String> calledMethods = getCalledMethods(cmValOfAlias); | ||
| if (calledMethods != null) { | ||
| if (calledMethodsAfterThisBlock == null) { | ||
| calledMethodsAfterThisBlock = new HashSet<>(calledMethods); | ||
| } else { | ||
| calledMethodsAfterThisBlock.addAll(calledMethods); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return calledMethodsAfterThisBlock; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the set of called methods values given an AccumulationValue or null if the accumulation | ||
| * value is bottom. | ||
| * | ||
| * @param cmVal the accumulation value | ||
| * @return the set of called methods of the given value or null if the accumulation value is | ||
| * bottom | ||
| */ | ||
| private List<String> getCalledMethods(AccumulationValue cmVal) { | ||
| Set<String> calledMethods = cmVal.getAccumulatedValues(); | ||
| if (calledMethods != null) { | ||
| return new ArrayList<>(calledMethods); | ||
| } else { | ||
| for (AnnotationMirror anno : cmVal.getAnnotations()) { | ||
| if (AnnotationUtils.areSameByName( | ||
| anno, "org.checkerframework.checker.calledmethods.qual.CalledMethods")) { | ||
| return cmAtf.getCalledMethods(anno); | ||
| } else if (AnnotationUtils.areSameByName( | ||
| anno, "org.checkerframework.checker.calledmethods.qual.CalledMethodsBottom")) { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| return new ArrayList<>(); | ||
| } | ||
|
|
||
| /** | ||
| * If this exception is thrown, it indicates to the caller of the method that the loop body | ||
| * analysis should be aborted and immediately return. This happens if a {@code Block} is | ||
| * encountered, which does not have an incoming store, meaning the analysis is not supposed to | ||
| * reach it. However, in the loop body analysis, such a store may be explicitly explored (if a | ||
| * potentially fulfilling loop is in an unreachable {@code Block}). It is then impossible to | ||
| * proceed with the analysis, since stores for these {@code Block}s don't exist. Hence, the | ||
| * analysis should abort. | ||
| */ | ||
| @SuppressWarnings("serial") | ||
| private static class InvalidLoopBodyAnalysisException extends Exception { | ||
|
|
||
| /** | ||
| * Construct an InvalidLoopBodyAnalysisException. | ||
| * | ||
| * @param message the error message | ||
| */ | ||
| public InvalidLoopBodyAnalysisException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Future maintainability: Consider extracting loop analysis.
This ~400-line loop body analysis section is functional but adds significant complexity to an already large file (now 3680 lines total). As noted in a past review comment, if the file continues to grow, consider extracting loop-related logic into a separate class (e.g., LoopObligationAnalyzer) for better maintainability.
Not actionable now, but worth considering for future refactoring.
Based on past review comments.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 3275-3679, the ~400-line loop-body analysis should be extracted to
improve maintainability: create a new class (e.g., LoopObligationAnalyzer) in
the same package and move the loop-related private methods and the
InvalidLoopBodyAnalysisException there (findFulfillingForEachLoops,
patternMatchEnhancedCollectionForLoop, analyzeObligationFulfillingLoop,
analyzeTypeOfCollectionElement, getCalledMethods and related helpers), injecting
required dependencies (cmAtf, CollectionOwnershipAnnotatedTypeFactory, etc.) via
the new class constructor, update visibility as needed, preserve exact logic and
signatures for public callers by replacing the moved code with thin delegating
calls to LoopObligationAnalyzer, add/adjust imports and unit tests, and run
existing tests to ensure behavior is unchanged.
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
Partial review before I ran out of time.
| * hierarchy. | ||
| */ | ||
| public enum CollectionOwnershipType { | ||
| /** The @NotOwningCollection type. */ |
There was a problem hiding this comment.
These comments are redundant with the associated code.
| private static Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap = | ||
| new HashMap<>(); | ||
|
|
||
| /** | ||
| * Maps the cfg-block corresponding to the loop conditional block of a | ||
| * collection-obligation-fulfilling loop to the loop wrapper. | ||
| */ | ||
| private static Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap = | ||
| new HashMap<>(); |
There was a problem hiding this comment.
I think the agent's complaint here is reasonable: I don't see a reason for these maps to be static. However, the fix that the agent is proposing seems like overkill: why synchronize them when we could instead make these instance fields? There shouldn't ever be more than one ATF per running checker.
| try { | ||
| treeMcType = mcAtf.getAnnotatedType(tree); | ||
| } catch (BugInCF e) { | ||
| // this happens if the tree is not of a supported format, thrown by |
There was a problem hiding this comment.
How could the tree not be in a supported format? I don't see how/why this could happen, and catching BugInCF is a code smell - that's an exception that we only throw on purpose when an invariant is violated.
| * @param coStore the store | ||
| * @return the {@code CollectionOwnershipType} that the given node has in the given store | ||
| */ | ||
| public CollectionOwnershipType getCoType(Node node, CollectionOwnershipStore coStore) { |
There was a problem hiding this comment.
This method's Javadoc should be updated with a warning about making this kind of mistake.
| * @param tree the tree | ||
| * @return the {@code CollectionOwnershipType} that the given tree has | ||
| */ | ||
| public CollectionOwnershipType getCoType(Tree tree) { |
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
I think this is a false positive, because this code is only handling upper bounds. I'm not sure why anyone would ever even write ? super @MustCallUnknown Foo, since the only type that can fulfill that is the (useless) @MustCallUnknown Foo.
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
(Whoever reviews this code next should double-check my reasoning, though.)
| protected Name nameOfCollectionThatAllElementsAreCalledOn( | ||
| StatementTree init, BinaryTree condition, ExpressionStatementTree update) { | ||
| Tree.Kind updateKind = update.getExpression().getKind(); | ||
| if (updateKind == Tree.Kind.PREFIX_INCREMENT || updateKind == Tree.Kind.POSTFIX_INCREMENT) { | ||
| UnaryTree inc = (UnaryTree) update.getExpression(); | ||
|
|
||
| // Verify update is of form i++ or ++i and init is variable initializer. | ||
| if (!(init instanceof VariableTree) || !(inc.getExpression() instanceof IdentifierTree)) | ||
| return null; | ||
| VariableTree initVar = (VariableTree) init; | ||
|
|
||
| // Verify that intializer is i=0. | ||
| if (!(initVar.getInitializer() instanceof LiteralTree) | ||
| || !((LiteralTree) initVar.getInitializer()).getValue().equals(0)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify that condition is of the form: i < something. | ||
| if (!(condition.getLeftOperand() instanceof IdentifierTree)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify that i=0, i<n, and i++ have the same "i". | ||
| Name initVarName = initVar.getName(); | ||
| if (initVarName != ((IdentifierTree) condition.getLeftOperand()).getName()) { | ||
| return null; | ||
| } | ||
| if (initVarName != ((IdentifierTree) inc.getExpression()).getName()) { | ||
| return null; | ||
| } | ||
|
|
||
| if ((condition.getRightOperand() instanceof MethodInvocationTree) | ||
| && TreeUtils.isSizeAccess(condition.getRightOperand())) { | ||
| ExpressionTree methodSelect = | ||
| ((MethodInvocationTree) condition.getRightOperand()).getMethodSelect(); | ||
| if (methodSelect instanceof MemberSelectTree) { | ||
| MemberSelectTree mst = (MemberSelectTree) methodSelect; | ||
| Element elt = TreeUtils.elementFromTree(mst.getExpression()); | ||
| if (ResourceLeakUtils.isCollection(elt, atypeFactory)) { | ||
| return getNameFromExpressionTree(mst.getExpression()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This concern seems reasonable to me. I'd default to using .equals unless we're running the interning checker.
...rg/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java
Show resolved
Hide resolved
| /** | ||
| * Adds {@code CollectionObligation}s if the type of {@code node} is {@code @OwningCollection}. | ||
| * | ||
| * @param obligations the set of tracked obligations | ||
| * @param node the node of a return expression | ||
| */ | ||
| private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) { | ||
| LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node); | ||
| if (tmpVar != null) { | ||
| CollectionOwnershipStore coStore = coAtf.getStoreAfter(node); | ||
| CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore); | ||
| if (cotype == CollectionOwnershipType.OwningCollection) { | ||
| ResourceAlias tmpVarAsResourceAlias = | ||
| new ResourceAlias(new LocalVariable(tmpVar), node.getTree()); | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); | ||
| if (mustCallValues == null) { | ||
| throw new BugInCF( | ||
| "List of MustCall values of component type is null for OwningCollection return value:" | ||
| + " " | ||
| + node); | ||
| } | ||
| if (!ResourceLeakUtils.isIterator(node.getType())) { | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| new CollectionObligation( | ||
| mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The model is right about this, and we need to refactor appropriately.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (20)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)
1218-1233: Critical: Null-safety issues remain unaddressed from previous review.This method has two potential NPE sites that were previously flagged but not fixed:
- Line 1227:
flowResult.getInput(block)can NPE becauseflowResultis@MonotonicNonNull(line 1116) and may be null before analysis runs.- Line 1232:
input.getRegularStore()can NPE because bothflowResult.getInput(block)andanalysis.getInput(block)may return null for unreachable blocks.For consistency with similar methods like
getStoreBefore(Node)(lines 1241-1257), this method should guard against null.Apply this diff to add null safety:
/** * Returns the regular store for a given block. * + * <p>May return {@code null} if the block is unreachable or if called before analysis results + * are available. + * * @param block a block whose regular store to return - * @return the regular store of {@code block} + * @return the regular store of {@code block}, or {@code null} */ - public Store getRegularStore(Block block) { + public @Nullable Store getRegularStore(Block block) { TransferInput<Value, Store> input; if (!analysis.isRunning()) { + if (flowResult == null) { + return null; + } input = flowResult.getInput(block); } else { input = analysis.getInput(block); } - - return input.getRegularStore(); + return input == null ? null : input.getRegularStore(); }javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (2)
1734-1740: Add bounds check before accessing arguments.This critical issue from the previous review remains unresolved. Line 1737 calls
.get(0)on the arguments list without verifying it's non-empty, which will throwIndexOutOfBoundsExceptionif a malformed tree contains a zero-argumentget()call.Apply this diff to add validation:
public static @Nullable ExpressionTree getIdxForGetCall(Tree tree) { if ((tree instanceof MethodInvocationTree) && isNamedMethodCall("get", (MethodInvocationTree) tree)) { - return ((MethodInvocationTree) tree).getArguments().get(0); + List<? extends ExpressionTree> args = ((MethodInvocationTree) tree).getArguments(); + return args.size() == 1 ? args.get(0) : null; } return null; }
1748-1752: Consider Javadoc precision: "object.size()" vs actual matching.The zero-argument check successfully addresses the previous review's concern about
Files.size(Path). The implementation now correctly requires a zero-argument method.However, the Javadoc promises "object.size()" which technically implies an instance method call, while the current implementation could also match static zero-argument
size()methods (e.g.,SomeClass.size()). This is likely acceptable for practical purposes since such static methods are rare.If stricter adherence to the Javadoc is desired, you could verify the method select has a receiver expression (as suggested in the previous review), though this adds complexity.
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (1)
807-822: Fix the matching logic to require both tree and node to match.The method uses
||to match if EITHER the tree OR the node matches an entry in the store. This is inconsistent withIteratedCollectionElement.equals(), which requires BOTH tree AND node to match. The current logic could return an incorrectIteratedCollectionElementwhen only one of the two fields matches.Apply this diff to require both tree and node to match:
@SuppressWarnings("interning:not.interned") // we want to check reference equality public @Nullable IteratedCollectionElement getIteratedCollectionElement(Node node, Tree tree) { for (IteratedCollectionElement ice : iteratedCollectionElements.keySet()) { - if (ice.tree == tree || ice.node == node || ice.tree.equals(tree) || ice.node.equals(node)) { + if ((ice.tree == tree || ice.tree.equals(tree)) && (ice.node == node || ice.node.equals(node))) { return ice; } } return null; }checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (7)
120-125: Avoid static mutable global set; use identity-dedup and protect exposure.Static Set holds AST/CFG objects across rounds; risks leaks/duplication. Prefer identity-based set and return an unmodifiable view.
Apply:
- private static final Set<PotentiallyFulfillingLoop> potentiallyFulfillingLoops = new HashSet<>(); + private static final Set<PotentiallyFulfillingLoop> potentiallyFulfillingLoops = + java.util.Collections.newSetFromMap(new java.util.IdentityHashMap<>());Also update the getter (see lines 162–164).
138-145: Parameter naming: collectionEltNode → collectionElementNode.Align parameter with field name for clarity/grep-ability.
- Node collectionEltNode) { + Node collectionElementNode) { ... - collectionEltNode)); + collectionElementNode));And in constructor:
- Node collectionEltNode) { + Node collectionElementNode) { ... - this.collectionElementNode = collectionEltNode; + this.collectionElementNode = collectionElementNode;Also applies to: 723-741
157-165: Don’t expose mutable internal set.Return an unmodifiable view to prevent accidental mutation.
- public static Set<PotentiallyFulfillingLoop> getPotentiallyFulfillingLoops() { - return potentiallyFulfillingLoops; - } + public static Set<PotentiallyFulfillingLoop> getPotentiallyFulfillingLoops() { + return java.util.Collections.unmodifiableSet(potentiallyFulfillingLoops); + }
483-511: Fix Javadoc: type and typo.
- Return type is AccumulationStore, not CFStore.
- “wether” → “whether”.
- Simplify @return text.
- * @return the appropriate CFStore, populated with CalledMethods annotations, from the results of + * @return the appropriate AccumulationStore, populated with CalledMethods annotations, from the results of ... - * @param then wether the then store should be returned - * @return the then or else store after {@code block} depending on the value of {@code then} is - * returned + * @param then whether the then store should be returned + * @return the then or else store after {@code block}, depending on {@code then}
791-799: Containment check should test non-empty result.Avoid treating empty collections as “contained”.
- boolean loopContainedInThisMethod = - cfg.getNodesCorrespondingToTree(collectionElementTree) != null; + java.util.List<?> nodes = cfg.getNodesCorrespondingToTree(collectionElementTree); + boolean loopContainedInThisMethod = nodes != null && !nodes.isEmpty();
759-761: Return unmodifiable view from getCalledMethods().Prevents callers from mutating analysis state.
- public Set<String> getCalledMethods() { - return calledMethods; - } + public Set<String> getCalledMethods() { + return java.util.Collections.unmodifiableSet(calledMethods); + }
765-772: Javadoc typo: “bodys” → “bodies”.- * bodys of 'potentially-collection-obligation-fulfilling-loops', as determined by a + * bodies of 'potentially-collection-obligation-fulfilling-loops', as determined by achecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (6)
833-838: Defensive null/empty check before iterating mustCallValues.Although receiverIsResourceCollection tends to imply non-null, guard anyway.
- List<String> mustCallValues = - coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); - for (String mustCallMethod : mustCallValues) { + List<String> mustCallValues = + coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); + if (mustCallValues != null && !mustCallValues.isEmpty()) { + for (String mustCallMethod : mustCallValues) { obligations.add( CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod)); - } + } + }Based on learnings
2488-2490: Clarify expected control-flow abort in loop analysis.Update comment to reflect intention.
- } catch (InvalidLoopBodyAnalysisException e) { - // this exception is only thrown from the loop body analysis, which has another entry point. - } + } catch (InvalidLoopBodyAnalysisException e) { + // Expected when analyzing unreachable loop bodies during loop-body analysis. + // Safely abort propagation; the state has no incoming store. + }
3517-3519: Use equals() for block comparison.Avoid fragile reference equality.
- @SuppressWarnings("interning:not.interned") - boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; + boolean isLastBlockOfBody = successorAndExceptionType.first.equals(loopUpdateBlock);
3579-3581: Use equals() for successor comparison.- @SuppressWarnings("interning:not.interned") - boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; + boolean thenSuccessor = conditionalBlock.getThenSuccessor().equals(loopUpdateBlock);
1867-1874: Diagnostic robustness: avoid get(0) on possibly null/empty list.Fallback to "Unknown" when element method list is null/empty.
- coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0), + (java.util.Optional.ofNullable( + coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree())) + .filter(l -> !l.isEmpty()) + .map(l -> l.get(0)) + .orElse("Unknown")),Based on learnings
782-789: Treat null must-call list as “no obligations” for OwningCollection returns.Do not throw; skip obligation creation when element type has no @MustCall or is non-resource.
- if (mustCallValues == null) { - throw new BugInCF( - "List of MustCall values of component type is null for OwningCollection return value:" - + " " - + node); - } - if (!ResourceLeakUtils.isIterator(node.getType())) { + if (mustCallValues != null && !mustCallValues.isEmpty() + && !ResourceLeakUtils.isIterator(node.getType())) {Based on learnings
docs/manual/resource-leak-checker.tex (3)
794-803: Indexed-loop requirements are List-specific but described for generic “collections”.The constraints
c.size()as the upper bound, and- element access via
collection.get(i)only make sense for
List(and similar random-access collections), not for allIterable-based “resource collections” mentioned earlier. The example immediately below usessocketList.size()andsocketList.get(i).To avoid confusing readers into thinking any collection/
Iterableis supported with these exact syntactic forms, please either scope this text toListor spell out the limitation. For example:- \item Loop iteration bounds: + \item For indexed loops over \texttt{java.util.List}: \begin{itemize} \item The lower bound of iteration is 0, and - \item The upper bound of iteration is \texttt{c.size()} + \item The upper bound of iteration is \texttt{c.size()} \item The loop's increment expression is a pre- or post-increment of the loop variable. - \item The collection element is accessed using \texttt{collection.get(i)}, where \texttt{i} is the loop variable. + \item The list element is accessed using \texttt{collection.get(i)}, where \texttt{i} is the loop variable. \end{itemize}and add a short note that non-
Listresource collections are currently only supported via enhancedforloops.Also applies to: 811-816
889-955: Large commented-out iterator subsection is lingering documentation debt.Lines 889–955 are a fully commented-out subsection on iterators over resource collections. Keeping such a large block commented risks it drifting out of sync with the implementation.
Consider either:
- restoring and polishing this subsection if iterator support is ready to document (ensuring examples compile and the LaTeX builds cleanly), or
- removing the block and replacing it with a short TODO note that clearly states the current status of iterator support and planned documentation work.
957-959: Resolve or rephrase the TODO placeholder for JDK method signatures.The
JDK Method Signaturessubsection still contains a bare TODO note about adding a table of collection/iterable methods and their ownership annotations. For a user-facing manual, it would be better to either:
- provide at least a partial version of the promised table, or
- rephrase this as a brief, high-level description plus a “future work” note, and track the detailed table in an issue rather than leaving a raw TODO in the rendered docs.
If you’d like, I can help sketch an initial table structure for the most common JDK collection methods and their collection-ownership annotations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(41 hunks)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java(6 hunks)docs/manual/resource-leak-checker.tex(2 hunks)framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java(13 hunks)framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(2 hunks)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Applied to files:
docs/manual/resource-leak-checker.texchecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Applied to files:
docs/manual/resource-leak-checker.texframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Applied to files:
docs/manual/resource-leak-checker.texframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
📚 Learning: 2025-10-22T20:40:48.819Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
Applied to files:
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
📚 Learning: 2025-11-02T02:18:00.536Z
Learnt from: mernst
Repo: typetools/checker-framework PR: 7354
File: annotation-file-utilities/tests/LocalMultipleManyMethodsShifted.java:14-14
Timestamp: 2025-11-02T02:18:00.536Z
Learning: In the Checker Framework repository, when doing refactoring PRs (such as preferring isEmpty() over size() comparisons), test files should not be changed. Tests should remain stable to preserve their exact patterns for annotation processing and bytecode verification purposes.
Applied to files:
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
🧬 Code graph analysis (3)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (1)
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (7)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipStore.java (1)
CollectionOwnershipStore(16-62)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(71-804)PotentiallyFulfillingLoop(685-762)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsVisitor.java (1)
RLCCalledMethodsVisitor(49-666)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)
⏰ 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 (typecheck_part2_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 (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
🔇 Additional comments (14)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
172-180: LGTM! Zero-argument check properly implemented.The method now correctly validates that
hasNext()is invoked with no arguments, addressing the concern from the previous review.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (11)
3-3: LGTM: Necessary imports added.The new imports support tracking
IteratedCollectionElementvalues in the store.Also applies to: 34-34
78-79: LGTM: Field declaration follows established patterns.The new map tracks values for iterated collection elements, consistent with how the store tracks values for local variables, fields, arrays, and other expressions.
151-151: LGTM: Proper initialization in constructor.
172-172: LGTM: Proper copying in copy constructor.The map is correctly copied to avoid aliasing between store instances.
513-513: LGTM: IteratedCollectionElement added as storable type.This allows the store to track values for iterated collection elements.
648-654: LGTM: Correct insertion and merge logic.The implementation follows the established pattern for other expression types, properly merging old and new values.
785-787: LGTM: Proper value clearing logic.The implementation correctly removes IteratedCollectionElement entries from the store.
835-837: LGTM: Correct value retrieval logic.The implementation properly retrieves values for IteratedCollectionElement expressions from the map.
1236-1246: LGTM: Correct upper bound merge logic with null guard.The implementation properly merges IteratedCollectionElement values from both stores, with appropriate null checking before insertion into the new store. This is consistent with the merge logic for other expression types.
1330-1336: LGTM: Correct superset comparison logic.The implementation properly checks that this store contains all IteratedCollectionElement entries from the other store, consistent with the pattern used for other maps.
1420-1423: LGTM: Proper visualization support added.The implementation correctly adds IteratedCollectionElement entries to the store visualization, following the established pattern for other expression types. This assumes the
CFGVisualizerinterface has been extended with thevisualizeStoreIteratedCollectionEltmethod.checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (1)
3369-3373: Wrong file location in review comment.The code pattern
iterVarDecl.getName() == loopVariable.getName()exists inchecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javaat line 3371, not inRLCCalledMethodsAnnotatedTypeFactory.java(which has only 804 lines). Update the file reference to target the correct location.Likely an incorrect or invalid review comment.
docs/manual/resource-leak-checker.tex (1)
138-157: New collection-annotation subsection reads clearly and matches later semantics.The introduction of
@OwningCollection,@NotOwningCollection, and@CollectionFieldDestructorplus the stated defaults for parameters, fields, and returns is consistent with the detailed behavior described in the later collections sections. No changes needed here.
...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
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Outdated
Show resolved
Hide resolved
...java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java:
- Line 914: The static mutable field `uid` in class CFGTranslationPhaseOne
causes a race when multiple instances run concurrently; change `protected static
long uid = 0;` to an instance field (e.g., `protected long uid = 0;`) and update
the uniqueName() method to use the instance `uid` (incrementing it in a
thread-safe manner for that instance) — if truly global, replace `long uid` with
an AtomicLong and use `uid.incrementAndGet()` in uniqueName(); adjust all
references to `uid` and ensure uniqueName() reflects the chosen fix.
In
@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java:
- Around line 1196-1211: getRegularStore may NPE when flowResult is null or
input/regular store is null; change getRegularStore to be null-safe by (a)
updating its signature and Javadoc to return @Nullable Store, (b) guarding the
flowResult.getInput(block) call (if !analysis.isRunning() && flowResult == null
then return null), (c) also check that the obtained TransferInput<Value,Store>
input is non-null and return input.getRegularStore() only if input != null and
input.getRegularStore() != null, otherwise return null; reference symbols:
getRegularStore, flowResult, analysis.getInput, flowResult.getInput,
TransferInput, input.getRegularStore().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javaframework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javaframework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Applied to files:
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
📚 Learning: 2025-11-02T02:18:00.536Z
Learnt from: mernst
Repo: typetools/checker-framework PR: 7354
File: annotation-file-utilities/tests/LocalMultipleManyMethodsShifted.java:14-14
Timestamp: 2025-11-02T02:18:00.536Z
Learning: In the Checker Framework repository, when doing refactoring PRs (such as preferring isEmpty() over size() comparisons), test files should not be changed. Tests should remain stable to preserve their exact patterns for annotation processing and bytecode verification purposes.
Applied to files:
framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java
⏰ 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 (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
🔇 Additional comments (2)
framework/src/main/java/org/checkerframework/framework/type/QualifierHierarchy.java (1)
782-782: Suppression is appropriate for this debugging utility.The
@SuppressWarnings("collectionownership:argument")annotation is appropriately narrow and justified. TheassertSameSizemethod only formats@MustCallUnknowncollections for error messages viaStringsPlume.joinwithout transferring ownership, so suppressing the collection ownership argument warning is correct. This pattern is also used consistently inStubGenerator.formatListandValueCheckerUtils.convertToStringVal.framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)
64-64: LGTM: Import added for new API.The import is correctly placed and necessary for the new
getRegularStore(Block)method.
|
|
||
| /** The UID for the next unique name. */ | ||
| protected long uid = 0; | ||
| protected static long uid = 0; |
There was a problem hiding this comment.
Race condition: unsynchronized access to shared mutable static field.
Making uid static introduces a race condition when multiple CFGTranslationPhaseOne instances are created concurrently (e.g., during parallel compilation). The increment operation uid++ on line 923 is not atomic, which can lead to:
- Lost updates when multiple threads read, increment, and write back the same value
- Non-unique names being generated, violating the purpose of
uniqueName()
Additionally, this contradicts the project pattern documented in the learnings: "prefer instance fields over static state for analysis caches."
🔒 Recommended fix: use instance field or AtomicLong
Option 1 (Preferred): Revert to instance field
- protected static long uid = 0;
+ protected long uid = 0;Option 2: If global uniqueness is required, use AtomicLong
- protected static long uid = 0;
+ protected static final AtomicLong uid = new AtomicLong(0);Then update uniqueName():
protected String uniqueName(String prefix) {
- return prefix + "#num" + uid++;
+ return prefix + "#num" + uid.getAndIncrement();
}Based on learnings, instance fields are preferred over static state in this codebase.
🤖 Prompt for AI Agents
In
@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
at line 914, The static mutable field `uid` in class CFGTranslationPhaseOne
causes a race when multiple instances run concurrently; change `protected static
long uid = 0;` to an instance field (e.g., `protected long uid = 0;`) and update
the uniqueName() method to use the instance `uid` (incrementing it in a
thread-safe manner for that instance) — if truly global, replace `long uid` with
an AtomicLong and use `uid.incrementAndGet()` in uniqueName(); adjust all
references to `uid` and ensure uniqueName() reflects the chosen fix.
| /** | ||
| * Returns the regular store for a given block. | ||
| * | ||
| * @param block a block whose regular store to return | ||
| * @return the regular store of {@code block} | ||
| */ | ||
| public Store getRegularStore(Block block) { | ||
| TransferInput<Value, Store> input; | ||
| if (!analysis.isRunning()) { | ||
| input = flowResult.getInput(block); | ||
| } else { | ||
| input = analysis.getInput(block); | ||
| } | ||
|
|
||
| return input.getRegularStore(); | ||
| } |
There was a problem hiding this comment.
Add null safety guards to prevent NPE.
The method can throw a NullPointerException in two scenarios:
-
When
flowResultis null: Line 1205 callsflowResult.getInput(block)without checking ifflowResultis null. Evidence that this can occur:getInferredValueFor(lines 2068-2073) explicitly guards against this case with the comment "When parsing stub or ajava files, the analysis is not running (it has not yet started), and flowResult is null". -
Inconsistent nullability: Similar methods like
getStoreBefore(Tree)(line 1189) andgetStoreBefore(Node)(line 1219) return@Nullable Store, but this returns non-nullableStore. Ifinputorinput.getRegularStore()can be null, callers have no indication they need to handle it.
🛡️ Suggested fix to add null safety
/**
* Returns the regular store for a given block.
*
* @param block a block whose regular store to return
* @return the regular store of {@code block}
*/
- public Store getRegularStore(Block block) {
+ public @Nullable Store getRegularStore(Block block) {
TransferInput<Value, Store> input;
- if (!analysis.isRunning()) {
+ if (!analysis.isRunning() && flowResult == null) {
+ // When parsing stub or ajava files, the analysis is not running and flowResult is null.
+ return null;
+ } else if (!analysis.isRunning()) {
input = flowResult.getInput(block);
} else {
input = analysis.getInput(block);
}
+ if (input == null) {
+ return null;
+ }
return input.getRegularStore();
}Alternatively, if callers are expected to only invoke this method when flowResult is guaranteed to be non-null, document this precondition clearly in the Javadoc.
🤖 Prompt for AI Agents
In
@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
around lines 1196 - 1211, getRegularStore may NPE when flowResult is null or
input/regular store is null; change getRegularStore to be null-safe by (a)
updating its signature and Javadoc to return @Nullable Store, (b) guarding the
flowResult.getInput(block) call (if !analysis.isRunning() && flowResult == null
then return null), (c) also check that the obtained TransferInput<Value,Store>
input is non-null and return input.getRegularStore() only if input != null and
input.getRegularStore() != null, otherwise return null; reference symbols:
getRegularStore, flowResult, analysis.getInput, flowResult.getInput,
TransferInput, input.getRegularStore().
…-framework into rlc-collections-redesign
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:
- Around line 3340-3437: In
MustCallConsistencyAnalyzer.patternMatchEnhancedCollectionForLoop replace the
identity comparison of Name objects with an equality check: change the line that
sets isAssignmentOfIterVar (currently using iterVarDecl.getName() ==
loopVariable.getName()) to use Name.equals(...) or
Objects.equals(iterVarDecl.getName(), loopVariable.getName()) so the iterator
variable name is compared by value rather than by reference; ensure you import
java.util.Objects if you use Objects.equals and update only that comparison in
the method.
- Around line 767-798: The method addObligationsForOwningCollectionReturn
currently throws BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null; change this to treat a null mustCallValues as "no obligations" by simply
skipping obligation creation: after retrieving mustCallValues, check if it is
null and return (or continue) without throwing, so only non-null lists are
iterated to create CollectionObligation instances (preserve existing checks like
ResourceLeakUtils.isIterator and use tmpVarAsResourceAlias and
MethodExitKind.ALL as before).
- Around line 1933-1940: In the OwningCollection case in
MustCallConsistencyAnalyzer, avoid calling .get(0) directly on
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); instead,
store that result in a local variable (e.g., mustCallValues), check it for null
and !mustCallValues.isEmpty() before using mustCallValues.get(0), and only pass
the first element to checker.reportError when present; if null/empty, handle the
missing value path consistently with the earlier fix (skip using get(0) or use a
safe fallback) so no NPE can occur.
- Around line 1867-1876: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) in
MustCallConsistencyAnalyzer may NPE or IOBE; before passing that value into
checker.reportError, retrieve the list into a local variable, check for null or
isEmpty(), and if empty use the string "Unknown" (or a safe default) as the
element/method name argument; then call checker.reportError with either the
first element or the "Unknown" fallback to avoid exceptions.
- Around line 808-851: The code in
addObligationsForCreatesCollectionObligationAnno calls
coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()) and
iterates mustCallValues without a null check, risking an NPE; guard the loop by
checking if mustCallValues is non-null before iterating (i.e., if
(mustCallValues != null) { for (...)
obligations.add(CollectionObligation.fromTree(...)); }), keeping the existing
behavior when the method returns null for non-resource elements and leaving the
subsequent checkEnclosingMethodIsCreatesMustCallFor logic unchanged.
- Around line 511-583: CollectionObligation overrides equals(...) to include
mustCallMethod but doesn't override hashCode(), breaking the equals/hashCode
contract; add a matching hashCode() method in class CollectionObligation that
combines super.hashCode() with mustCallMethod.hashCode() (handle mustCallMethod
possibly null consistently) so instances used in hash-based collections behave
correctly. Ensure the new hashCode() is consistent with equals() and lives
alongside the existing equals(...) override.
In @docs/manual/resource-leak-checker.tex:
- Around line 957-958: The docs contain a leftover TODO in the JDK Method
Signatures subsection (label resource-leak-collections-jdk-methods) that must be
resolved before release; replace the placeholder with a concrete table of common
java.util collection/iterable method signatures annotated with the collection
ownership annotations used in this PR (e.g., methods like Collection.add,
Collection.remove, Iterable.iterator, List.get, Map.put etc., showing whether
they transfer/retain/borrow ownership), or if the table is not ready, remove the
TODO and add a short “To be completed” note with a linked issue number; update
the subsection text to reference the annotation names exactly as used in the PR
and ensure the LaTeX label/resource-leak-collections-jdk-methods remains
correct.
- Around line 959-1001: Remove or replace the large commented-out legacy
documentation block about the Resource Leak Checker (the section discussing
@Owning, @NotOwning, @MustCall defaults and examples like List<Socket>) — either
delete the entire commented block to clean up the docs or convert it into a
concise "historical note" paragraph summarizing the prior limitations and that
they are now resolved; update the surrounding resource-leak-checker section to
reference the short note if kept.
- Around line 731-737: The example's catch block incorrectly calls
System.out.println(e.printStackTrace()) which is invalid because
printStackTrace() returns void; update the catch to either call
e.printStackTrace() directly or print the exception via System.out.println(e)
(or use a logger), leaving the loop and the Socket s / sockets / s.close() logic
unchanged so compilation succeeds.
📜 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.javadocs/manual/resource-leak-checker.tex
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Applied to files:
docs/manual/resource-leak-checker.texchecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Applied to files:
docs/manual/resource-leak-checker.texchecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Applied to files:
docs/manual/resource-leak-checker.texchecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
📚 Learning: 2025-10-22T20:40:48.819Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
Applied to files:
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
📚 Learning: 2025-11-02T02:18:00.536Z
Learnt from: mernst
Repo: typetools/checker-framework PR: 7354
File: annotation-file-utilities/tests/LocalMultipleManyMethodsShifted.java:14-14
Timestamp: 2025-11-02T02:18:00.536Z
Learning: In the Checker Framework repository, when doing refactoring PRs (such as preferring isEmpty() over size() comparisons), test files should not be changed. Tests should remain stable to preserve their exact patterns for annotation processing and bytecode verification purposes.
Applied to files:
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
⏰ 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 (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
🔇 Additional comments (14)
docs/manual/resource-leak-checker.tex (6)
138-158: LGTM!The new subsection for collection annotations is well-structured, correctly introduces the three new annotations, and appropriately cross-references the detailed sections later in the document.
643-667: LGTM!The introduction clearly defines what constitutes a "resource collection" and explains the dual tracking mechanism (ownership type system + dataflow analysis). The examples effectively illustrate the distinction between resource and non-resource collections.
668-688: LGTM!The type hierarchy is clearly presented with the ASCII diagram, and each qualifier's meaning is well-explained. The note about user-writable annotations being limited to
@OwningCollectionand@NotOwningCollectionis helpful.
767-816: LGTM!The section clearly explains how to fulfill collection obligations using loops. The requirements for indexed
forloops (bounds checking, no reassignment, etc.) are well-specified and the examples are helpful.
818-882: LGTM!The documentation for resource collection fields is thorough. The
Aggregatorexample effectively demonstrates:
- Default field ownership
- Constructor with
@OwningCollectionparameter@CreatesMustCallForfor mutating methods@CollectionFieldDestructorfor the disposal methodThe enumerated verification rules and ownership transfer restrictions are clearly explained.
883-887: LGTM!Clear explanation of the alternative pattern using
@NotOwningCollectionfor fields where the enclosing class should not take responsibility.checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)
177-184: LGTM! Field additions and constructor changes are well-structured.The new fields
coAtfandisLoopBodyAnalysisare appropriately added as instance fields, and the constructor properly initializes them. This design aligns with the preference for instance state over static state for analysis-related data.Also applies to: 715-722
280-320: LGTM! Well-designed extension methods forObligation.The
getReplacementmethod provides a clean extension point for subclasses via dynamic dispatch. ThefromTreefactory method appropriately handles bothExpressionTreeandVariableTree, with clear error messaging for unsupported types. ThecanBeSatisfiedThrough(Tree)method correctly checks both direct tree equality and JavaExpression equivalence.Also applies to: 369-387
2151-2217: LGTM! Well-refactored method with clear return semantics.The method has been updated to return a boolean indicating whether the check succeeded, which provides better API semantics than the previous void return. The early type check for
FieldAccessNodeis appropriate, and all code paths properly return a boolean value.
2239-2243: LGTM! Properly handles nested field access.The addition of the
FieldAccessNodecase with recursive handling correctly supports nested field access patterns likeobj.field.subfield, which was previously not handled.
3180-3193: LGTM! Clean abstraction for formatting method names.The
formatMustCallMethodhelper properly handles the special case ofUNKNOWN_METHOD_NAMEby returning a user-friendly "Unknown" string, which aligns with the pattern for handling missing method name information in diagnostics.
2964-2989: LGTM! Proper handling ofCollectionObligationin error reporting.The special case handling for
CollectionObligationcorrectly reports collection-specific errors and uses the "Unknown" fallback for missing method names, consistent with the pattern established informatMustCallMethod.
3439-3557: LGTM! Well-structured loop body analysis.The method properly validates preconditions with
Objects.requireNonNull, handles the empty loop body case early, and correctly catchesInvalidLoopBodyAnalysisExceptionto abort unreachable loop analysis. The use of==for block comparison at line 3520 is appropriate since block identity is the correct check here.
2537-2558: LGTM! Well-designed exception for aborting unreachable loop analysis.The
InvalidLoopBodyAnalysisExceptionis appropriately used to signal when loop body analysis encounters unreachable blocks. The exception is properly caught at call sites (lines 2490, 3544), and the detailed comment at lines 2540-2553 clearly explains why this approach is sound.Also applies to: 3661-3681
| /** Obligation to call a certain method on all elements of a collection. */ | ||
| static class CollectionObligation extends Obligation { | ||
|
|
||
| /** The method that must be called on all elements of the collection. */ | ||
| public String mustCallMethod; | ||
|
|
||
| /** | ||
| * Create a CollectionObligation from a set of resource aliases. | ||
| * | ||
| * @param mustCallMethod the method to be called on all elements of the collection | ||
| * @param resourceAliases a set of resource aliases | ||
| * @param whenToEnforce when this Obligation should be enforced | ||
| */ | ||
| public CollectionObligation( | ||
| String mustCallMethod, | ||
| Set<ResourceAlias> resourceAliases, | ||
| Set<MethodExitKind> whenToEnforce) { | ||
| super(resourceAliases, whenToEnforce); | ||
| this.mustCallMethod = mustCallMethod; | ||
| } | ||
|
|
||
| /** | ||
| * Create a CollectionObligation from an Obligation. | ||
| * | ||
| * @param obligation the obligation to create a CollectionObligation from | ||
| * @param mustCallMethod the method that must be called on the elements of the collection | ||
| */ | ||
| private CollectionObligation(Obligation obligation, String mustCallMethod) { | ||
| super(obligation.resourceAliases, obligation.whenToEnforce); | ||
| this.mustCallMethod = mustCallMethod; | ||
| } | ||
|
|
||
| /** | ||
| * Creates and returns a CollectionObligation derived from the given expression. | ||
| * | ||
| * @param tree the tree from which to create the CollectionObligation. Must be {@code | ||
| * ExpressionTree} or {@code VariableTree}. | ||
| * @param mustCallMethod the method that must be called on the elements of the collection | ||
| * @return a CollectionObligation derived from the given tree | ||
| */ | ||
| public static CollectionObligation fromTree(Tree tree, String mustCallMethod) { | ||
| return new CollectionObligation(Obligation.fromTree(tree), mustCallMethod); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a new CollectionObligation. | ||
| * | ||
| * <p>We need this method since we frequently need to replace obligations. If the old obligation | ||
| * was a CollectionObligation, we want the replacement to be as well. Dynamic dispatch then | ||
| * allows us to simply call getReplacement() on an obligation and get the replacement of the | ||
| * right class. | ||
| * | ||
| * @param resourceAliases set of resource aliases for the new obligation | ||
| * @return a new CollectionObligation with the passed traits | ||
| */ | ||
| @Override | ||
| public CollectionObligation getReplacement( | ||
| Set<ResourceAlias> resourceAliases, Set<MethodExitKind> whenToEnforce) { | ||
| return new CollectionObligation(this.mustCallMethod, resourceAliases, whenToEnforce); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(@Nullable Object obj) { | ||
| if (obj == this) { | ||
| return true; | ||
| } | ||
| if (!(obj instanceof CollectionObligation)) { | ||
| return false; | ||
| } | ||
| return super.equals(obj) | ||
| && ((CollectionObligation) obj).mustCallMethod.equals(this.mustCallMethod); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Override hashCode() when overriding equals().
The CollectionObligation class overrides equals() to include the mustCallMethod field but does not override hashCode(). This violates the general contract for hashCode() and can lead to incorrect behavior when CollectionObligation instances are used in hash-based collections (such as the obligations sets throughout this file).
🔧 Proposed fix
@Override
public boolean equals(@Nullable Object obj) {
if (obj == this) {
return true;
}
if (!(obj instanceof CollectionObligation)) {
return false;
}
return super.equals(obj)
&& ((CollectionObligation) obj).mustCallMethod.equals(this.mustCallMethod);
}
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(super.hashCode(), mustCallMethod);
+ }
}🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 511 - 583, CollectionObligation overrides equals(...) to include
mustCallMethod but doesn't override hashCode(), breaking the equals/hashCode
contract; add a matching hashCode() method in class CollectionObligation that
combines super.hashCode() with mustCallMethod.hashCode() (handle mustCallMethod
possibly null consistently) so instances used in hash-based collections behave
correctly. Ensure the new hashCode() is consistent with equals() and lives
alongside the existing equals(...) override.
| /** | ||
| * Adds {@code CollectionObligation}s if the type of {@code node} is {@code @OwningCollection}. | ||
| * | ||
| * @param obligations the set of tracked obligations | ||
| * @param node the node of a return expression | ||
| */ | ||
| private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) { | ||
| LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node); | ||
| if (tmpVar != null) { | ||
| CollectionOwnershipStore coStore = coAtf.getStoreAfter(node); | ||
| CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore); | ||
| if (cotype == CollectionOwnershipType.OwningCollection) { | ||
| ResourceAlias tmpVarAsResourceAlias = | ||
| new ResourceAlias(new LocalVariable(tmpVar), node.getTree()); | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); | ||
| if (mustCallValues == null) { | ||
| throw new BugInCF( | ||
| "List of MustCall values of component type is null for OwningCollection return value:" | ||
| + " " | ||
| + node); | ||
| } | ||
| if (!ResourceLeakUtils.isIterator(node.getType())) { | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| new CollectionObligation( | ||
| mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Handle null mustCallValues gracefully instead of throwing.
Lines 783-788 throw a BugInCF when getMustCallValuesOfResourceCollectionComponent returns null. According to the established pattern for this codebase, a null result should be treated as "no obligations" and the method should skip creating CollectionObligations rather than throwing an exception. This can occur for @OwningCollection over non-resource element types.
Based on learnings, this needs to be changed to treat null as a no-op case.
🔧 Proposed fix
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
if (mustCallValues == null) {
- throw new BugInCF(
- "List of MustCall values of component type is null for OwningCollection return value:"
- + " "
- + node);
+ // No obligations for non-resource element types
+ return;
}🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 767 - 798, The method addObligationsForOwningCollectionReturn
currently throws BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null; change this to treat a null mustCallValues as "no obligations" by simply
skipping obligation creation: after retrieving mustCallValues, check if it is
null and return (or continue) without throwing, so only non-null lists are
iterated to create CollectionObligation instances (preserve existing checks like
ResourceLeakUtils.isIterator and use tmpVarAsResourceAlias and
MethodExitKind.ALL as before).
| private void addObligationsForCreatesCollectionObligationAnno( | ||
| Set<Obligation> obligations, MethodInvocationNode node) { | ||
| ExecutableElement methodElement = TreeUtils.elementFromUse(node.getTree()); | ||
| boolean hasCreatesCollectionObligation = | ||
| coAtf.getDeclAnnotation(methodElement, CreatesCollectionObligation.class) != null; | ||
| if (hasCreatesCollectionObligation) { | ||
| Node receiverNode = node.getTarget().getReceiver(); | ||
| receiverNode = removeCastsAndGetTmpVarIfPresent(receiverNode); | ||
| boolean receiverIsResourceCollection = coAtf.isResourceCollection(receiverNode.getTree()); | ||
| if (!receiverIsResourceCollection) { | ||
| return; | ||
| } | ||
| boolean receiverIsOwningField = | ||
| coAtf.isOwningCollectionField(TreeUtils.elementFromTree(receiverNode.getTree())); | ||
|
|
||
| CollectionOwnershipStore coStore = coAtf.getStoreBefore(node); | ||
| CollectionOwnershipType receiverType = coAtf.getCoType(receiverNode, coStore); | ||
| if (receiverType == null) { | ||
| throw new BugInCF("Method receiver not in collection ownership store: " + receiverNode); | ||
| } | ||
| switch (receiverType) { | ||
| case OwningCollectionWithoutObligation: | ||
| // fall through | ||
| case OwningCollection: | ||
| if (!receiverIsOwningField) { | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod)); | ||
| } | ||
| } | ||
| if (receiverIsOwningField) { | ||
| TreePath currentPath = cmAtf.getPath(node.getTree()); | ||
| MethodTree enclosingMethodTree = TreePathUtil.enclosingMethod(currentPath); | ||
| if (enclosingMethodTree != null) { | ||
| checkEnclosingMethodIsCreatesMustCallFor(receiverNode, enclosingMethodTree); | ||
| } | ||
| } | ||
| break; | ||
| default: | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check for mustCallValues to prevent potential NPE.
At line 834, getMustCallValuesOfResourceCollectionComponent is called but there's no null check before iterating over the result at lines 835-838. According to the established pattern, this method can return null for non-resource element types, which would cause a NullPointerException.
🔧 Proposed fix
switch (receiverType) {
case OwningCollectionWithoutObligation:
// fall through
case OwningCollection:
if (!receiverIsOwningField) {
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree());
+ if (mustCallValues == null) {
+ // No obligations for non-resource element types
+ break;
+ }
for (String mustCallMethod : mustCallValues) {
obligations.add(
CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod));
}
}🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 808 - 851, The code in
addObligationsForCreatesCollectionObligationAnno calls
coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()) and
iterates mustCallValues without a null check, risking an NPE; guard the loop by
checking if mustCallValues is non-null before iterating (i.e., if
(mustCallValues != null) { for (...)
obligations.add(CollectionObligation.fromTree(...)); }), keeping the existing
behavior when the method returns null for non-resource elements and leaving the
subsequent checkEnclosingMethodIsCreatesMustCallFor logic unchanged.
| private void handleAssignmentToCollectionElementVariable( | ||
| Set<Obligation> obligations, AssignmentNode node, LocalVariableNode lhsVar, Node rhsExpr) { | ||
| if (!isLoopBodyAnalysis) { | ||
| return; | ||
| } | ||
| Obligation oldObligation = null, newObligation = null; | ||
| for (Obligation o : obligations) { | ||
| if (oldObligation != null && newObligation != null) { | ||
| break; | ||
| } | ||
| for (ResourceAlias alias : o.resourceAliases) { | ||
| if ((alias.tree instanceof ExpressionTree) | ||
| && (rhsExpr.getTree() instanceof ExpressionTree) | ||
| && TreeUtils.sameTree( | ||
| (ExpressionTree) alias.tree, (ExpressionTree) rhsExpr.getTree())) { | ||
| Set<ResourceAlias> newResourceAliasesForObligation = | ||
| new LinkedHashSet<>(o.resourceAliases); | ||
| // It is possible to observe assignments to temporary variables, e.g., | ||
| // synthetic assignments to ternary expression variables in the CFG. For such | ||
| // cases, use the tree associated with the temp var for the resource alias, | ||
| // as that is the tree where errors should be reported. | ||
| Tree treeForAlias = | ||
| // I don't think we need a tempVar here, since the only case where we're interested | ||
| // in such an assignment in the loopBodyAnalysis is if the lhs is an actual variable | ||
| // to be used as an alias, so we don't care about the case where lhs is a temp-var. | ||
| // typeFactory.isTempVar(lhsVar) | ||
| // ? typeFactory.getTreeForTempVar(lhsVar) | ||
| // : node.getTree(); | ||
| node.getTree(); | ||
| ResourceAlias aliasForAssignment = | ||
| new ResourceAlias(new LocalVariable(lhsVar), treeForAlias); | ||
| newResourceAliasesForObligation.add(aliasForAssignment); | ||
| oldObligation = o; | ||
| newObligation = new Obligation(newResourceAliasesForObligation, o.whenToEnforce); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (oldObligation != null && newObligation != null) { | ||
| obligations.remove(oldObligation); | ||
| obligations.add(newObligation); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing or addressing commented code and uncertain comment.
Lines 1543-1550 contain a large commented-out code block with a comment expressing uncertainty ("I don't think we need a tempVar here"). If the commented code is not needed, it should be removed to improve code clarity. If there's uncertainty about whether temp var handling is needed, this should be verified and documented more clearly.
| } | ||
| } else { | ||
| // is an initialization block. Not supported. | ||
| checker.reportError( | ||
| node, | ||
| "unfulfilled.collection.obligations", | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0), | ||
| lhs.getTree(), | ||
| "Field assignment outside method or declaration might overwrite field's current value"); | ||
| } |
There was a problem hiding this comment.
Add null and empty checks before accessing mustCallValues.
At line 1873, the code calls .get(0) on the result of getMustCallValuesOfResourceCollectionComponent() without checking if the result is null or empty. This could cause a NullPointerException or IndexOutOfBoundsException. Based on learnings, when constructing diagnostics that reference an element method name, you should fall back to "Unknown" if the list is null/empty.
🔧 Proposed fix
+ List<String> mustCallValues =
+ coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree());
+ String methodName = (mustCallValues == null || mustCallValues.isEmpty())
+ ? "Unknown"
+ : mustCallValues.get(0);
checker.reportError(
node,
"unfulfilled.collection.obligations",
- coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0),
+ methodName,
lhs.getTree(),
"Field assignment outside method or declaration might overwrite field's current value");🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 1867 - 1876, The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) in
MustCallConsistencyAnalyzer may NPE or IOBE; before passing that value into
checker.reportError, retrieve the list into a local variable, check for null or
isEmpty(), and if empty use the string "Unknown" (or a safe default) as the
element/method name argument; then call checker.reportError with either the
first element or the "Unknown" fallback to avoid exceptions.
| case OwningCollection: | ||
| // assignment not allowed | ||
| checker.reportError( | ||
| node.getTree(), | ||
| "unfulfilled.collection.obligations", | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0), | ||
| lhs.getTree(), | ||
| "Field assignment might overwrite field's current value"); |
There was a problem hiding this comment.
Add null and empty checks before accessing mustCallValues (second occurrence).
Similar to the previous issue, line 1938 calls .get(0) on the result of getMustCallValuesOfResourceCollectionComponent() without null or empty checks. This is a duplicate of the pattern identified earlier and needs the same fix.
🔧 Proposed fix
+ List<String> mustCallValues =
+ coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree());
+ String methodName = (mustCallValues == null || mustCallValues.isEmpty())
+ ? "Unknown"
+ : mustCallValues.get(0);
checker.reportError(
node.getTree(),
"unfulfilled.collection.obligations",
- coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0),
+ methodName,
lhs.getTree(),
"Field assignment might overwrite field's current value");🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 1933 - 1940, In the OwningCollection case in
MustCallConsistencyAnalyzer, avoid calling .get(0) directly on
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); instead,
store that result in a local variable (e.g., mustCallValues), check it for null
and !mustCallValues.isEmpty() before using mustCallValues.get(0), and only pass
the first element to checker.reportError when present; if null/empty, handle the
missing value path consistently with the earlier fix (skip using get(0) or use a
safe fallback) so no NPE can occur.
| private void patternMatchEnhancedCollectionForLoop( | ||
| MethodInvocationNode methodInvocationNode, ControlFlowGraph cfg) { | ||
| boolean nodeIsDesugaredFromEnhancedForLoop = | ||
| methodInvocationNode.getIterableExpression() != null; | ||
| if (nodeIsDesugaredFromEnhancedForLoop) { | ||
| // this is the Iterator.next() call desugared from an enhanced-for-loop | ||
| EnhancedForLoopTree loop = methodInvocationNode.getEnhancedForLoop(); | ||
| if (loop == null) { | ||
| throw new BugInCF( | ||
| "MethodInvocationNode.iterableExpression should be non-null iff" | ||
| + " MethodInvocationNode.enhancedForLoop is non-null"); | ||
| } | ||
|
|
||
| // Find the first block of the loop body. | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // until the assignment of the loop iterator variable is found, which is the last | ||
| // desugared instruction. The next block is then the start of the loop body. | ||
| VariableTree loopVariable = loop.getVariable(); | ||
| SingleSuccessorBlock ssblock = (SingleSuccessorBlock) methodInvocationNode.getBlock(); | ||
| Iterator<Node> nodeIterator = ssblock.getNodes().iterator(); | ||
| Node loopVarNode = null; | ||
| Node node; | ||
| boolean isAssignmentOfIterVar; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| ssblock = (SingleSuccessorBlock) ssblock.getSuccessor(); | ||
| nodeIterator = ssblock.getNodes().iterator(); | ||
| } | ||
| node = nodeIterator.next(); | ||
| isAssignmentOfIterVar = false; | ||
| if ((node instanceof AssignmentNode) && (node.getTree() instanceof VariableTree)) { | ||
| loopVarNode = ((AssignmentNode) node).getTarget(); | ||
| VariableTree iterVarDecl = (VariableTree) node.getTree(); | ||
| isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName(); | ||
| } | ||
| } while (!isAssignmentOfIterVar); | ||
| Block loopBodyEntryBlock = ssblock.getSuccessor(); | ||
|
|
||
| // Find the loop-body-condition | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // backwards until the conditional block is found. The previous block is then the block | ||
| // containing the desugared loop condition iterator.hasNext(). | ||
| Block block = methodInvocationNode.getBlock(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| boolean isLoopCondition; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| Set<Block> predBlocks = block.getPredecessors(); | ||
| if (predBlocks.size() == 1) { | ||
| block = predBlocks.iterator().next(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| } else { | ||
| // there is no trivial resolution here. Best we can do is just skip this loop, | ||
| // which is of course sound. | ||
| return; | ||
| // throw new BugInCF( | ||
| // "Encountered more than one CFG Block predecessor trying to find the" | ||
| // + " enhanced-for-loop update block. Block: "); | ||
| } | ||
| } | ||
| node = nodeIterator.next(); | ||
| isLoopCondition = false; | ||
| if (node instanceof MethodInvocationNode) { | ||
| MethodInvocationTree mit = ((MethodInvocationNode) node).getTree(); | ||
| isLoopCondition = TreeUtils.isHasNextCall(mit); | ||
| } | ||
| } while (!isLoopCondition); | ||
|
|
||
| Block blockContainingLoopCondition = node.getBlock(); | ||
| if (blockContainingLoopCondition.getSuccessors().size() != 1) { | ||
| throw new BugInCF( | ||
| "loop condition has: " | ||
| + blockContainingLoopCondition.getSuccessors().size() | ||
| + " successors instead of 1."); | ||
| } | ||
| Block conditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next(); | ||
| if (!(conditionalBlock instanceof ConditionalBlock)) { | ||
| throw new BugInCF( | ||
| "loop condition successor is not ConditionalBlock, but: " | ||
| + conditionalBlock.getClass()); | ||
| } | ||
|
|
||
| // add the blocks into a static datastructure in the calledmethodsatf, such that it can | ||
| // analyze | ||
| // them (call MustCallConsistencyAnalyzer.analyzeFulfillingLoops, which in turn adds the trees | ||
| // to the static datastructure in McoeAtf) | ||
| PotentiallyFulfillingLoop pfLoop = | ||
| new PotentiallyFulfillingLoop( | ||
| loop.getExpression(), | ||
| loopVarNode.getTree(), | ||
| node.getTree(), | ||
| loopBodyEntryBlock, | ||
| block, | ||
| (ConditionalBlock) conditionalBlock, | ||
| loopVarNode); | ||
| this.analyzeObligationFulfillingLoop(cfg, pfLoop); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use equals() instead of == for Name comparison.
At line 3373, the code uses == to compare Name objects:
isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName();According to the established pattern for this codebase, Name comparisons should use Name.equals() or Objects.equals() instead of identity comparison (==), unless the Interning Checker is explicitly used to guarantee reference equality.
🔧 Proposed fix
- isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName();
+ isAssignmentOfIterVar = Objects.equals(iterVarDecl.getName(), loopVariable.getName());🤖 Prompt for AI Agents
In
@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 3340 - 3437, In
MustCallConsistencyAnalyzer.patternMatchEnhancedCollectionForLoop replace the
identity comparison of Name objects with an equality check: change the line that
sets isAssignmentOfIterVar (currently using iterVarDecl.getName() ==
loopVariable.getName()) to use Name.equals(...) or
Objects.equals(iterVarDecl.getName(), loopVariable.getName()) so the iterator
variable name is compared by value rather than by reference; ensure you import
java.util.Objects if you use Objects.equals and update only that comparison in
the method.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/manual/resource-leak-checker.tex:
- Around line 645-651: Replace the vague phrase "properly disposed of" in the
sentence starting "What the checker effectively verifies is that any resource
collection is properly disposed of." with the more precise wording "required
methods are definitely called on all elements of any resource collection" so the
description aligns with must-call terminology used elsewhere in the document.
- Around line 867-871: Summary: The phrase "Assignments in initializer blocks
are not permitted" is ambiguous about whether it means instance initializers,
static initializers, or both; replace it with an explicit statement such as
"Assignments in instance or static initializer blocks are not permitted" (or
specify only instance or only static if that was the intent) and update the
surrounding text to use that clarified terminology; specifically modify the
sentence beginning "Assignments in initializer blocks are not permitted" so it
names "instance initializer blocks" and/or "static initializer blocks" and
ensure consistency with the rest of the paragraph where constructor and method
entry semantics are discussed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/manual/resource-leak-checker.tex
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Applied to files:
docs/manual/resource-leak-checker.tex
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Applied to files:
docs/manual/resource-leak-checker.tex
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Applied to files:
docs/manual/resource-leak-checker.tex
⏰ 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 (inference_part1_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 (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
🔇 Additional comments (4)
docs/manual/resource-leak-checker.tex (4)
138-158: LGTM! Clear introduction to collection ownership annotations.The subsection effectively introduces the new collection ownership annotations with appropriate cross-references to detailed sections. The organization and writing are clear and professional.
668-766: LGTM! Comprehensive documentation of the ownership type system.The type hierarchy, defaults, and ownership transfer rules are clearly explained with helpful code examples. The documentation effectively covers parameter defaults, return type defaults, and assignment behavior.
889-956: Clarify status of commented-out iterator section.This large commented-out section (~67 lines) documents iterator behavior over resource collections. If this feature is implemented and ready, the section should be uncommented. If it's not yet ready or has been deferred, the commented section should be removed to avoid confusion.
Consider either:
- Uncomment if the iterator feature is fully implemented and tested
- Remove if this is deferred work
- Add a brief note explaining why it's commented (if it's intentionally kept for near-term work)
Please clarify the status of iterator support and whether this documentation should be included in this PR.
873-880: Excellent documentation of field ownership restrictions.The restrictions on owned resource collection fields are clearly enumerated and well-justified by the explanation that "the checker doesn't allow transferring the ownership away from such a field." This helps users understand both what is prohibited and why.
The four bullet points clearly cover the main restriction scenarios.
| The Resource Leak Checker handles homogeneous collections of resources. In a homogeneous collection, every element | ||
| has exactly the same must-call and called-methods properties. Instances of \texttt{java.util.Iterable} are supported; | ||
| this section calls those ``collections''. | ||
| % Usage of \texttt{java.util.Iterator}s over \texttt{java.util.Iterable}s are also supported, but they are not considered collections. | ||
|
|
||
| What the checker effectively verifies is that any resource collection is properly disposed of. That is, there is some loop that definitely calls the required methods on all elements of the collection. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider clarifying "properly disposed of" for precision.
Line 650 states "What the checker effectively verifies is that any resource collection is properly disposed of." Consider being more precise, such as: "What the checker effectively verifies is that required methods are definitely called on all elements of any resource collection."
This aligns better with the must-call terminology used throughout the rest of the documentation.
🤖 Prompt for AI Agents
In @docs/manual/resource-leak-checker.tex around lines 645 - 651, Replace the
vague phrase "properly disposed of" in the sentence starting "What the checker
effectively verifies is that any resource collection is properly disposed of."
with the more precise wording "required methods are definitely called on all
elements of any resource collection" so the description aligns with must-call
terminology used elsewhere in the document.
| \begin{itemize} | ||
| \item Write \code{List<? extends Socket>}. This rejects calls to \<add()> | ||
| or other methods that require an instance of the type variable, but it | ||
| preserves some of the behavior (e.g., calls to \<remove()> are permitted). | ||
| This is the best choice most of the time if the \<List> is not intended to | ||
| be owning. | ||
| \item Write \code{List<@MustCall Socket>}. This makes it an error to | ||
| add a Socket to the list, since the type of the Socket is | ||
| \<@MustCall("close")> but the list requires \<@MustCall()>. | ||
| \item Suppress one or more warnings. | ||
| \item Assignments in initializer blocks are not permitted. | ||
| \item Assignments in a declaration initializer are permitted if the right-hand side is at most \texttt{@OwningCollectionWithoutObligation}. | ||
| \item Thanks to the above two restrictions, it can be assumed that an owned resource collection field is \texttt{@OwningCollectionWithoutObligation} at the beginning of a constructor. An assignment to the field is now allowed if the field is \texttt{@OwningCollectionWithoutObligation} just before the assignment. This condition holds in any other (non-constructor) method too, but the field is \texttt{@OwningCollection} at the method entrance, unlike for a constructor. A legal assignment to an owning resource collection field resolves the obligation of the right-hand side. | ||
| \end{itemize} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify "initializer blocks" terminology.
Line 868 states "Assignments in initializer blocks are not permitted" but it's not immediately clear whether this refers to instance initializers, static initializers, or both. Consider being more specific, e.g., "Assignments in instance or static initializer blocks are not permitted."
🤖 Prompt for AI Agents
In @docs/manual/resource-leak-checker.tex around lines 867 - 871, Summary: The
phrase "Assignments in initializer blocks are not permitted" is ambiguous about
whether it means instance initializers, static initializers, or both; replace it
with an explicit statement such as "Assignments in instance or static
initializer blocks are not permitted" (or specify only instance or only static
if that was the intent) and update the surrounding text to use that clarified
terminology; specifically modify the sentence beginning "Assignments in
initializer blocks are not permitted" so it names "instance initializer blocks"
and/or "static initializer blocks" and ensure consistency with the rest of the
paragraph where constructor and method entry semantics are discussed.
| \subsectionAndLabel{JDK Method Signatures}{resource-leak-collections-jdk-methods} | ||
| [[TODO: Add a table with a number of collection/iterable methods and their collection ownership annotations]] |
There was a problem hiding this comment.
Address TODO before merging.
The placeholder "[[TODO: Add a table with a number of collection/iterable methods and their collection ownership annotations]]" indicates incomplete documentation. Documentation for JDK method signatures would help users understand how standard library methods interact with the collection ownership system.
This should be completed before merging, or if the documentation is being deferred, add a note indicating that users should refer to the annotated JDK sources or provide a link to where this information can be found.
Would you like me to help generate a starter table of common JDK method signatures based on the implementation in the codebase?
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests