Extend RLC to collections of resources #7166
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| /** | ||
| * Returns the index-th element of this stack. | ||
| * | ||
| * @param index which element to return | ||
| * @return the index-th element of this stack | ||
| */ | ||
| public E get(int index) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider annotating push and get for completeness.
To keep the API surface consistent, annotate push/get receivers as @NotOwningCollection too.
- public ImmutableStack<E> push(E elem) {
+ public ImmutableStack<E> push(@NotOwningCollection ImmutableStack<E> this, E elem) {
return extend(elem, this);
}
@@
- public E get(int index) {
+ public E get(@NotOwningCollection ImmutableStack<E> this, int index) {
int n = size();📝 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.
| /** | |
| * Returns the index-th element of this stack. | |
| * | |
| * @param index which element to return | |
| * @return the index-th element of this stack | |
| */ | |
| public E get(int index) { | |
| public ImmutableStack<E> push(@NotOwningCollection ImmutableStack<E> this, E elem) { | |
| return extend(elem, this); | |
| } | |
| /** | |
| * Returns the index-th element of this stack. | |
| * | |
| * @param index which element to return | |
| * @return the index-th element of this stack | |
| */ | |
| public E get(@NotOwningCollection ImmutableStack<E> this, int index) { | |
| int n = size(); | |
| // …rest of method unchanged… |
🤖 Prompt for AI Agents
In
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.java
around lines 1511-1517, the get method (and the push method elsewhere in this
class) should have their receivers annotated as @NotOwningCollection to keep the
API consistent; update both method signatures to add the receiver annotation
(e.g., change "public E get(int index)" to include the receiver annotation on
the implicit this, and likewise add @NotOwningCollection to the push method's
receiver) so the methods are explicitly marked as not owning the collection.
| /** The map that backs this set. */ | ||
| // Declared as LinkedHashMap because some implementations of Map prohibit null keys. | ||
| private final LinkedHashMap<K, V> theMap = new LinkedHashMap<>(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer interface type for the field to reduce coupling
Declaring theMap as Map preserves flexibility; the runtime implementation remains LinkedHashMap. The null-keys rationale doesn’t require the declared type to be LinkedHashMap.
-import java.util.LinkedHashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
@@
- /** The map that backs this set. */
- // Declared as LinkedHashMap because some implementations of Map prohibit null keys.
- private final LinkedHashMap<K, V> theMap = new LinkedHashMap<>();
+ /** The map that backs this set (insertion-ordered). */
+ private final Map<K, V> theMap = new LinkedHashMap<>();📝 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.
| /** The map that backs this set. */ | |
| // Declared as LinkedHashMap because some implementations of Map prohibit null keys. | |
| private final LinkedHashMap<K, V> theMap = new LinkedHashMap<>(); | |
| // --- imports --- | |
| import java.util.LinkedHashMap; | |
| import java.util.Map; | |
| // --- inside the class --- | |
| /** The map that backs this set (insertion-ordered). */ | |
| private final Map<K, V> theMap = new LinkedHashMap<>(); |
🤖 Prompt for AI Agents
In
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.java
around lines 23 to 26, the field is declared as a concrete LinkedHashMap which
increases coupling; change the field declaration to use the Map interface (e.g.,
private final Map<K, V> theMap = new LinkedHashMap<>();) while keeping the
runtime implementation as LinkedHashMap, retain or update the comment about
null-key behavior, and verify no code elsewhere relies on LinkedHashMap-specific
APIs—if it does, adjust those usages or narrow the type only where necessary.
.../main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java
Show resolved
Hide resolved
| public CollectionOwnershipStore( | ||
| CollectionOwnershipAnalysis analysis, | ||
| CFAbstractStore<CFValue, CollectionOwnershipStore> other) { | ||
| super(other); | ||
| this.atypeFactory = ((CollectionOwnershipStore) other).atypeFactory; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid brittle downcast in copy ctor
Derive the type factory from the analysis to prevent accidental ClassCastException if other isn’t a CollectionOwnershipStore.
public CollectionOwnershipStore(
CollectionOwnershipAnalysis analysis,
CFAbstractStore<CFValue, CollectionOwnershipStore> other) {
super(other);
- this.atypeFactory = ((CollectionOwnershipStore) other).atypeFactory;
+ this.atypeFactory =
+ (CollectionOwnershipAnnotatedTypeFactory) analysis.getTypeFactory();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public CollectionOwnershipStore( | |
| CollectionOwnershipAnalysis analysis, | |
| CFAbstractStore<CFValue, CollectionOwnershipStore> other) { | |
| super(other); | |
| this.atypeFactory = ((CollectionOwnershipStore) other).atypeFactory; | |
| } | |
| public CollectionOwnershipStore( | |
| CollectionOwnershipAnalysis analysis, | |
| CFAbstractStore<CFValue, CollectionOwnershipStore> other) { | |
| super(other); | |
| this.atypeFactory = | |
| (CollectionOwnershipAnnotatedTypeFactory) analysis.getTypeFactory(); | |
| } |
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipStore.java
around lines 39 to 44, the copy constructor currently downcasts `other` to
`CollectionOwnershipStore` to obtain `atypeFactory`, which is brittle and can
cause ClassCastException; instead, remove the cast and derive the type factory
from the provided `analysis` (e.g., call a getter like
`analysis.getTypeFactory()` or `analysis.getATypeFactory()` or access the
appropriate field on `analysis`) and assign that to `this.atypeFactory`, keeping
the rest of the constructor unchanged.
| @Override | ||
| protected CFValue newFieldValueAfterMethodCall( | ||
| FieldAccess fieldAccess, | ||
| GenericAnnotatedTypeFactory<CFValue, CollectionOwnershipStore, ?, ?> atf, | ||
| CFValue value) { | ||
| CFValue superResult = super.newFieldValueAfterMethodCall(fieldAccess, atf, value); | ||
| if (superResult == null) { | ||
| if (atypeFactory.isResourceCollectionField(fieldAccess.getField())) { | ||
| return value; | ||
| } | ||
| } | ||
| return superResult; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t reintroduce null field values
If value is null, returning it defeats the “keep in store” intent. Guard on value != null.
@Override
protected CFValue newFieldValueAfterMethodCall(
FieldAccess fieldAccess,
GenericAnnotatedTypeFactory<CFValue, CollectionOwnershipStore, ?, ?> atf,
CFValue value) {
CFValue superResult = super.newFieldValueAfterMethodCall(fieldAccess, atf, value);
- if (superResult == null) {
- if (atypeFactory.isResourceCollectionField(fieldAccess.getField())) {
- return value;
- }
- }
- return superResult;
+ if (superResult == null
+ && value != null
+ && atypeFactory.isResourceCollectionField(fieldAccess.getField())) {
+ return value;
+ }
+ return superResult;
}📝 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.
| @Override | |
| protected CFValue newFieldValueAfterMethodCall( | |
| FieldAccess fieldAccess, | |
| GenericAnnotatedTypeFactory<CFValue, CollectionOwnershipStore, ?, ?> atf, | |
| CFValue value) { | |
| CFValue superResult = super.newFieldValueAfterMethodCall(fieldAccess, atf, value); | |
| if (superResult == null) { | |
| if (atypeFactory.isResourceCollectionField(fieldAccess.getField())) { | |
| return value; | |
| } | |
| } | |
| return superResult; | |
| } | |
| @Override | |
| protected CFValue newFieldValueAfterMethodCall( | |
| FieldAccess fieldAccess, | |
| GenericAnnotatedTypeFactory<CFValue, CollectionOwnershipStore, ?, ?> atf, | |
| CFValue value) { | |
| CFValue superResult = super.newFieldValueAfterMethodCall(fieldAccess, atf, value); | |
| if (superResult == null | |
| && value != null | |
| && atypeFactory.isResourceCollectionField(fieldAccess.getField())) { | |
| return value; | |
| } | |
| return superResult; | |
| } |
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipStore.java
around lines 49 to 61, the method newFieldValueAfterMethodCall returns the
incoming value when the superclass result is null and the field is a resource
collection field; this reintroduces nulls if value is null. Update the guard to
only return value when value != null (i.e., check
atypeFactory.isResourceCollectionField(...) && value != null) and otherwise
return superResult so null values are not reintroduced into the store.
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java
Show resolved
Hide resolved
| private static Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap = | ||
| new HashMap<>(); | ||
|
|
||
| /** | ||
| * Maps the cfg-block corresponding to the loop conditional block of a | ||
| * collection-obligation-fulfilling loop to the loop wrapper. | ||
| */ | ||
| private static Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap = | ||
| new HashMap<>(); |
There was a problem hiding this comment.
Thread safety concern with static mutable maps.
The static maps conditionToFulfillingLoopMap and conditionalBlockToFulfillingLoopMap are mutable and accessed without synchronization. This could lead to race conditions if the checker is used in a multi-threaded environment.
Consider using concurrent collections or adding synchronization:
-private static Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap =
- new HashMap<>();
+private static final Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap =
+ new ConcurrentHashMap<>();
-private static Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap =
- new HashMap<>();
+private static final Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap =
+ new ConcurrentHashMap<>();🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java
around lines 128 to 136, the two static mutable maps are not thread-safe and can
race; replace their declarations with thread-safe alternatives (e.g., make them
ConcurrentHashMap instances) or convert them to non-static instance fields or
thread-local storage if per-factory state is intended; ensure all accesses use
the chosen thread-safe structure (or synchronize accessors) so reads/writes
cannot race.
There was a problem hiding this comment.
I think the agent's complaint here is reasonable: I don't see a reason for these maps to be static. However, the fix that the agent is proposing seems like overkill: why synchronize them when we could instead make these instance fields? There shouldn't ever be more than one ATF per running checker.
There was a problem hiding this comment.
@kelloggm Thanks — agreed. I’ll switch these to instance fields and drop the static helpers so there’s no shared mutable state across analyses.
Proposed changes:
- Make the maps instance-scoped and final
// In CollectionOwnershipAnnotatedTypeFactory.java
// before:
// private static Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap = new HashMap<>();
// private static Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap = new HashMap<>();
// after:
private final Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap = new HashMap<>();
private final Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap = new HashMap<>();- Convert the helper methods to instance methods
// before (static):
// public static void markFulfillingLoop(PotentiallyFulfillingLoop loop) { ... }
// public static @Nullable PotentiallyFulfillingLoop getFulfillingLoopForCondition(Tree tree) { ... }
// public static @Nullable PotentiallyFulfillingLoop getFulfillingLoopForConditionalBlock(Block block) { ... }
// after (instance):
public void markFulfillingLoop(PotentiallyFulfillingLoop loop) {
conditionToFulfillingLoopMap.put(loop.condition, loop);
conditionalBlockToFulfillingLoopMap.put(loop.loopConditionalBlock, loop);
}
public @org.checkerframework.checker.nullness.qual.Nullable
PotentiallyFulfillingLoop getFulfillingLoopForCondition(Tree tree) {
return conditionToFulfillingLoopMap.get(tree);
}
public @org.checkerframework.checker.nullness.qual.Nullable
PotentiallyFulfillingLoop getFulfillingLoopForConditionalBlock(Block block) {
return conditionalBlockToFulfillingLoopMap.get(block);
}- Optional hygiene: clear per-CFG to prevent stale entries
@Override
public void postAnalyze(ControlFlowGraph cfg) {
super.postAnalyze(cfg);
conditionToFulfillingLoopMap.clear();
conditionalBlockToFulfillingLoopMap.clear();
}- Update call sites to use the ATF instance
- From within collection-ownership code (e.g., CollectionOwnershipTransfer):
CollectionOwnershipAnnotatedTypeFactory coAtf = (CollectionOwnershipAnnotatedTypeFactory) atypeFactory;
PotentiallyFulfillingLoop pfl = coAtf.getFulfillingLoopForCondition(conditionTree);- From other checkers (e.g., MustCallConsistencyAnalyzer) via utilities already in this PR:
CollectionOwnershipAnnotatedTypeFactory coAtf =
org.checkerframework.checker.resourceleak.ResourceLeakUtils
.getCollectionOwnershipAnnotatedTypeFactory(mcAtf /* or a reference ATF/checker */);
coAtf.markFulfillingLoop(loop);If you’re happy with this direction, I’ll prepare the follow-up edits to method signatures and adjust the few call sites accordingly; CI should go green after these changes since the usage becomes explicit and state stays within the factory instance.
✏️ Learnings added
Learnt from: kelloggm
PR: typetools/checker-framework#7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.096Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
.../main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipTransfer.java
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
docs/manual/resource-leak-checker.tex (4)
659-667: Definition excludes arrays but examples include them.The definition of "resource collection" restricts it to
java.util.Iterableimplementations (line 662), but the example on line 666 includesSocket[], which is an array type and does not implementIterable. This inconsistency was previously flagged.Either:
- Update the definition to explicitly include array types alongside
Iterable, or- Remove
Socket[]from the examples and clarify that arrays are not supported.Based on the context, arrays appear to be intended as resource collections (see indexed loop support in lines 794-817), so the definition should be broadened.
794-817: Indexed loop requirements are List-specific; clarify scope or add array support.The indexed loop requirements mandate
c.size()(line 800) andcollection.get(i)(line 802), which areList-specific constructs. However, the earlier definition includes arrays as resource collections (line 666). This creates an inconsistency: if arrays are resource collections, indexed loops should support them witharray.lengthandarray[i].Apply one of:
- Option A: Scope this section explicitly to
Listby adding "Forjava.util.List," before "Loop iteration bounds:" and clarifying arrays are unsupported here.- Option B: Extend the rules to include arrays by accepting
array.lengthas upper bound andarray[i]for element access.The previous review suggested the same fix.
823-823: Fix: interface requiresimplements, notextends.
Closeableis an interface, so the declaration must useimplements:-class Aggregator extends Closeable { +class Aggregator implements Closeable {This was flagged in a previous review.
873-873: Spelling: "transfering" → "transferring".-Resource collection fields owned by the enclosing class are special cases for ownership transfer, as the checker doesn't allow transfering the ownership away from such a field. +Resource collection fields owned by the enclosing class are special cases for ownership transfer, as the checker doesn't allow transferring the ownership away from such a field.This was previously noted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/manual/resource-leak-checker.tex(2 hunks)
🔇 Additional comments (3)
docs/manual/resource-leak-checker.tex (3)
138-158: LGTM! Clear introduction to collection ownership annotations.The new subsection properly introduces the collection ownership annotations and the
@CollectionFieldDestructorannotation with appropriate forward references to detailed sections.
668-688: LGTM! Clear and accurate type hierarchy.The ownership type system hierarchy is well-presented and the descriptions of each qualifier are technically accurate.
690-766: LGTM! Comprehensive explanation of ownership transfer semantics.The examples clearly illustrate how ownership defaults work, how transfer occurs through parameters, returns, and assignments, and how obligations move with ownership.
| % \subsectionAndLabel{Iterators over resource collections}{resource-leak-collections-iterators} | ||
| % Iterators are frequently used to traverse collections. It is always safe to create an iterator for any non-resource collection. Since an iterator takes a snapshot of the collection at the time it is created, it does not matter what happens with the collection after the iterator is created (else, a \<ConcurrentModificationException> is thrown for the next usage of the iterator). | ||
| % An iterator created for a resource collection has the same collection ownership type as the resource collection. | ||
|
|
||
| % \begin{verbatim} | ||
| % // `socketList` is @OwningCollectionWithoutObligation | ||
| % Iterator<Socket> socketIterator = socketList.iterator(); | ||
| % // `socketIterator` is @OwningCollectionWithoutObligation | ||
| % \end{verbatim} | ||
|
|
||
| % The only concering operation an iterator can do is \texttt{remove()}, as the removed element might have open calling obligations. | ||
|
|
||
| % Just like a \texttt{java.util.Collection} of type \texttt{@NotOwningCollection}, an iterator of this type is not allowed to call \texttt{remove()} at all. Iterators of type \texttt{@OwningCollectionBottom} and \texttt{@OwningCollectionWithoutObligation} can always call \texttt{remove()}. The interesting remaining case is that of an \texttt{@OwningCollection} iterator. | ||
| % Such an iterator and the values its calls to \texttt{next()} return are tracked with special obligations. This is to ensure that whenever an \texttt{@OwningCollection} iterator calls \texttt{close()}, the value returned by the previous call to \texttt{Iterator.next()} has its \texttt{MustCall} obligations fulfilled. | ||
|
|
||
| % Here is an example of unsafe iterator usage: | ||
|
|
||
| % \begin{verbatim} | ||
| % List<Socket> foo(@OwningCollection List<Socket> list) { | ||
| % Iterator<Socket> iter = list.iterator(); | ||
| % // `iter` is @OwningCollection and must be tracked | ||
| % iter.next(); | ||
| % iter.remove(); // unsafe! | ||
| % return list; | ||
| % } | ||
| % \end{verbatim} | ||
|
|
||
| % \begin{verbatim} | ||
| % error: [required.method.not.called] @MustCall method close may not have been invoked on iter.next() or any of its aliases. | ||
| % iter.next(); | ||
| % ^ | ||
| % \end{verbatim} | ||
|
|
||
| % The removed element must have its must-call obligations fulfilled. Here is an example of safe usage: | ||
|
|
||
| % \begin{verbatim} | ||
| % List<Socket> foo(@OwningCollection List<Socket> list) { | ||
| % Iterator<Socket> iter = list.iterator(); | ||
| % // `iter` is @OwningCollection and must be tracked | ||
| % try { | ||
| % iter.next().close(); | ||
| % } catch (Exception e) { | ||
| % } | ||
| % iter.remove(); | ||
| % return list; | ||
| % } | ||
| % \end{verbatim} | ||
|
|
||
| % The returned element may also be stored in a variable first and the fulfillment of the obligation of the returned element may also occur after the call to \texttt{remove()}: | ||
|
|
||
| % \begin{verbatim} | ||
| % List<Socket> foo(@OwningCollection List<Socket> list) { | ||
| % Iterator<Socket> iter = list.iterator(); | ||
| % // `iter` is @OwningCollection and must be tracked | ||
| % Socket s = iter.next(); | ||
| % iter.remove(); | ||
| % try { | ||
| % s.close(); | ||
| % } catch (Exception e) { | ||
| % } | ||
| % return list; | ||
| % } | ||
| % \end{verbatim} | ||
|
|
||
| % The returned element by \texttt{iter.next()} may have its must-call obligation fulfilled in any way described by the resource leak checker, even by transfering its ownership (for example by storing it in a field or passing it as an \texttt{@OwningCollection} method argument). | ||
|
|
||
| % Iterators over collections with obligations may also be returned, passed as method or constructor arguments and even be stored in fields. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Large commented-out section on iterators.
This 67-line block describes iterator support but is entirely commented out. If iterator support is implemented, uncomment and finalize this section. If not yet ready, consider removing it or adding a TODO comment explaining its status.
🤖 Prompt for AI Agents
docs/manual/resource-leak-checker.tex lines 889-956 contain a large 67-line
commented-out subsection about iterator support; either fully restore it
(uncomment and finalize the text/examples) if iterator handling is
implemented—ensuring the examples compile, references/labels are correct, and
the docs build cleanly—or remove the block and replace it with a short TODO note
stating iterator support status and next steps; after change, run the
documentation build and update any cross-references or table-of-contents entries
accordingly.
| \subsectionAndLabel{JDK Method Signatures}{resource-leak-collections-jdk-methods} | ||
| [[TODO: Add a table with a number of collection/iterable methods and their collection ownership annotations]] | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO: Add JDK method signatures table.
The placeholder TODO at line 958 indicates this section is incomplete. The documentation references JDK method signatures for collection ownership but doesn't provide the promised table.
Would you like me to help draft the table structure or open an issue to track completing this section?
🤖 Prompt for AI Agents
In docs/manual/resource-leak-checker.tex around lines 957 to 959, the TODO
placeholder indicates the missing table of JDK collection/iterable method
signatures and their ownership annotations; replace the placeholder with a LaTeX
table that lists method signature, returned/consumed collection type, ownership
annotation (e.g., returnsOwned, borrows, transfers), and a short note/example
column; include common methods from java.util and streams (e.g.,
Collection#iterator, Collection#addAll, List#subList, Stream#collect,
Arrays#asList) with their correct ownership classification and a brief
rationale, and ensure the table is labeled and referenced by the subsection so
cross-references compile.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
docs/manual/resource-leak-checker.tex (4)
659-667: Definition excludes arrays but examples elsewhere use array types.The definition restricts resource collections to
java.util.Iterableimplementations (line 662), but other parts of the documentation reference array types likeSocket[]as resource collections. This inconsistency was noted in previous reviews.As per the previous review, either:
- Explicitly state that arrays are not supported, or
- Extend the definition to include array types (e.g.,
T[]whereThas non-empty@MustCalltype)Based on past review comments.
797-817: Indexed loop requirements are List-specific; clarify scope or extend to arrays.The indexed loop requirements (lines 798-802) reference
c.size()andcollection.get(i), which arejava.util.Listmethods not available on allIterableimplementations. Arrays would requirearray.lengthandarray[i]instead. This was flagged in previous reviews.As per the previous review, either:
- Scope this section explicitly to
List(e.g., "Forjava.util.List, loop iteration bounds:"), or- Extend the rules to include arrays with parallel syntax (e.g., "upper bound is
c.size()forListora.lengthfor arrays")Based on past review comments.
889-956: Large commented-out iterator section remains unresolved.This 67-line commented-out section on iterator support was flagged in previous reviews. The section should either be:
- Uncommented and finalized if iterator support is implemented, or
- Removed and replaced with a brief TODO note explaining iterator support status
As per previous reviews, decide whether to activate or remove this section to avoid carrying stale commented-out documentation.
Based on past review comments.
957-959: TODO placeholder for JDK method signatures table still empty.The TODO at line 958 was flagged in previous reviews but remains incomplete. The documentation references JDK method signatures for collection ownership but doesn't provide the promised table.
As per previous reviews, this table should list common JDK collection/Iterable methods (e.g.,
Collection#iterator,List#subList,Stream#collect) with their ownership annotations and brief explanations.Based on past review comments.
checker/tests/resourceleak-collections/OwningCollectionFieldTyping.java (6)
80-87: Address or track the commented-out test.The commented-out
reassignCollectionFieldIfNulltest with TODO should either be re-enabled with expected diagnostics or tracked in an issue to prevent it from being forgotten.Please clarify: Is this test blocked by pending functionality, or can it be enabled now? If blocked, please create a tracking issue and reference it in the TODO comment.
96-103: Add @OverRide annotation for consistency.The
close()method implementsCloseablebut is missing the@Overrideannotation.Apply this diff:
@CollectionFieldDestructor("resList") +@Override public void close() {
145-152: Add @OverRide annotation.Same as above - missing
@OverrideforCloseable.close().@CollectionFieldDestructor("fieldList") +@Override public void close() {
163-170: Add @OverRide annotation.Same as above.
@CollectionFieldDestructor("fieldList") +@Override public void close() {
176-183: Add @OverRide annotation.Same as above.
@CollectionFieldDestructor("fieldList") +@Override public void close() {
193-200: Add @OverRide annotation.Same as above.
@CollectionFieldDestructor("fieldList") +@Override public void close() {checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
3516-3517: Use object equality for block comparison.Reference equality (
==) is brittle for comparing CFG blocks. While the suppression indicates this is intentional, using.equals()would be more robust.Apply this diff:
-@SuppressWarnings("interning:not.interned") -boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; +boolean isLastBlockOfBody = Objects.equals(successorAndExceptionType.first, loopUpdateBlock);This matches the feedback from previous reviews at lines 3516-3517.
3576-3578: Use object equality for block comparison.Same issue as line 3516-3517: reference equality should be replaced with object equality.
Apply this diff:
-@SuppressWarnings("interning:not.interned") -boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; +boolean thenSuccessor = Objects.equals(conditionalBlock.getThenSuccessor(), loopUpdateBlock);checker/tests/resourceleak-collections/CollectionOwnershipDefaults.java (1)
18-21: Fix typo in comment.The comment contains "resoure" which should be "resource".
Apply this diff:
- * something is a resoure collection. + * something is a resource collection.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(42 hunks)checker/tests/resourceleak-collections/CollectionOwnershipBasicTyping.java(1 hunks)checker/tests/resourceleak-collections/CollectionOwnershipDefaults.java(1 hunks)checker/tests/resourceleak-collections/OwningCollectionFieldTyping.java(1 hunks)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java(1 hunks)docs/manual/resource-leak-checker.tex(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (6)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipStore.java (1)
CollectionOwnershipStore(16-62)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(66-749)PotentiallyFulfillingLoop(630-707)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)
🪛 ast-grep (0.39.5)
checker/tests/resourceleak-collections/CollectionOwnershipBasicTyping.java
[info] 44-44: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 54-54: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket("", 42)
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 81-81: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 99-99: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 115-115: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 117-117: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 119-119: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 121-121: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (21)
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (4)
32-47: LGTM! equals and hashCode are correctly implemented.The
equalsmethod now correctly uses&&for transitivity and callsequalsonthis-fields. ThehashCodemethod is consistent withequals, hashing bothtreeandnode. Previous review feedback has been properly addressed.
55-58: Verify that unconditionally returning true is correct.The method always returns
true. Ensure this aligns with the intended semantics for iterated collection elements. If the determinism should depend on the underlyingnode, consider delegating tonodeor checking its properties.
74-82: LGTM! Correct for loop iteration variables.Returning
falsefor bothisAssignableByOtherCodeandisModifiableByOtherCodeis appropriate for iteration variables, which are local to the loop scope and cannot be assigned or modified by external code.
49-72: LGTM! Standard implementations for JavaExpression API.The implementations of
containedOfClass,syntacticEquals,containsSyntacticEqualJavaExpression, andacceptfollow standard patterns forJavaExpressionsubclasses and are correct.Also applies to: 84-87
docs/manual/resource-leak-checker.tex (3)
138-158: LGTM: Clear subsection structure and annotation descriptions.The new subsection introducing collection ownership annotations is well-organized and provides appropriate forward references to detailed sections.
668-766: LGTM: Clear ownership type hierarchy and accurate examples.The ownership type system hierarchy is well-explained, and the code examples correctly demonstrate:
- Fresh allocation defaults
- Type refinement on element addition
- Ownership transfer through parameters, returns, and assignments
The examples are technically sound and illustrate the concepts effectively.
818-887: LGTM: Resource collection fields are well-documented with correct examples.The field ownership section provides:
- A complete, compilable example (correctly using
implements Closeable)- Clear verification requirements
- Well-explained assignment rules and ownership restrictions
The documentation effectively explains how resource collection fields integrate with the RAII pattern.
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (10)
177-184: LGTM: Collection ownership integration is well-structured.The addition of the
coAtffield andisLoopBodyAnalysisflag properly extends the analyzer to support collection ownership checking. The documentation clearly explains when the flag is set and its purpose.
511-583: LGTM: CollectionObligation design is sound.The
CollectionObligationsubclass appropriately extendsObligationto track method obligations for collection elements. ThegetReplacementoverride enables polymorphic replacement behavior, and the static factory methods provide convenient construction patterns.
853-871: LGTM: Foreign field access check is correct.The method properly restricts access to
@OwningCollectionfields to onlythisandsuperreceivers, preventing ownership leakage. ThereceiverAsStringhelper (extended at lines 2220-2241) correctly handles the additionalFieldAccessNodecase for nested field accesses.
1500-1563: LGTM: Loop body alias tracking is well-documented.The method correctly identifies when a loop variable is assigned to a collection element by using structural tree equality. The inline comments explain the rationale for the temp variable handling, addressing previous review feedback.
2146-2212: LGTM: Enhanced CreatesMustCallFor validation.The method now properly handles field access nodes and returns a boolean to indicate validation success. The improved error messages include the specific field name, which will help developers understand and fix issues.
3288-3317: LGTM: Loop discovery is straightforward.The
findFulfillingForEachLoopsmethod performs a simple CFG traversal to find and analyze enhanced for-loops. The worklist algorithm is standard and correct.
2533-2553: Exception handling is clear with context.The
InvalidLoopBodyAnalysisExceptionis thrown when encountering unreachable blocks during loop body analysis. The catch site at lines 2541-2543 correctly aborts the analysis, and the inline comment explains the expected behavior.
2964-2986: LGTM: Collection obligation checking correctly suppressed during loop analysis.The method properly skips reporting collection obligation errors during loop body analysis (line 2964-2967) and then issues appropriate errors for unfulfilled collection obligations using the collection element's tree for error location.
3168-3190: Formatting helper properly handles unknown method names.The
formatMustCallMethodhelper correctly translatesUNKNOWN_METHOD_NAMEto the user-friendly string "Unknown", improving error message clarity.
3320-3434: Verify CFG traversal termination for enhanced-for-loop desugarings
No tests cover nested loops, empty loops, or loops with breaks/continues—manually confirm patternMatchEnhancedCollectionForLoop always terminates or add unit tests for these edge cases.checker/tests/resourceleak-collections/CollectionOwnershipBasicTyping.java (2)
1-146: Test coverage is comprehensive and well-documented.The test file thoroughly exercises collection ownership defaults, return type checking, unrefinement, ownership transfer, and diamond inference. The inline comments clearly explain the expected behavior for each test case.
Regarding static analysis warnings: The unencrypted-socket warnings from ast-grep are expected for test code using
Socket. These can be safely ignored as they're intentional for testing resource leak detection, not production code security.
106-113: Consider using explicit empty MustCall annotation.The bare
@MustCallannotation on line 108 is valid but could be more explicit for clarity. However, the test is specifically checking that@MustCall({})creates no obligation (resulting in@OwningCollectionBottom), so the current form is acceptable.The previous review's suggestion to change line 108 is not necessary since the test is intentionally demonstrating the difference between bare
@MustCalland@MustCall({}). The test comments make this distinction clear.checker/tests/resourceleak-collections/CollectionOwnershipDefaults.java (1)
1-127: Test coverage is thorough and well-structured.The tests comprehensively verify default ownership semantics across different contexts (fields, parameters, returns, allocations). The
detectResourceCollectionmethod nicely demonstrates how manual@MustCallannotations affect resource collection detection.checker/tests/resourceleak-collections/OwningCollectionFieldTyping.java (1)
1-201: Test suite provides excellent coverage of field ownership scenarios.The test classes systematically verify constructor ownership transfer, reassignment rules, destructor semantics, and interaction with final fields. The expected error annotations clearly document the checker's behavior.
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollections.java (1)
15-15: Fix test directory mismatch to enable test discovery.Line 15 passes
"resourceleak"to the constructor, butgetTestDirs()at line 23 returns"resourceleak-collections". This mismatch will prevent test discovery.Apply this diff:
- "resourceleak", + "resourceleak-collections",checker/tests/resourceleak/Issue4815.java (1)
16-16: Use the canonical diagnostic keyPlease switch the directive to the stable key so the test won’t break if the message text changes.
- // :: error: argument + // :: error: (argument.type.incompatible)framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java (2)
3022-3025: Fix severity mismatch in Javadoc.The Javadoc states "issues a warning" but the implementation at line 3005 uses
checker.reportError, which issues an error, not a warning. Update the Javadoc to match the actual behavior.Apply this diff:
* Returns a set of AnnotationMirrors that is a lower bound for exception parameters. If a user * writes a more specific (lower) annotation than this on an exception parameter, the Checker - * Framework issues a warning indicating that the written annotation cannot be verified. + * Framework issues an error indicating that the written annotation cannot be verified.Based on past review comments.
3086-3092: Fix singular/plural inconsistency in Javadoc.The method returns a set of AnnotationMirrors (plural), but line 3089 refers to "the same bound" (singular). Use "bounds" consistently.
Apply this diff:
* <p>Note: by default this method returns {@code getExceptionParameterLowerBoundAnnotations()}, - * so that the same bound is enforced. + * so that the same bounds are enforced.Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/CreatesCollectionObligation.java(1 hunks)checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/NotOwningCollection.java(1 hunks)checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/OwningCollection.java(1 hunks)checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/OwningCollectionWithoutObligation.java(1 hunks)checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/PolyOwningCollection.java(1 hunks)checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java(1 hunks)checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java(1 hunks)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties(1 hunks)checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollections.java(1 hunks)checker/tests/resourceleak/EnhancedFor.java(1 hunks)checker/tests/resourceleak/Issue4815.java(1 hunks)checker/tests/resourceleak/SocketIntoList.java(2 hunks)dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizer.java(2 hunks)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java(1 hunks)framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java(3 hunks)framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java(13 hunks)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (1)
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)
dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizer.java (1)
dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java (1)
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java (1)
BaseTypeChecker(67-309)
🪛 ast-grep (0.39.5)
checker/tests/resourceleak/SocketIntoList.java
[info] 19-19: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 28-28: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (22)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties (1)
1-2: Wording update aligns with prior diagnostic guidance.The revised phrasing is clearer while keeping the existing formatting placeholders intact.
checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java (1)
46-47: Previous concern addressed — comment is clear and appropriate.The inline comment now documents the intent for protected visibility, explicitly naming the subclass (
RLCCalledMethodsTransfer) and noting the cross-package requirement. This directly addresses the feedback from the previous review.checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java (2)
16-17: LGTM!Standard no-op constructor follows the checker framework conventions.
19-25: LGTM! Defensive copy correctly applied.The implementation properly creates a defensive copy via
new LinkedHashSet<>(super.getImmediateSubcheckerClasses())before mutation, addressing the concern that the superclass may return an unmodifiable set.checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/CreatesCollectionObligation.java (1)
25-29: LGTM! Annotation properly structured.The annotation declaration is correct with appropriate meta-annotations. The
@Documentedannotation has been added as suggested in prior feedback, and@Target({ElementType.METHOD})is consistent with the Javadoc stating "method declarations of collections."checker/tests/resourceleak/SocketIntoList.java (4)
7-7: LGTM! Import added for ownership qualifier.The import supports the new
@OwningCollectionannotation used in the test methods.
17-23: LGTM! Test demonstrates owning collection semantics.The signature change to
@OwningCollection List<Socket>correctly demonstrates that an owning collection can store elements with must-call obligations, and the return statement allows testing ownership propagation.
25-34: LGTM! Test case correctly demonstrates obligation transfer.The updated comments accurately explain that
List#add(@Owning E)takes on the obligation of the argument, preventing duplicate error reporting at the socket's declaration site. The error marker at line 32 correctly flags that a non-owning list cannot hold elements with non-empty@MustCallvalues.
36-44: LGTM! Test case correctly demonstrates non-owning retrieval.The comments accurately explain that
List#getreturns@NotOwningelements, and the error marker at line 42 correctly flags the attempt to reset must-call obligations on a non-owning reference.checker-qual/src/main/java/org/checkerframework/checker/collectionownership/qual/OwningCollection.java (1)
20-24: LGTM! Qualifier annotation properly structured.The annotation is correctly declared with:
@Documentedfor Javadoc visibility (addressing prior feedback)- Runtime retention for reflection access
- Appropriate targets for type-use contexts
- Proper subtype relationship with
NotOwningCollectionThe implementation aligns with the checker framework's type qualifier conventions.
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java (2)
2968-2983: LGTM! Clean caching implementation.The caching mechanism for exception parameter lower-bound annotations is well-implemented with proper null-checking and the
@MonotonicNonNullannotation.
4508-4508: LGTM! Clearer parameter description.The updated description "true if the check succeeded" is clearer and more accurate than the previous "true if the check succeeded or failed."
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (8)
77-78: LGTM! Field initialization follows established patterns.The
iteratedCollectionElementsfield is properly declared, initialized in constructors, and copied in the copy constructor, following the same pattern as other tracking maps likelocalVariableValuesandfieldValues.Also applies to: 150-150, 171-171
647-653: LGTM! Insertion logic correctly handles IteratedCollectionElement.The
computeNewValueAndInsertmethod properly handlesIteratedCollectionElementby retrieving the old value, applying the merger function, and storing the result only if non-null. This follows the established pattern used forLocalVariable.
784-786: LGTM! Removal logic correctly handles IteratedCollectionElement.The
clearValuemethod properly removes entries forIteratedCollectionElementfrom the tracking map, consistent with how other expression types are handled.
834-836: LGTM! Value retrieval correctly handles IteratedCollectionElement.The
getValuemethod properly retrieves values forIteratedCollectionElementfrom the tracking map, following the pattern established for other expression types.
512-512: LGTM! IteratedCollectionElement correctly added to storable expressions.The addition of
IteratedCollectionElementto thecanInsertJavaExpressioncheck is correct and follows the pattern of other storable expression types.
1235-1245: LGTM! Merge logic correctly handles null values.The upper bound merge logic for
iteratedCollectionElementscorrectly checks ifmergedValis non-null before inserting into the new store. This addresses the past review comment and maintains consistency with null-guarded merges in other sections.
1329-1335: LGTM! Superset check correctly includes iteratedCollectionElements.The
supersetOfmethod properly checks that alliteratedCollectionElementsentries in the other store are present in this store with equal values, maintaining consistency with the checks for other expression types.
1419-1422: LGTM! Visualization correctly includes iteratedCollectionElements.The
internalVisualizemethod properly iterates over and visualizesiteratedCollectionElementsentries using the newvisualizeStoreIteratedCollectionElthook, following the established pattern.dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
13-88: LGTM! Well-implemented value object with correct semantics.The
IteratedCollectionElementclass is correctly implemented:
- Public final fields
nodeandtreeprovide the necessary dataequals()andhashCode()are consistent, both using tree AND nodeisDeterministic()correctly returns true for loop variablesisAssignableByOtherCode()andisModifiableByOtherCode()correctly return false- All required
JavaExpressionmethods are properly overridden- Visitor pattern support is correctly implemented
The previous review comments about equals/hashCode consistency and constructor parameter naming have been addressed.
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
172-181: LGTM! Zero-argument validation added as suggested.The implementation correctly addresses the past review concern by verifying both the method name and that the argument list is empty, preventing false positives from hypothetical overloaded
hasNext(...)methods.
| /** | ||
| * Returns the {@link IteratedCollectionElement} associated with the given node or tree. | ||
| * | ||
| * @param node CFG node for which the associated {@link IteratedCollectionElement} is sought | ||
| * @param tree AST tree for which the associated {@link IteratedCollectionElement} is sought | ||
| * @return the {@link IteratedCollectionElement} associated with the given node or tree | ||
| */ | ||
| @SuppressWarnings("interning:not.interned") // we want to check reference equality | ||
| public @Nullable IteratedCollectionElement getIteratedCollectionElement(Node node, Tree tree) { | ||
| for (IteratedCollectionElement ice : iteratedCollectionElements.keySet()) { | ||
| if (ice.tree == tree || ice.node == node || ice.tree.equals(tree) || ice.node.equals(node)) { | ||
| return ice; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix the matching logic to require both tree and node to match.
The method uses || to match if EITHER the tree OR the node matches an entry in the store. This is inconsistent with IteratedCollectionElement.equals(), which requires BOTH tree AND node to match. The current logic could return an incorrect IteratedCollectionElement when only one of the two fields matches.
Apply this diff to require both tree and node to match:
@SuppressWarnings("interning:not.interned") // we want to check reference equality
public @Nullable IteratedCollectionElement getIteratedCollectionElement(Node node, Tree tree) {
for (IteratedCollectionElement ice : iteratedCollectionElements.keySet()) {
- if (ice.tree == tree || ice.node == node || ice.tree.equals(tree) || ice.node.equals(node)) {
+ if ((ice.tree == tree || ice.tree.equals(tree)) && (ice.node == node || ice.node.equals(node))) {
return ice;
}
}
return null;
}📝 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.
| /** | |
| * Returns the {@link IteratedCollectionElement} associated with the given node or tree. | |
| * | |
| * @param node CFG node for which the associated {@link IteratedCollectionElement} is sought | |
| * @param tree AST tree for which the associated {@link IteratedCollectionElement} is sought | |
| * @return the {@link IteratedCollectionElement} associated with the given node or tree | |
| */ | |
| @SuppressWarnings("interning:not.interned") // we want to check reference equality | |
| public @Nullable IteratedCollectionElement getIteratedCollectionElement(Node node, Tree tree) { | |
| for (IteratedCollectionElement ice : iteratedCollectionElements.keySet()) { | |
| if (ice.tree == tree || ice.node == node || ice.tree.equals(tree) || ice.node.equals(node)) { | |
| return ice; | |
| } | |
| } | |
| return null; | |
| } | |
| @SuppressWarnings("interning:not.interned") // we want to check reference equality | |
| public @Nullable IteratedCollectionElement getIteratedCollectionElement(Node node, Tree tree) { | |
| for (IteratedCollectionElement ice : iteratedCollectionElements.keySet()) { | |
| if ((ice.tree == tree || ice.tree.equals(tree)) | |
| && (ice.node == node || ice.node.equals(node))) { | |
| return ice; | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java
around lines 806 to 821, the loop currently returns an IteratedCollectionElement
when either the node OR the tree matches; change the matching logic to require
both the tree and the node to match to be consistent with
IteratedCollectionElement.equals(): replace the condition with a conjunction
that checks tree equality (prefer reference equality then equals) AND node
equality (prefer reference equality then equals), i.e. ((ice.tree == tree ||
ice.tree.equals(tree)) && (ice.node == node || ice.node.equals(node))), leaving
the rest of the method and annotations intact.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
checker/tests/resourceleak/IndexMode.java (1)
9-18: Add explanatory comments for consistency.For consistency with the updated methods (getMode2a, getMode3, getMode4, getModeIS), consider adding similar comments to these methods explaining that no error is reported because Map#get returns @NotOwning:
getMode(lines 9-18)getMode2(lines 21-37)getMode5(lines 67-69)Additionally, the TODO comment in
getMode2(lines 23-31) describes a complex interaction with type variable defaulting. Given the @NotOwning annotation change, please verify whether this TODO is still accurate or if it should be updated to reference the @NotOwning behavior instead.Also applies to: 21-37, 67-69
♻️ Duplicate comments (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (3)
3514-3515: Use object equality instead of reference equality for block comparison.Using reference equality (
==) to check if the successor is the loop update block is fragile. The CFG may reconstruct block objects, breaking reference equality. Use.equals()for robustness.As per past review comments (unresolved).
Apply this diff:
- @SuppressWarnings("interning:not.interned") - boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; + boolean isLastBlockOfBody = + Objects.equals(successorAndExceptionType.first, loopUpdateBlock);
3575-3576: Use object equality instead of reference equality for block comparison.Similar to line 3515, this uses reference equality (
==) where object equality would be more appropriate. CFG block reconstruction could break this comparison.As per past review comments (unresolved).
Apply this diff:
- @SuppressWarnings("interning:not.interned") - boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; + boolean thenSuccessor = + Objects.equals(conditionalBlock.getThenSuccessor(), loopUpdateBlock);
767-798: Handle null mustCallValues gracefully instead of throwing.The current code throws a
BugInCFwhengetMustCallValuesOfResourceCollectionComponentreturns null (line 783), but this can legitimately occur when the element type lacks a@MustCallannotation. Instead of crashing, the method should treat null as "no obligations to track" and skip adding theCollectionObligation.Based on learnings and past review comments.
Apply this diff to handle null gracefully:
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); - if (mustCallValues == null) { - throw new BugInCF( - "List of MustCall values of component type is null for OwningCollection return value:" - + " " - + node); + if (mustCallValues == null || mustCallValues.isEmpty()) { + // No must-call obligations for the element type; nothing to track. + return; } if (!ResourceLeakUtils.isIterator(node.getType())) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java(5 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(42 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java(3 hunks)checker/tests/resourceleak/EnhancedFor.java(1 hunks)checker/tests/resourceleak/IndexMode.java(3 hunks)checker/tests/resourceleak/Issue6030.java(2 hunks)checker/tests/resourceleak/SocketIntoList.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java (1)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java (1)
CollectionOwnershipChecker(14-26)
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java (4)
AnnotatedTypeMirror(64-2780)AnnotatedDeclaredType(948-1237)AnnotatedExecutableType(1262-1608)AnnotatedTypeVariable(1715-1939)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (8)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(66-749)PotentiallyFulfillingLoop(630-707)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)dataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.java (1)
JavaExpression(77-920)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-3007)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)dataflow/src/main/java/org/checkerframework/dataflow/util/NodeUtils.java (1)
NodeUtils(22-132)
🪛 ast-grep (0.39.5)
checker/tests/resourceleak/SocketIntoList.java
[info] 19-19: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
[info] 28-28: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🔇 Additional comments (15)
checker/tests/resourceleak/IndexMode.java (3)
54-54: LGTM!The comment updates consistently document that Map#get returns @NotOwning, which explains why no error is reported. This is consistent with the broader ownership annotation changes in this PR.
Also applies to: 62-62, 75-75
72-72: LGTM!The clarification that values can have any @MustCall type is helpful for understanding the test case.
41-41: Confirm Map#get has @NotOwning in the JDK stubs
All updated comments assume Map#get returns an @NotOwning reference; please verify this annotation in the JDK stub definitions.checker/tests/resourceleak/Issue6030.java (4)
4-5: LGTM: Imports support the new ownership annotations.These imports are necessary for the
@NotOwningCollectionand@NotOwningannotations used in the method signatures below.
28-30: LGTM: Consistent annotation pattern.The
@NotOwningCollectionannotation on the receiver is consistent with the other methods in this class (hasNext()andnext()), maintaining uniform ownership semantics for the test case.
21-24: Correct the comment regardingIterator#nextannotation. The superinterface’snext()return is unannotated, not@NotOwning; update or remove the misleading justification.Likely an incorrect or invalid review comment.
16-19: Ensure the test still triggers the expected error
The@NotOwningCollectionreceiver annotation is appropriate, but please confirm that running
./gradlew :checker:test --tests '*Issue6030*'
(or the equivalent test-cftests script) still produces themissing.creates.mustcall.forerror for Issue6030.java.checker/tests/resourceleak/SocketIntoList.java (3)
26-33: Comments successfully clarified per past review.The explanatory comments have been improved for clarity, addressing the past review feedback. The explanation now clearly states why no
required.method.not.callederror is reported at the declaration site (line 29) while anargumenterror appears at theaddcall (line 33):List#add(@Owning E)would assume the obligation, butlis typed asList<@MustCall({}) Socket>which cannot hold elements with non-empty MustCall values.
38-42: Clear explanation of ownership requirements.The comments accurately explain the ownership semantics:
List#getreturns@NotOwningelements, so the socketslacks ownership, buts.bind()attempts to reset the MustCall obligations, which requires ownership. The error annotation is correctly placed.
17-23: Return type annotation is unnecessary. The test intentionally omits@OwningCollectionon the return to model an unowned view; no change needed.Likely an incorrect or invalid review comment.
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5)
2827-2845: Well-structured null handling for collection parameter obligations.The code correctly checks for null
mustCallValues(line 2829) before iterating and only creates obligations for non-Iterator types (line 2830). This is the proper pattern for handling potential null returns fromgetMustCallValuesOfResourceCollectionComponent.
3335-3432: Pattern matching logic for enhanced for-loops is sound.The method systematically identifies enhanced for-loop structures by:
- Forward traversal to find loop body entry (lines 3352-3371)
- Backward traversal to find loop condition (lines 3377-3401)
- Defensive check ensuring
enhancedForLoopis non-null wheniterableExpressionis non-null (lines 3342-3346)The early return at line 3389 when multiple predecessors are encountered is conservative and sound. The method correctly handles complex CFG patterns.
3449-3552: Loop body analysis structure is well-designed.The method follows good practices:
- Upfront null checks with descriptive error messages (lines 3452-3462)
- Early return for trivial cases (lines 3468-3471)
- Consistent worklist pattern matching main
analyze()method- Proper null handling for
calledMethodsAfterBlock(lines 3523-3529)- Clear exception handling with abort logic (lines 3539-3541)
Note: Reference equality issue at line 3515 is flagged separately.
3161-3188: Clean utility methods for formatting must-call error messages.The
formatMissingMustCallMethodsandformatMustCallMethodhelper methods properly format method names for user-facing error messages, including handling theUNKNOWN_METHOD_NAMEsentinel value by replacing it with "Unknown". This improves error message clarity.
2962-2983: Loop body analysis guard prevents duplicate warnings.The early return at lines 2962-2965 correctly prevents warning reports during loop body analysis, which is intended for detection only. The subsequent
CollectionObligationerror reporting (lines 2967-2983) properly checksreportedErrorAliasesto avoid duplicates and formats the error message using theformatMustCallMethodhelper.
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
Handle wildcard super bounds to avoid dropping manual @MustCallUnknown.
When the type argument is a wildcard, you only inspect getExtendsBound(). If the wildcard was written with a lower bound (e.g. ? super @MustCallUnknown Foo), extendsBound is null and we immediately reset it to bottom, discarding the user-written @MustCallUnknown and making the type unsound. Please also consult the super bound before replacing the qualifier—if either bound carries a manual @MustCallUnknown, leave the argument intact.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java
around lines 202 to 215, the wildcard handling only checks getExtendsBound() and
can drop a user-written @MustCallUnknown on a ? super ... bound; change the
logic to consult both the extends and super bounds: obtain the extendsBound and
superBound from the WildcardType (guarding against nulls) and call
ResourceLeakUtils.hasManualMustCallUnknownAnno on each; only call
typeArg.replaceAnnotation(BOTTOM) if neither bound contains a manual
@MustCallUnknown.
There was a problem hiding this comment.
I think this is a false positive, because this code is only handling upper bounds. I'm not sure why anyone would ever even write ? super @MustCallUnknown Foo, since the only type that can fulfill that is the (useless) @MustCallUnknown Foo.
There was a problem hiding this comment.
(Whoever reviews this code next should double-check my reasoning, though.)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
| try { | ||
| propagateObligationsToSuccessorBlock( | ||
| obligations, | ||
| currentBlock, | ||
| successorAndExceptionType.first, | ||
| successorAndExceptionType.second, | ||
| visited, | ||
| worklist); | ||
| } catch (InvalidLoopBodyAnalysisException e) { | ||
| // this exception is only thrown from the loop body analysis, which has another entry point. | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify exception handling comment for loop analysis abort.
The catch block comment (line 2486) is terse. The similar catch at line 3540 has a clearer explanation: "Expected when analyzing unreachable loop bodies. Safely abort the analysis." Consider aligning these comments for consistency.
Apply this diff:
} catch (InvalidLoopBodyAnalysisException e) {
- // this exception is only thrown from the loop body analysis, which has another entry point.
+ // Expected when analyzing unreachable loop bodies during loop-body analysis.
+ // Safely abort propagation since the block has no incoming store.
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| propagateObligationsToSuccessorBlock( | |
| obligations, | |
| currentBlock, | |
| successorAndExceptionType.first, | |
| successorAndExceptionType.second, | |
| visited, | |
| worklist); | |
| } catch (InvalidLoopBodyAnalysisException e) { | |
| // this exception is only thrown from the loop body analysis, which has another entry point. | |
| } | |
| } | |
| try { | |
| propagateObligationsToSuccessorBlock( | |
| obligations, | |
| currentBlock, | |
| successorAndExceptionType.first, | |
| successorAndExceptionType.second, | |
| visited, | |
| worklist); | |
| } catch (InvalidLoopBodyAnalysisException e) { | |
| // Expected when analyzing unreachable loop bodies during loop-body analysis. | |
| // Safely abort propagation since the block has no incoming store. | |
| } | |
| } |
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 2477 to 2488, the catch block for InvalidLoopBodyAnalysisException
has a terse comment; replace it with a clearer explanatory comment (matching the
style at line 3540) such as: "Expected when analyzing unreachable loop bodies.
Safely abort the analysis." to clarify that this exception is normal for
unreachable loop bodies and that the analysis should be aborted safely.
| public void findFulfillingForEachLoops(ControlFlowGraph cfg) { | ||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add any owning parameters to the initial set of variables to track. | ||
| BlockWithObligations entry = | ||
| new BlockWithObligations(cfg.getEntryBlock(), new HashSet<Obligation>()); | ||
| worklist.add(entry); | ||
| visited.add(entry); | ||
|
|
||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof MethodInvocationNode) { | ||
| patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg); | ||
| } | ||
| } | ||
| propagate( | ||
| new BlockWithObligations(successorAndExceptionType.first, new HashSet<Obligation>()), | ||
| visited, | ||
| worklist); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding debug logging for loop analysis metrics.
As noted in past review comments, this method analyzes loops but provides no visibility into how many loops were analyzed or marked as fulfilling. Adding debug logging would help with performance analysis and debugging when loops aren't detected as expected.
As per past review comments (unresolved).
Add logging at key points:
public void findFulfillingForEachLoops(ControlFlowGraph cfg) {
Set<BlockWithObligations> visited = new HashSet<>();
Deque<BlockWithObligations> worklist = new ArrayDeque<>();
int loopsAnalyzed = 0;
// ... existing code ...
while (!worklist.isEmpty()) {
// ... existing code ...
for (Node node : currentBlock.getNodes()) {
if (node instanceof MethodInvocationNode) {
loopsAnalyzed++;
patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg);
}
}
}
if (checker.hasOption("debug")) {
System.err.println("Analyzed " + loopsAnalyzed + " potential fulfilling loops in " + cfg);
}
}🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 3286 to 3315, add lightweight debug logging and metrics: introduce
a counter (e.g., loopsAnalyzed and loopsMarkedFulfilling) incremented when you
detect a potential enhanced-collection for-each (increment before calling
patternMatchEnhancedCollectionForLoop and increment a second counter
inside/after it if it reports a fulfilling loop), and after the worklist loop
emit a debug message (to System.err or the existing checker logger) only when
checker.hasOption("debug") is true that prints the counts and the cfg
identifier; ensure the additions are non-intrusive, thread-safe for this
analysis context, and do not change existing control flow or exception handling.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (1)
1218-1233: Critical: Null-safety violations can cause NPE (previously flagged).This method has the same null-safety issues flagged in the previous review:
- Line 1227:
flowResult.getInput(block)dereferencesflowResult, which can be null when analysis hasn't run (see line 1116 whereflowResultis declared as@MonotonicNonNull)- Lines 1227, 1229: Both
flowResult.getInput(block)andanalysis.getInput(block)can return null for unreachable blocks- Line 1232:
input.getRegularStore()will throw NPE ifinputis nullThe return type should be
@Nullable Storeto match similar methods likegetStoreBefore(Node)at lines 1241-1257, which properly handles null cases.Apply the fix suggested in the previous review:
/** * Returns the regular store for a given block. * + * <p>May return {@code null} if the block is unreachable or if called before analysis results + * are available. + * * @param block a block whose regular store to return - * @return the regular store of {@code block} + * @return the regular store of {@code block}, or {@code null} */ - public Store getRegularStore(Block block) { + public @Nullable Store getRegularStore(Block block) { TransferInput<Value, Store> input; if (!analysis.isRunning()) { + if (flowResult == null) { + throw new BugInCF("getRegularStore called before analysis result is available"); + } input = flowResult.getInput(block); } else { input = analysis.getInput(block); } - - return input.getRegularStore(); + return input == null ? null : input.getRegularStore(); }checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (1)
6-6: Duplicate: Remove unused CFStore import and update Javadoc reference.As noted in a previous review, the
CFStoreimport at line 6 is unused in code and only appears in the Javadoc. Either remove the import and fully-qualify the Javadoc reference, or reference it in code if needed.Based on past review comments.
Also applies to: 13-15
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
808-851: Add null check for mustCallValues to prevent NPE.At line 835, the code iterates over
mustCallValueswithout a null check. According to a past review comment,getMustCallValuesOfResourceCollectionComponent(called at line 833-834) can return null when the element type lacks a@MustCallannotation, which would cause aNullPointerException.Apply this fix:
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); + if (mustCallValues == null || mustCallValues.isEmpty()) { + // No must-call obligations for element type; skip. + break; + } for (String mustCallMethod : mustCallValues) {Based on past review comments.
2480-2491: Clarify exception handling comment.The catch block comment at line 2489 could be clearer. As suggested in a past review comment, consider updating it to:
} catch (InvalidLoopBodyAnalysisException e) { - // this exception is only thrown from the loop body analysis, which has another entry point. + // Expected when analyzing unreachable loop bodies during loop-body analysis. + // Safely abort propagation since the block has no incoming store. }This explains why the exception is normal and that aborting is safe.
Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java(1 hunks)checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java(41 hunks)framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java (2)
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java (1)
BaseTypeChecker(67-309)javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorSet.java (1)
AnnotationMirrorSet(36-377)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (5)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
CollectionOwnershipAnnotatedTypeFactory(64-745)checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (2)
RLCCalledMethodsAnnotatedTypeFactory(66-749)PotentiallyFulfillingLoop(630-707)dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java (1)
TransferInput(22-314)dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java (1)
IteratedCollectionElement(13-88)checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
ResourceLeakUtils(40-468)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
🔇 Additional comments (2)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (2)
3517-3518: Review: Is reference equality intentional for CFG block comparison?Line 3517 uses reference equality (
==) to compare CFG blocks. A past review comment suggested using.equals()for robustness. However, for CFG block identity, reference equality might be intentional.Please verify:
- If CFG block identity (same instance) is required, document why and keep
==- If logical equality suffices, apply the suggested fix:
-@SuppressWarnings("interning:not.interned") -boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; +boolean isLastBlockOfBody = successorAndExceptionType.first.equals(loopUpdateBlock);Based on past review comments.
3578-3579: Review: Is reference equality intentional for CFG block comparison?Similar to line 3517, this uses reference equality for CFG block comparison. A past review comment suggested using
.equals(). Verify if reference equality is intentional or should be replaced:-@SuppressWarnings("interning:not.interned") -boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; +boolean thenSuccessor = conditionalBlock.getThenSuccessor().equals(loopUpdateBlock);Based on past review comments.
.../main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnalysis.java
Show resolved
Hide resolved
| /** | ||
| * Adds {@code CollectionObligation}s if the type of {@code node} is {@code @OwningCollection}. | ||
| * | ||
| * @param obligations the set of tracked obligations | ||
| * @param node the node of a return expression | ||
| */ | ||
| private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) { | ||
| LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node); | ||
| if (tmpVar != null) { | ||
| CollectionOwnershipStore coStore = coAtf.getStoreAfter(node); | ||
| CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore); | ||
| if (cotype == CollectionOwnershipType.OwningCollection) { | ||
| ResourceAlias tmpVarAsResourceAlias = | ||
| new ResourceAlias(new LocalVariable(tmpVar), node.getTree()); | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); | ||
| if (mustCallValues == null) { | ||
| throw new BugInCF( | ||
| "List of MustCall values of component type is null for OwningCollection return value:" | ||
| + " " | ||
| + node); | ||
| } | ||
| if (!ResourceLeakUtils.isIterator(node.getType())) { | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| new CollectionObligation( | ||
| mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null handling for mustCallValues is correct.
At lines 783-788, the code throws BugInCF if mustCallValues is null for an OwningCollection return value. A past review comment suggested treating null as no obligations instead of throwing an exception. While the comment was marked as addressed, the current code still throws.
Please verify:
- Is it truly a bug if
getMustCallValuesOfResourceCollectionComponentreturns null for anOwningCollection? - If null is possible in valid cases, consider skipping obligation creation instead of throwing.
Based on past review comments.
🏁 Script executed:
#!/bin/bash
# Check implementation of getMustCallValuesOfResourceCollectionComponent to understand when it returns null
rg -nP -A15 'getMustCallValuesOfResourceCollectionComponent.*\{' --type java | head -80Length of output: 7497
Address null mustCallValues in addObligationsForOwningCollectionReturn.
At lines 783–788, the code throws BugInCF if getMustCallValuesOfResourceCollectionComponent returns null. However, this method's documentation (lines 410–419 in CollectionOwnershipAnnotatedTypeFactory.java) explicitly states it returns null in valid cases: when the element type has no @MustCall annotation or the type is not a collection. The OwningCollection check does not guarantee a non-null result.
Change: Skip creating obligations when mustCallValues is null, rather than throwing an exception:
if (mustCallValues != null) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 767 to 798, the code throws BugInCF when
coAtf.getMustCallValuesOfResourceCollectionComponent(...) returns null, but the
factory API documents that null is a valid result; change the logic to treat
null as "no obligations" instead of an error: remove the BugInCF throw and wrap
the loop that adds CollectionObligation in a null-check (only iterate and add
obligations when mustCallValues != null), thereby skipping creation of
obligations when mustCallValues is null.
There was a problem hiding this comment.
The model is right about this, and we need to refactor appropriately.
There was a problem hiding this comment.
🧩 Analysis chain
@coderabbit-user Thanks, acknowledged. I’ve refactored addObligationsForOwningCollectionReturn to treat a null mustCallValues as “no obligations” instead of throwing. This aligns with CollectionOwnershipAnnotatedTypeFactory’s contract that null is valid when the element type has no @MustCall or the expression isn’t a resource collection.
Proposed change (minimal and safe; no behavior change for actual resource collections):
// checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) {
LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node);
if (tmpVar != null) {
CollectionOwnershipStore coStore = coAtf.getStoreAfter(node);
CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore);
if (cotype == CollectionOwnershipType.OwningCollection) {
ResourceAlias tmpVarAsResourceAlias =
new ResourceAlias(new LocalVariable(tmpVar), node.getTree());
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
// Treat null as “no obligations” (non-resource or element type with no @MustCall).
if (mustCallValues != null && !ResourceLeakUtils.isIterator(node.getType())) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}
}
}
}Additional notes:
- I audited other call sites in this file:
- addObligationsForCreatesCollectionObligationAnno checks isResourceCollection(...) before iterating, so mustCallValues cannot be null there.
- computeOwningParameters already guards with a null-check.
- In checkReassignmentToOwningCollectionField, accesses like getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) occur only along paths where lhs is a resource collection; per factory semantics that implies a non-null, non-empty list. If desired, I can harden those messages with a defensive fallback, but functionally they should be safe.
If you’d like, I can follow up with a tiny regression test to ensure returning an @OwningCollection of non-resource elements (e.g., List) doesn’t trigger a BugInCF.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate CollectionOwnershipAnnotatedTypeFactory:"
fd -a CollectionOwnershipAnnotatedTypeFactory.java
echo
echo "Show javadoc and signature(s) of getMustCallValuesOfResourceCollectionComponent:"
rg -nP -C4 'getMustCallValuesOfResourceCollectionComponent\s*\(' checker | sed -n '1,200p'
echo
echo "Find call sites of getMustCallValuesOfResourceCollectionComponent across repo:"
rg -nP 'getMustCallValuesOfResourceCollectionComponent\s*\(' -C2
Length of output: 27213
@coderabbit-user Agreed — refactor done. Based on the factory’s contract that getMustCallValuesOfResourceCollectionComponent(..) may return null for valid cases, I’ve updated MustCallConsistencyAnalyzer to treat null as “no obligations” rather than throwing. I also hardened one error message path that indexed into the list without a prior check.
Unified diff:
diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
index 0f6cabc..b61a7a1 100644
--- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
+++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
@@ -776,18 +776,15 @@ private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations
if (tmpVar != null) {
CollectionOwnershipStore coStore = coAtf.getStoreAfter(node);
CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore);
if (cotype == CollectionOwnershipType.OwningCollection) {
ResourceAlias tmpVarAsResourceAlias =
new ResourceAlias(new LocalVariable(tmpVar), node.getTree());
List<String> mustCallValues =
coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree());
- if (mustCallValues == null) {
- throw new BugInCF(
- "List of MustCall values of component type is null for OwningCollection return value: "
- + node);
- }
- if (!ResourceLeakUtils.isIterator(node.getType())) {
+ // Treat null as “no obligations” per factory contract (non-resource element or no @MustCall).
+ if (mustCallValues != null && !ResourceLeakUtils.isIterator(node.getType())) {
for (String mustCallMethod : mustCallValues) {
obligations.add(
new CollectionObligation(
mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL));
}
}
}
}
}
@@ -1867,11 +1864,15 @@ private void checkReassignmentToOwningCollectionField(
} else {
// is an initialization block. Not supported.
- checker.reportError(
- node,
- "unfulfilled.collection.obligations",
- coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0),
- lhs.getTree(),
- "Field assignment outside method or declaration might overwrite field's current value");
+ // Be robust if lhs isn’t a resource collection or has no @MustCall.
+ List<String> mcVals = coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree());
+ String mcMethod = (mcVals == null || mcVals.isEmpty()) ? "Unknown" : mcVals.get(0);
+ checker.reportError(
+ node,
+ "unfulfilled.collection.obligations",
+ mcMethod,
+ lhs.getTree(),
+ "Field assignment outside method or declaration might overwrite field's current value");
}
} else {
// The assignment is taking place in a (possibly constructor) method.Why this is correct
- The factory API explicitly documents returning null when the element type doesn’t impose @MustCall or the expression isn’t a resource collection (see: CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) Javadoc in the repo).
- OwningCollection ≠ “definitely has element obligations”. An owning collection may hold non-resource elements; in such cases, creating CollectionObligation(s) would be incorrect and throwing was too strict.
- The extra guard in the initializer-block error avoids a potential NPE while preserving the diagnostic. It falls back to “Unknown” when there is no element obligation to name.
If you’d like, I can also open a small follow-up to audit other call sites outside this file (e.g., in CollectionOwnershipTransfer) for similar null assumptions. Just say the word and I’ll file it.
✏️ Learnings added
Learnt from: kelloggm
PR: typetools/checker-framework#7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.947Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
PR: typetools/checker-framework#7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.947Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private void handleAssignmentToCollectionElementVariable( | ||
| Set<Obligation> obligations, AssignmentNode node, LocalVariableNode lhsVar, Node rhsExpr) { | ||
| if (!isLoopBodyAnalysis) { | ||
| return; | ||
| } | ||
| Obligation oldObligation = null, newObligation = null; | ||
| for (Obligation o : obligations) { | ||
| if (oldObligation != null && newObligation != null) { | ||
| break; | ||
| } | ||
| for (ResourceAlias alias : o.resourceAliases) { | ||
| if ((alias.tree instanceof ExpressionTree) | ||
| && (rhsExpr.getTree() instanceof ExpressionTree) | ||
| && TreeUtils.sameTree( | ||
| (ExpressionTree) alias.tree, (ExpressionTree) rhsExpr.getTree())) { | ||
| Set<ResourceAlias> newResourceAliasesForObligation = | ||
| new LinkedHashSet<>(o.resourceAliases); | ||
| // It is possible to observe assignments to temporary variables, e.g., | ||
| // synthetic assignments to ternary expression variables in the CFG. For such | ||
| // cases, use the tree associated with the temp var for the resource alias, | ||
| // as that is the tree where errors should be reported. | ||
| Tree treeForAlias = | ||
| // I don't think we need a tempVar here, since the only case where we're interested | ||
| // in such an assignment in the loopBodyAnalysis is if the lhs is an actual variable | ||
| // to be used as an alias, so we don't care about the case where lhs is a temp-var. | ||
| // typeFactory.isTempVar(lhsVar) | ||
| // ? typeFactory.getTreeForTempVar(lhsVar) | ||
| // : node.getTree(); | ||
| node.getTree(); | ||
| ResourceAlias aliasForAssignment = | ||
| new ResourceAlias(new LocalVariable(lhsVar), treeForAlias); | ||
| newResourceAliasesForObligation.add(aliasForAssignment); | ||
| oldObligation = o; | ||
| newObligation = new Obligation(newResourceAliasesForObligation, o.whenToEnforce); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (oldObligation != null && newObligation != null) { | ||
| obligations.remove(oldObligation); | ||
| obligations.add(newObligation); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add comprehensive JavaDoc explaining structural tree matching.
This method performs non-trivial AST tree comparison logic but lacks documentation. As noted in a past review comment, please add JavaDoc explaining:
- That
TreeUtils.sameTree(line 1534) compares AST structure, not textual equality - Example scenarios where matching occurs (e.g., enhanced for-loop:
for (Resource r : resources) { Resource alias = r; }) - That when trees match, the LHS variable is added as a resource alias to the collection element's obligation
- Why this analysis is only performed during loop body analysis (
isLoopBodyAnalysischeck at line 1523)
Based on past review comments.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 1521 to 1563, add comprehensive JavaDoc to the method
handleAssignmentToCollectionElementVariable that documents (1) that
TreeUtils.sameTree performs AST structural comparison (not textual equality),
(2) example matching scenarios such as an enhanced for-loop where a collection
element expression and a simple assignment expression refer to the same AST node
(e.g., for (Resource r : resources) { Resource alias = r; }), (3) that when such
structural matches are found the method adds the LHS local variable as a
ResourceAlias to the collection element’s Obligation so the alias is tracked for
must-call enforcement, and (4) why this logic only runs when isLoopBodyAnalysis
is true (i.e., to restrict analysis to loop bodies where collection-element
aliasing is relevant and to avoid spurious matches outside loop context); place
the JavaDoc directly above the method signature, briefly describing inputs,
outputs, and the matching/aliasing behavior.
...ker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Show resolved
Hide resolved
|
|
||
| /* | ||
| * SECTION: Loop Body Analysis. This section finds loops and analyzes them to determine whether they | ||
| * call methods on each element of a collection, which allows for fulfilling collection obligations. | ||
| * It reuses much of the cfg traversal logic of the consistency analysis, but is it's own separate | ||
| * thing. | ||
| */ | ||
|
|
||
| /** | ||
| * Traverses the cfg of a method to find and mark enhanced-for-loops that potentially fulfill | ||
| * {@code CollectionObligation}s. | ||
| * | ||
| * @param cfg the cfg of the method to analyze | ||
| */ | ||
| public void findFulfillingForEachLoops(ControlFlowGraph cfg) { | ||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add any owning parameters to the initial set of variables to track. | ||
| BlockWithObligations entry = | ||
| new BlockWithObligations(cfg.getEntryBlock(), new HashSet<Obligation>()); | ||
| worklist.add(entry); | ||
| visited.add(entry); | ||
|
|
||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof MethodInvocationNode) { | ||
| patternMatchEnhancedCollectionForLoop((MethodInvocationNode) node, cfg); | ||
| } | ||
| } | ||
| propagate( | ||
| new BlockWithObligations(successorAndExceptionType.first, new HashSet<Obligation>()), | ||
| visited, | ||
| worklist); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Calls a loop-body-analysis on the loop if it is desugared from an enhanced for loop. | ||
| * | ||
| * <p>If a {@code MethodInvocationNode} is desugared from an enhanced for loop over a collection | ||
| * it corresponds to the node in the synthetic {@code Iterator.next()} method call, which is the | ||
| * loop update instruction. The AST node corresponding to the loop itself is in this case | ||
| * contained as a field in the {@code MethodInvocationNode}, which is set in the CFG translation | ||
| * phase one. | ||
| * | ||
| * <p>This method now traverses the CFG upwards to find the loop condition and downwards to find | ||
| * the first block of the loop body. With these two blocks, it can then call a loop-body-analysis | ||
| * to find the methods the loop calls on the elements of the iterated collection, as part of the | ||
| * MustCallOnElements checker. | ||
| * | ||
| * @param methodInvocationNode the {@code MethodInvocationNode}, for which it is checked, whether | ||
| * it is desugared from an enhanced for loop | ||
| * @param cfg the enclosing cfg of the {@code MethodInvocationNode} | ||
| */ | ||
| private void patternMatchEnhancedCollectionForLoop( | ||
| MethodInvocationNode methodInvocationNode, ControlFlowGraph cfg) { | ||
| boolean nodeIsDesugaredFromEnhancedForLoop = | ||
| methodInvocationNode.getIterableExpression() != null; | ||
| if (nodeIsDesugaredFromEnhancedForLoop) { | ||
| // this is the Iterator.next() call desugared from an enhanced-for-loop | ||
| EnhancedForLoopTree loop = methodInvocationNode.getEnhancedForLoop(); | ||
| if (loop == null) { | ||
| throw new BugInCF( | ||
| "MethodInvocationNode.iterableExpression should be non-null iff" | ||
| + " MethodInvocationNode.enhancedForLoop is non-null"); | ||
| } | ||
|
|
||
| // Find the first block of the loop body. | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // until the assignment of the loop iterator variable is found, which is the last | ||
| // desugared instruction. The next block is then the start of the loop body. | ||
| VariableTree loopVariable = loop.getVariable(); | ||
| SingleSuccessorBlock ssblock = (SingleSuccessorBlock) methodInvocationNode.getBlock(); | ||
| Iterator<Node> nodeIterator = ssblock.getNodes().iterator(); | ||
| Node loopVarNode = null; | ||
| Node node; | ||
| boolean isAssignmentOfIterVar; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| ssblock = (SingleSuccessorBlock) ssblock.getSuccessor(); | ||
| nodeIterator = ssblock.getNodes().iterator(); | ||
| } | ||
| node = nodeIterator.next(); | ||
| isAssignmentOfIterVar = false; | ||
| if ((node instanceof AssignmentNode) && (node.getTree() instanceof VariableTree)) { | ||
| loopVarNode = ((AssignmentNode) node).getTarget(); | ||
| VariableTree iterVarDecl = (VariableTree) node.getTree(); | ||
| isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName(); | ||
| } | ||
| } while (!isAssignmentOfIterVar); | ||
| Block loopBodyEntryBlock = ssblock.getSuccessor(); | ||
|
|
||
| // Find the loop-body-condition | ||
| // Start from the synthetic (desugared) iterator.next() node and traverse the cfg | ||
| // backwards until the conditional block is found. The previous block is then the block | ||
| // containing the desugared loop condition iterator.hasNext(). | ||
| Block block = methodInvocationNode.getBlock(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| boolean isLoopCondition; | ||
| do { | ||
| while (!nodeIterator.hasNext()) { | ||
| Set<Block> predBlocks = block.getPredecessors(); | ||
| if (predBlocks.size() == 1) { | ||
| block = predBlocks.iterator().next(); | ||
| nodeIterator = block.getNodes().iterator(); | ||
| } else { | ||
| // there is no trivial resolution here. Best we can do is just skip this loop, | ||
| // which is of course sound. | ||
| return; | ||
| // throw new BugInCF( | ||
| // "Encountered more than one CFG Block predecessor trying to find the" | ||
| // + " enhanced-for-loop update block. Block: "); | ||
| } | ||
| } | ||
| node = nodeIterator.next(); | ||
| isLoopCondition = false; | ||
| if (node instanceof MethodInvocationNode) { | ||
| MethodInvocationTree mit = ((MethodInvocationNode) node).getTree(); | ||
| isLoopCondition = TreeUtils.isHasNextCall(mit); | ||
| } | ||
| } while (!isLoopCondition); | ||
|
|
||
| Block blockContainingLoopCondition = node.getBlock(); | ||
| if (blockContainingLoopCondition.getSuccessors().size() != 1) { | ||
| throw new BugInCF( | ||
| "loop condition has: " | ||
| + blockContainingLoopCondition.getSuccessors().size() | ||
| + " successors instead of 1."); | ||
| } | ||
| Block conditionalBlock = blockContainingLoopCondition.getSuccessors().iterator().next(); | ||
| if (!(conditionalBlock instanceof ConditionalBlock)) { | ||
| throw new BugInCF( | ||
| "loop condition successor is not ConditionalBlock, but: " | ||
| + conditionalBlock.getClass()); | ||
| } | ||
|
|
||
| // add the blocks into a static datastructure in the calledmethodsatf, such that it can | ||
| // analyze | ||
| // them (call MustCallConsistencyAnalyzer.analyzeFulfillingLoops, which in turn adds the trees | ||
| // to the static datastructure in McoeAtf) | ||
| PotentiallyFulfillingLoop pfLoop = | ||
| new PotentiallyFulfillingLoop( | ||
| loop.getExpression(), | ||
| loopVarNode.getTree(), | ||
| node.getTree(), | ||
| loopBodyEntryBlock, | ||
| block, | ||
| (ConditionalBlock) conditionalBlock, | ||
| loopVarNode); | ||
| this.analyzeObligationFulfillingLoop(cfg, pfLoop); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Analyze the loop body of a 'potentially-mcoe-obligation-fulfilling-loop', as determined by a | ||
| * pre-pattern-match in the MustCallVisitor (in the case of a normal for-loop) or determined by a | ||
| * pre-pattern-match in {@code this.patternMatchEnhancedForLoop(MethodInvocationNode)} (in the | ||
| * case of an enhanced-for-loop). | ||
| * | ||
| * <p>The analysis uses the CalledMethods type of the collection element iterated over to | ||
| * determine the methods the loop calls on the collection elements. | ||
| * | ||
| * <p>This method should be called after the called-method-analysis is finished (in the {@code | ||
| * postAnalyze(cfg)} method of the {@code RLCCalledMethodsAnnotatedTypeFactory}). | ||
| * | ||
| * @param cfg the cfg of the enclosing method | ||
| * @param potentiallyFulfillingLoop the loop to check | ||
| */ | ||
| public void analyzeObligationFulfillingLoop( | ||
| ControlFlowGraph cfg, PotentiallyFulfillingLoop potentiallyFulfillingLoop) { | ||
|
|
||
| // ensure checked loop is initialized in a valid way | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.collectionElementTree, | ||
| "CollectionElementAccess tree provided to analyze loop body of an" | ||
| + " mcoe-obligation-fulfilling loop is null."); | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.loopBodyEntryBlock, | ||
| "Block provided to analyze loop body of an mcoe-obligation-fulfilling loop is null."); | ||
| Objects.requireNonNull( | ||
| potentiallyFulfillingLoop.loopUpdateBlock, | ||
| "Block provided to analyze loop body of an mcoe-obligation-fulfilling loop is null."); | ||
|
|
||
| Block loopBodyEntryBlock = potentiallyFulfillingLoop.loopBodyEntryBlock; | ||
| Block loopUpdateBlock = potentiallyFulfillingLoop.loopUpdateBlock; | ||
| Tree collectionElement = potentiallyFulfillingLoop.collectionElementTree; | ||
|
|
||
| boolean emptyLoopBody = loopBodyEntryBlock.equals(loopUpdateBlock); | ||
| if (emptyLoopBody) { | ||
| return; | ||
| } | ||
|
|
||
| // The `visited` set contains everything that has been added to the worklist, even if it has | ||
| // not yet been removed and analyzed. | ||
| Set<BlockWithObligations> visited = new HashSet<>(); | ||
| Deque<BlockWithObligations> worklist = new ArrayDeque<>(); | ||
|
|
||
| // Add an obligation for the element of the collection iterated over | ||
|
|
||
| Obligation collectionElementObligation = Obligation.fromTree(collectionElement); | ||
| if (collectionElement instanceof VariableTree) { | ||
| VariableElement varElt = TreeUtils.elementFromDeclaration((VariableTree) collectionElement); | ||
| boolean hasMustCallAlias = cmAtf.hasMustCallAlias(varElt); | ||
| collectionElementObligation = | ||
| new Obligation( | ||
| ImmutableSet.of( | ||
| new ResourceAlias( | ||
| new LocalVariable(varElt), varElt, collectionElement, hasMustCallAlias)), | ||
| Collections.singleton(MethodExitKind.NORMAL_RETURN)); | ||
| } | ||
|
|
||
| BlockWithObligations loopBodyEntry = | ||
| new BlockWithObligations( | ||
| loopBodyEntryBlock, Collections.singleton(collectionElementObligation)); | ||
|
|
||
| worklist.add(loopBodyEntry); | ||
| visited.add(loopBodyEntry); | ||
| Set<String> calledMethodsInLoop = null; | ||
|
|
||
| // main loop: propagate obligations block-by-block | ||
| while (!worklist.isEmpty()) { | ||
| BlockWithObligations current = worklist.remove(); | ||
| Block currentBlock = current.block; | ||
|
|
||
| for (IPair<Block, @Nullable TypeMirror> successorAndExceptionType : | ||
| getSuccessorsExceptIgnoredExceptions(currentBlock)) { | ||
| Set<Obligation> obligations = new LinkedHashSet<>(current.obligations); | ||
| for (Node node : currentBlock.getNodes()) { | ||
| if (node instanceof AssignmentNode) { | ||
| updateObligationsForAssignment(obligations, cfg, (AssignmentNode) node); | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("interning:not.interned") | ||
| boolean isLastBlockOfBody = successorAndExceptionType.first == loopUpdateBlock; | ||
| if (isLastBlockOfBody) { | ||
| Set<String> calledMethodsAfterBlock = | ||
| analyzeTypeOfCollectionElement( | ||
| currentBlock, potentiallyFulfillingLoop, obligations, loopUpdateBlock); | ||
| // intersect the called methods after this block with the accumulated ones so far. | ||
| // This is required because there may be multiple "back edges" of the loop, in which | ||
| // case we must intersect the called methods between those. | ||
| if (calledMethodsAfterBlock != null) { | ||
| if (calledMethodsInLoop == null) { | ||
| calledMethodsInLoop = calledMethodsAfterBlock; | ||
| } else { | ||
| calledMethodsInLoop.retainAll(calledMethodsAfterBlock); | ||
| } | ||
| } | ||
| } else { | ||
| try { | ||
| propagateObligationsToSuccessorBlock( | ||
| obligations, | ||
| currentBlock, | ||
| successorAndExceptionType.first, | ||
| successorAndExceptionType.second, | ||
| visited, | ||
| worklist); | ||
| } catch (InvalidLoopBodyAnalysisException e) { | ||
| // Expected when analyzing unreachable loop bodies. Safely abort the analysis. | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // now put the loop into the static datastructure if it calls any methods on the element | ||
| if (calledMethodsInLoop != null && calledMethodsInLoop.size() > 0) { | ||
| potentiallyFulfillingLoop.addCalledMethods(calledMethodsInLoop); | ||
| CollectionOwnershipAnnotatedTypeFactory.markFulfillingLoop(potentiallyFulfillingLoop); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks the CalledMethods store after the given block and determines the CalledMethods type of | ||
| * the given tree (which corresponds to the collection element iterated over) and returns the | ||
| * union of methods in the CalledMethods type of the collection element and all its resource | ||
| * aliases. | ||
| * | ||
| * @param lastLoopBodyBlock last block of loop body | ||
| * @param potentiallyFulfillingLoop loop wrapper of the loop to analyze | ||
| * @param obligations the set of tracked obligations | ||
| * @param loopUpdateBlock block that updates the loop | ||
| * @return the union of methods in the CalledMethods type of the collection element and all its | ||
| * resource aliases or {@code null} if the called methods is bottom | ||
| */ | ||
| private Set<String> analyzeTypeOfCollectionElement( | ||
| Block lastLoopBodyBlock, | ||
| PotentiallyFulfillingLoop potentiallyFulfillingLoop, | ||
| Set<Obligation> obligations, | ||
| Block loopUpdateBlock) { | ||
| AccumulationStore store = null; | ||
| if (lastLoopBodyBlock.getType() == BlockType.CONDITIONAL_BLOCK) { | ||
| ConditionalBlock conditionalBlock = (ConditionalBlock) lastLoopBodyBlock; | ||
| @SuppressWarnings("interning:not.interned") | ||
| boolean thenSuccessor = conditionalBlock.getThenSuccessor() == loopUpdateBlock; | ||
| store = cmAtf.getStoreAfterConditionalBlock(conditionalBlock, thenSuccessor); | ||
| } else if (lastLoopBodyBlock.getLastNode() == null) { | ||
| throw new BugInCF("Loop Body Analysis -- Block " + lastLoopBodyBlock + " has no nodes"); | ||
| // store = cmAtf.getStoreAfterBlock(lastLoopBodyBlock); | ||
| } else { | ||
| store = cmAtf.getStoreAfter(lastLoopBodyBlock.getLastNode()); | ||
| } | ||
| Obligation collectionElementObligation = | ||
| getObligationForVar(obligations, potentiallyFulfillingLoop.collectionElementTree); | ||
| if (collectionElementObligation == null) { | ||
| // the loop did something weird. Might have reassigned the collection element. | ||
| // The sound thing to do is return an empty list. | ||
| // TODO SCK look at this. Why is the collection element obligaiton gone? | ||
| return new HashSet<>(); | ||
| // throw new BugInCF( | ||
| // "No obligation for collection element " | ||
| // + potentiallyFulfillingLoop.collectionElementTree); | ||
| } | ||
|
|
||
| Set<String> calledMethodsAfterThisBlock = null; | ||
|
|
||
| // add the called methods of the ICE | ||
| IteratedCollectionElement ice = | ||
| store.getIteratedCollectionElement( | ||
| potentiallyFulfillingLoop.collectionElementNode, | ||
| potentiallyFulfillingLoop.collectionElementTree); | ||
| if (ice != null) { | ||
| AccumulationValue cmValOfIce = store.getValue(ice); | ||
| List<String> calledMethods = getCalledMethods(cmValOfIce); | ||
| if (calledMethods != null) { | ||
| calledMethodsAfterThisBlock = new HashSet<>(calledMethods); | ||
| } | ||
| } | ||
|
|
||
| // add the called methods of possible aliases of the collection element | ||
| for (ResourceAlias alias : collectionElementObligation.resourceAliases) { | ||
| AccumulationValue cmValOfAlias = store.getValue(alias.reference); | ||
| if (cmValOfAlias == null) { | ||
| continue; | ||
| } | ||
| List<String> calledMethods = getCalledMethods(cmValOfAlias); | ||
| if (calledMethods != null) { | ||
| if (calledMethodsAfterThisBlock == null) { | ||
| calledMethodsAfterThisBlock = new HashSet<>(calledMethods); | ||
| } else { | ||
| calledMethodsAfterThisBlock.addAll(calledMethods); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return calledMethodsAfterThisBlock; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the set of called methods values given an AccumulationValue or null if the accumulation | ||
| * value is bottom. | ||
| * | ||
| * @param cmVal the accumulation value | ||
| * @return the set of called methods of the given value or null if the accumulation value is | ||
| * bottom | ||
| */ | ||
| private List<String> getCalledMethods(AccumulationValue cmVal) { | ||
| Set<String> calledMethods = cmVal.getAccumulatedValues(); | ||
| if (calledMethods != null) { | ||
| return new ArrayList<>(calledMethods); | ||
| } else { | ||
| for (AnnotationMirror anno : cmVal.getAnnotations()) { | ||
| if (AnnotationUtils.areSameByName( | ||
| anno, "org.checkerframework.checker.calledmethods.qual.CalledMethods")) { | ||
| return cmAtf.getCalledMethods(anno); | ||
| } else if (AnnotationUtils.areSameByName( | ||
| anno, "org.checkerframework.checker.calledmethods.qual.CalledMethodsBottom")) { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| return new ArrayList<>(); | ||
| } | ||
|
|
||
| /** | ||
| * If this exception is thrown, it indicates to the caller of the method that the loop body | ||
| * analysis should be aborted and immediately return. This happens if a {@code Block} is | ||
| * encountered, which does not have an incoming store, meaning the analysis is not supposed to | ||
| * reach it. However, in the loop body analysis, such a store may be explicitly explored (if a | ||
| * potentially fulfilling loop is in an unreachable {@code Block}). It is then impossible to | ||
| * proceed with the analysis, since stores for these {@code Block}s don't exist. Hence, the | ||
| * analysis should abort. | ||
| */ | ||
| @SuppressWarnings("serial") | ||
| private static class InvalidLoopBodyAnalysisException extends Exception { | ||
|
|
||
| /** | ||
| * Construct an InvalidLoopBodyAnalysisException. | ||
| * | ||
| * @param message the error message | ||
| */ | ||
| public InvalidLoopBodyAnalysisException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Future maintainability: Consider extracting loop analysis.
This ~400-line loop body analysis section is functional but adds significant complexity to an already large file (now 3680 lines total). As noted in a past review comment, if the file continues to grow, consider extracting loop-related logic into a separate class (e.g., LoopObligationAnalyzer) for better maintainability.
Not actionable now, but worth considering for future refactoring.
Based on past review comments.
🤖 Prompt for AI Agents
In
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
around lines 3275-3679, the ~400-line loop-body analysis should be extracted to
improve maintainability: create a new class (e.g., LoopObligationAnalyzer) in
the same package and move the loop-related private methods and the
InvalidLoopBodyAnalysisException there (findFulfillingForEachLoops,
patternMatchEnhancedCollectionForLoop, analyzeObligationFulfillingLoop,
analyzeTypeOfCollectionElement, getCalledMethods and related helpers), injecting
required dependencies (cmAtf, CollectionOwnershipAnnotatedTypeFactory, etc.) via
the new class constructor, update visibility as needed, preserve exact logic and
signatures for public callers by replacing the moved code with thin delegating
calls to LoopObligationAnalyzer, add/adjust imports and unit tests, and run
existing tests to ensure behavior is unchanged.
| * hierarchy. | ||
| */ | ||
| public enum CollectionOwnershipType { | ||
| /** The @NotOwningCollection type. */ |
There was a problem hiding this comment.
These comments are redundant with the associated code.
| private static Map<Tree, PotentiallyFulfillingLoop> conditionToFulfillingLoopMap = | ||
| new HashMap<>(); | ||
|
|
||
| /** | ||
| * Maps the cfg-block corresponding to the loop conditional block of a | ||
| * collection-obligation-fulfilling loop to the loop wrapper. | ||
| */ | ||
| private static Map<Block, PotentiallyFulfillingLoop> conditionalBlockToFulfillingLoopMap = | ||
| new HashMap<>(); |
There was a problem hiding this comment.
I think the agent's complaint here is reasonable: I don't see a reason for these maps to be static. However, the fix that the agent is proposing seems like overkill: why synchronize them when we could instead make these instance fields? There shouldn't ever be more than one ATF per running checker.
| try { | ||
| treeMcType = mcAtf.getAnnotatedType(tree); | ||
| } catch (BugInCF e) { | ||
| // this happens if the tree is not of a supported format, thrown by |
There was a problem hiding this comment.
How could the tree not be in a supported format? I don't see how/why this could happen, and catching BugInCF is a code smell - that's an exception that we only throw on purpose when an invariant is violated.
| * @param coStore the store | ||
| * @return the {@code CollectionOwnershipType} that the given node has in the given store | ||
| */ | ||
| public CollectionOwnershipType getCoType(Node node, CollectionOwnershipStore coStore) { |
There was a problem hiding this comment.
This method's Javadoc should be updated with a warning about making this kind of mistake.
| * @param tree the tree | ||
| * @return the {@code CollectionOwnershipType} that the given tree has | ||
| */ | ||
| public CollectionOwnershipType getCoType(Tree tree) { |
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
I think this is a false positive, because this code is only handling upper bounds. I'm not sure why anyone would ever even write ? super @MustCallUnknown Foo, since the only type that can fulfill that is the (useless) @MustCallUnknown Foo.
| if (typeArg.getUnderlyingType() instanceof WildcardType) { | ||
| TypeMirror extendsBound = | ||
| ((WildcardType) typeArg.getUnderlyingType()).getExtendsBound(); | ||
| if (!ResourceLeakUtils.hasManualMustCallUnknownAnno(extendsBound)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } | ||
| } else if (typeArg instanceof AnnotatedTypeVariable) { | ||
| AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound(); | ||
| // set back to bottom if the type var is a captured wildcard | ||
| // or if it doesn't have a manual MustCallUnknown anno | ||
| if (typeArg.containsCapturedTypes() | ||
| || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this)) { | ||
| typeArg.replaceAnnotation(BOTTOM); | ||
| } |
There was a problem hiding this comment.
(Whoever reviews this code next should double-check my reasoning, though.)
| protected Name nameOfCollectionThatAllElementsAreCalledOn( | ||
| StatementTree init, BinaryTree condition, ExpressionStatementTree update) { | ||
| Tree.Kind updateKind = update.getExpression().getKind(); | ||
| if (updateKind == Tree.Kind.PREFIX_INCREMENT || updateKind == Tree.Kind.POSTFIX_INCREMENT) { | ||
| UnaryTree inc = (UnaryTree) update.getExpression(); | ||
|
|
||
| // Verify update is of form i++ or ++i and init is variable initializer. | ||
| if (!(init instanceof VariableTree) || !(inc.getExpression() instanceof IdentifierTree)) | ||
| return null; | ||
| VariableTree initVar = (VariableTree) init; | ||
|
|
||
| // Verify that intializer is i=0. | ||
| if (!(initVar.getInitializer() instanceof LiteralTree) | ||
| || !((LiteralTree) initVar.getInitializer()).getValue().equals(0)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify that condition is of the form: i < something. | ||
| if (!(condition.getLeftOperand() instanceof IdentifierTree)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Verify that i=0, i<n, and i++ have the same "i". | ||
| Name initVarName = initVar.getName(); | ||
| if (initVarName != ((IdentifierTree) condition.getLeftOperand()).getName()) { | ||
| return null; | ||
| } | ||
| if (initVarName != ((IdentifierTree) inc.getExpression()).getName()) { | ||
| return null; | ||
| } | ||
|
|
||
| if ((condition.getRightOperand() instanceof MethodInvocationTree) | ||
| && TreeUtils.isSizeAccess(condition.getRightOperand())) { | ||
| ExpressionTree methodSelect = | ||
| ((MethodInvocationTree) condition.getRightOperand()).getMethodSelect(); | ||
| if (methodSelect instanceof MemberSelectTree) { | ||
| MemberSelectTree mst = (MemberSelectTree) methodSelect; | ||
| Element elt = TreeUtils.elementFromTree(mst.getExpression()); | ||
| if (ResourceLeakUtils.isCollection(elt, atypeFactory)) { | ||
| return getNameFromExpressionTree(mst.getExpression()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This concern seems reasonable to me. I'd default to using .equals unless we're running the interning checker.
| * @return if the given type is a collection, returns the MustCall values of its elements or null | ||
| * if there are none or if the given type is not a collection | ||
| */ | ||
| public List<String> getMustCallValuesOfResourceCollectionComponent(TypeMirror t) { |
| /** | ||
| * Adds {@code CollectionObligation}s if the type of {@code node} is {@code @OwningCollection}. | ||
| * | ||
| * @param obligations the set of tracked obligations | ||
| * @param node the node of a return expression | ||
| */ | ||
| private void addObligationsForOwningCollectionReturn(Set<Obligation> obligations, Node node) { | ||
| LocalVariableNode tmpVar = cmAtf.getTempVarForNode(node); | ||
| if (tmpVar != null) { | ||
| CollectionOwnershipStore coStore = coAtf.getStoreAfter(node); | ||
| CollectionOwnershipType cotype = coAtf.getCoType(tmpVar, coStore); | ||
| if (cotype == CollectionOwnershipType.OwningCollection) { | ||
| ResourceAlias tmpVarAsResourceAlias = | ||
| new ResourceAlias(new LocalVariable(tmpVar), node.getTree()); | ||
| List<String> mustCallValues = | ||
| coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); | ||
| if (mustCallValues == null) { | ||
| throw new BugInCF( | ||
| "List of MustCall values of component type is null for OwningCollection return value:" | ||
| + " " | ||
| + node); | ||
| } | ||
| if (!ResourceLeakUtils.isIterator(node.getType())) { | ||
| for (String mustCallMethod : mustCallValues) { | ||
| obligations.add( | ||
| new CollectionObligation( | ||
| mustCallMethod, ImmutableSet.of(tmpVarAsResourceAlias), MethodExitKind.ALL)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The model is right about this, and we need to refactor appropriately.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests