RLC extension for resource collection#7602
RLC extension for resource collection#7602iamsanjaymalakar wants to merge 475 commits intotypetools:masterfrom
Conversation
…-framework into rlc-collections-redesign
…-framework into rlc-collections-redesign
… into rlc-collections-redesign
…-framework into rlc-collections-redesign
…-framework into rlc-collections-redesign
The split is mostly by responsibility, not by checker ownership.
I'm not sure about how can I move all of them into CollectionOwnershipChecker. Let me know if you have any suggestions. @msridhar |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java (1)
1460-1465: 🧹 Nitpick | 🔵 TrivialAnnotate
get(int)explicitly to match the new accessor contract.
peek()andpop()now spell out the receiver qualifier, but Line 1498get(int)still exposes the same element-access path via defaults alone. For a checker-facing API, that inconsistency makes the ownership surface harder to audit and easier to regress.♻️ Proposed fix
- public E get(int index) { + public E get(`@NotOwningCollection` ImmutableStack<E> this, int index) {Also applies to: 1472-1476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java` around lines 1460 - 1465, The get(int) accessor must be annotated the same as peek() and pop(): add the explicit receiver qualifier "@NotOwningCollection ImmutableStack<E> this" to the get(int) method signature(s) so the ownership contract is spelled out (match the annotations on peek() and pop()), ensuring the ImmutableStack<E> receiver is annotated for checker-facing consistency.
♻️ Duplicate comments (8)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
1728-1732:⚠️ Potential issue | 🔴 CriticalGuard empty argument lists before indexing
getArguments().get(0).Line 1731 can throw
IndexOutOfBoundsExceptionfor zero-argget()calls, which can crash analysis paths.🐛 Proposed fix
public static `@Nullable` ExpressionTree getIdxForGetCall(Tree tree) { - if ((tree instanceof MethodInvocationTree) - && isNamedMethodCall("get", (MethodInvocationTree) tree)) { - return ((MethodInvocationTree) tree).getArguments().get(0); + if (tree instanceof MethodInvocationTree) { + MethodInvocationTree invocationTree = (MethodInvocationTree) tree; + if (isNamedMethodCall("get", invocationTree) && !invocationTree.getArguments().isEmpty()) { + return invocationTree.getArguments().get(0); + } } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java` around lines 1728 - 1732, getIdxForGetCall can throw IndexOutOfBoundsException when a zero-argument "get" call is encountered; update the method to check the argument list size before calling getArguments().get(0) (e.g., test ((MethodInvocationTree) tree).getArguments().isEmpty() or size() > 0) and only return the first argument when present; keep the existing isNamedMethodCall("get", (MethodInvocationTree) tree) check and return null for empty-arg cases to avoid the exception.docs/manual/resource-leak-checker.tex (5)
825-826:⚠️ Potential issue | 🟠 MajorResolve the static-field rule conflict for
@NotOwningCollection.Lines 825-826 say any static resource-collection field is rejected, but Lines 888-893 say
@NotOwningCollectionfields behave like ordinary non-owning references. Those rules conflict unless static@NotOwningCollectionfields are explicitly carved out as legal, or forbidden in both places.Also applies to: 888-893
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/manual/resource-leak-checker.tex` around lines 825 - 826, The documentation currently contradicts itself about static resource-collection fields: the paragraph around the rule for resource collection ownership rejects any static resource-collection field, while the later section describing `@NotOwningCollection` implies such fields behave like ordinary non-owning references; fix by reconciling the policy and making it explicit — either allow static fields when annotated `@NotOwningCollection` or forbid static fields universally. Update the text around the general rule (the “static fields are not supported” sentence) and the `@NotOwningCollection` section so they state the same policy, e.g., “Static resource-collection fields are forbidden except when annotated `@NotOwningCollection`, which makes them legal non-owning references” or “Static resource-collection fields are forbidden, and `@NotOwningCollection` is not permitted on static fields,” and ensure both places reference `@NotOwningCollection` by name for clarity.
963-964:⚠️ Potential issue | 🟡 MinorRemove the rendered TODO before merge.
This placeholder will ship as unfinished manual text. Replace it with the intended table or drop the subsection for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/manual/resource-leak-checker.tex` around lines 963 - 964, The rendered TODO inside the subsection \subsectionAndLabel{JDK Method Signatures}{resource-leak-collections-jdk-methods} must be removed before merge; either replace the placeholder "[[TODO: Add a table with a number of collection/iterable methods and their collection ownership annotations]]" with the actual table of JDK collection/iterable methods and their ownership annotations, or delete the entire subsection header and its placeholder content if the table is not ready, ensuring no TODO/unrendered placeholder remains in the final document.
146-163:⚠️ Potential issue | 🟠 MajorDocument
@CreatesCollectionObligationalongside the other collection annotations.This overview still omits
@CreatesCollectionObligation, so readers never learn how methods that add new per-element obligations to an existing collection are modeled or how that differs from@CollectionFieldDestructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/manual/resource-leak-checker.tex` around lines 146 - 163, Add documentation for `@CreatesCollectionObligation` alongside the existing collection annotations (OwningCollection, NotOwningCollection, CollectionFieldDestructor): state that `@CreatesCollectionObligation` marks methods that create per-element obligations for an existing resource collection, document its parameters (if any) and intended targets, give an example and explicitly contrast its semantics with CollectionFieldDestructor (which cleans up whole fields) and with owning/not-owning defaults, and link to the relevant Section~\ref{resource-leak-collections} and the elements subsection so readers can find details on per-element obligation modeling.
661-662:⚠️ Potential issue | 🟠 MajorDon’t describe
@NotOwningCollectionas completely non-mutating.These paragraphs still say non-owning references cannot add, remove, or overwrite elements. That overstates the restriction: the checker permits mutations that insert non-owning/safe elements. Please document that exception so the manual matches the checker.
Also applies to: 728-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/manual/resource-leak-checker.tex` around lines 661 - 662, Update the two paragraphs describing non-owning references to avoid saying they are completely non-mutating: state that references annotated with `@NotOwningCollection` (or otherwise non-owning) are restricted from arbitrary mutation but may perform mutations that only insert or replace elements that are themselves non-owning/safe (e.g., elements annotated as non-owning or otherwise provably safe); revise the wording to mention this permitted exception and mirror the same clarification in the other occurrence of the paragraph to keep the manual consistent.
775-812:⚠️ Potential issue | 🟡 MinorThis section still under-documents the accepted loop shapes.
It only documents enhanced
forand one indexed-forpattern. If iterator/whiletraversals are now certified too, spell out those forms and their checks here; otherwise narrow the wording so the manual matches the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/manual/resource-leak-checker.tex` around lines 775 - 812, The docs currently only describe enhanced for and one indexed-for pattern for satisfying collection obligations but the implementation also accepts additional traversal shapes (or conversely the docs overclaim); update docs in resource-leak-checker.tex to either (a) enumerate the exact accepted loop shapes and checks (include iterator/while traversals and their required properties: no early termination, no writes to loop variable/collection, accesses via iterator.next() or collection.get(i) as appropriate, and reliance on CalledMethods/@EnsuresCalledMethods for per-element calls) or (b) narrow the wording to only claim enhanced-for and the indexed-for pattern shown; reference the same terminology used in the text (CalledMethods, `@EnsuresCalledMethods`("m"), `@OwningCollection`, `@OwningCollectionWithoutObligation`) so the manual matches the implementation.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (2)
78-79:⚠️ Potential issue | 🟠 MajorInvalidate iterated-element facts on mutations, too.
iteratedCollectionElementsis now first-class store state, but the mutation paths still never evict it. Side-effecting calls and theremoveConflicting(...)paths for field/array/local assignments can all change the iterator or backing collection state, so keeping these entries makes the store stale and can leak called-method facts across iterations.Minimal soundness fix
@@ if (hasSideEffect) { + iteratedCollectionElements.clear(); @@ protected void removeConflicting(FieldAccess fieldAccess, `@Nullable` V val) { + iteratedCollectionElements.clear(); @@ protected void removeConflicting(ArrayAccess arrayAccess, `@Nullable` V val) { + iteratedCollectionElements.clear(); @@ protected void removeConflicting(LocalVariable var) { + iteratedCollectionElements.clear();Also applies to: 632-638
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 78 - 79, The iteratedCollectionElements map must be cleared/updated whenever the store is mutated so iterator-related facts don't leak; modify all mutation/assignment paths (including removeConflicting(...), field/array/local assignment handlers, and any side-effecting call paths) to remove or invalidate entries in iteratedCollectionElements when the target or its backing collection/element could change. Locate uses of iteratedCollectionElements and add logic in CFAbstractStore mutation methods (and in removeConflicting(...)) to evict affected IteratedCollectionElement keys (or clear the map on broad-unknown mutations) so iterator facts are not preserved across mutations.
817-820:⚠️ Potential issue | 🟡 MinorRequire both key components to match in
getIteratedCollectionElement.This
||chain is looser thanIteratedCollectionElement.equals(...), which keys entries on bothtreeandnode. Two distinct keys that share only one component can therefore resolve to the wrong store entry, and the result depends onHashMapiteration order.Suggested fix
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)) { + boolean treeMatches = ice.tree == tree || ice.tree.equals(tree); + boolean nodeMatches = ice.node == node || ice.node.equals(node); + if (treeMatches && nodeMatches) { return ice; } } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 817 - 820, getIteratedCollectionElement currently matches a key if either the node or tree matches, which can return the wrong IteratedCollectionElement; change the matching logic in getIteratedCollectionElement to require both components match (i.e., tree AND node) using null-safe equality (e.g., Objects.equals) against iteratedCollectionElements' keys (IteratedCollectionElement ice), so replace the || chain with a conjunction that checks both ice.tree vs tree and ice.node vs node (or delegating to IteratedCollectionElement.equals(...) if appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/manual/resource-leak-checker.tex`:
- Around line 665-672: The definition of "resource collection" is inconsistent
with the example: update the rule so the type parameter upper bound can be
either a non-empty `@MustCall` type or `@MustCallUnknown` (i.e., treat
`@MustCallUnknown` as a valid must-call obligation for the purposes of the
checker), and then keep the example Iterable<? extends `@MustCallUnknown` Object>
as-is; reference the "resource collection" term and the `@MustCall` /
`@MustCallUnknown` type parameter upper bound in the sentence to make the intent
explicit.
In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`:
- Around line 1199-1214: The method getRegularStore(Block) may dereference a
null TransferInput because flowResult.getInput(block) and
analysis.getInput(block) are `@Nullable`; change the method to return `@Nullable`
Store and defend against null by checking if input is null and returning null
(rather than calling input.getRegularStore()), keeping the existing behavior
when input is non-null; update the method signature and any Javadoc to reflect
`@Nullable` and reference getRegularStore(Block), flowResult.getInput,
analysis.getInput, and TransferInput<Value, Store> in your changes.
---
Outside diff comments:
In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java`:
- Around line 1460-1465: The get(int) accessor must be annotated the same as
peek() and pop(): add the explicit receiver qualifier "@NotOwningCollection
ImmutableStack<E> this" to the get(int) method signature(s) so the ownership
contract is spelled out (match the annotations on peek() and pop()), ensuring
the ImmutableStack<E> receiver is annotated for checker-facing consistency.
---
Duplicate comments:
In `@docs/manual/resource-leak-checker.tex`:
- Around line 825-826: The documentation currently contradicts itself about
static resource-collection fields: the paragraph around the rule for resource
collection ownership rejects any static resource-collection field, while the
later section describing `@NotOwningCollection` implies such fields behave like
ordinary non-owning references; fix by reconciling the policy and making it
explicit — either allow static fields when annotated `@NotOwningCollection` or
forbid static fields universally. Update the text around the general rule (the
“static fields are not supported” sentence) and the `@NotOwningCollection` section
so they state the same policy, e.g., “Static resource-collection fields are
forbidden except when annotated `@NotOwningCollection`, which makes them legal
non-owning references” or “Static resource-collection fields are forbidden, and
`@NotOwningCollection` is not permitted on static fields,” and ensure both places
reference `@NotOwningCollection` by name for clarity.
- Around line 963-964: The rendered TODO inside the subsection
\subsectionAndLabel{JDK Method
Signatures}{resource-leak-collections-jdk-methods} must be removed before merge;
either replace the placeholder "[[TODO: Add a table with a number of
collection/iterable methods and their collection ownership annotations]]" with
the actual table of JDK collection/iterable methods and their ownership
annotations, or delete the entire subsection header and its placeholder content
if the table is not ready, ensuring no TODO/unrendered placeholder remains in
the final document.
- Around line 146-163: Add documentation for `@CreatesCollectionObligation`
alongside the existing collection annotations (OwningCollection,
NotOwningCollection, CollectionFieldDestructor): state that
`@CreatesCollectionObligation` marks methods that create per-element obligations
for an existing resource collection, document its parameters (if any) and
intended targets, give an example and explicitly contrast its semantics with
CollectionFieldDestructor (which cleans up whole fields) and with
owning/not-owning defaults, and link to the relevant
Section~\ref{resource-leak-collections} and the elements subsection so readers
can find details on per-element obligation modeling.
- Around line 661-662: Update the two paragraphs describing non-owning
references to avoid saying they are completely non-mutating: state that
references annotated with `@NotOwningCollection` (or otherwise non-owning) are
restricted from arbitrary mutation but may perform mutations that only insert or
replace elements that are themselves non-owning/safe (e.g., elements annotated
as non-owning or otherwise provably safe); revise the wording to mention this
permitted exception and mirror the same clarification in the other occurrence of
the paragraph to keep the manual consistent.
- Around line 775-812: The docs currently only describe enhanced for and one
indexed-for pattern for satisfying collection obligations but the implementation
also accepts additional traversal shapes (or conversely the docs overclaim);
update docs in resource-leak-checker.tex to either (a) enumerate the exact
accepted loop shapes and checks (include iterator/while traversals and their
required properties: no early termination, no writes to loop
variable/collection, accesses via iterator.next() or collection.get(i) as
appropriate, and reliance on CalledMethods/@EnsuresCalledMethods for per-element
calls) or (b) narrow the wording to only claim enhanced-for and the indexed-for
pattern shown; reference the same terminology used in the text (CalledMethods,
`@EnsuresCalledMethods`("m"), `@OwningCollection`,
`@OwningCollectionWithoutObligation`) so the manual matches the implementation.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 78-79: The iteratedCollectionElements map must be cleared/updated
whenever the store is mutated so iterator-related facts don't leak; modify all
mutation/assignment paths (including removeConflicting(...), field/array/local
assignment handlers, and any side-effecting call paths) to remove or invalidate
entries in iteratedCollectionElements when the target or its backing
collection/element could change. Locate uses of iteratedCollectionElements and
add logic in CFAbstractStore mutation methods (and in removeConflicting(...)) to
evict affected IteratedCollectionElement keys (or clear the map on broad-unknown
mutations) so iterator facts are not preserved across mutations.
- Around line 817-820: getIteratedCollectionElement currently matches a key if
either the node or tree matches, which can return the wrong
IteratedCollectionElement; change the matching logic in
getIteratedCollectionElement to require both components match (i.e., tree AND
node) using null-safe equality (e.g., Objects.equals) against
iteratedCollectionElements' keys (IteratedCollectionElement ice), so replace the
|| chain with a conjunction that checks both ice.tree vs tree and ice.node vs
node (or delegating to IteratedCollectionElement.equals(...) if appropriate).
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 1728-1732: getIdxForGetCall can throw IndexOutOfBoundsException
when a zero-argument "get" call is encountered; update the method to check the
argument list size before calling getArguments().get(0) (e.g., test
((MethodInvocationTree) tree).getArguments().isEmpty() or size() > 0) and only
return the first argument when present; keep the existing
isNamedMethodCall("get", (MethodInvocationTree) tree) check and return null for
empty-arg cases to avoid the exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 106e3096-831b-4d3d-8be4-0c36f754abe3
📒 Files selected for processing (7)
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javadocs/manual/resource-leak-checker.texframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javaframework/src/main/java/org/checkerframework/framework/stub/StubGenerator.javaframework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java
| For the purposes of this checker, \textit{resource collections} are precisely defined, as the objects of interest for both the type system and obligation tracking. A Java expression is a \textit{resource collection} if it is: | ||
|
|
||
| \begin{enumerate} | ||
| \item A \texttt{java.util.Iterable} or implementation thereof (which includes the entire Java Collections framework), and:%, or a \texttt{java.util.Iterator} | ||
| \item Its type parameter upper bound has non-empty \texttt{MustCall} type. | ||
| \end{enumerate} | ||
|
|
||
| For example, expressions of type \texttt{List<Socket>} or \texttt{Iterable<? extends @MustCallUnknown Object>} are both resource collections, but one of type \texttt{Set<String>} is not. |
There was a problem hiding this comment.
Align the formal definition with the example.
Line 669 says a resource collection must have an element upper bound with a non-empty @MustCall type, but Line 672 then lists Iterable<? extends @MustCallUnknown Object> as an example. Please either broaden the definition to cover unknown must-call obligations or replace the example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/manual/resource-leak-checker.tex` around lines 665 - 672, The definition
of "resource collection" is inconsistent with the example: update the rule so
the type parameter upper bound can be either a non-empty `@MustCall` type or
`@MustCallUnknown` (i.e., treat `@MustCallUnknown` as a valid must-call obligation
for the purposes of the checker), and then keep the example Iterable<? extends
`@MustCallUnknown` Object> as-is; reference the "resource collection" term and the
`@MustCall` / `@MustCallUnknown` type parameter upper bound in the sentence to make
the intent explicit.
| /** | ||
| * 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how similar store-retrieval methods handle null
# Look for getInput calls and null handling patterns
rg -n -A5 -B2 'getInput\(' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
# Check if TransferInput getInput can return null based on its declaration
ast-grep --pattern 'TransferInput<$_, $_> getInput($_) { $$$ }'Repository: typetools/checker-framework
Length of output: 335
🏁 Script executed:
# Find and examine the getStoreBefore method to compare
rg -n -A10 'getStoreBefore\(Tree' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javaRepository: typetools/checker-framework
Length of output: 1414
🏁 Script executed:
# Find the class definition of flowResult and analysis to understand their types
rg -n 'flowResult\s*=' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java | head -5
rg -n 'analysis\s*=' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java | head -5
# Also look for field declarations
rg -n -B2 'private.*flowResult\|protected.*flowResult\|public.*flowResult' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
rg -n -B2 'private.*analysis\|protected.*analysis\|public.*analysis' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javaRepository: typetools/checker-framework
Length of output: 352
🏁 Script executed:
# Check for `@Nullable` annotations in the file to understand the pattern
rg -n '@Nullable' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java | head -20Repository: typetools/checker-framework
Length of output: 1562
🏁 Script executed:
# Search for any usage of getInput with null checking
rg -n -B2 -A2 'getInput.*null\|null.*getInput' framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javaRepository: typetools/checker-framework
Length of output: 53
🏁 Script executed:
# Search for getInput method definitions in the codebase
fd -t f '\.java$' | xargs rg -l 'getInput\(' | head -10Repository: typetools/checker-framework
Length of output: 881
🏁 Script executed:
# Look for TransferResult or FlowResult class definitions
fd -t f 'TransferResult.java|FlowResult.java|AnalysisResult.java' framework/Repository: typetools/checker-framework
Length of output: 53
🏁 Script executed:
# Search for method signature of getInput in analysis and flowResult related classes
rg -n 'TransferInput.*getInput|AnalysisResult.*getInput' framework/ -t javaRepository: typetools/checker-framework
Length of output: 53
🏁 Script executed:
# Find the AnalysisResult class and look for getInput method
rg -n -A10 'public.*getInput\(Block' dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.javaRepository: typetools/checker-framework
Length of output: 404
🏁 Script executed:
# Also search in Analysis class
rg -n -A10 'public.*getInput\(Block' dataflow/src/main/java/org/checkerframework/dataflow/analysis/Analysis.javaRepository: typetools/checker-framework
Length of output: 53
🏁 Script executed:
# Check AbstractAnalysis as well
rg -n -A10 'getInput\(Block' dataflow/src/main/java/org/checkerframework/dataflow/analysis/AbstractAnalysis.javaRepository: typetools/checker-framework
Length of output: 53
🏁 Script executed:
# Look for the return type annotation on getInput methods
rg -n '@Nullable.*getInput\(|getInput.*@Nullable' dataflow/src/main/java/org/checkerframework/dataflow/analysis/Repository: typetools/checker-framework
Length of output: 651
Potential null pointer dereference on input.getRegularStore().
Both flowResult.getInput(block) and analysis.getInput(block) can return null (confirmed by @Nullable annotation in their signatures), but there is no null check before calling input.getRegularStore(). This is inconsistent with similar methods in this class like getStoreBefore(Tree), getStoreAfter(Tree), getRegularExitStore(Tree), and getExceptionalExitStore(Tree), which all return @Nullable Store.
Consider either adding a null check and returning @Nullable Store, or documenting a precondition that the block must have a valid input. Given the pattern in this class, returning @Nullable Store is more consistent with related methods.
Suggested fix
- public Store getRegularStore(Block block) {
+ public `@Nullable` Store getRegularStore(Block block) {
TransferInput<Value, Store> input;
if (!analysis.isRunning()) {
input = flowResult.getInput(block);
} else {
input = analysis.getInput(block);
}
+ if (input == null) {
+ return null;
+ }
return input.getRegularStore();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`
around lines 1199 - 1214, The method getRegularStore(Block) may dereference a
null TransferInput because flowResult.getInput(block) and
analysis.getInput(block) are `@Nullable`; change the method to return `@Nullable`
Store and defend against null by checking if input is null and returning null
(rather than calling input.getRegularStore()), keeping the existing behavior
when input is non-null; update the method signature and any Javadoc to reflect
`@Nullable` and reference getRegularStore(Block), flowResult.getInput,
analysis.getInput, and TransferInput<Value, Store> in your changes.
|
Thanks, @iamsanjaymalakar. To make sure we are on the same page, here's what I think needs to happen at a high level. We need to identify loops that iterate over a (resource) collection. When we identify a loop, I'm assuming we know all CFG nodes in the loop, the loop head, and the loop variable corresponding to an element of the collection. For each such loop, we need to check two things:
If a loop over a resource collection satisfies 1 and 2, it is a certified disposal loop. Then, in inference of collection ownership types, if a reference to a collection I think it's also useful to keep in mind the current sub-checker relationships for the RLC: Now to your comment:
All checkers can have a visitor that operates over ASTs. Why does this logic need to live in the
Again, all checkers usually do inference and have access to CFGs (including the Must Call Checker). I don't understand why the bullets listed above need to happen in the RLC Called Methods Checker.
I don't understand what "orchestration" or "resolved" mean in this sentence.
This one I don't understand either. Is the idea that the check being performed at back edges is a consistency check, so it's somehow nice to re-use the consistency analyzer logic for this? Do we handle cases like must-aliases of the loop variable then? I guess that idea makes sense, but I still don't know why it's getting called from the RLC Called Methods Checker.
This makes sense. I just don't understand why the orchestration is so complicated. Is it because the Collection Ownership Checker cannot easily access the types inferred by the sub-checkers? |
Not at the initial match point. There we only know the source-level loop shape and the relevant trees. The full CFG structure is recovered later; for enhanced
There is no fundamental reason it has to be in
This does not inherently have to live in
The AST match gives us a candidate loop, but not all of the CFG information that MCCA needs to certify it. After the missing CFG pieces are filled in, the loop is in a resolved state. By “orchestration”, I mean that RLCCalledMethodsAnnotatedTypeFactory is doing the bookkeeping around that process: it tracks the candidates, resolves the missing CFG pieces, and then calls MCCA on the resolved loops.
Yes, the reason it is in MCCA is basically that this is already using the same consistency machinery: it seeds an obligation for the iterated element, propagates obligations through the loop body, updates aliases through assignments, and then checks the @CalledMethods information for the element and its aliases at the relevant back edges.
I agree with you on this. I think we'll need a seprate place/class that does all the orchestration. The matched/resolved loop state is tracked on the RLCC Called Methods side, and the certification needs to run in a postAnalyze(...) phase after Called Methods analysis has finished, so that is where the orchestration ended up. |
|
Quick question, why do we need to do AST matching to find source-level loop shapes? Why can't everything be done at the CFG level? |
@msridhar I'm not too sure about how hard that would be. I extended following the initial implementation (indexed-for in AST, enhanced for via CFG) to while loops. Matching in the AST stage seemed natuaral to find the loop condition and the collection being iterated (just to identify the potential loops). I don't know how complicated it would be to do so from CFG. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (1)
261-266:⚠️ Potential issue | 🔴 CriticalSkip the current checker when probing for an enclosing RLC hierarchy.
Starting the scan at
checkerstill makes a standaloneMustCallCheckerlook like it is inside the RLC hierarchy, so this helper can call the RLCC factory lookup when no enclosing RLC checker exists. Start fromchecker.getParentChecker()or otherwise ignore the initial checker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java` around lines 261 - 266, getRlccAtfIfPartOfHierarchy incorrectly begins scanning at the current checker, causing a standalone MustCallChecker to be treated as part of the RLC hierarchy; change the probe to skip the current checker by initializing currentChecker to checker.getParentChecker() (or otherwise start from the parent) and then iterate while currentChecker != null, checking ResourceLeakUtils.rlcCheckers for currentChecker.getClass().getCanonicalName() and calling ResourceLeakUtils.getRLCCalledMethodsAnnotatedTypeFactory(this) only when an enclosing RLC checker is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 293-337: The current loop in MustCallAnnotatedTypeFactory only
recurses into nested declared type arguments when
ResourceLeakUtils.isCollection(adt.getUnderlyingType()) is true, so declared
inner collections (e.g., Optional<List<?>>) aren't visited; change the logic so
that the handling that forces wildcard/typevar element annotations to BOTTOM
remains when the outer type is a collection, but the recursive call to
replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg)
is executed for any AnnotatedDeclaredType type argument regardless of the outer
isCollection check—i.e., move or duplicate the branch that checks
typeArg.getKind() == TypeKind.DECLARED out of the isCollection guard (or add a
second pass over adt.getTypeArguments() that calls
replaceCollectionTypeVarsWithBottomIfTop for DECLARED args) so nested declared
types are always descended into.
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 261-266: getRlccAtfIfPartOfHierarchy incorrectly begins scanning
at the current checker, causing a standalone MustCallChecker to be treated as
part of the RLC hierarchy; change the probe to skip the current checker by
initializing currentChecker to checker.getParentChecker() (or otherwise start
from the parent) and then iterate while currentChecker != null, checking
ResourceLeakUtils.rlcCheckers for currentChecker.getClass().getCanonicalName()
and calling ResourceLeakUtils.getRLCCalledMethodsAnnotatedTypeFactory(this) only
when an enclosing RLC checker is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5217275d-7bce-4d89-8cd5-3b72ed0670d4
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.javachecker/tests/resourceleak-collections/ArraysAsListZeroArgs.java
| if (ResourceLeakUtils.isCollection(adt.getUnderlyingType())) { | ||
| for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) { | ||
| if (typeArg == null) { | ||
| continue; | ||
| } | ||
| if (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR) { | ||
| if (tree != null && tree instanceof NewClassTree) { | ||
| if (((NewClassTree) tree).getTypeArguments().isEmpty()) { | ||
| // Diamond [new Class()<>]. Not explicit generic type param. | ||
| // This will be inferred later. Don't put it to bottom here. | ||
| continue; | ||
| } | ||
| } | ||
| AnnotationMirror mcAnno = typeArg.getAnnotationInHierarchy(TOP); | ||
| boolean typeArgIsMcUnknown = | ||
| mcAnno != null | ||
| && processingEnv | ||
| .getTypeUtils() | ||
| .isSameType(mcAnno.getAnnotationType(), TOP.getAnnotationType()); | ||
| if (typeArgIsMcUnknown) { | ||
| // check whether the upper bounds have manual MustCallUnknown annotations, in which case | ||
| // they should | ||
| // not be reset to bottom. For example like this: | ||
| // void m(List<? extends @MustCallUnknown Object>) {} | ||
|
|
||
| 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); | ||
| } | ||
| } else { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } | ||
| } else if (typeArg.getKind() == TypeKind.DECLARED) { | ||
| replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg); |
There was a problem hiding this comment.
Recurse into nested declared type arguments even when the outer type is not a collection.
The ResourceLeakUtils.isCollection(...) guard currently wraps both the rewrite and the recursive descent. As a result, types like Optional<List<?>> or Holder<Iterator<?>> never visit their inner collection/iterator arguments, so those element types keep @MustCallUnknown instead of being forced to bottom.
♻️ Suggested fix
private void replaceCollectionTypeVarsWithBottomIfTop(Tree tree, AnnotatedDeclaredType adt) {
- if (ResourceLeakUtils.isCollection(adt.getUnderlyingType())) {
- for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) {
+ boolean currentIsCollection = ResourceLeakUtils.isCollection(adt.getUnderlyingType());
+ for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) {
if (typeArg == null) {
continue;
}
- if (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR) {
+ if (currentIsCollection
+ && (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR)) {
...
} else if (typeArg.getKind() == TypeKind.DECLARED) {
replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg);
}
- }
}
}📝 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.
| if (ResourceLeakUtils.isCollection(adt.getUnderlyingType())) { | |
| for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) { | |
| if (typeArg == null) { | |
| continue; | |
| } | |
| if (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR) { | |
| if (tree != null && tree instanceof NewClassTree) { | |
| if (((NewClassTree) tree).getTypeArguments().isEmpty()) { | |
| // Diamond [new Class()<>]. Not explicit generic type param. | |
| // This will be inferred later. Don't put it to bottom here. | |
| continue; | |
| } | |
| } | |
| AnnotationMirror mcAnno = typeArg.getAnnotationInHierarchy(TOP); | |
| boolean typeArgIsMcUnknown = | |
| mcAnno != null | |
| && processingEnv | |
| .getTypeUtils() | |
| .isSameType(mcAnno.getAnnotationType(), TOP.getAnnotationType()); | |
| if (typeArgIsMcUnknown) { | |
| // check whether the upper bounds have manual MustCallUnknown annotations, in which case | |
| // they should | |
| // not be reset to bottom. For example like this: | |
| // void m(List<? extends @MustCallUnknown Object>) {} | |
| 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); | |
| } | |
| } else { | |
| typeArg.replaceAnnotation(BOTTOM); | |
| } | |
| } | |
| } else if (typeArg.getKind() == TypeKind.DECLARED) { | |
| replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg); | |
| boolean currentIsCollection = ResourceLeakUtils.isCollection(adt.getUnderlyingType()); | |
| for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) { | |
| if (typeArg == null) { | |
| continue; | |
| } | |
| if (currentIsCollection | |
| && (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR)) { | |
| if (tree != null && tree instanceof NewClassTree) { | |
| if (((NewClassTree) tree).getTypeArguments().isEmpty()) { | |
| // Diamond [new Class()<>]. Not explicit generic type param. | |
| // This will be inferred later. Don't put it to bottom here. | |
| continue; | |
| } | |
| } | |
| AnnotationMirror mcAnno = typeArg.getAnnotationInHierarchy(TOP); | |
| boolean typeArgIsMcUnknown = | |
| mcAnno != null | |
| && processingEnv | |
| .getTypeUtils() | |
| .isSameType(mcAnno.getAnnotationType(), TOP.getAnnotationType()); | |
| if (typeArgIsMcUnknown) { | |
| // check whether the upper bounds have manual MustCallUnknown annotations, in which case | |
| // they should | |
| // not be reset to bottom. For example like this: | |
| // void m(List<? extends `@MustCallUnknown` Object>) {} | |
| 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); | |
| } | |
| } else { | |
| typeArg.replaceAnnotation(BOTTOM); | |
| } | |
| } | |
| } else if (typeArg.getKind() == TypeKind.DECLARED) { | |
| replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`
around lines 293 - 337, The current loop in MustCallAnnotatedTypeFactory only
recurses into nested declared type arguments when
ResourceLeakUtils.isCollection(adt.getUnderlyingType()) is true, so declared
inner collections (e.g., Optional<List<?>>) aren't visited; change the logic so
that the handling that forces wildcard/typevar element annotations to BOTTOM
remains when the outer type is a collection, but the recursive call to
replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg)
is executed for any AnnotatedDeclaredType type argument regardless of the outer
isCollection check—i.e., move or duplicate the branch that checks
typeArg.getKind() == TypeKind.DECLARED out of the isCollection guard (or add a
second pass over adt.getTypeArguments() that calls
replaceCollectionTypeVarsWithBottomIfTop for DECLARED args) so nested declared
types are always descended into.
|
@msridhar I agree the current structure is more scattered than ideal, and I also agree that the long-term direction should be to consolidate more of the orchestration under Collection Ownership checker. But I do not think the right end state is to move all of the logic there. The refactoring I have in mind is not “move everything into Collection Ownership”, but rather: Collection Ownership should own the traversal bookkeeping/lifecycle, while MCCA remains the proof engine. Concretely, I think a cleaner design would be to introduce a separate coordination layer owned by the Collection Ownership side. That layer would own the candidate loops (via own visitor, instead of I think the certification trigger still has to happen at the Called Methods post-analysis point, or at an equivalent lifecycle point where the Called Methods stores are already available. If we instead wait and do the certification in Collection Ownership’s own post-analysis, then the normal Collection Ownership transfer for that same analysis has already run by that point, so the certified result arrives too late to affect the current CO flow pass. In other words, doing certification there would mean we would need to run Collection Ownership flow again, or introduce some second pass just to consume the newly certified loops. So in summary the proposed direction: add a CollectionOwnershipCoordinator to own bookkeeping for loops and the API used by CollectionOwnershipTransfer, while keeping certification in MCCA. Certification should still be triggered at CM postAnalyze, since that is when the Called Methods stores are definetly available (I can't think of a better point); doing it in CO post-analysis would require rerunning CO or adding a second pass. |
|
@iamsanjaymalakar didn't we discuss overriding some pre-analysis hook in our meeting last week? What happened to that? I don't really know what you mean by a coordination layer. That is not standard terminology for the Checker Framework. Please be more concrete about what you are proposing. |
@msridhar There is not currently a standard pre-analysis hook in the CF lifecycle. The relevant lifecycle points are: analyze(...), then postAnalyzeAfterFirstMethodAnalysis(...) for methods, and finally postAnalyze(...). As the pre-analysis hook needs to run before flow analysis, but after the CFG exists phase, I'll need to override
One note: I can't just call Let me know if this answers your question. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (1)
825-835:⚠️ Potential issue | 🟠 MajorReplace non-atomic static counter with AtomicLong for thread-safe synthetic name generation.
The static
uidfield and its non-atomic increment inuniqueName()create a data race. Multiple CFGTranslationPhaseOne instances across concurrent CFG builds can lose increments and generate duplicate names. This mirrors the unsafe pattern inLabel.java(line 14, 37), while other dataflow classes (BlockImpl, Node, CFAbstractStore) correctly useAtomicLongwithgetAndIncrement().🔧 Required fix
+import java.util.concurrent.atomic.AtomicLong; + /** The UID for the next unique name. */ - protected static long uid = 0; + protected static final AtomicLong uid = new AtomicLong(); @@ protected String uniqueName(String prefix) { - return prefix + "#num" + uid++; + return prefix + "#num" + uid.getAndIncrement(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java` around lines 825 - 835, Replace the non-atomic static long uid and its increment in uniqueName(String prefix) with an AtomicLong to make synthetic name generation thread-safe: change the static field uid to an AtomicLong (initialized to 0) and update uniqueName(String) to call uid.getAndIncrement() when composing the returned string; ensure the symbol names uid and uniqueName(...) are used so you update the correct field and method.annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java (1)
1371-1388:⚠️ Potential issue | 🟡 MinorRestore
@throwsJavadoc onpeek()andpop().Both methods still throw
IllegalStateExceptionon an empty stack (Lines 1373 and 1385), but the@throwstags were removed from their Javadoc. Re-add them so the API contract stays documented. Same applies toget(int)at Line 1409 which still throwsNoSuchElementException.📝 Proposed fix
/** * Returns the top element of the stack, without modifying the stack. * * `@return` the top element of the stack + * `@throws` IllegalStateException if the stack is empty */ public E peek(`@NotOwningCollection` ImmutableStack<E> this) {/** * Returns all of the stack except the top element. * * `@return` all of the stack except the top element + * `@throws` IllegalStateException if the stack is empty */ public ImmutableStack<E> pop(`@NotOwningCollection` ImmutableStack<E> this) {/** * Returns the index-th element of this stack. * * `@param` index which element to return * `@return` the index-th element of this stack + * `@throws` NoSuchElementException if index is out of range */ public E get(int index) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java` around lines 1371 - 1388, Add Javadoc `@throws` tags to document the runtime exceptions already thrown: in the Javadoc for peek() add "@throws IllegalStateException if the stack is empty" (for method peek), for pop() add "@throws IllegalStateException if the stack is empty" (for method pop), and for get(int) add "@throws NoSuchElementException if the requested index is not present" (for method get(int)). Place each `@throws` in the corresponding method's Javadoc block immediately under the description so the API contract matches the thrown exceptions.
♻️ Duplicate comments (16)
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java (1)
110-155:⚠️ Potential issue | 🟠 MajorOwnership contract still inconsistent across insertion APIs.
add(V)at Line 124 transfers ownership via@Owning V o, butadd(V, int, int)(Line 112) andreplace(V v)(Line 153) both calltheMap.put(key, o)/theMap.put(keyer.getKeyFor(v), v)— the same storage sink — without@Owningon the element parameter. That creates bypass paths around the ownership guarantee established in the single-argadd. Also update the abstract declarations inKeyedSetto keep the interface and implementation in sync.📝 Proposed fix
`@Override` public V add( `@NotOwningCollection` LinkedHashKeyedSet<K, V> this, - V o, + `@Owning` V o, int conflictBehavior, int equalBehavior) {`@Override` - public V replace(V v) { + public V replace(`@Owning` V v) { return theMap.put(keyer.getKeyFor(v), v); }And mirror the same
@Owningqualifier onKeyedSet#add(V, int, int)andKeyedSet#replace(V).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java` around lines 110 - 155, The ownership annotations are inconsistent: add(`@Owning` V o) declares transfer but add(V o, int conflictBehavior, int equalBehavior) and replace(V v) do not, allowing bypass of ownership guarantees; update the signatures of LinkedHashKeyedSet.add(V, int, int) and LinkedHashKeyedSet.replace(V) to accept `@Owning` V (same as add(`@Owning` V)), and mirror those `@Owning` annotations in the abstract/interface declarations in KeyedSet (KeyedSet#add(V,int,int) and KeyedSet#replace(V)) so all insertion/replacement paths consistently require ownership; also add or adjust any necessary imports/annotations to satisfy the compiler.checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (7)
907-914:⚠️ Potential issue | 🔴 CriticalTreat null component must-call values as “no collection obligation.”
getMustCallValuesOfResourceCollectionComponent(...)can legitimately returnnull; throwing here crashes on valid@OwningCollectionuses over non-resource elements. Skip obligation creation instead.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); - } - if (!ResourceLeakUtils.isIterator(node.getType())) { - for (String mustCallMethod : mustCallValues) { - obligations.add( - new CollectionObligation( - mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); - } + if (mustCallValues == null + || mustCallValues.isEmpty() + || ResourceLeakUtils.isIterator(node.getType())) { + return; + } + for (String mustCallMethod : mustCallValues) { + obligations.add( + new CollectionObligation( + mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); }Based on learnings: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 907 - 914, In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, remove the BugInCF throw when coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns null and instead treat null as "no collection obligation": check the returned mustCallValues from getMustCallValuesOfResourceCollectionComponent(...) and if it is null simply skip creating any CollectionObligation(s) for this OwningCollection return (do not create obligations or throw), leaving normal processing for non-null results intact.
3726-3729:⚠️ Potential issue | 🟠 MajorGuard null CalledMethods values before calling
getCalledMethods.
store.getValue(ice)can returnnull, butgetCalledMethodsimmediately dereferencescmVal. This can crash loop-body certification.Proposed fix
if (ice != null) { AccumulationValue cmValOfIce = store.getValue(ice); - List<String> calledMethods = getCalledMethods(cmValOfIce); - if (calledMethods != null) { - calledMethodsAfterThisBlock = new HashSet<>(calledMethods); + if (cmValOfIce != null) { + List<String> calledMethods = getCalledMethods(cmValOfIce); + if (calledMethods != null) { + calledMethodsAfterThisBlock = new HashSet<>(calledMethods); + } } }Also applies to: 3767-3768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 3726 - 3729, The code calls getCalledMethods(cmValOfIce) without guarding against store.getValue(ice) returning null; add a null-check on the AccumulationValue (cmValOfIce) before invoking getCalledMethods and skip/continue the loop when cmValOfIce is null (do the same null-guarding at the other occurrence around lines handling ice at the 3767-3768 area), ensuring calledMethods is only computed when cmValOfIce != null.
2103-2109:⚠️ Potential issue | 🔴 CriticalDo not crash when the current collection field value is bottom/null.
OwningCollectionBottomrepresents the field currently holdingnull; overwriting it is the safe null-to-collection case, not aBugInCF.Proposed fix direction
} else if (lhsCoType == CollectionOwnershipType.OwningCollectionBottom) { - // Bottom represents null; it’s allowed on RHS of assignment. - throw new BugInCF( - "Expression " - + node - + " has resource collection operand, but `@OwningCollectionBottom` type."); + // Bottom represents null; overwriting it is safe. + if (isOwningCollectionField) { + Set<Obligation> obs = getObligationsForVar(obligations, rhs.getTree()); + for (Obligation o : obs) { + obligations.remove(o); + } + } + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 2103 - 2109, The code incorrectly treats CollectionOwnershipType.OwningCollectionBottom as an error; in MustCallConsistencyAnalyzer replace the throw of BugInCF in the branch checking lhsCoType == CollectionOwnershipType.OwningCollectionBottom with logic that treats OwningCollectionBottom (null) as allowed on the RHS of assignment — simply skip/error-reporting and proceed as the safe null-to-collection overwrite case (remove the BugInCF throw for that case and let normal assignment handling continue).
2118-2127:⚠️ Potential issue | 🟠 MajorDo not remove RHS obligations after a failed
@CreatesMustCallForcheck.
checkEnclosingMethodIsCreatesMustCallFor(...)returnsfalseafter reporting an error, but this path still removes RHS obligations. That makes the analysis behave as if ownership transferred anyway and can hide the original leak.Proposed fix
if (rhsCoType == CollectionOwnershipType.OwningCollection || rhsCoType == CollectionOwnershipType.NotOwningCollection) { - checkEnclosingMethodIsCreatesMustCallFor(lhs, enclosingMethodTree); - if (isOwningCollectionField) { + boolean hasCreatesMustCallFor = + checkEnclosingMethodIsCreatesMustCallFor(lhs, enclosingMethodTree); + if (hasCreatesMustCallFor && isOwningCollectionField) { Set<Obligation> obligationsForVar = getObligationsForVar(obligations, rhs.getTree()); for (Obligation obligation : obligationsForVar) { obligations.remove(obligation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 2118 - 2127, The code in MustCallConsistencyAnalyzer calls checkEnclosingMethodIsCreatesMustCallFor(lhs, enclosingMethodTree) but ignores its boolean result and still removes RHS obligations; change this to capture the result (e.g., boolean creates = checkEnclosingMethodIsCreatesMustCallFor(...)) and only call getObligationsForVar(...) and remove obligations from the obligations set when creates is true; ensure you reference checkEnclosingMethodIsCreatesMustCallFor, getObligationsForVar, and the obligations.remove(...) loop so RHS obligations are not removed after a failed check.
3559-3572:⚠️ Potential issue | 🟠 MajorTreat a null back-edge summary as loop verification failure.
When
analyzeTypeOfCollectionElement(...)returnsnull, this code skips the intersection and preserves methods learned from other back edges. A single unsatisfied back edge can still let the loop be marked fulfilling.Proposed fix
Set<String> calledMethodsAfterBlock = analyzeTypeOfCollectionElement( currentBlock, resolvedPotentiallyFulfillingLoop, obligations, loopUpdateBlock); @@ - if (calledMethodsAfterBlock != null) { - if (calledMethodsInLoop == null) { - calledMethodsInLoop = calledMethodsAfterBlock; - } else { - calledMethodsInLoop.retainAll(calledMethodsAfterBlock); - } + if (calledMethodsAfterBlock == null) { + return; + } + if (calledMethodsInLoop == null) { + calledMethodsInLoop = calledMethodsAfterBlock; + } else { + calledMethodsInLoop.retainAll(calledMethodsAfterBlock); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 3559 - 3572, The current code ignores a null summary from analyzeTypeOfCollectionElement(...) and thus can incorrectly accept the loop; change the handling so that when calledMethodsAfterBlock == null you treat that as a verification failure: set calledMethodsInLoop = null (or another failure sentinel) and break/return immediately so the loop is marked failing instead of preserving prior intersections. Update the block around isLastBlockOfBody where calledMethodsAfterBlock is used (references: isLastBlockOfBody, analyzeTypeOfCollectionElement, calledMethodsInLoop, calledMethodsAfterBlock) to implement this early-failure behavior.
2051-2056:⚠️ Potential issue | 🟠 MajorGuard collection component method lookup before indexing.
These diagnostics call
.get(0)on a list that may benullor empty, turning an error report intoNullPointerException/IndexOutOfBoundsException. Use a safe fallback method name.Proposed fix direction
+ 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");Apply the same safe-access pattern at Line 2134.
Based on learnings: When constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Also applies to: 2131-2136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 2051 - 2056, The call to coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) can NPE/IndexOutOfBounds when the list is null or empty; update MustCallConsistencyAnalyzer to safely fetch the method name (use "Unknown" fallback) before calling checker.reportError so you pass a non-null string. Specifically, wrap or replace the direct .get(0) access (the getMustCallValuesOfResourceCollectionComponent(...) expression used in the checker.reportError call) with a helper or inline check that returns the first element if present else "Unknown"; apply the same safe-access pattern to the analogous reportError usages around the block referenced (also at the other call sites in the 2131-2136/2134 region).
933-975:⚠️ Potential issue | 🟠 MajorOnly create collection obligations on the normal invocation edge.
updateObligationsForInvocationruns once per successor edge. These collection helpers run on exceptional edges too, fabricating collection obligations and consuming inserted-value obligations even when the invocation may have thrown before mutating/returning.Proposed fix direction
- addObligationsForOwningCollectionReturn(obligations, node); + if (exceptionType == null) { + addObligationsForOwningCollectionReturn(obligations, node); + } if (node instanceof MethodInvocationNode) { - addObligationsForCreatesCollectionObligationAnno(obligations, (MethodInvocationNode) node); + addObligationsForCreatesCollectionObligationAnno( + obligations, (MethodInvocationNode) node, exceptionType); }private void addObligationsForCreatesCollectionObligationAnno( - Set<Obligation> obligations, MethodInvocationNode node) { + Set<Obligation> obligations, + MethodInvocationNode node, + `@Nullable` TypeMirror exceptionType) { + if (exceptionType != null) { + return; + }Also ensure obligations created after a successful insert are enforced on all later exits, not only normal return.
Also applies to: 1085-1089
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 933 - 975, The collection-obligation helpers (addObligationsForCreatesCollectionObligationAnno, consumeInsertedArgumentObligationIfSingleElementInsert, reportNeverEnforcedCollectionObligationIfNeeded, checkEnclosingMethodIsCreatesMustCallFor) are being invoked on every successor edge; change the call site in updateObligationsForInvocation (or add a boolean parameter to addObligationsForCreatesCollectionObligationAnno) so these helpers run only for the normal/successor (i.e., successful-invocation) edge and early-return on exceptional edges, and then ensure the obligations created on the normal edge are propagated/enforced for all later exits (e.g., by adding the newly created CollectionObligation objects into the stores/obligation sets used for subsequent exceptional and normal exit processing so later exits will check them).checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java (1)
311-333:⚠️ Potential issue | 🟠 MajorThread the same
WholeProgramInferencethrough the whole inference write path.Lines 311-313 switch field annotation writes to
coAtf.getWholeProgramInference(), but the later helper calls still fetch WPI fromresourceLeakAtf. If those factories diverge, inferred annotations are split across WPI instances.Proposed direction
- addEnsuresCalledMethodsForDisposedFields(); + addEnsuresCalledMethodsForDisposedFields(wpi); ... - addMustCallAliasToFormalParameter(indexOfParam); + addMustCallAliasToFormalParameter(indexOfParam, wpi); ... - addInheritableMustCallToClass(); + addInheritableMustCallToClass(wpi);Then update those helpers, and any helper they call, to use the passed
wpiinstead ofresourceLeakAtf.getWholeProgramInference().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java` around lines 311 - 333, The code creates a local WholeProgramInference wpi from coAtf but later helpers still call resourceLeakAtf.getWholeProgramInference(), which can split annotations across instances; change the helpers (addEnsuresCalledMethodsForDisposedFields, addMustCallAliasToFormalParameter, addInheritableMustCallToClass and any helpers they call) to accept a WholeProgramInference parameter and use the passed wpi instead of calling resourceLeakAtf.getWholeProgramInference(), then update the calls in MustCallInference (the section using coAtf and returnObligationToParameter) to pass the local wpi through so all writes use the same WholeProgramInference instance.checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (2)
293-338:⚠️ Potential issue | 🟠 MajorRecurse into nested declared type arguments even when the outer type is not a collection.
The recursive descent is inside the
isCollection(adt.getUnderlyingType())guard, so types likeOptional<List<?>>never visit the innerList<?>; its element type can remain@MustCallUnknown.Proposed fix
- if (ResourceLeakUtils.isCollection(adt.getUnderlyingType())) { - for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) { + boolean currentIsCollection = ResourceLeakUtils.isCollection(adt.getUnderlyingType()); + for (AnnotatedTypeMirror typeArg : adt.getTypeArguments()) { if (typeArg == null) { continue; } - if (typeArg.getKind() == TypeKind.WILDCARD || typeArg.getKind() == TypeKind.TYPEVAR) { + if (currentIsCollection + && (typeArg.getKind() == TypeKind.WILDCARD + || typeArg.getKind() == TypeKind.TYPEVAR)) { if (tree != null && tree instanceof NewClassTree) { if (((NewClassTree) tree).getTypeArguments().isEmpty()) { // Diamond [new Class()<>]. Not explicit generic type param. @@ - } else if (typeArg.getKind() == TypeKind.DECLARED) { + } else if (typeArg.getKind() == TypeKind.DECLARED) { replaceCollectionTypeVarsWithBottomIfTop(null, (AnnotatedDeclaredType) typeArg); } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java` around lines 293 - 338, The current logic only recurses into declared type arguments when ResourceLeakUtils.isCollection(adt.getUnderlyingType()) is true, so nested declared args (e.g., Optional<List<?>>) are skipped; modify MustCallAnnotatedTypeFactory so that after handling the collection-specific branch it still iterates over adt.getTypeArguments() and calls replaceCollectionTypeVarsWithBottomIfTop(tree, (AnnotatedDeclaredType) typeArg) for any typeArg with getKind() == TypeKind.DECLARED (or extract that recursion into a helper and invoke it unconditionally for declared type args), ensuring you still preserve the existing collection-specific handling (the block that checks wildcards, TYPEVARs, TOP/BOTTOM, and uses ResourceLeakUtils.hasManualMustCallUnknownAnno).
261-267:⚠️ Potential issue | 🔴 CriticalDo not match the current standalone Must Call checker as an RLC ancestor.
Starting from
checkeritself lets a standalone Must Call run matchResourceLeakUtils.rlcCheckers, thengetRLCCalledMethodsAnnotatedTypeFactory(this)can walk a missing RLC parent and crash. Start from the parent checker, or otherwise ignore the current checker instance.Proposed fix
- SourceChecker currentChecker = checker; + SourceChecker currentChecker = checker.getParentChecker(); while (currentChecker != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java` around lines 261 - 267, The loop in getRlccAtfIfPartOfHierarchy starts from the current checker, which allows a standalone MustCall checker to match ResourceLeakUtils.rlcCheckers and cause getRLCCalledMethodsAnnotatedTypeFactory(this) to walk a missing RLC parent and crash; change the start of the search to the parent checker (e.g., set currentChecker = checker.getParentChecker() before the while loop or otherwise skip the initial checker instance) so the method scans ancestors only, keeping references to ResourceLeakUtils.rlcCheckers and ResourceLeakUtils.getRLCCalledMethodsAnnotatedTypeFactory unchanged.checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (3)
1518-1534:⚠️ Potential issue | 🟠 MajorUse the back-edge source as the loop update block.
For a back edge
source -> target,targetBlockis the loop header/condition path. Storing it asloopUpdateBlockmakes later store reads observe the header state instead of the latch state after a full iteration.Suggested fix
// Prefer the tightest loop. This helps nested-loop disambiguation. if (naturalLoop.size() < bestLoopSize) { bestLoopSize = naturalLoop.size(); - bestLoopUpdateBlock = backEdge.targetBlock; + bestLoopUpdateBlock = backEdge.sourceBlock; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 1518 - 1534, The loop selection is using backEdge.targetBlock (the loop header) as the update/latch, which causes later store reads to see header state instead of latch state; change the assignment to use backEdge.sourceBlock (the back-edge source/latch) when setting bestLoopUpdateBlock inside the loop over WhileLoopResolutionCache.BlockEdge (whileLoopCache.getBackEdges()), keeping the same checks for naturalLoop.contains(bodyEntryBlock) and conditionalBlock and the tightest-loop preference (naturalLoop.size() < bestLoopSize).
1375-1380:⚠️ Potential issue | 🟠 MajorUse value equality for
Namecomparison.
VariableTree.getName()returns aName; comparing with==can miss the loop-variable assignment when equal names are not the same object. Use.equals(...).Suggested fix
loopVariableNode = ((AssignmentNode) node).getTarget(); VariableTree iteratorVariableDeclaration = (VariableTree) node.getTree(); isAssignmentOfLoopVariable = - iteratorVariableDeclaration.getName() == loopVariable.getName(); + iteratorVariableDeclaration.getName().equals(loopVariable.getName());Based on learnings, prefer using
Name.equals(...)orObjects.equals(...)forcom.sun.source.util.Namecomparisons instead of==/!=.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 1375 - 1380, Replace the reference equality check for Names with a value equality check: in the block where you compute isAssignmentOfLoopVariable (inside RLCCalledMethodsAnnotatedTypeFactory, the AssignmentNode/VariableTree handling that uses iteratorVariableDeclaration.getName() and loopVariable.getName()), change the comparison from "==" to use Name.equals(...) or Objects.equals(...) so equal names are detected even when not the same object; ensure null-safety (use Objects.equals) if either name can be null.
405-472:⚠️ Potential issue | 🟠 MajorPreserve loop state for lambda CFGs.
Loop state is still keyed only by enclosing
MethodTree, andgetEnclosingMethodTree(...)ignoresUnderlyingAST.Kind.LAMBDA. A loop recorded inside a lambda can survive the enclosing method pass unanalyzed, then be removed at Line 1203 before the lambda CFG’spostAnalyze(...)can consume it.Also applies to: 1187-1204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 405 - 472, The code currently keys loop state only by MethodTree and getEnclosingMethodTree ignores UnderlyingAST.Kind.LAMBDA, causing loop state created inside lambdas to be removed before the lambda CFG postAnalyze can use it; fix by making the enclosing-key logic lambda-aware: update getEnclosingMethodTree (or add a new getEnclosingNodeKey) to handle UnderlyingAST.Kind.LAMBDA (use UnderlyingAST.CFGLambda to obtain the LambdaTree or other unique lambda key), change collectionLoopStateByEnclosingMethod’s key type from MethodTree to a common supertype (e.g., Tree or Object) or a dedicated key wrapper, and update getMethodCollectionLoopState, removeMethodCollectionLoopState, getPotentiallyFulfillingCollectionLoops, and getResolvedPotentiallyFulfillingCollectionLoops to use that new key so lambda CFGs preserve and access their MethodCollectionLoopState.checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (2)
227-230:⚠️ Potential issue | 🔴 CriticalAvoid casting
MustCallNoCreatesMustCallForCheckertoCollectionOwnershipChecker.This branch still accepts
MustCallNoCreatesMustCallForChecker, then immediately casts it toCollectionOwnershipChecker, which will throwClassCastExceptionwhen reached. Route Must Call checkers through their parent instead, and only cast after aninstanceof CollectionOwnershipCheckercheck.Suggested fix
- if ("CollectionOwnershipChecker".equals(className) - || "MustCallNoCreatesMustCallForChecker".equals(className)) { + if ("CollectionOwnershipChecker".equals(className)) { return (CollectionOwnershipAnnotatedTypeFactory) ((CollectionOwnershipChecker) referenceChecker).getTypeFactory(); - } else if ("RLCCalledMethodsChecker".equals(className)) { + } else if ("RLCCalledMethodsChecker".equals(className) + || "MustCallChecker".equals(className) + || "MustCallNoCreatesMustCallForChecker".equals(className)) { return getCollectionOwnershipAnnotatedTypeFactory(referenceChecker.getParentChecker()); - } else if ("MustCallChecker".equals(className)) { - return getCollectionOwnershipAnnotatedTypeFactory(referenceChecker.getParentChecker());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java` around lines 227 - 230, The branch currently accepts "MustCallNoCreatesMustCallForChecker" then casts referenceChecker to CollectionOwnershipChecker causing ClassCastException; change the logic so that if className equals "MustCallNoCreatesMustCallForChecker" you first route to the parent checker (e.g., replace referenceChecker with its parent checker), remove the direct string-based cast, and then perform an instanceof check like "if (referenceChecker instanceof CollectionOwnershipChecker)" before casting to CollectionOwnershipChecker and calling getTypeFactory() to return a CollectionOwnershipAnnotatedTypeFactory.
454-467:⚠️ Potential issue | 🟠 MajorDetect only explicit
@MustCallUnknownannotations here.
getPrimaryAnnotationInHierarchy(mcAtf.TOP)returns the effective qualifier, so an unannotated type defaulted to top is treated as manually annotated@MustCallUnknown. Delegate to theTypeMirroroverload or inspectannotatedTypeMirror.getUnderlyingType().getAnnotationMirrors()instead.Suggested fix
public static boolean hasManualMustCallUnknownAnno( AnnotatedTypeMirror annotatedTypeMirror, MustCallAnnotatedTypeFactory mcAtf) { if (annotatedTypeMirror == null) { return false; } - AnnotationMirror manualMcAnno = annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP); - if (manualMcAnno == null) { - return false; - } - if (AnnotationUtils.areSameByName(manualMcAnno, MustCallUnknown.class.getCanonicalName())) { - return true; - } - return false; + return hasManualMustCallUnknownAnno(annotatedTypeMirror.getUnderlyingType()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java` around lines 454 - 467, The current hasManualMustCallUnknownAnno(AnnotatedTypeMirror, MustCallAnnotatedTypeFactory) incorrectly treats an effective TOP qualifier as an explicit `@MustCallUnknown` because it uses annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP); change it to detect only explicit annotations by delegating to the TypeMirror overload or by inspecting the underlying type's annotation mirrors (annotatedTypeMirror.getUnderlyingType().getAnnotationMirrors()) and checking for AnnotationUtils.areSameByName(..., MustCallUnknown.class.getCanonicalName()); update the method to return true only when an explicit `@MustCallUnknown` is present on the underlying TypeMirror rather than when the effective qualifier is TOP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java`:
- Around line 69-72: The iterator method in LinkedHashKeyedSet currently
annotates only the receiver and returns an unannotated Iterator, breaking the
polymorphic ownership link; update the method signature of
LinkedHashKeyedSet.iterator to return a `@PolyOwningCollection` Iterator<V>
(matching the pattern used by Insertions.iterator and ASTPath.iterator) so the
returned KeyedSetIterator preserves the owning/NonOwning relationship with the
collection.
In
`@framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/JavaDiagnosticReader.java`:
- Around line 264-269: Fix the Javadoc on the reader-advance method: change the
incorrect field reference "NextLineNumber" to the actual field name
"nextLineNumber" and update the `@throws` line to state that IOException is
propagated from reader.readLine(); reference the fields nextLine and
nextLineNumber and the reader.readLine() call so the comment clearly maps to the
code.
---
Outside diff comments:
In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java`:
- Around line 1371-1388: Add Javadoc `@throws` tags to document the runtime
exceptions already thrown: in the Javadoc for peek() add "@throws
IllegalStateException if the stack is empty" (for method peek), for pop() add
"@throws IllegalStateException if the stack is empty" (for method pop), and for
get(int) add "@throws NoSuchElementException if the requested index is not
present" (for method get(int)). Place each `@throws` in the corresponding method's
Javadoc block immediately under the description so the API contract matches the
thrown exceptions.
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`:
- Around line 825-835: Replace the non-atomic static long uid and its increment
in uniqueName(String prefix) with an AtomicLong to make synthetic name
generation thread-safe: change the static field uid to an AtomicLong
(initialized to 0) and update uniqueName(String) to call uid.getAndIncrement()
when composing the returned string; ensure the symbol names uid and
uniqueName(...) are used so you update the correct field and method.
---
Duplicate comments:
In
`@annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java`:
- Around line 110-155: The ownership annotations are inconsistent: add(`@Owning` V
o) declares transfer but add(V o, int conflictBehavior, int equalBehavior) and
replace(V v) do not, allowing bypass of ownership guarantees; update the
signatures of LinkedHashKeyedSet.add(V, int, int) and
LinkedHashKeyedSet.replace(V) to accept `@Owning` V (same as add(`@Owning` V)), and
mirror those `@Owning` annotations in the abstract/interface declarations in
KeyedSet (KeyedSet#add(V,int,int) and KeyedSet#replace(V)) so all
insertion/replacement paths consistently require ownership; also add or adjust
any necessary imports/annotations to satisfy the compiler.
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 293-338: The current logic only recurses into declared type
arguments when ResourceLeakUtils.isCollection(adt.getUnderlyingType()) is true,
so nested declared args (e.g., Optional<List<?>>) are skipped; modify
MustCallAnnotatedTypeFactory so that after handling the collection-specific
branch it still iterates over adt.getTypeArguments() and calls
replaceCollectionTypeVarsWithBottomIfTop(tree, (AnnotatedDeclaredType) typeArg)
for any typeArg with getKind() == TypeKind.DECLARED (or extract that recursion
into a helper and invoke it unconditionally for declared type args), ensuring
you still preserve the existing collection-specific handling (the block that
checks wildcards, TYPEVARs, TOP/BOTTOM, and uses
ResourceLeakUtils.hasManualMustCallUnknownAnno).
- Around line 261-267: The loop in getRlccAtfIfPartOfHierarchy starts from the
current checker, which allows a standalone MustCall checker to match
ResourceLeakUtils.rlcCheckers and cause
getRLCCalledMethodsAnnotatedTypeFactory(this) to walk a missing RLC parent and
crash; change the start of the search to the parent checker (e.g., set
currentChecker = checker.getParentChecker() before the while loop or otherwise
skip the initial checker instance) so the method scans ancestors only, keeping
references to ResourceLeakUtils.rlcCheckers and
ResourceLeakUtils.getRLCCalledMethodsAnnotatedTypeFactory unchanged.
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 907-914: In
MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, remove the
BugInCF throw when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null and instead treat null as "no collection obligation": check the returned
mustCallValues from getMustCallValuesOfResourceCollectionComponent(...) and if
it is null simply skip creating any CollectionObligation(s) for this
OwningCollection return (do not create obligations or throw), leaving normal
processing for non-null results intact.
- Around line 3726-3729: The code calls getCalledMethods(cmValOfIce) without
guarding against store.getValue(ice) returning null; add a null-check on the
AccumulationValue (cmValOfIce) before invoking getCalledMethods and
skip/continue the loop when cmValOfIce is null (do the same null-guarding at the
other occurrence around lines handling ice at the 3767-3768 area), ensuring
calledMethods is only computed when cmValOfIce != null.
- Around line 2103-2109: The code incorrectly treats
CollectionOwnershipType.OwningCollectionBottom as an error; in
MustCallConsistencyAnalyzer replace the throw of BugInCF in the branch checking
lhsCoType == CollectionOwnershipType.OwningCollectionBottom with logic that
treats OwningCollectionBottom (null) as allowed on the RHS of assignment —
simply skip/error-reporting and proceed as the safe null-to-collection overwrite
case (remove the BugInCF throw for that case and let normal assignment handling
continue).
- Around line 2118-2127: The code in MustCallConsistencyAnalyzer calls
checkEnclosingMethodIsCreatesMustCallFor(lhs, enclosingMethodTree) but ignores
its boolean result and still removes RHS obligations; change this to capture the
result (e.g., boolean creates = checkEnclosingMethodIsCreatesMustCallFor(...))
and only call getObligationsForVar(...) and remove obligations from the
obligations set when creates is true; ensure you reference
checkEnclosingMethodIsCreatesMustCallFor, getObligationsForVar, and the
obligations.remove(...) loop so RHS obligations are not removed after a failed
check.
- Around line 3559-3572: The current code ignores a null summary from
analyzeTypeOfCollectionElement(...) and thus can incorrectly accept the loop;
change the handling so that when calledMethodsAfterBlock == null you treat that
as a verification failure: set calledMethodsInLoop = null (or another failure
sentinel) and break/return immediately so the loop is marked failing instead of
preserving prior intersections. Update the block around isLastBlockOfBody where
calledMethodsAfterBlock is used (references: isLastBlockOfBody,
analyzeTypeOfCollectionElement, calledMethodsInLoop, calledMethodsAfterBlock) to
implement this early-failure behavior.
- Around line 2051-2056: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) can
NPE/IndexOutOfBounds when the list is null or empty; update
MustCallConsistencyAnalyzer to safely fetch the method name (use "Unknown"
fallback) before calling checker.reportError so you pass a non-null string.
Specifically, wrap or replace the direct .get(0) access (the
getMustCallValuesOfResourceCollectionComponent(...) expression used in the
checker.reportError call) with a helper or inline check that returns the first
element if present else "Unknown"; apply the same safe-access pattern to the
analogous reportError usages around the block referenced (also at the other call
sites in the 2131-2136/2134 region).
- Around line 933-975: The collection-obligation helpers
(addObligationsForCreatesCollectionObligationAnno,
consumeInsertedArgumentObligationIfSingleElementInsert,
reportNeverEnforcedCollectionObligationIfNeeded,
checkEnclosingMethodIsCreatesMustCallFor) are being invoked on every successor
edge; change the call site in updateObligationsForInvocation (or add a boolean
parameter to addObligationsForCreatesCollectionObligationAnno) so these helpers
run only for the normal/successor (i.e., successful-invocation) edge and
early-return on exceptional edges, and then ensure the obligations created on
the normal edge are propagated/enforced for all later exits (e.g., by adding the
newly created CollectionObligation objects into the stores/obligation sets used
for subsequent exceptional and normal exit processing so later exits will check
them).
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java`:
- Around line 311-333: The code creates a local WholeProgramInference wpi from
coAtf but later helpers still call resourceLeakAtf.getWholeProgramInference(),
which can split annotations across instances; change the helpers
(addEnsuresCalledMethodsForDisposedFields, addMustCallAliasToFormalParameter,
addInheritableMustCallToClass and any helpers they call) to accept a
WholeProgramInference parameter and use the passed wpi instead of calling
resourceLeakAtf.getWholeProgramInference(), then update the calls in
MustCallInference (the section using coAtf and returnObligationToParameter) to
pass the local wpi through so all writes use the same WholeProgramInference
instance.
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java`:
- Around line 227-230: The branch currently accepts
"MustCallNoCreatesMustCallForChecker" then casts referenceChecker to
CollectionOwnershipChecker causing ClassCastException; change the logic so that
if className equals "MustCallNoCreatesMustCallForChecker" you first route to the
parent checker (e.g., replace referenceChecker with its parent checker), remove
the direct string-based cast, and then perform an instanceof check like "if
(referenceChecker instanceof CollectionOwnershipChecker)" before casting to
CollectionOwnershipChecker and calling getTypeFactory() to return a
CollectionOwnershipAnnotatedTypeFactory.
- Around line 454-467: The current
hasManualMustCallUnknownAnno(AnnotatedTypeMirror, MustCallAnnotatedTypeFactory)
incorrectly treats an effective TOP qualifier as an explicit `@MustCallUnknown`
because it uses annotatedTypeMirror.getPrimaryAnnotationInHierarchy(mcAtf.TOP);
change it to detect only explicit annotations by delegating to the TypeMirror
overload or by inspecting the underlying type's annotation mirrors
(annotatedTypeMirror.getUnderlyingType().getAnnotationMirrors()) and checking
for AnnotationUtils.areSameByName(...,
MustCallUnknown.class.getCanonicalName()); update the method to return true only
when an explicit `@MustCallUnknown` is present on the underlying TypeMirror rather
than when the effective qualifier is TOP.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 1518-1534: The loop selection is using backEdge.targetBlock (the
loop header) as the update/latch, which causes later store reads to see header
state instead of latch state; change the assignment to use backEdge.sourceBlock
(the back-edge source/latch) when setting bestLoopUpdateBlock inside the loop
over WhileLoopResolutionCache.BlockEdge (whileLoopCache.getBackEdges()), keeping
the same checks for naturalLoop.contains(bodyEntryBlock) and conditionalBlock
and the tightest-loop preference (naturalLoop.size() < bestLoopSize).
- Around line 1375-1380: Replace the reference equality check for Names with a
value equality check: in the block where you compute isAssignmentOfLoopVariable
(inside RLCCalledMethodsAnnotatedTypeFactory, the AssignmentNode/VariableTree
handling that uses iteratorVariableDeclaration.getName() and
loopVariable.getName()), change the comparison from "==" to use Name.equals(...)
or Objects.equals(...) so equal names are detected even when not the same
object; ensure null-safety (use Objects.equals) if either name can be null.
- Around line 405-472: The code currently keys loop state only by MethodTree and
getEnclosingMethodTree ignores UnderlyingAST.Kind.LAMBDA, causing loop state
created inside lambdas to be removed before the lambda CFG postAnalyze can use
it; fix by making the enclosing-key logic lambda-aware: update
getEnclosingMethodTree (or add a new getEnclosingNodeKey) to handle
UnderlyingAST.Kind.LAMBDA (use UnderlyingAST.CFGLambda to obtain the LambdaTree
or other unique lambda key), change collectionLoopStateByEnclosingMethod’s key
type from MethodTree to a common supertype (e.g., Tree or Object) or a dedicated
key wrapper, and update getMethodCollectionLoopState,
removeMethodCollectionLoopState, getPotentiallyFulfillingCollectionLoops, and
getResolvedPotentiallyFulfillingCollectionLoops to use that new key so lambda
CFGs preserve and access their MethodCollectionLoopState.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62687f54-df22-4c87-992b-2fbb9948c2db
📒 Files selected for processing (13)
annotation-file-utilities/src/main/java/org/checkerframework/afu/annotator/find/Insertions.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/classfile/ClassAnnotationSceneWriter.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.javachecker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.javachecker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javaframework-test/src/main/java/org/checkerframework/framework/test/diagnostics/JavaDiagnosticReader.javaframework/src/main/java/org/checkerframework/common/accumulation/AccumulationTransfer.javaframework/src/main/java/org/checkerframework/common/value/ValueCheckerUtils.java
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java (1)
824-834:⚠️ Potential issue | 🟠 MajorKeep the synthetic-name counter instance-scoped.
Making
uidstatic makes every artificial variable name depend on whatever CFGs were translated earlier, which adds unnecessary cross-analysis state and order-dependent names foriter,index,switchExpr,tempPostfix, etc. The previous instance field already guaranteed uniqueness within a single translation.Suggested fix
- protected static long uid = 0; + protected long uid = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java` around lines 824 - 834, The synthetic-name counter `uid` is currently static, causing cross-analysis and order-dependent names; change its declaration in class CFGTranslationPhaseOne from "protected static long uid = 0;" to an instance field (e.g., "protected long uid = 0;") so that uniqueName(String prefix) uses an instance-scoped counter and each translation gets its own sequence; update any code that assumed a static field to use the instance field if applicable.
♻️ Duplicate comments (24)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
1696-1699:⚠️ Potential issue | 🔴 CriticalGuard argument access in
getIdxForGetCallto avoid crash.Line 1698 dereferences argument index
0unconditionally. A zero-argget()call will throwIndexOutOfBoundsException.🐛 Proposed fix
public static `@Nullable` ExpressionTree getIdxForGetCall(Tree tree) { - if ((tree instanceof MethodInvocationTree mit) && isNamedMethodCall("get", mit)) { + if ((tree instanceof MethodInvocationTree mit) + && isNamedMethodCall("get", mit) + && !mit.getArguments().isEmpty()) { return mit.getArguments().get(0); } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java` around lines 1696 - 1699, getIdxForGetCall currently assumes the MethodInvocationTree mit has at least one argument and calls mit.getArguments().get(0), which can throw IndexOutOfBoundsException for zero-arg get() calls; update getIdxForGetCall to first check the argument list (e.g., mit.getArguments().isEmpty() or size() > 0) before accessing index 0 and return null when there are no arguments so zero-arg calls are safely handled.checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (1)
177-221:⚠️ Potential issue | 🟠 MajorAlways recurse into nested declared type arguments.
replaceCollectionTypeVarsWithBottomIfTopstill only descends intoAnnotatedDeclaredTypearguments when the outer type is a collection. That means wrappers likeOptional<List<?>>orHolder<Iterator<?>>still skip the inner collection/iterator and leave its element type at@MustCallUnknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java` around lines 177 - 221, The current logic only enters the loop that calls replaceCollectionTypeVarsWithBottomIfTop when ResourceLeakUtils.isCollection(adt.getUnderlyingType()), so nested declared type arguments inside non-collection wrappers (e.g., Optional<List<?>>) are never traversed; update the traversal so that for any AnnotatedDeclaredType you always recurse into its type arguments and call replaceCollectionTypeVarsWithBottomIfTop on each declared type argument (use the existing method replaceCollectionTypeVarsWithBottomIfTop and the same handling used for typeArg.getKind() == TypeKind.DECLARED), not just when the outer adt is a collection, ensuring inner collections/iterators have their element types reset from `@MustCallUnknown` to `@MustCallBottom` when appropriate.framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)
1199-1214:⚠️ Potential issue | 🟠 MajorHandle missing block input in
getRegularStore.
flowResult.getInput(block)/analysis.getInput(block)can be absent for unreachable or unanalyzed blocks. This dereferencesinputunconditionally, so callers get an NPE instead of the nullable behavior used by the neighboring store accessors.Suggested fix
- public Store getRegularStore(Block block) { + public `@Nullable` Store getRegularStore(Block block) { TransferInput<Value, Store> input; if (!analysis.isRunning()) { input = flowResult.getInput(block); } else { input = analysis.getInput(block); } - - return input.getRegularStore(); + if (input == null) { + return null; + } + return input.getRegularStore(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java` around lines 1199 - 1214, The getRegularStore method dereferences the TransferInput from flowResult.getInput(block) or analysis.getInput(block) without null-check, causing NPE for unreachable/unanalyzed blocks; update getRegularStore to check whether the selected TransferInput is null and return null (matching neighboring store accessors' nullable behavior) when absent instead of calling input.getRegularStore(), using the same selection logic around analysis.isRunning(), and reference the symbols getRegularStore, flowResult.getInput, analysis.getInput, and TransferInput in your change.checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java (1)
143-149:⚠️ Potential issue | 🟠 MajorUse value equality for
Namecomparisons.These
Namechecks rely on reference equality. That can silently miss valid matches or accept the wrong symbol when the compiler implementation does not reuse the sameNameinstance. Use.equals(...)/Objects.equals(...)throughout this matcher.Based on learnings: In
MustCallVisitor.java(Checker Framework), prefer usingName.equals(...)orObjects.equals(...)forcom.sun.source.util.Namecomparisons instead of==/!=.Also applies to: 190-225, 252-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java` around lines 143 - 149, The code in IndexedForDisposalLoopMatcher uses reference equality (== / !=) for com.sun.source.util.Name comparisons which is unsafe; replace these with value equality using .equals(...) or Objects.equals(...) for initVarName vs ((IdentifierTree) condition.getLeftOperand()).getName() and initVarName vs ((IdentifierTree) inc.getExpression()).getName(), and update the other similar checks in this class (the other Name comparisons around the later matching logic) to use .equals/Objects.equals so name comparisons are by value rather than reference.checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (1)
183-191:⚠️ Potential issue | 🟠 MajorLambda disposal loops still never reach discovery.
GenericAnnotatedTypeFactory.postCFGConstruction(...)is invoked for lambda CFGs too, but this guard only records disposal loops forMETHOD. That means a disposal loop inside a lambda is never available toRLCCalledMethodsTransfer.initialStore(...), so collection obligations inside lambdas cannot be discharged through this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 183 - 191, The postCFGConstruction method only calls ResourceLeakUtils.getCollectionOwnershipAnnotatedTypeFactory(this).discoverDisposalLoops(cfg) when ast.getKind() == UnderlyingAST.Kind.METHOD, which skips lambda CFGs; update the guard so disposal-loop discovery also runs for lambda CFGs (e.g., ast.getKind() == UnderlyingAST.Kind.METHOD || ast.getKind() == UnderlyingAST.Kind.LAMBDA) or remove the kind check entirely so discoverDisposalLoops(cfg) runs for lambda CFGs as well, ensuring RLCCalledMethodsTransfer.initialStore(...) sees disposal loops inside lambdas.checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java (2)
313-313: 🧹 Nitpick | 🔵 TrivialPrefer
isEmpty()oversize() == 0for clarity.Suggested improvement
- boolean isDiamond = node.getTree().getTypeArguments().size() == 0; + boolean isDiamond = node.getTree().getTypeArguments().isEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java` at line 313, The expression checking for an empty type-arguments list uses size() == 0 which is less clear; replace the check in CollectionOwnershipTransfer where isDiamond is assigned (currently using node.getTree().getTypeArguments().size() == 0) with the clearer isEmpty() call on the type arguments collection (e.g., node.getTree().getTypeArguments().isEmpty()) so isDiamond uses isEmpty() instead of size() == 0.
253-282:⚠️ Potential issue | 🟠 MajorGuard nullable
argElembefore callinggetKind().
TreeUtils.elementFromTree(arg.getTree())can returnnullfor temp vars, method results, and other non-element trees. Line 276 callsargElem.getKind()without checking for null, which will throwNullPointerException.Proposed fix
if (transferOwnership) { - if (argElem.getKind().isField()) { + if (argElem != null && argElem.getKind().isField()) { checker.reportError( arg.getTree(), "transfer.owningcollection.field.ownership", arg.getTree().toString()); } else { replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java` around lines 253 - 282, Guard against a null Element returned by TreeUtils.elementFromTree(arg.getTree()) before calling getKind(): in CollectionOwnershipTransfer (around the block using argElem, transferOwnership, and the calls to checker.reportError and replaceInStores) add a null-check so that if argElem is null it is treated as "not a field" (i.e., call replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION)) and only call argElem.getKind().isField() when argElem != null; update the conditional that currently does if (argElem.getKind().isField()) ... else ... to first test argElem != null.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (2)
79-80:⚠️ Potential issue | 🟠 MajorEvict iterated-element facts after mutations.
The
iteratedCollectionElementsmap is not cleared in mutation/invalidation paths. Methods likeupdateForMethodCall,removeConflicting(FieldAccess, ...),removeConflicting(ArrayAccess, ...), andremoveConflicting(LocalVariable)do not invalidate entries initeratedCollectionElements, which could allow stale facts to leak across iterations or after side-effecting operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 79 - 80, The map iteratedCollectionElements can retain stale facts across mutations; update the mutation/invalidation paths to evict affected entries by clearing or removing matching keys from iteratedCollectionElements: in updateForMethodCall(...) and the removeConflicting(...) overloads (removeConflicting(FieldAccess,...), removeConflicting(ArrayAccess,...), removeConflicting(LocalVariable,...)) identify which IteratedCollectionElement keys reference the mutated target and remove them (or clear the whole map when a conservative global mutation occurs) so iteratedCollectionElements cannot leak outdated facts after side-effecting operations.
841-848:⚠️ Potential issue | 🟡 MinorLoose OR matching logic should require both node and tree to match.
The condition on line 843 uses OR logic across four comparisons, returning the first
IteratedCollectionElementwhere any condition is true. SinceIteratedCollectionElement.equals()requires bothtreeandnodeto match, the lookup should also enforce both requirements to avoid returning incorrect entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 841 - 848, The lookup in getIteratedCollectionElement currently returns an IteratedCollectionElement if any of four comparisons match; change the logic to require both node and tree to match (mirroring IteratedCollectionElement.equals). In method getIteratedCollectionElement, iterate iteratedCollectionElements.keySet() and replace the compound condition (ice.tree == tree || ice.node == node || ice.tree.equals(tree) || ice.node.equals(node)) with a check that both components match (e.g., (ice.tree == tree || (ice.tree != null && ice.tree.equals(tree))) && (ice.node == node || (ice.node != null && ice.node.equals(node)))), then return ice only when both match.checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (1)
41-41:⚠️ Potential issue | 🟡 MinorMinor Javadoc formatting issue.
Line 41: missing space between "exact" and "{
@linkThrowable}".Proposed fix
- * <p>The policy follows the Resource Leak Checker configuration, but keeps exact{`@link` Throwable} + * <p>The policy follows the Resource Leak Checker configuration, but keeps exact {`@link` Throwable}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java` at line 41, In the Javadoc for class CollectionOwnershipAnalysis update the comment that currently reads "exact{`@link` Throwable}" to insert a space so it becomes "exact {`@link` Throwable}"; locate the Javadoc in CollectionOwnershipAnalysis.java and adjust the spacing between "exact" and the {`@link` Throwable} tag to fix the formatting.checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java (1)
88-92:⚠️ Potential issue | 🟠 MajorFall back to declared receiver qualifier when flow store has no entry.
When
getCoTypeAtTreereturnsnull(no store entry), the method exits early without checking the declared type. This skips the@CreatesCollectionObligationenforcement for receivers whose ownership is only available from the declared type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java` around lines 88 - 92, When getCoTypeAtTree(receiverTree, storeBefore) returns null, don't return immediately; instead obtain the declared/annotated receiver type (via CollectionOwnershipAnnotatedTypeFactory.CollectionOwnershipType from the annotated type factory using receiverTree or getAnnotatedType(receiverTree)) and assign it to receiverType so the subsequent `@CreatesCollectionObligation` check runs even when the flow store lacks an entry; keep the existing null guards and only return if both flow and declared lookups produce null.checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (5)
1023-1025:⚠️ Potential issue | 🟠 MajorDon't compare
Nameinstances with==.Different
Nameobjects can have equal content, so this can miss the matching parameter slot and fail to propagate the declaration annotation to the use site.Proposed fix
- if (params.get(i).getSimpleName() == elt.getSimpleName()) { + if (params.get(i).getSimpleName().contentEquals(elt.getSimpleName())) { type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)); break; }Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java` around lines 1023 - 1025, The code in CollectionOwnershipAnnotatedTypeFactory compares Name objects using == (params.get(i).getSimpleName() == elt.getSimpleName()), which can miss matches; change the comparison to use equals (e.g., params.get(i).getSimpleName().equals(elt.getSimpleName()) or Objects.equals(...)) so the correct parameter slot from paramTypes is located and the declaration annotation is propagated via type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)).
691-693:⚠️ Potential issue | 🔴 CriticalGuard
getAnnotatedType(tree)here and returnnullon unsupported trees.This helper is called from the resource-leak analysis on arbitrary receiver trees. Without the same
BugInCFguard used byisResourceCollection(Tree), unsupported tree shapes still crash analysis instead of yielding “no obligations”.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java` around lines 691 - 693, Guard the call to mcAtf.getAnnotatedType(tree) inside CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent by catching the same BugInCF/unsupported-tree condition used in isResourceCollection(Tree) and return null when the tree shape is unsupported; update getMustCallValuesOfResourceCollectionComponent to return null instead of letting the exception bubble. Then update MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn to treat a null return from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” (i.e., skip creating CollectionObligation instances for OwningCollection element types when the annotated type cannot be determined) so analysis does not crash on non-resource element types.
488-516:⚠️ Potential issue | 🟡 MinorMirror the null guard from
isOwningCollectionField(...).
isResourceCollectionField(...)andisOwningCollectionParameter(...)both dereferenceeltimmediately. Callers already pass nullable elements elsewhere in this PR, so these helpers should returnfalseinstead of NPEing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java` around lines 488 - 516, Both isResourceCollectionField(Element elt) and isOwningCollectionParameter(Element elt) assume elt is non-null and can NPE; mirror the null guard used elsewhere by adding an initial null check in each method (e.g., if (elt == null) return false;) before any elt dereferences. This ensures calls to isResourceCollectionField and isOwningCollectionParameter (which call elt.getKind(), elt.asType(), getAnnotatedType(elt), etc.) safely return false for null elements without changing downstream logic that uses getAnnotatedType, getCoType, or compares to CollectionOwnershipType.OwningCollection.
609-620:⚠️ Potential issue | 🟠 MajorUse stub-aware ownership checks for inserted arguments.
Element.getAnnotation(...)ignores annotations supplied via stub files. A stubbed@Owningor@NotOwningdeclaration will be misclassified here, which breaks collection-obligation transfer soundness. UserlAtf.hasOwning(...)/rlAtf.hasNotOwning(...)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java` around lines 609 - 620, The code in CollectionOwnershipAnnotatedTypeFactory uses Element.getAnnotation(...) on insertedElement (from insertedArgumentTree) which ignores stub-file annotations; replace those checks with the stub-aware queries rlAtf.hasNotOwning(insertedElement) and rlAtf.hasOwning(insertedElement) so the logic that returns CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING or MAY_BE_OWNING remains the same but respects stub declarations; update both the initial NotOwning check and the parameter Owning check to call rlAtf.hasNotOwning(...) / rlAtf.hasOwning(...) on insertedElement instead of getAnnotation(...).
657-678:⚠️ Potential issue | 🟠 Major
Map<K,V>still falls through as “no component obligations”.
ResourceLeakUtils.isCollection(...)treatsMapas collection-like, but both overloads only extract a component when there is exactly one type argument. Resource obligations on map values are silently skipped.Also applies to: 706-722
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java` around lines 657 - 678, getMustCallValuesOfResourceCollectionComponent currently only extracts a single component type argument, so Map<K,V> (which has two type arguments) falls through and value obligations are ignored; update the extraction logic in getMustCallValuesOfResourceCollectionComponent to handle both single-arg collection types and two-arg map-like types: if atm is an AnnotatedDeclaredType with one type argument use that, and if it has two type arguments (i.e., map-like) select the second argument as the component; then pass that component to ResourceLeakUtils.getMcValues(componentType, mcAtf) as before (refer to getMustCallValuesOfResourceCollectionComponent, ResourceLeakUtils.isCollection, AnnotatedDeclaredType, and ResourceLeakUtils.getMcValues).checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)
3547-3560:⚠️ Potential issue | 🔴 CriticalA
nullback-edge summary must invalidate the whole loop proof.If one back edge returns
null, skipping the intersection preserves methods learned from other back edges and can still certify an unverified disposal loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 3547 - 3560, If any back-edge summary (calledMethodsAfterBlock returned from analyzeTypeOfCollectionElement for the block within the loop) is null, the entire loop proof must be invalidated instead of preserving prior intersections; update the logic in the isLastBlockOfBody branch so that when calledMethodsAfterBlock == null you set calledMethodsInLoop = null (and avoid retaining previous values), using the existing variables calledMethodsInLoop, calledMethodsAfterBlock, currentBlock, disposalLoop, obligations, and loopUpdateBlock to locate the code and enforce that a single null summary clears the accumulated calledMethodsInLoop.
2048-2053:⚠️ Potential issue | 🟠 MajorGuard collection method-name lookup before calling
.get(0).Both diagnostics assume
getMustCallValuesOfResourceCollectionComponent(...)returns a non-empty list. It can returnnullor an empty list, so these error paths can throw instead of reporting. Use the existing unknown-name fallback when no element method is available.Based on learnings: When constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Also applies to: 2131-2135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 2048 - 2053, The call to coAtf.getMustCallValuesOfResourceCollectionComponent(...) in MustCallConsistencyAnalyzer is assumed non-empty before calling .get(0), which can cause NPEs or IndexOutOfBounds; update both places that call checker.reportError(...) (the one using that list and the similar block around the other reportError) to first check for null or empty and extract a safe method-name (use the existing "Unknown" fallback used elsewhere) before passing it into checker.reportError; i.e., compute a safeMethodName = list != null && !list.isEmpty() ? list.get(0) : "Unknown" (or reuse the module's fallback helper) and use that value in the reportError invocation.
3713-3715:⚠️ Potential issue | 🟠 MajorHandle
store.getValue(ice) == nullbefore callinggetCalledMethods(...).
getCalledMethodsdereferences its argument, so this path can NPE on bottom / no-live-element states instead of treating the back-edge summary as unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 3713 - 3715, The code calls getCalledMethods(cmValOfIce) without checking that store.getValue(ice) returned a non-null AccumulationValue; guard against null by assigning AccumulationValue cmValOfIce = store.getValue(ice) and then checking cmValOfIce != null before invoking getCalledMethods(cmValOfIce) (or treat a null as "no summary" and skip the back-edge handling). Update the branch that uses cmValOfIce/calledMethods (referenced symbols: store.getValue, ice, cmValOfIce, getCalledMethods) to return/continue when cmValOfIce is null so getCalledMethods is never called on null.
965-974:⚠️ Potential issue | 🟠 MajorDon't consume the inserted value after
checkEnclosingMethodIsCreatesMustCallFor(...)fails.On the error path, the code still removes the caller-side obligation for the inserted element. That makes the analysis behave as if ownership transferred anyway, which can hide a later leak on the original value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 965 - 974, The code currently always calls consumeInsertedArgumentObligationIfSingleElementInsert(...), even when checkEnclosingMethodIsCreatesMustCallFor(...) reports a failure; change the control flow so the obligation is only consumed when the enclosing-method check succeeds. Modify checkEnclosingMethodIsCreatesMustCallFor (or add a small wrapper) to return a boolean success flag (e.g., true when no error reported) or provide a way to detect failure, then call consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node) only if that flag is true; update references in MustCallConsistencyAnalyzer around receiverIsOwningField / receiverNode / enclosingMethodTree accordingly.
899-914:⚠️ Potential issue | 🔴 CriticalTreat
nullcomponent must-call values as “no obligations”.
getMustCallValuesOfResourceCollectionComponent(...)can legitimately returnnullfor@OwningCollectionover non-resource element types. ThrowingBugInCFhere turns a valid program into an analyzer crash.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); - } - if (!ResourceLeakUtils.isIterator(node.getType())) { - for (String mustCallMethod : mustCallValues) { - obligations.add( - new CollectionObligation( - mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); - } + if (mustCallValues == null || mustCallValues.isEmpty() + || ResourceLeakUtils.isIterator(node.getType())) { + return; + } + for (String mustCallMethod : mustCallValues) { + obligations.add( + new CollectionObligation( + mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); }Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 899 - 914, In addObligationsForOwningCollectionReturn, stop throwing a BugInCF when coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns null; instead treat null as “no obligations” and skip creating CollectionObligation(s) for that case. Locate the check that assigns mustCallValues from getMustCallValuesOfResourceCollectionComponent, and if mustCallValues is null simply return (or continue) without creating ResourceAlias tmpVarAsResourceAlias or adding obligations to the obligations set; preserve existing behavior for non-null mustCallValues. Ensure references include addObligationsForOwningCollectionReturn, cmAtf.getTempVarForNode, coAtf.getStoreAfter / getCoType, and coAtf.getMustCallValuesOfResourceCollectionComponent.
536-607:⚠️ Potential issue | 🟠 Major
CollectionObligationstill breaks theequals/hashCodecontract.
mustCallMethodparticipates inequals, buthashCodeis inherited fromObligation, and the field is mutable even thoughObligationis treated as deeply immutable. Distinct obligations can collide or be corrupted in hash-based collections.Proposed fix
/** The method that must be called on all elements of the collection. */ - public String mustCallMethod; + public final String mustCallMethod; @@ `@Override` public boolean equals(`@Nullable` Object obj) { @@ return super.equals(obj) && collectionObligation.mustCallMethod.equals(this.mustCallMethod); } + + `@Override` + public int hashCode() { + return Objects.hash(super.hashCode(), mustCallMethod); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 536 - 607, CollectionObligation breaks equals/hashCode because mustCallMethod is mutable and not included in hashCode; make mustCallMethod final (change declaration to final and update constructors to set it) and override hashCode in CollectionObligation to combine super.hashCode() with mustCallMethod.hashCode() (e.g., 31*super.hashCode()+mustCallMethod.hashCode()) so equals and hashCode remain consistent for CollectionObligation instances; keep getReplacement() behavior as-is.
2100-2105:⚠️ Potential issue | 🔴 Critical
OwningCollectionBottomis the nullable-field case, not aBugInCF.This branch is reached when the field currently holds
null. Overwritingnullwith a new owning collection should be treated as a safe assignment instead of aborting analysis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 2100 - 2105, In MustCallConsistencyAnalyzer, the branch that checks "lhsCoType == CollectionOwnershipType.OwningCollectionBottom" should not throw a BugInCF because OwningCollectionBottom represents a nullable field being overwritten; replace the throw with code that treats this as a safe assignment (e.g., a no-op or appropriate continuation of analysis) and update the comment to note that OwningCollectionBottom means the field may be null and can be safely assigned; refer to the lhsCoType check in MustCallConsistencyAnalyzer and remove the BugInCF construction for this case.
933-974:⚠️ Potential issue | 🟠 MajorModel
@CreatesCollectionObligationonly on the normal edge, and enforce it on all later exits.This helper is executed per successor edge, so it currently creates obligations on exceptional successors too, and
CollectionObligation.fromTree(...)leaveswhenToEnforceatNORMAL_RETURN. That can fabricate obligations in catch paths and under-enforce them after a successful insert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 933 - 974, The code currently creates collection obligations on every successor edge; change addObligationsForCreatesCollectionObligationAnno so it only creates obligations on the normal-successor (i.e., guard early with a check for the normal/ordinary flow edge for the MethodInvocationNode) and when constructing the obligation use the variant of CollectionObligation that sets enforcement to "later exits" (or pass the appropriate whenToEnforce value instead of leaving it at NORMAL_RETURN) so the obligation is enforced on all subsequent exits; update the CollectionObligation.fromTree call to the constructor/factory that accepts the whenToEnforce flag (or set the flag after creation) and only call it when the node is on the normal path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`:
- Around line 126-129: The code calls
disposalLoopCalledMethods.containsAll(requiredElementMethods) without checking
for null from atypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
causing an NPE; update the logic in CollectionOwnershipTransfer to treat a null
requiredElementMethods as “no obligations” by first checking if
requiredElementMethods is null and, if so, skip creating any
CollectionObligation(s) (i.e., do not call containsAll or proceed with
obligation creation); specifically guard the requiredElementMethods variable
returned by getMustCallValuesOfResourceCollectionComponent(collectionExpression)
before uses like disposalLoopCalledMethods.containsAll(...) and short-circuit
the obligation-creation path when it is null.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java`:
- Around line 119-145: The current getNameFromExpressionTree collapses
member-selected fields to only the terminal Name (mst.getIdentifier()), losing
distinction between different receiver expressions; change it to return a unique
identifier for the whole access instead of just the simple name—e.g., return the
Element obtained via TreeUtils.elementFromUse(mst) (or otherwise return a
representation that includes the receiver, not just mst.getIdentifier()) so
callers like IndexedForDisposalLoopMatcher can distinguish this.resources vs
other.resources; update the method signature/return type accordingly and replace
the branch that returns mst.getIdentifier() with returning the element (or a
qualified expression representation) and adjust callers to use the
element/qualified representation.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 143-144: The comparison in EnhancedForDisposalLoopResolver using
"iteratorVariableDeclaration.getName() == loopVariable.getName()" can give false
negatives; change it to use value equality (e.g.,
iteratorVariableDeclaration.getName().equals(loopVariable.getName()) or
Objects.equals(...)) when computing isAssignmentOfLoopVariable so Name instances
are compared correctly and null-safely; update the expression where
isAssignmentOfLoopVariable is set to use equals/Objects.equals on getName()
rather than ==.
- Around line 127-137: The code casts blocks to SingleSuccessorBlock without
checking types and can loop forever; update the logic around
methodInvocationNode.getBlock() and the successor traversal to first verify
instanceof SingleSuccessorBlock before casting, handle the case when
getSuccessor() returns null or a non-SingleSuccessorBlock (break/return or skip)
and guard against empty getNodes() to avoid infinite while loops; specifically
add instanceof checks before the casts to SingleSuccessorBlock and ensure the
loop that advances singleSuccessorBlock stops when successor is null or not the
expected type, and handle/return appropriately in the surrounding method
(EnhancedForDisposalLoopResolver) to preserve correctness of loopVariableNode
discovery.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 65-68: The matcher currently calls
TreeUtils.withoutParens(tree.getCondition()) without checking for a null
condition (ForLoopTree.getCondition() can be null for `for(;;)`), causing a NPE;
in IndexedForDisposalLoopMatcher, guard against a null condition by checking
tree.getCondition() (or the local before calling TreeUtils.withoutParens) and
immediately reject/return from the matcher when it's null so the code doesn't
attempt to strip parentheses from a null value.
- Around line 184-233: The scanner currently descends into nested scopes
(lambdas and local/anonymous classes) and can misattribute actions inside those
scopes to the outer loop; update the anonymous TreeScanner (scanner) to skip
nested scopes by overriding visitLambdaExpression(LambdaExpressionTree),
visitClass(ClassTree) and visitNewClass(NewClassTree) (or other nested-scope
nodes you see) to return null without calling super, so the scanner does not
traverse into lambda bodies or local/anonymous class bodies and only inspects
statements directly in the loop body; reference the existing scanner variable
and the methods visitUnary, visitAssignment, visitMethodInvocation to keep their
logic intact.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 659-665: In markWriteIfTargetsHeaderOrCollection, replace
reference-equality checks on javax.lang.model.element.Name with value-equality:
instead of comparing assigned == headerVar and assigned == collectionVarName,
call assigned.equals(headerVar) and (collectionVarName != null &&
assigned.equals(collectionVarName)); ensure you null-check assigned before
calling equals (or flip to headerVar.equals(assigned) if headerVar is non-null)
so the method correctly detects matching Names.
- Around line 751-752: Replace the reference-equality check in
WhileDisposalLoopMatcher where you compare Name objects: instead of "recv !=
null && recv == headerVar" use a value-equality check (e.g., "recv != null &&
recv.equals(headerVar)" or preferably "Objects.equals(recv, headerVar)") so Name
comparisons use equals() rather than ==; locate this via the
getNameFromExpressionTree(ms.getExpression()) call and the headerVar reference.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 451-457: getStoreAfterConditionalBlock currently asserts
transferInput non-null which fails when flowResult.getInput(block) is absent for
unreachable ConditionalBlock; replace the assert with an explicit null check:
call TransferInput<AccumulationValue,AccumulationStore> transferInput =
flowResult.getInput(block); if transferInput == null then return a safe empty
AccumulationStore (e.g. AccumulationStore.empty() or new AccumulationStore() —
create a static empty factory if needed); otherwise return
transferInput.getThenStore() or getElseStore() as before. Ensure you reference
getStoreAfterConditionalBlock, flowResult.getInput, TransferInput, and
AccumulationStore when making the change.
In
`@checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.java`:
- Line 15: The test sets testDir to "resourceleak" but getTestDirs enumerates
"resourceleak-collections", causing mismatched test directories; update the
ResourceLeakCollectionsTest constructor (or the testDir variable) so it uses the
same directory string as getTestDirs (e.g., change testDir to
"resourceleak-collections"), or alternatively change getTestDirs to use the
constructor's testDir value—ensure the constructor, the testDir field, and the
getTestDirs method all reference the identical directory name.
In
`@dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java`:
- Around line 13-86: Add a toString() override on IteratedCollectionElement that
returns a concise, informative representation for debugging (e.g., include the
tree and/or node); implement it as a public `@Override` String toString() method
in the IteratedCollectionElement class that formats the output like
"IteratedCollectionElement(" + tree + ")" (or include node if desired) so
instances used as store keys or in logs are readable.
---
Outside diff comments:
In
`@dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java`:
- Around line 824-834: The synthetic-name counter `uid` is currently static,
causing cross-analysis and order-dependent names; change its declaration in
class CFGTranslationPhaseOne from "protected static long uid = 0;" to an
instance field (e.g., "protected long uid = 0;") so that uniqueName(String
prefix) uses an instance-scoped counter and each translation gets its own
sequence; update any code that assumed a static field to use the instance field
if applicable.
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java`:
- Line 41: In the Javadoc for class CollectionOwnershipAnalysis update the
comment that currently reads "exact{`@link` Throwable}" to insert a space so it
becomes "exact {`@link` Throwable}"; locate the Javadoc in
CollectionOwnershipAnalysis.java and adjust the spacing between "exact" and the
{`@link` Throwable} tag to fix the formatting.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java`:
- Around line 1023-1025: The code in CollectionOwnershipAnnotatedTypeFactory
compares Name objects using == (params.get(i).getSimpleName() ==
elt.getSimpleName()), which can miss matches; change the comparison to use
equals (e.g., params.get(i).getSimpleName().equals(elt.getSimpleName()) or
Objects.equals(...)) so the correct parameter slot from paramTypes is located
and the declaration annotation is propagated via
type.replaceAnnotation(paramTypes.get(i).getAnnotationInHierarchy(TOP)).
- Around line 691-693: Guard the call to mcAtf.getAnnotatedType(tree) inside
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent
by catching the same BugInCF/unsupported-tree condition used in
isResourceCollection(Tree) and return null when the tree shape is unsupported;
update getMustCallValuesOfResourceCollectionComponent to return null instead of
letting the exception bubble. Then update
MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn to treat a
null return from
CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
as “no obligations” (i.e., skip creating CollectionObligation instances for
OwningCollection element types when the annotated type cannot be determined) so
analysis does not crash on non-resource element types.
- Around line 488-516: Both isResourceCollectionField(Element elt) and
isOwningCollectionParameter(Element elt) assume elt is non-null and can NPE;
mirror the null guard used elsewhere by adding an initial null check in each
method (e.g., if (elt == null) return false;) before any elt dereferences. This
ensures calls to isResourceCollectionField and isOwningCollectionParameter
(which call elt.getKind(), elt.asType(), getAnnotatedType(elt), etc.) safely
return false for null elements without changing downstream logic that uses
getAnnotatedType, getCoType, or compares to
CollectionOwnershipType.OwningCollection.
- Around line 609-620: The code in CollectionOwnershipAnnotatedTypeFactory uses
Element.getAnnotation(...) on insertedElement (from insertedArgumentTree) which
ignores stub-file annotations; replace those checks with the stub-aware queries
rlAtf.hasNotOwning(insertedElement) and rlAtf.hasOwning(insertedElement) so the
logic that returns CollectionMutatorArgumentKind.DEFINITELY_NON_OWNING or
MAY_BE_OWNING remains the same but respects stub declarations; update both the
initial NotOwning check and the parameter Owning check to call
rlAtf.hasNotOwning(...) / rlAtf.hasOwning(...) on insertedElement instead of
getAnnotation(...).
- Around line 657-678: getMustCallValuesOfResourceCollectionComponent currently
only extracts a single component type argument, so Map<K,V> (which has two type
arguments) falls through and value obligations are ignored; update the
extraction logic in getMustCallValuesOfResourceCollectionComponent to handle
both single-arg collection types and two-arg map-like types: if atm is an
AnnotatedDeclaredType with one type argument use that, and if it has two type
arguments (i.e., map-like) select the second argument as the component; then
pass that component to ResourceLeakUtils.getMcValues(componentType, mcAtf) as
before (refer to getMustCallValuesOfResourceCollectionComponent,
ResourceLeakUtils.isCollection, AnnotatedDeclaredType, and
ResourceLeakUtils.getMcValues).
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`:
- Line 313: The expression checking for an empty type-arguments list uses size()
== 0 which is less clear; replace the check in CollectionOwnershipTransfer where
isDiamond is assigned (currently using node.getTree().getTypeArguments().size()
== 0) with the clearer isEmpty() call on the type arguments collection (e.g.,
node.getTree().getTypeArguments().isEmpty()) so isDiamond uses isEmpty() instead
of size() == 0.
- Around line 253-282: Guard against a null Element returned by
TreeUtils.elementFromTree(arg.getTree()) before calling getKind(): in
CollectionOwnershipTransfer (around the block using argElem, transferOwnership,
and the calls to checker.reportError and replaceInStores) add a null-check so
that if argElem is null it is treated as "not a field" (i.e., call
replaceInStores(res, argJE, atypeFactory.NOTOWNINGCOLLECTION)) and only call
argElem.getKind().isField() when argElem != null; update the conditional that
currently does if (argElem.getKind().isField()) ... else ... to first test
argElem != null.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.java`:
- Around line 88-92: When getCoTypeAtTree(receiverTree, storeBefore) returns
null, don't return immediately; instead obtain the declared/annotated receiver
type (via CollectionOwnershipAnnotatedTypeFactory.CollectionOwnershipType from
the annotated type factory using receiverTree or getAnnotatedType(receiverTree))
and assign it to receiverType so the subsequent `@CreatesCollectionObligation`
check runs even when the flow store lacks an entry; keep the existing null
guards and only return if both flow and declared lookups produce null.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 143-149: The code in IndexedForDisposalLoopMatcher uses reference
equality (== / !=) for com.sun.source.util.Name comparisons which is unsafe;
replace these with value equality using .equals(...) or Objects.equals(...) for
initVarName vs ((IdentifierTree) condition.getLeftOperand()).getName() and
initVarName vs ((IdentifierTree) inc.getExpression()).getName(), and update the
other similar checks in this class (the other Name comparisons around the later
matching logic) to use .equals/Objects.equals so name comparisons are by value
rather than reference.
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 177-221: The current logic only enters the loop that calls
replaceCollectionTypeVarsWithBottomIfTop when
ResourceLeakUtils.isCollection(adt.getUnderlyingType()), so nested declared type
arguments inside non-collection wrappers (e.g., Optional<List<?>>) are never
traversed; update the traversal so that for any AnnotatedDeclaredType you always
recurse into its type arguments and call
replaceCollectionTypeVarsWithBottomIfTop on each declared type argument (use the
existing method replaceCollectionTypeVarsWithBottomIfTop and the same handling
used for typeArg.getKind() == TypeKind.DECLARED), not just when the outer adt is
a collection, ensuring inner collections/iterators have their element types
reset from `@MustCallUnknown` to `@MustCallBottom` when appropriate.
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 3547-3560: If any back-edge summary (calledMethodsAfterBlock
returned from analyzeTypeOfCollectionElement for the block within the loop) is
null, the entire loop proof must be invalidated instead of preserving prior
intersections; update the logic in the isLastBlockOfBody branch so that when
calledMethodsAfterBlock == null you set calledMethodsInLoop = null (and avoid
retaining previous values), using the existing variables calledMethodsInLoop,
calledMethodsAfterBlock, currentBlock, disposalLoop, obligations, and
loopUpdateBlock to locate the code and enforce that a single null summary clears
the accumulated calledMethodsInLoop.
- Around line 2048-2053: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) in
MustCallConsistencyAnalyzer is assumed non-empty before calling .get(0), which
can cause NPEs or IndexOutOfBounds; update both places that call
checker.reportError(...) (the one using that list and the similar block around
the other reportError) to first check for null or empty and extract a safe
method-name (use the existing "Unknown" fallback used elsewhere) before passing
it into checker.reportError; i.e., compute a safeMethodName = list != null &&
!list.isEmpty() ? list.get(0) : "Unknown" (or reuse the module's fallback
helper) and use that value in the reportError invocation.
- Around line 3713-3715: The code calls getCalledMethods(cmValOfIce) without
checking that store.getValue(ice) returned a non-null AccumulationValue; guard
against null by assigning AccumulationValue cmValOfIce = store.getValue(ice) and
then checking cmValOfIce != null before invoking getCalledMethods(cmValOfIce)
(or treat a null as "no summary" and skip the back-edge handling). Update the
branch that uses cmValOfIce/calledMethods (referenced symbols: store.getValue,
ice, cmValOfIce, getCalledMethods) to return/continue when cmValOfIce is null so
getCalledMethods is never called on null.
- Around line 965-974: The code currently always calls
consumeInsertedArgumentObligationIfSingleElementInsert(...), even when
checkEnclosingMethodIsCreatesMustCallFor(...) reports a failure; change the
control flow so the obligation is only consumed when the enclosing-method check
succeeds. Modify checkEnclosingMethodIsCreatesMustCallFor (or add a small
wrapper) to return a boolean success flag (e.g., true when no error reported) or
provide a way to detect failure, then call
consumeInsertedArgumentObligationIfSingleElementInsert(obligations, node) only
if that flag is true; update references in MustCallConsistencyAnalyzer around
receiverIsOwningField / receiverNode / enclosingMethodTree accordingly.
- Around line 899-914: In addObligationsForOwningCollectionReturn, stop throwing
a BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()) returns
null; instead treat null as “no obligations” and skip creating
CollectionObligation(s) for that case. Locate the check that assigns
mustCallValues from getMustCallValuesOfResourceCollectionComponent, and if
mustCallValues is null simply return (or continue) without creating
ResourceAlias tmpVarAsResourceAlias or adding obligations to the obligations
set; preserve existing behavior for non-null mustCallValues. Ensure references
include addObligationsForOwningCollectionReturn, cmAtf.getTempVarForNode,
coAtf.getStoreAfter / getCoType, and
coAtf.getMustCallValuesOfResourceCollectionComponent.
- Around line 536-607: CollectionObligation breaks equals/hashCode because
mustCallMethod is mutable and not included in hashCode; make mustCallMethod
final (change declaration to final and update constructors to set it) and
override hashCode in CollectionObligation to combine super.hashCode() with
mustCallMethod.hashCode() (e.g., 31*super.hashCode()+mustCallMethod.hashCode())
so equals and hashCode remain consistent for CollectionObligation instances;
keep getReplacement() behavior as-is.
- Around line 2100-2105: In MustCallConsistencyAnalyzer, the branch that checks
"lhsCoType == CollectionOwnershipType.OwningCollectionBottom" should not throw a
BugInCF because OwningCollectionBottom represents a nullable field being
overwritten; replace the throw with code that treats this as a safe assignment
(e.g., a no-op or appropriate continuation of analysis) and update the comment
to note that OwningCollectionBottom means the field may be null and can be
safely assigned; refer to the lhsCoType check in MustCallConsistencyAnalyzer and
remove the BugInCF construction for this case.
- Around line 933-974: The code currently creates collection obligations on
every successor edge; change addObligationsForCreatesCollectionObligationAnno so
it only creates obligations on the normal-successor (i.e., guard early with a
check for the normal/ordinary flow edge for the MethodInvocationNode) and when
constructing the obligation use the variant of CollectionObligation that sets
enforcement to "later exits" (or pass the appropriate whenToEnforce value
instead of leaving it at NORMAL_RETURN) so the obligation is enforced on all
subsequent exits; update the CollectionObligation.fromTree call to the
constructor/factory that accepts the whenToEnforce flag (or set the flag after
creation) and only call it when the node is on the normal path.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 183-191: The postCFGConstruction method only calls
ResourceLeakUtils.getCollectionOwnershipAnnotatedTypeFactory(this).discoverDisposalLoops(cfg)
when ast.getKind() == UnderlyingAST.Kind.METHOD, which skips lambda CFGs; update
the guard so disposal-loop discovery also runs for lambda CFGs (e.g.,
ast.getKind() == UnderlyingAST.Kind.METHOD || ast.getKind() ==
UnderlyingAST.Kind.LAMBDA) or remove the kind check entirely so
discoverDisposalLoops(cfg) runs for lambda CFGs as well, ensuring
RLCCalledMethodsTransfer.initialStore(...) sees disposal loops inside lambdas.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 79-80: The map iteratedCollectionElements can retain stale facts
across mutations; update the mutation/invalidation paths to evict affected
entries by clearing or removing matching keys from iteratedCollectionElements:
in updateForMethodCall(...) and the removeConflicting(...) overloads
(removeConflicting(FieldAccess,...), removeConflicting(ArrayAccess,...),
removeConflicting(LocalVariable,...)) identify which IteratedCollectionElement
keys reference the mutated target and remove them (or clear the whole map when a
conservative global mutation occurs) so iteratedCollectionElements cannot leak
outdated facts after side-effecting operations.
- Around line 841-848: The lookup in getIteratedCollectionElement currently
returns an IteratedCollectionElement if any of four comparisons match; change
the logic to require both node and tree to match (mirroring
IteratedCollectionElement.equals). In method getIteratedCollectionElement,
iterate iteratedCollectionElements.keySet() and replace the compound condition
(ice.tree == tree || ice.node == node || ice.tree.equals(tree) ||
ice.node.equals(node)) with a check that both components match (e.g., (ice.tree
== tree || (ice.tree != null && ice.tree.equals(tree))) && (ice.node == node ||
(ice.node != null && ice.node.equals(node)))), then return ice only when both
match.
In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`:
- Around line 1199-1214: The getRegularStore method dereferences the
TransferInput from flowResult.getInput(block) or analysis.getInput(block)
without null-check, causing NPE for unreachable/unanalyzed blocks; update
getRegularStore to check whether the selected TransferInput is null and return
null (matching neighboring store accessors' nullable behavior) when absent
instead of calling input.getRegularStore(), using the same selection logic
around analysis.isRunning(), and reference the symbols getRegularStore,
flowResult.getInput, analysis.getInput, and TransferInput in your change.
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 1696-1699: getIdxForGetCall currently assumes the
MethodInvocationTree mit has at least one argument and calls
mit.getArguments().get(0), which can throw IndexOutOfBoundsException for
zero-arg get() calls; update getIdxForGetCall to first check the argument list
(e.g., mit.getArguments().isEmpty() or size() > 0) before accessing index 0 and
return null when there are no arguments so zero-arg calls are safely handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49f0d195-b2db-4da3-8ef3-7eceb187f82f
📒 Files selected for processing (20)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.javachecker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.javachecker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.javachecker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipVisitor.javachecker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoop.javachecker/src/main/java/org/checkerframework/checker/collectionownership/DisposalLoopScanner.javachecker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.javachecker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.javachecker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.javachecker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsTransfer.javachecker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.javadataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javaframework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java
| List<String> requiredElementMethods = | ||
| atypeFactory.getMustCallValuesOfResourceCollectionComponent(collectionExpression); | ||
| if (disposalLoopCalledMethods != null | ||
| && disposalLoopCalledMethods.containsAll(requiredElementMethods)) { |
There was a problem hiding this comment.
Guard against null requiredElementMethods before calling containsAll.
getMustCallValuesOfResourceCollectionComponent can return null for collections whose element type has no must-call obligations. Passing null to containsAll on line 129 will throw NullPointerException.
Proposed fix
if (collectionCoType == CollectionOwnershipType.OwningCollection) {
List<String> requiredElementMethods =
atypeFactory.getMustCallValuesOfResourceCollectionComponent(collectionExpression);
- if (disposalLoopCalledMethods != null
+ if (requiredElementMethods != null
+ && disposalLoopCalledMethods != null
&& disposalLoopCalledMethods.containsAll(requiredElementMethods)) {Based on learnings: "treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as 'no obligations' and skip creating CollectionObligation(s); do not throw."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java`
around lines 126 - 129, The code calls
disposalLoopCalledMethods.containsAll(requiredElementMethods) without checking
for null from atypeFactory.getMustCallValuesOfResourceCollectionComponent(...)
causing an NPE; update the logic in CollectionOwnershipTransfer to treat a null
requiredElementMethods as “no obligations” by first checking if
requiredElementMethods is null and, if so, skip creating any
CollectionObligation(s) (i.e., do not call containsAll or proceed with
obligation creation); specifically guard the requiredElementMethods variable
returned by getMustCallValuesOfResourceCollectionComponent(collectionExpression)
before uses like disposalLoopCalledMethods.containsAll(...) and short-circuit
the obligation-creation path when it is null.
| static Name getNameFromExpressionTree(ExpressionTree expr) { | ||
| if (expr == null) { | ||
| return null; | ||
| } | ||
| switch (expr.getKind()) { | ||
| case IDENTIFIER -> { | ||
| return ((com.sun.source.tree.IdentifierTree) expr).getName(); | ||
| } | ||
| case MEMBER_SELECT -> { | ||
| MemberSelectTree mst = (MemberSelectTree) expr; | ||
| Element elt = TreeUtils.elementFromUse(mst); | ||
| if (elt.getKind() == ElementKind.FIELD) { | ||
| return mst.getIdentifier(); | ||
| } else if (elt.getKind() == ElementKind.METHOD) { | ||
| return getNameFromExpressionTree(mst.getExpression()); | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
| case METHOD_INVOCATION -> { | ||
| return getNameFromExpressionTree(((MethodInvocationTree) expr).getMethodSelect()); | ||
| } | ||
| default -> { | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't collapse member-selected collections to a bare Name.
For field/member accesses this returns only the terminal identifier. That makes this.resources, other.resources, and any other same-named field indistinguishable to callers like IndexedForDisposalLoopMatcher, which can then bind a loop header/body check to the wrong collection instance. Compare full expressions or elements here, not just the simple name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.java`
around lines 119 - 145, The current getNameFromExpressionTree collapses
member-selected fields to only the terminal Name (mst.getIdentifier()), losing
distinction between different receiver expressions; change it to return a unique
identifier for the whole access instead of just the simple name—e.g., return the
Element obtained via TreeUtils.elementFromUse(mst) (or otherwise return a
representation that includes the receiver, not just mst.getIdentifier()) so
callers like IndexedForDisposalLoopMatcher can distinguish this.resources vs
other.resources; update the method signature/return type accordingly and replace
the branch that returns mst.getIdentifier() with returning the element (or a
qualified expression representation) and adjust callers to use the
element/qualified representation.
| private void markWriteIfTargetsHeaderOrCollection(ExpressionTree lhs) { | ||
| Name assigned = CollectionOwnershipUtils.getNameFromExpressionTree(lhs); | ||
| if (assigned != null) { | ||
| if (assigned == headerVar) illegal.set(true); | ||
| if (collectionVarName != null && assigned == collectionVarName) illegal.set(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use equals() for Name comparisons instead of ==.
Lines 662-663 use reference equality (==) for javax.lang.model.element.Name comparisons. Name instances may not be interned across different contexts, leading to false negatives when detecting writes to header or collection variables.
Proposed fix
private void markWriteIfTargetsHeaderOrCollection(ExpressionTree lhs) {
Name assigned = CollectionOwnershipUtils.getNameFromExpressionTree(lhs);
if (assigned != null) {
- if (assigned == headerVar) illegal.set(true);
- if (collectionVarName != null && assigned == collectionVarName) illegal.set(true);
+ if (assigned.equals(headerVar)) illegal.set(true);
+ if (collectionVarName != null && assigned.equals(collectionVarName)) illegal.set(true);
}
}Based on learnings: prefer using Name.equals(...) for Name comparisons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 659 - 665, In markWriteIfTargetsHeaderOrCollection, replace
reference-equality checks on javax.lang.model.element.Name with value-equality:
instead of comparing assigned == headerVar and assigned == collectionVarName,
call assigned.equals(headerVar) and (collectionVarName != null &&
assigned.equals(collectionVarName)); ensure you null-check assigned before
calling equals (or flip to headerVar.equals(assigned) if headerVar is non-null)
so the method correctly detects matching Names.
| Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression()); | ||
| return recv != null && recv == headerVar; |
There was a problem hiding this comment.
Use equals() for Name comparison instead of ==.
Line 752 uses reference equality for Name comparison.
Proposed fix
Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression());
- return recv != null && recv == headerVar;
+ return recv != null && recv.equals(headerVar);📝 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.
| Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression()); | |
| return recv != null && recv == headerVar; | |
| Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression()); | |
| return recv != null && recv.equals(headerVar); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 751 - 752, Replace the reference-equality check in
WhileDisposalLoopMatcher where you compare Name objects: instead of "recv !=
null && recv == headerVar" use a value-equality check (e.g., "recv != null &&
recv.equals(headerVar)" or preferably "Objects.equals(recv, headerVar)") so Name
comparisons use equals() rather than ==; locate this via the
getNameFromExpressionTree(ms.getExpression()) call and the headerVar reference.
| public AccumulationStore getStoreAfterConditionalBlock(ConditionalBlock block, boolean then) { | ||
| TransferInput<AccumulationValue, AccumulationStore> transferInput = flowResult.getInput(block); | ||
| assert transferInput != null : "@AssumeAssertion(nullness): transferInput should be non-null"; | ||
| if (then) { | ||
| return transferInput.getThenStore(); | ||
| } | ||
| return transferInput.getElseStore(); |
There was a problem hiding this comment.
Don't rely on assert for missing conditional-block input.
flowResult.getInput(block) can be absent for unreachable blocks. With assertions disabled this falls through to transferInput.getThenStore() / getElseStore() and crashes with an NPE.
Suggested fix
- public AccumulationStore getStoreAfterConditionalBlock(ConditionalBlock block, boolean then) {
+ public `@Nullable` AccumulationStore getStoreAfterConditionalBlock(
+ ConditionalBlock block, boolean then) {
TransferInput<AccumulationValue, AccumulationStore> transferInput = flowResult.getInput(block);
- assert transferInput != null : "@AssumeAssertion(nullness): transferInput should be non-null";
+ if (transferInput == null) {
+ return null;
+ }
if (then) {
return transferInput.getThenStore();
}
return transferInput.getElseStore();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`
around lines 451 - 457, getStoreAfterConditionalBlock currently asserts
transferInput non-null which fails when flowResult.getInput(block) is absent for
unreachable ConditionalBlock; replace the assert with an explicit null check:
call TransferInput<AccumulationValue,AccumulationStore> transferInput =
flowResult.getInput(block); if transferInput == null then return a safe empty
AccumulationStore (e.g. AccumulationStore.empty() or new AccumulationStore() —
create a static empty factory if needed); otherwise return
transferInput.getThenStore() or getElseStore() as before. Ensure you reference
getStoreAfterConditionalBlock, flowResult.getInput, TransferInput, and
AccumulationStore when making the change.
| super( | ||
| testFiles, | ||
| ResourceLeakChecker.class, | ||
| "resourceleak", |
There was a problem hiding this comment.
Use the same test directory in constructor and getTestDirs.
Line 15 sets testDir to "resourceleak", but Line 23 discovers files under "resourceleak-collections". This can cause the harness to resolve configuration/diagnostics relative to a different directory than the one being enumerated.
Proposed fix
- "resourceleak",
+ "resourceleak-collections",📝 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.
| "resourceleak", | |
| "resourceleak-collections", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollectionsTest.java`
at line 15, The test sets testDir to "resourceleak" but getTestDirs enumerates
"resourceleak-collections", causing mismatched test directories; update the
ResourceLeakCollectionsTest constructor (or the testDir variable) so it uses the
same directory string as getTestDirs (e.g., change testDir to
"resourceleak-collections"), or alternatively change getTestDirs to use the
constructor's testDir value—ensure the constructor, the testDir field, and the
getTestDirs method all reference the identical directory name.
| public class IteratedCollectionElement extends JavaExpression { | ||
| /** The CFG node for this collection element. */ | ||
| public final Node node; | ||
|
|
||
| /** The AST node for this collection element. */ | ||
| public final Tree tree; | ||
|
|
||
| /** | ||
| * Creates a new IteratedCollectionElement. | ||
| * | ||
| * @param node CFG node | ||
| * @param tree an AST tree | ||
| */ | ||
| public IteratedCollectionElement(Node node, Tree tree) { | ||
| super(node.getType()); | ||
| this.node = node; | ||
| this.tree = tree; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(@Nullable Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (!(obj instanceof IteratedCollectionElement other)) { | ||
| return false; | ||
| } | ||
| return this.tree.equals(other.tree) && this.node.equals(other.node); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(tree, node); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") // generic cast | ||
| @Override | ||
| public <T extends JavaExpression> @Nullable T containedOfClass(Class<T> clazz) { | ||
| return getClass() == clazz ? (T) this : null; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isDeterministic(AnnotationProvider provider) { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean syntacticEquals(JavaExpression je) { | ||
| if (!(je instanceof IteratedCollectionElement other)) { | ||
| return false; | ||
| } | ||
| return this.equals(other); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean containsSyntacticEqualJavaExpression(JavaExpression other) { | ||
| return syntacticEquals(other); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isAssignableByOtherCode() { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isModifiableByOtherCode() { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public <R, P> R accept(JavaExpressionVisitor<R, P> visitor, P p) { | ||
| return visitor.visitIteratedCollectionElement(this, p); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a toString() method for debugging.
The class lacks a toString() override. Since IteratedCollectionElement is used as a store key and may appear in error messages or debug output, a meaningful string representation would help with troubleshooting.
Suggested implementation
`@Override`
public String toString() {
return "IteratedCollectionElement(" + tree + ")";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java`
around lines 13 - 86, Add a toString() override on IteratedCollectionElement
that returns a concise, informative representation for debugging (e.g., include
the tree and/or node); implement it as a public `@Override` String toString()
method in the IteratedCollectionElement class that formats the output like
"IteratedCollectionElement(" + tree + ")" (or include node if desired) so
instances used as store keys or in logs are readable.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java (1)
655-660:⚠️ Potential issue | 🟡 MinorUse value equality for
Namecomparisons.Line 658, Line 659, and Line 748 compare
Nameobjects with==, which can mis-match equivalent names and break loop matching/invalid-write detection.Proposed fix
private void markWriteIfTargetsHeaderOrCollection(ExpressionTree lhs) { Name assigned = CollectionOwnershipUtils.getNameFromExpressionTree(lhs); if (assigned != null) { - if (assigned == headerVar) illegal.set(true); - if (collectionVarName != null && assigned == collectionVarName) illegal.set(true); + if (assigned.equals(headerVar)) illegal.set(true); + if (collectionVarName != null && assigned.equals(collectionVarName)) illegal.set(true); } } @@ Name recv = CollectionOwnershipUtils.getNameFromExpressionTree(ms.getExpression()); - return recv != null && recv == headerVar; + return recv != null && recv.equals(headerVar); }#!/bin/bash # Verify there are no remaining reference-equality checks on Name for these comparisons. rg -n --type=java -C2 '\b(assigned|recv)\s*==\s*(headerVar|collectionVarName)\b' \ checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java # Expected: no matchesBased on learnings: prefer using
Name.equals(...)orObjects.equals(...)forNamecomparisons instead of==/!=.Also applies to: 747-748
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java` around lines 655 - 660, Replace reference-equality checks on Name with value-equality: in WhileDisposalLoopMatcher (method markWriteIfTargetsHeaderOrCollection) change comparisons like assigned == headerVar and assigned == collectionVarName to use Name.equals(...) or Objects.equals(assigned, headerVar) / Objects.equals(assigned, collectionVarName); likewise update the recv == headerVar / recv == collectionVarName checks around the recv handling (lines referenced near 747-748) to use equals/Objects.equals so equivalent Name instances compare correctly.checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java (2)
64-65:⚠️ Potential issue | 🟠 MajorStill guard the loop condition before stripping parens.
ForLoopTree.getCondition()can benull, soTreeUtils.withoutParens(...)will still throw onfor (;;)before the matcher can reject the loop.Suggested fix
- ExpressionTree condition = TreeUtils.withoutParens(tree.getCondition()); + ExpressionTree rawCondition = tree.getCondition(); + if (rawCondition == null) { + return null; + } + ExpressionTree condition = TreeUtils.withoutParens(rawCondition);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java` around lines 64 - 65, The code calls TreeUtils.withoutParens(tree.getCondition()) without guarding against tree.getCondition() being null, which will throw for an empty-condition for-loop; modify the logic in IndexedForDisposalLoopMatcher (the method handling the ForLoopTree) to first check tree.getCondition() != null (and only then call TreeUtils.withoutParens), returning/rejecting the loop early if the condition is null so the matcher doesn't throw.
181-226:⚠️ Potential issue | 🟠 MajorSkip nested scopes in the body scan.
This scanner still descends into lambdas and local/anonymous classes, so a write or
collection.get(i)inside an unrelated nested scope can incorrectly certify or invalidate the outer loop.Suggested fix
import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.CompoundAssignmentTree; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.ForLoopTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; import com.sun.source.tree.UnaryTree; import com.sun.source.tree.VariableTree; @@ new TreeScanner<Void, Void>() { + `@Override` + public Void visitLambdaExpression(LambdaExpressionTree tree, Void p) { + return null; + } + + `@Override` + public Void visitClass(ClassTree tree, Void p) { + return null; + } + + `@Override` + public Void visitNewClass(NewClassTree tree, Void p) { + return null; + } + `@Override` public Void visitUnary(UnaryTree tree, Void p) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java` around lines 181 - 226, The TreeScanner currently descends into nested scopes (lambdas and local/anonymous classes) and thus can see writes or collection.get(i) in unrelated scopes; fix by overriding the scope-entry visit methods on the anonymous TreeScanner (the scanner variable in IndexedForDisposalLoopMatcher) to stop traversal into nested scopes — add overrides for visitLambdaExpression(LambdaExpressionTree), visitNewClass(NewClassTree) and visitClass(ClassTree) that simply return null (do not call super) so the scanner skips descending into those nested scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`:
- Around line 165-169: The current backward scan accepts the first hasNext() it
finds (in the block where node instanceof MethodInvocationNode) without ensuring
that the found MethodInvocationTree actually belongs to the target enhanced-for
condition `tree`; update the check so after obtaining methodInvocationTree and
confirming TreeUtils.isHasNextCall(methodInvocationTree) you also verify the
found methodInvocationTree is the same call (or is contained in the same parent
tree) as the target `tree` before setting isLoopCondition/returning — i.e.,
compare methodInvocationTree to `tree` (or its relevant parent) to ensure the
hasNext() belongs to the target enhanced-for loop.
- Around line 172-184: The code currently throws BugInCF when the CFG shape
doesn't match (in the checks on
blockContainingLoopCondition.getSuccessors().size() and the instanceof check for
ConditionalBlock), but we should instead reject the candidate by returning null;
change both throw new BugInCF(...) occurrences in
EnhancedForDisposalLoopResolver (the checks using blockContainingLoopCondition,
maybeConditionalBlock and the instanceof ConditionalBlock conditionalBlock) to
return null so the resolver gracefully skips non-matching CFG shapes rather than
crashing.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 78-95: The code currently casts loopConditionBlock to
SingleSuccessorBlock and its successor to ConditionalBlock without checks
causing potential ClassCastException; update IndexedForDisposalLoopMatcher to
verify types (use instanceof) before casting: ensure loopConditionBlock is a
SingleSuccessorBlock, get its successor and ensure that successor is a
ConditionalBlock, and only then obtain loopBodyEntryBlock and construct the
DisposalLoop; if any instanceof checks fail (or
loopUpdateBlock/nodeForCollectionElt are null) return null—mirror the pattern
used in WhileDisposalLoopMatcher.findConditionalSuccessor() for safe CFG
handling.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 826-829: In WhileDisposalLoopMatcher, the code selects the wrong
block for loop updates by storing backEdge.targetBlock (the loop header) into
bestLoopUpdateBlock; change the assignment to use the back-edge source
(backEdge.sourceBlock) so bestLoopUpdateBlock points to the loop tail/update
block, and ensure the variable types and any subsequent uses of
bestLoopUpdateBlock still match this source-block semantics.
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`:
- Around line 64-65: The code calls TreeUtils.withoutParens(tree.getCondition())
without guarding against tree.getCondition() being null, which will throw for an
empty-condition for-loop; modify the logic in IndexedForDisposalLoopMatcher (the
method handling the ForLoopTree) to first check tree.getCondition() != null (and
only then call TreeUtils.withoutParens), returning/rejecting the loop early if
the condition is null so the matcher doesn't throw.
- Around line 181-226: The TreeScanner currently descends into nested scopes
(lambdas and local/anonymous classes) and thus can see writes or
collection.get(i) in unrelated scopes; fix by overriding the scope-entry visit
methods on the anonymous TreeScanner (the scanner variable in
IndexedForDisposalLoopMatcher) to stop traversal into nested scopes — add
overrides for visitLambdaExpression(LambdaExpressionTree),
visitNewClass(NewClassTree) and visitClass(ClassTree) that simply return null
(do not call super) so the scanner skips descending into those nested scopes.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`:
- Around line 655-660: Replace reference-equality checks on Name with
value-equality: in WhileDisposalLoopMatcher (method
markWriteIfTargetsHeaderOrCollection) change comparisons like assigned ==
headerVar and assigned == collectionVarName to use Name.equals(...) or
Objects.equals(assigned, headerVar) / Objects.equals(assigned,
collectionVarName); likewise update the recv == headerVar / recv ==
collectionVarName checks around the recv handling (lines referenced near
747-748) to use equals/Objects.equals so equivalent Name instances compare
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb66b8c1-1600-420b-887e-94d0e962bf1b
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.javachecker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.javachecker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java
| if (node instanceof MethodInvocationNode) { | ||
| MethodInvocationTree methodInvocationTree = ((MethodInvocationNode) node).getTree(); | ||
| isLoopCondition = | ||
| methodInvocationTree != null && TreeUtils.isHasNextCall(methodInvocationTree); | ||
| } |
There was a problem hiding this comment.
Require the discovered hasNext() to belong to the target loop.
This backward scan accepts the first hasNext() call it sees, even if it comes from another enhanced-for or iterator chain. That can bind the wrong condition block when nested loops are present. Check the matched MethodInvocationNode against tree before returning.
Proposed fix
if (node instanceof MethodInvocationNode) {
- MethodInvocationTree methodInvocationTree = ((MethodInvocationNode) node).getTree();
+ MethodInvocationNode invocationNode = (MethodInvocationNode) node;
+ MethodInvocationTree methodInvocationTree = invocationNode.getTree();
isLoopCondition =
- methodInvocationTree != null && TreeUtils.isHasNextCall(methodInvocationTree);
+ methodInvocationTree != null
+ && TreeUtils.isHasNextCall(methodInvocationTree)
+ && invocationNode.getEnhancedForLoop() == tree;
}📝 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.
| if (node instanceof MethodInvocationNode) { | |
| MethodInvocationTree methodInvocationTree = ((MethodInvocationNode) node).getTree(); | |
| isLoopCondition = | |
| methodInvocationTree != null && TreeUtils.isHasNextCall(methodInvocationTree); | |
| } | |
| if (node instanceof MethodInvocationNode) { | |
| MethodInvocationNode invocationNode = (MethodInvocationNode) node; | |
| MethodInvocationTree methodInvocationTree = invocationNode.getTree(); | |
| isLoopCondition = | |
| methodInvocationTree != null | |
| && TreeUtils.isHasNextCall(methodInvocationTree) | |
| && invocationNode.getEnhancedForLoop() == tree; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 165 - 169, The current backward scan accepts the first hasNext() it
finds (in the block where node instanceof MethodInvocationNode) without ensuring
that the found MethodInvocationTree actually belongs to the target enhanced-for
condition `tree`; update the check so after obtaining methodInvocationTree and
confirming TreeUtils.isHasNextCall(methodInvocationTree) you also verify the
found methodInvocationTree is the same call (or is contained in the same parent
tree) as the target `tree` before setting isLoopCondition/returning — i.e.,
compare methodInvocationTree to `tree` (or its relevant parent) to ensure the
hasNext() belongs to the target enhanced-for loop.
| Block blockContainingLoopCondition = node.getBlock(); | ||
| if (blockContainingLoopCondition.getSuccessors().size() != 1) { | ||
| throw new BugInCF( | ||
| "loop condition has: " | ||
| + blockContainingLoopCondition.getSuccessors().size() | ||
| + " successors instead of 1."); | ||
| } | ||
| Block maybeConditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next(); | ||
| if (!(maybeConditionalBlock instanceof ConditionalBlock conditionalBlock)) { | ||
| throw new BugInCF( | ||
| "loop condition successor is not ConditionalBlock, but: " | ||
| + maybeConditionalBlock.getClass()); | ||
| } |
There was a problem hiding this comment.
Return null on CFG shape mismatches instead of throwing.
A mismatch here should reject the candidate, not abort analysis. If the CFG contract does not guarantee this successor shape across all supported desugarings, BugInCF will turn a benign non-match into a checker crash.
Proposed fix
Block blockContainingLoopCondition = node.getBlock();
if (blockContainingLoopCondition.getSuccessors().size() != 1) {
- throw new BugInCF(
- "loop condition has: "
- + blockContainingLoopCondition.getSuccessors().size()
- + " successors instead of 1.");
+ return null;
}
Block maybeConditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next();
if (!(maybeConditionalBlock instanceof ConditionalBlock conditionalBlock)) {
- throw new BugInCF(
- "loop condition successor is not ConditionalBlock, but: "
- + maybeConditionalBlock.getClass());
+ return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/EnhancedForDisposalLoopResolver.java`
around lines 172 - 184, The code currently throws BugInCF when the CFG shape
doesn't match (in the checks on
blockContainingLoopCondition.getSuccessors().size() and the instanceof check for
ConditionalBlock), but we should instead reject the candidate by returning null;
change both throw new BugInCF(...) occurrences in
EnhancedForDisposalLoopResolver (the checks using blockContainingLoopCondition,
maybeConditionalBlock and the instanceof ConditionalBlock conditionalBlock) to
return null so the resolver gracefully skips non-matching CFG shapes rather than
crashing.
| Block loopConditionBlock = CollectionOwnershipUtils.firstBlockForTree(cfg, condition); | ||
| Block loopUpdateBlock = | ||
| CollectionOwnershipUtils.firstBlockForTree(cfg, update.getExpression()); | ||
| Node nodeForCollectionElt = | ||
| CollectionOwnershipUtils.anyNodeForTree(cfg, collectionElementTree); | ||
| if (loopUpdateBlock == null || loopConditionBlock == null || nodeForCollectionElt == null) { | ||
| return null; | ||
| } | ||
| Block conditionalBlock = ((SingleSuccessorBlock) loopConditionBlock).getSuccessor(); | ||
| Block loopBodyEntryBlock = ((ConditionalBlock) conditionalBlock).getThenSuccessor(); | ||
| return new DisposalLoop( | ||
| CollectionOwnershipUtils.baseExpression(collectionElementTree), | ||
| collectionElementTree, | ||
| nodeForCollectionElt, | ||
| CollectionOwnershipUtils.cfgAssociatedTreeFor(cfg, condition), | ||
| (ConditionalBlock) conditionalBlock, | ||
| loopBodyEntryBlock, | ||
| loopUpdateBlock); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "IndexedForDisposalLoopMatcher.java" | head -5Repository: typetools/checker-framework
Length of output: 179
🏁 Script executed:
wc -l ./checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.javaRepository: typetools/checker-framework
Length of output: 183
🏁 Script executed:
sed -n '70,100p' ./checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.javaRepository: typetools/checker-framework
Length of output: 1504
🏁 Script executed:
sed -n '1,50p' ./checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.javaRepository: typetools/checker-framework
Length of output: 1970
🏁 Script executed:
find . -type f -name "CollectionOwnershipUtils.java" | head -5Repository: typetools/checker-framework
Length of output: 174
🏁 Script executed:
wc -l ./checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.javaRepository: typetools/checker-framework
Length of output: 178
🏁 Script executed:
grep -n "firstBlockForTree" ./checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.javaRepository: typetools/checker-framework
Length of output: 152
🏁 Script executed:
sed -n '67,85p' ./checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipUtils.javaRepository: typetools/checker-framework
Length of output: 571
🏁 Script executed:
grep -r "firstBlockForTree" ./checker/src/main/java/org/checkerframework/checker/collectionownership/ -nRepository: typetools/checker-framework
Length of output: 845
🏁 Script executed:
sed -n '370,385p' ./checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.javaRepository: typetools/checker-framework
Length of output: 599
🏁 Script executed:
grep -n "findConditionalSuccessor" ./checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java | head -5Repository: typetools/checker-framework
Length of output: 278
🏁 Script executed:
sed -n '757,775p' ./checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.javaRepository: typetools/checker-framework
Length of output: 716
Add type checks before casting CFG blocks to prevent ClassCastException.
The code assumes loopConditionBlock is a SingleSuccessorBlock and its successor is a ConditionalBlock without type verification. If the CFG structure differs, this will fail with a ClassCastException instead of returning null. Similar code in WhileDisposalLoopMatcher.findConditionalSuccessor() demonstrates the proper pattern with instanceof checks.
Safer implementation
Block loopConditionBlock = CollectionOwnershipUtils.firstBlockForTree(cfg, condition);
Block loopUpdateBlock =
CollectionOwnershipUtils.firstBlockForTree(cfg, update.getExpression());
Node nodeForCollectionElt =
CollectionOwnershipUtils.anyNodeForTree(cfg, collectionElementTree);
if (loopUpdateBlock == null || loopConditionBlock == null || nodeForCollectionElt == null) {
return null;
}
- Block conditionalBlock = ((SingleSuccessorBlock) loopConditionBlock).getSuccessor();
- Block loopBodyEntryBlock = ((ConditionalBlock) conditionalBlock).getThenSuccessor();
+ if (!(loopConditionBlock instanceof SingleSuccessorBlock ssb)) {
+ return null;
+ }
+ Block conditionalBlock = ssb.getSuccessor();
+ if (!(conditionalBlock instanceof ConditionalBlock cb)) {
+ return null;
+ }
+ Block loopBodyEntryBlock = cb.getThenSuccessor();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/IndexedForDisposalLoopMatcher.java`
around lines 78 - 95, The code currently casts loopConditionBlock to
SingleSuccessorBlock and its successor to ConditionalBlock without checks
causing potential ClassCastException; update IndexedForDisposalLoopMatcher to
verify types (use instanceof) before casting: ensure loopConditionBlock is a
SingleSuccessorBlock, get its successor and ensure that successor is a
ConditionalBlock, and only then obtain loopBodyEntryBlock and construct the
DisposalLoop; if any instanceof checks fail (or
loopUpdateBlock/nodeForCollectionElt are null) return null—mirror the pattern
used in WhileDisposalLoopMatcher.findConditionalSuccessor() for safe CFG
handling.
| if (naturalLoop.size() < bestLoopSize) { | ||
| bestLoopSize = naturalLoop.size(); | ||
| bestLoopUpdateBlock = backEdge.targetBlock; | ||
| } |
There was a problem hiding this comment.
Select back-edge source as loop update block.
Line 828 stores backEdge.targetBlock, but for a back edge (source -> target), the update/tail block is the source. Using target likely points at the loop header instead of the update location.
Proposed fix
if (naturalLoop.size() < bestLoopSize) {
bestLoopSize = naturalLoop.size();
- bestLoopUpdateBlock = backEdge.targetBlock;
+ bestLoopUpdateBlock = backEdge.sourceBlock;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/collectionownership/WhileDisposalLoopMatcher.java`
around lines 826 - 829, In WhileDisposalLoopMatcher, the code selects the wrong
block for loop updates by storing backEdge.targetBlock (the loop header) into
bestLoopUpdateBlock; change the assignment to use the back-edge source
(backEdge.sourceBlock) so bestLoopUpdateBlock points to the loop tail/update
block, and ensure the variable types and any subsequent uses of
bestLoopUpdateBlock still match this source-block semantics.
|
@msridhar here's a summary of the new structure and the design changes:
BeforeDisposal loop handling was split across multiple phases and checkers:
After current change
So now CO owns eveything regarding disposal loops. Only the trigger for loop discovery is from RLCC. |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
It's going to take me a while to do a full review. Here are some very initial comments.
| } | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| CollectionOwnershipUtils.getSuccessorsExceptIgnoredExceptions(currentBlock, coAtf)) { |
There was a problem hiding this comment.
Why do we need to pay attention to ignored exceptions here?
There was a problem hiding this comment.
The reason for the ignored exception filtering is that one enhanced-for AST can correspond to more than one CFG occurrence around finally.
For example:
try {
...
} finally {
for (Resource r : resources) {
r.close();
}
}there is only one enhanced-for in the AST, but the CFG can contain multiple copies of that loop for two control-flow paths (normal+exceptional) through the finally.
The current resolver does a CFG search and returns the first matching occurrence it finds. If that search also follows exceptional successors, it can reach the exceptional copy of the loop before the normal copy, and then bind to the wrong CFG occurrence of the same source loop.
I've added comments for this in the code.
| if (loop == null) { | ||
| throw new BugInCF( | ||
| "MethodInvocationNode.iterableExpression should be non-null iff" | ||
| + " MethodInvocationNode.enhancedForLoop is non-null"); | ||
| } |
There was a problem hiding this comment.
Do we really need this check here? Given we check if loop != tree immediately after?
| */ | ||
| private @Nullable DisposalLoop resolveEnhancedForLoop( | ||
| MethodInvocationNode methodInvocationNode, @FindDistinct EnhancedForLoopTree tree) { | ||
| if (methodInvocationNode.getIterableExpression() == null) { |
There was a problem hiding this comment.
This is a pretty big function. Does it make sense to split it into smaller functions? Or, can we add some comments in the different parts to explain the different phases of the check?
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
More comments. I think it's best to address these comments before I continue further.
| import org.checkerframework.dataflow.cfg.node.Node; | ||
| import org.checkerframework.javacutil.TreeUtils; | ||
|
|
||
| /** Matches indexed `for` {@link DisposalLoop}'s that iterates over a resource collection. */ |
There was a problem hiding this comment.
| /** Matches indexed `for` {@link DisposalLoop}'s that iterates over a resource collection. */ | |
| /** Matches indexed `for` {@link DisposalLoop}s that iterate over a resource collection. */ |
| if (loopBodyStatements == null) { | ||
| return null; | ||
| } | ||
| StatementTree init = tree.getInitializer().get(0); |
There was a problem hiding this comment.
Do we need to check if getInitializer() returns a list of length 1?
| } | ||
| StatementTree init = tree.getInitializer().get(0); | ||
| ExpressionTree condition = TreeUtils.withoutParens(tree.getCondition()); | ||
| ExpressionStatementTree update = tree.getUpdate().get(0); |
There was a problem hiding this comment.
Do we need to check if getUpdate() returns a list of length 1?
| AtomicBoolean blockIsIllegal = new AtomicBoolean(false); | ||
| final ExpressionTree[] collectionElementTree = {null}; |
There was a problem hiding this comment.
Rather than creating these mutable locals to expose to the anonymous class, you can make scanner a named class with fields. Then, you can run the scan and then read out the result value via getters at the end.
| for (StatementTree stmt : statements) { | ||
| scanner.scan(stmt, null); | ||
| } |
There was a problem hiding this comment.
This function can just take the ForLoopTree as a parameter, and then apply its scanner to tree.getStatement(), rather than looping over the statements
| TreeScanner<Void, Void> scanner = | ||
| new TreeScanner<Void, Void>() { | ||
| @Override | ||
| public Void visitUnary(UnaryTree tree, Void p) { | ||
| switch (tree.getKind()) { | ||
| case PREFIX_DECREMENT, POSTFIX_DECREMENT, PREFIX_INCREMENT, POSTFIX_INCREMENT -> { | ||
| if (CollectionOwnershipUtils.getNameFromExpressionTree(tree.getExpression()) | ||
| == iterator) { | ||
| blockIsIllegal.set(true); | ||
| } | ||
| } | ||
| default -> {} | ||
| } | ||
| return super.visitUnary(tree, p); | ||
| } | ||
|
|
||
| @Override | ||
| public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void p) { | ||
| if (CollectionOwnershipUtils.getNameFromExpressionTree(tree.getVariable()) | ||
| == iterator) { | ||
| blockIsIllegal.set(true); | ||
| } | ||
| return super.visitCompoundAssignment(tree, p); | ||
| } | ||
|
|
||
| @Override | ||
| public Void visitAssignment(AssignmentTree tree, Void p) { | ||
| Name assignedVariable = | ||
| CollectionOwnershipUtils.getNameFromExpressionTree(tree.getVariable()); | ||
| if (assignedVariable == iterator || assignedVariable == identifierInHeader) { | ||
| blockIsIllegal.set(true); | ||
| } | ||
|
|
||
| return super.visitAssignment(tree, p); | ||
| } | ||
|
|
||
| @Override | ||
| public Void visitMethodInvocation(MethodInvocationTree mit, Void p) { | ||
| if (isIthCollectionElement(mit, iterator) | ||
| && identifierInHeader == CollectionOwnershipUtils.getNameFromExpressionTree(mit) | ||
| && identifierInHeader != null) { | ||
| collectionElementTree[0] = mit; | ||
| } | ||
| return super.visitMethodInvocation(mit, p); | ||
| } | ||
| }; | ||
|
|
||
| for (StatementTree stmt : statements) { | ||
| scanner.scan(stmt, null); | ||
| } |
| if (tree == null || index == null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We should not allow null parameters and just get rid of these checks
5 participants
This is the new PR for #7166