Extend RLC to collections of resources#7166
Closed
skehrli wants to merge 444 commits intotypetools:masterfrom
Closed
Extend RLC to collections of resources#7166skehrli wants to merge 444 commits intotypetools:masterfrom
skehrli wants to merge 444 commits intotypetools:masterfrom
Conversation
…CreatesMustCallFor anno on enclosing method
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/manual/resource-leak-checker.tex:
- Around line 645-651: Replace the vague phrase "properly disposed of" in the
sentence starting "What the checker effectively verifies is that any resource
collection is properly disposed of." with the more precise wording "required
methods are definitely called on all elements of any resource collection" so the
description aligns with must-call terminology used elsewhere in the document.
- Around line 867-871: Summary: The phrase "Assignments in initializer blocks
are not permitted" is ambiguous about whether it means instance initializers,
static initializers, or both; replace it with an explicit statement such as
"Assignments in instance or static initializer blocks are not permitted" (or
specify only instance or only static if that was the intent) and update the
surrounding text to use that clarified terminology; specifically modify the
sentence beginning "Assignments in initializer blocks are not permitted" so it
names "instance initializer blocks" and/or "static initializer blocks" and
ensure consistency with the rest of the paragraph where constructor and method
entry semantics are discussed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/manual/resource-leak-checker.tex
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.
Applied to files:
docs/manual/resource-leak-checker.tex
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: When handling collection ownership in MustCallConsistencyAnalyzer, do not throw if CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(..) returns null; treat it as “no obligations” and skip creating CollectionObligation(s). Also, when constructing diagnostics that reference an element method name, fall back to "Unknown" if the list is null/empty.
Applied to files:
docs/manual/resource-leak-checker.tex
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.
Applied to files:
docs/manual/resource-leak-checker.tex
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
🔇 Additional comments (4)
docs/manual/resource-leak-checker.tex (4)
138-158: LGTM! Clear introduction to collection ownership annotations.The subsection effectively introduces the new collection ownership annotations with appropriate cross-references to detailed sections. The organization and writing are clear and professional.
668-766: LGTM! Comprehensive documentation of the ownership type system.The type hierarchy, defaults, and ownership transfer rules are clearly explained with helpful code examples. The documentation effectively covers parameter defaults, return type defaults, and assignment behavior.
889-956: Clarify status of commented-out iterator section.This large commented-out section (~67 lines) documents iterator behavior over resource collections. If this feature is implemented and ready, the section should be uncommented. If it's not yet ready or has been deferred, the commented section should be removed to avoid confusion.
Consider either:
- Uncomment if the iterator feature is fully implemented and tested
- Remove if this is deferred work
- Add a brief note explaining why it's commented (if it's intentionally kept for near-term work)
Please clarify the status of iterator support and whether this documentation should be included in this PR.
873-880: Excellent documentation of field ownership restrictions.The restrictions on owned resource collection fields are clearly enumerated and well-justified by the explanation that "the checker doesn't allow transferring the ownership away from such a field." This helps users understand both what is prohibited and why.
The four bullet points clearly cover the main restriction scenarios.
Comment on lines
+645
to
+651
| The Resource Leak Checker handles homogeneous collections of resources. In a homogeneous collection, every element | ||
| has exactly the same must-call and called-methods properties. Instances of \texttt{java.util.Iterable} are supported; | ||
| this section calls those ``collections''. | ||
| % Usage of \texttt{java.util.Iterator}s over \texttt{java.util.Iterable}s are also supported, but they are not considered collections. | ||
|
|
||
| What the checker effectively verifies is that any resource collection is properly disposed of. That is, there is some loop that definitely calls the required methods on all elements of the collection. | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider clarifying "properly disposed of" for precision.
Line 650 states "What the checker effectively verifies is that any resource collection is properly disposed of." Consider being more precise, such as: "What the checker effectively verifies is that required methods are definitely called on all elements of any resource collection."
This aligns better with the must-call terminology used throughout the rest of the documentation.
🤖 Prompt for AI Agents
In @docs/manual/resource-leak-checker.tex around lines 645 - 651, Replace the
vague phrase "properly disposed of" in the sentence starting "What the checker
effectively verifies is that any resource collection is properly disposed of."
with the more precise wording "required methods are definitely called on all
elements of any resource collection" so the description aligns with must-call
terminology used elsewhere in the document.
Comment on lines
867
to
871
| \begin{itemize} | ||
| \item Write \code{List<? extends Socket>}. This rejects calls to \<add()> | ||
| or other methods that require an instance of the type variable, but it | ||
| preserves some of the behavior (e.g., calls to \<remove()> are permitted). | ||
| This is the best choice most of the time if the \<List> is not intended to | ||
| be owning. | ||
| \item Write \code{List<@MustCall Socket>}. This makes it an error to | ||
| add a Socket to the list, since the type of the Socket is | ||
| \<@MustCall("close")> but the list requires \<@MustCall()>. | ||
| \item Suppress one or more warnings. | ||
| \item Assignments in initializer blocks are not permitted. | ||
| \item Assignments in a declaration initializer are permitted if the right-hand side is at most \texttt{@OwningCollectionWithoutObligation}. | ||
| \item Thanks to the above two restrictions, it can be assumed that an owned resource collection field is \texttt{@OwningCollectionWithoutObligation} at the beginning of a constructor. An assignment to the field is now allowed if the field is \texttt{@OwningCollectionWithoutObligation} just before the assignment. This condition holds in any other (non-constructor) method too, but the field is \texttt{@OwningCollection} at the method entrance, unlike for a constructor. A legal assignment to an owning resource collection field resolves the obligation of the right-hand side. | ||
| \end{itemize} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify "initializer blocks" terminology.
Line 868 states "Assignments in initializer blocks are not permitted" but it's not immediately clear whether this refers to instance initializers, static initializers, or both. Consider being more specific, e.g., "Assignments in instance or static initializer blocks are not permitted."
🤖 Prompt for AI Agents
In @docs/manual/resource-leak-checker.tex around lines 867 - 871, Summary: The
phrase "Assignments in initializer blocks are not permitted" is ambiguous about
whether it means instance initializers, static initializers, or both; replace it
with an explicit statement such as "Assignments in instance or static
initializer blocks are not permitted" (or specify only instance or only static
if that was the intent) and update the surrounding text to use that clarified
terminology; specifically modify the sentence beginning "Assignments in
initializer blocks are not permitted" so it names "instance initializer blocks"
and/or "static initializer blocks" and ensure consistency with the rest of the
paragraph where constructor and method entry semantics are discussed.
Comment on lines
+957
to
+958
| \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]] |
Contributor
There was a problem hiding this comment.
Address TODO before merging.
The placeholder "[[TODO: Add a table with a number of collection/iterable methods and their collection ownership annotations]]" indicates incomplete documentation. Documentation for JDK method signatures would help users understand how standard library methods interact with the collection ownership system.
This should be completed before merging, or if the documentation is being deferred, add a note indicating that users should refer to the annotated JDK sources or provide a link to where this information can be found.
Would you like me to help generate a starter table of common JDK method signatures based on the implementation in the codebase?
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java (5)
763-764: 🧹 Nitpick | 🔵 TrivialDo not expose mutable called-methods backing set.
Return an unmodifiable view (or immutable copy) from
getCalledMethods().Suggested change
public Set<String> getCalledMethods() { - return calledMethods; + return Collections.unmodifiableSet(calledMethods); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 763 - 764, The getter RLCCalledMethodsAnnotatedTypeFactory.getCalledMethods currently returns the mutable backing field calledMethods; change it to return an unmodifiable view or immutable copy (for example Collections.unmodifiableSet(calledMethods) or new HashSet<>(calledMethods)) so callers cannot mutate the internal set while preserving the existing field name and behavior.
780-781: 🧹 Nitpick | 🔵 TrivialReuse cached
rlcinstead of re-fetching checker.Use the existing field for consistency and less indirection.
Suggested change
- MustCallConsistencyAnalyzer mustCallConsistencyAnalyzer = - new MustCallConsistencyAnalyzer(ResourceLeakUtils.getResourceLeakChecker(this), true); + MustCallConsistencyAnalyzer mustCallConsistencyAnalyzer = + new MustCallConsistencyAnalyzer(rlc, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 780 - 781, Replace the call to ResourceLeakUtils.getResourceLeakChecker(this) when creating the MustCallConsistencyAnalyzer with the already-cached field rlc to avoid re-fetching the checker; update the MustCallConsistencyAnalyzer instantiation (the mustCallConsistencyAnalyzer variable creation) to pass rlc instead of ResourceLeakUtils.getResourceLeakChecker(this) so the analyzer uses the existing cached checker.
795-797:⚠️ Potential issue | 🟠 MajorContainment check should require non-empty node mapping.
Line 796 checks only
!= null; if the API returns an empty collection, this may trigger loop analysis for non-contained trees.Suggested change
- boolean loopContainedInThisMethod = - cfg.getNodesCorrespondingToTree(collectionElementTree) != null; + List<?> nodes = cfg.getNodesCorrespondingToTree(collectionElementTree); + boolean loopContainedInThisMethod = nodes != null && !nodes.isEmpty();#!/bin/bash set -euo pipefail # Verify API semantics and current call-site usage. CFG_FILE="$(fd 'ControlFlowGraph.java$' | head -n1)" echo "Inspecting: ${CFG_FILE}" rg -nP 'getNodesCorrespondingToTree\s*\(' "$CFG_FILE" -C3 rg -nP 'return\s+Collections\.emptyList\(|return\s+null' "$CFG_FILE" -C3 || true echo echo "Null-only containment checks in Java sources:" rg -nP 'getNodesCorrespondingToTree\([^)]*\)\s*!=\s*null' --type java -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 795 - 797, The containment check currently uses cfg.getNodesCorrespondingToTree(collectionElementTree) != null which can be true for an empty collection; change it to first capture the result into a local (e.g., List/Collection nodes = cfg.getNodesCorrespondingToTree(collectionElementTree)) and then set loopContainedInThisMethod = (nodes != null && !nodes.isEmpty()) so the check requires a non-empty mapping; update the logic in RLCCalledMethodsAnnotatedTypeFactory where loopContainedInThisMethod is computed to use this non-empty check.
487-506:⚠️ Potential issue | 🟡 MinorFix Javadoc type wording and typos.
Javadocs still mention
CFStore(Line 492) and contain “wether”/“bodys” typos (Lines 504, 770).Also applies to: 768-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 487 - 506, The Javadoc for methods in RLCCalledMethodsAnnotatedTypeFactory uses the wrong type name and contains typos: replace occurrences of "CFStore" with "AccumulationStore" in the Javadoc for getStoreAfterBlock and the conditional-store method (the method returning then/else stores), and fix misspellings "wether" → "whether" and "bodys" → "bodies" (also update any duplicated occurrences around the conditional-store Javadoc). Ensure parameter descriptions and return tags reference AccumulationStore and correct spelling for clarity.
122-166:⚠️ Potential issue | 🟠 MajorAvoid static mutable loop-tracking state in the type factory.
potentiallyFulfillingLoopsbeing static and directly exposed creates cross-analysis contamination/leak risks. Make it an instance field and return an unmodifiable view.Based on learnings: 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.Refactor sketch
- private static final Set<PotentiallyFulfillingLoop> potentiallyFulfillingLoops = new HashSet<>(); + private final Set<PotentiallyFulfillingLoop> potentiallyFulfillingLoops = new HashSet<>(); - public static void addPotentiallyFulfillingLoop(...) + public void addPotentiallyFulfillingLoop(...) - public static Set<PotentiallyFulfillingLoop> getPotentiallyFulfillingLoops() { - return potentiallyFulfillingLoops; + public Set<PotentiallyFulfillingLoop> getPotentiallyFulfillingLoops() { + return Collections.unmodifiableSet(potentiallyFulfillingLoops); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java` around lines 122 - 166, Change the static mutable set potentiallyFulfillingLoops into an instance field and expose it only via an unmodifiable view: remove the static modifier from the field and initialize it (new HashSet<>()) in the RLCCalledMethodsAnnotatedTypeFactory constructor; make addPotentiallyFulfillingLoop and getPotentiallyFulfillingLoops instance methods (non-static) that operate on this instance field; have getPotentiallyFulfillingLoops return Collections.unmodifiableSet(potentiallyFulfillingLoops); and update any callers to invoke the instance methods on the RLCCalledMethodsAnnotatedTypeFactory object instead of statically.checker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties (1)
1-2:⚠️ Potential issue | 🟡 MinorUse consistent “of …” phrasing in override diagnostics.
These two messages still say “Incompatible ownership for …”; prior review requested “of return type” / “of parameter …” for precision and consistency.
Suggested wording update
-owning.override.return=Incompatible ownership for return.%nfound : no ownership annotation or `@Owning`%nrequired: `@NotOwning`%nConsequence: method %s in %s cannot override method %s in %s -owning.override.param=Incompatible ownership for parameter %s.%nfound : no ownership annotation or `@NotOwning`%nrequired: `@Owning`%nConsequence: method %s in %s cannot override method %s in %s +owning.override.return=Incompatible ownership of return type.%nfound : no ownership annotation or `@Owning`%nrequired: `@NotOwning`%nConsequence: method %s in %s cannot override method %s in %s +owning.override.param=Incompatible ownership of parameter %s.%nfound : no ownership annotation or `@NotOwning`%nrequired: `@Owning`%nConsequence: method %s in %s cannot override method %s in %s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties` around lines 1 - 2, Update the two diagnostic messages to use consistent "of ..." phrasing: change the key owning.override.return's message to refer to "of return type" (e.g., "Incompatible ownership of return type.%nfound ... required ...") and change owning.override.param's message to refer to "of parameter %s" (e.g., "Incompatible ownership of parameter %s.%nfound ... required ..."), keeping the rest of the wording and format intact so the %s placeholders and consequences remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties`:
- Around line 1-2: Update the two diagnostic messages to use consistent "of ..."
phrasing: change the key owning.override.return's message to refer to "of return
type" (e.g., "Incompatible ownership of return type.%nfound ... required ...")
and change owning.override.param's message to refer to "of parameter %s" (e.g.,
"Incompatible ownership of parameter %s.%nfound ... required ..."), keeping the
rest of the wording and format intact so the %s placeholders and consequences
remain unchanged.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.java`:
- Around line 763-764: The getter
RLCCalledMethodsAnnotatedTypeFactory.getCalledMethods currently returns the
mutable backing field calledMethods; change it to return an unmodifiable view or
immutable copy (for example Collections.unmodifiableSet(calledMethods) or new
HashSet<>(calledMethods)) so callers cannot mutate the internal set while
preserving the existing field name and behavior.
- Around line 780-781: Replace the call to
ResourceLeakUtils.getResourceLeakChecker(this) when creating the
MustCallConsistencyAnalyzer with the already-cached field rlc to avoid
re-fetching the checker; update the MustCallConsistencyAnalyzer instantiation
(the mustCallConsistencyAnalyzer variable creation) to pass rlc instead of
ResourceLeakUtils.getResourceLeakChecker(this) so the analyzer uses the existing
cached checker.
- Around line 795-797: The containment check currently uses
cfg.getNodesCorrespondingToTree(collectionElementTree) != null which can be true
for an empty collection; change it to first capture the result into a local
(e.g., List/Collection nodes =
cfg.getNodesCorrespondingToTree(collectionElementTree)) and then set
loopContainedInThisMethod = (nodes != null && !nodes.isEmpty()) so the check
requires a non-empty mapping; update the logic in
RLCCalledMethodsAnnotatedTypeFactory where loopContainedInThisMethod is computed
to use this non-empty check.
- Around line 487-506: The Javadoc for methods in
RLCCalledMethodsAnnotatedTypeFactory uses the wrong type name and contains
typos: replace occurrences of "CFStore" with "AccumulationStore" in the Javadoc
for getStoreAfterBlock and the conditional-store method (the method returning
then/else stores), and fix misspellings "wether" → "whether" and "bodys" →
"bodies" (also update any duplicated occurrences around the conditional-store
Javadoc). Ensure parameter descriptions and return tags reference
AccumulationStore and correct spelling for clarity.
- Around line 122-166: Change the static mutable set potentiallyFulfillingLoops
into an instance field and expose it only via an unmodifiable view: remove the
static modifier from the field and initialize it (new HashSet<>()) in the
RLCCalledMethodsAnnotatedTypeFactory constructor; make
addPotentiallyFulfillingLoop and getPotentiallyFulfillingLoops instance methods
(non-static) that operate on this instance field; have
getPotentiallyFulfillingLoops return
Collections.unmodifiableSet(potentiallyFulfillingLoops); and update any callers
to invoke the instance methods on the RLCCalledMethodsAnnotatedTypeFactory
object instead of statically.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsAnnotatedTypeFactory.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/messages.properties
…to rlc-collections-redesign
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/dependenttypes/DependentTypesError.java`:
- Line 28: The regex in DependentTypesError (Pattern.compile("\\[error for
expression: (.*); error: [.*]\\]")) incorrectly uses a character class [.*]
instead of a second capturing group, causing groupCount() == 2 and group(2) to
fail; change the pattern so it defines two capturing groups to match the
FORMAT_STRING's two placeholders (e.g. replace [.*] with (.*) or an appropriate
capturing pattern), update the Pattern.compile call accordingly, and run tests
to ensure groupCount() and subsequent group(1)/group(2) usages succeed.
In `@framework/tests/h1h2checker/TypeRefinement.java`:
- Around line 14-15: The test has a split/malformed directive across two lines
(line 14 contains "// :: error: [h1h2checker.h1invalid.forbidden] :: warning:"
and line 15 contains a plain comment with the warning key), so merge them into a
single directive comment and ensure both the error and warning keys are on the
same directive line (e.g., a single "// :: error:
[h1h2checker.h1invalid.forbidden] :: warning:
(cast.unsafe.constructor.invocation)" comment) so the test harness can parse the
warning key; update the directive near the TypeRefinement test declaration to
use that single-line format.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
checker/tests/README.mdchecker/tests/i18n-formatter/ConversionCategoryTest.javachecker/tests/i18n-formatter/HasFormat.javachecker/tests/i18n-formatter/I18nFormat.javachecker/tests/i18n-formatter/I18nFormatForTest.javachecker/tests/i18n-formatter/IsFormat.javachecker/tests/i18n-formatter/ManualExampleI18nFormatter.javachecker/tests/i18n-formatter/Syntax.javaframework/src/main/java/org/checkerframework/framework/util/dependenttypes/DependentTypesError.javaframework/tests/h1h2checker/TypeRefinement.javaframework/tests/reportmodifiers/TestModifiers.javaframework/tests/reporttreekinds/TestTreeKinds.java
…to rlc-collections-redesign
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/tests/h1h2checker/TypeRefinement.java (1)
14-15:⚠️ Potential issue | 🔴 CriticalConsolidate split warning directive to single line.
The warning directive is split across two lines: line 14 ends with
:: warning:without a key, and line 15 contains the key in a plain comment rather than a directive comment. This malformed pattern prevents the test harness from parsing the warning key.🔧 Suggested fix (as previously recommended)
- // :: error: [h1h2checker.h1invalid.forbidden] :: warning: - // [cast.unsafe.constructor.invocation] + // :: error: [h1h2checker.h1invalid.forbidden] :: warning: [cast.unsafe.constructor.invocation]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/tests/h1h2checker/TypeRefinement.java` around lines 14 - 15, Consolidate the split test-harness directive into a single well-formed comment: replace the two-line malformed sequence (the line containing ":: error: [h1h2checker.h1invalid.forbidden] :: warning:" and the following line containing just "[cast.unsafe.constructor.invocation]") with one complete directive comment so each warning key is part of the same directive (e.g., a single comment that includes both [h1h2checker.h1invalid.forbidden] and [cast.unsafe.constructor.invocation] as part of the :: error/:: warning directive). Locate the directive near TypeRefinement.java's test assertion and update the comment so the harness can parse the warning keys correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/tests/h1h2checker/TypeRefinement.java`:
- Around line 14-15: Consolidate the split test-harness directive into a single
well-formed comment: replace the two-line malformed sequence (the line
containing ":: error: [h1h2checker.h1invalid.forbidden] :: warning:" and the
following line containing just "[cast.unsafe.constructor.invocation]") with one
complete directive comment so each warning key is part of the same directive
(e.g., a single comment that includes both [h1h2checker.h1invalid.forbidden] and
[cast.unsafe.constructor.invocation] as part of the :: error/:: warning
directive). Locate the directive near TypeRefinement.java's test assertion and
update the comment so the harness can parse the warning keys correctly.
…to rlc-collections-redesign
Contributor
|
@skehrli it seems the tests are failing. Could you take a look? Then I'll do a review pass. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`:
- Around line 208-214: In the branch handling AnnotatedTypeVariable (where
typeArg is an AnnotatedTypeVariable and upperBound is assigned), change the call
that checks for a manual `@MustCallUnknown` so it uses the source-level TypeMirror
overload instead of the AnnotatedTypeMirror overload; specifically, replace
ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this) with the
TypeMirror-based check (e.g.,
ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound.getUnderlyingType(),
this) or the equivalent TypeMirror parameter) so only explicit source
annotations on the upper bound suppress replacing the annotation with BOTTOM in
MustCallAnnotatedTypeFactory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8eb24f2-7597-4286-bf88-a6d1e35b3949
📒 Files selected for processing (5)
annotation-file-utilities/src/main/java/org/checkerframework/afu/annotator/find/Insertions.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/WrapperMap.javachecker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
Comment on lines
+208
to
+214
| } 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); |
Contributor
There was a problem hiding this comment.
This branch preserves implicit TOP as if it were a manual @MustCallUnknown.
ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this) calls the AnnotatedTypeMirror overload, and that helper reads getPrimaryAnnotationInHierarchy(TOP) in checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java:454-467. That makes a defaulted top on an unannotated upper bound look the same as a user-written @MustCallUnknown, so List<T> can keep @MustCallUnknown instead of being rewritten to @MustCall({}). Please use the source-level TypeMirror check here so only explicit annotations suppress the rewrite.
Possible fix
} else if (typeArg instanceof AnnotatedTypeVariable) {
AnnotatedTypeMirror upperBound = ((AnnotatedTypeVariable) typeArg).getUpperBound();
+ TypeMirror upperBoundType =
+ upperBound == null ? null : upperBound.getUnderlyingType();
// 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)) {
+ || !ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBoundType)) {
typeArg.replaceAnnotation(BOTTOM);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java`
around lines 208 - 214, In the branch handling AnnotatedTypeVariable (where
typeArg is an AnnotatedTypeVariable and upperBound is assigned), change the call
that checks for a manual `@MustCallUnknown` so it uses the source-level TypeMirror
overload instead of the AnnotatedTypeMirror overload; specifically, replace
ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound, this) with the
TypeMirror-based check (e.g.,
ResourceLeakUtils.hasManualMustCallUnknownAnno(upperBound.getUnderlyingType(),
this) or the equivalent TypeMirror parameter) so only explicit source
annotations on the upper bound suppress replacing the annotation with BOTTOM in
MustCallAnnotatedTypeFactory.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
1730-1734:⚠️ Potential issue | 🔴 CriticalGuard argument arity before indexing.
Line 1733 can throw
IndexOutOfBoundsExceptionfor zero-argget()calls. Validate arity and only return the index expression for single-argument calls.Suggested fix
public static `@Nullable` ExpressionTree getIdxForGetCall(Tree tree) { if ((tree instanceof MethodInvocationTree) && isNamedMethodCall("get", (MethodInvocationTree) tree)) { - return ((MethodInvocationTree) tree).getArguments().get(0); + List<? extends ExpressionTree> args = ((MethodInvocationTree) tree).getArguments(); + return args.size() == 1 ? args.get(0) : null; } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java` around lines 1730 - 1734, getIdxForGetCall currently assumes a single-argument get() and calls getArguments().get(0) which can throw IndexOutOfBoundsException; update the method to check the MethodInvocationTree's argument arity (use ((MethodInvocationTree) tree).getArguments().size() == 1) and only return getArguments().get(0) when there is exactly one argument, otherwise return null; reference the getIdxForGetCall method, the MethodInvocationTree type and its getArguments() accessor when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 1730-1734: getIdxForGetCall currently assumes a single-argument
get() and calls getArguments().get(0) which can throw IndexOutOfBoundsException;
update the method to check the MethodInvocationTree's argument arity (use
((MethodInvocationTree) tree).getArguments().size() == 1) and only return
getArguments().get(0) when there is exactly one argument, otherwise return null;
reference the getIdxForGetCall method, the MethodInvocationTree type and its
getArguments() accessor when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a321768a-a93f-4b34-ace1-8b86e596ce8e
📒 Files selected for processing (2)
annotation-file-utilities/src/main/java/org/checkerframework/afu/annotator/find/Insertions.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java
…-framework into rlc-collections-redesign
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java (6)
3370-3375:⚠️ Potential issue | 🟠 MajorUse
equals()instead of==forNamecomparison.At line 3373, the code uses
==to compareNameobjects:isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName();Per established patterns for this codebase,
Namecomparisons should useName.equals()orObjects.equals()instead of identity comparison (==), unless the Interning Checker is explicitly used to guarantee reference equality.🔧 Proposed fix
- isAssignmentOfIterVar = iterVarDecl.getName() == loopVariable.getName(); + isAssignmentOfIterVar = Objects.equals(iterVarDecl.getName(), loopVariable.getName());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 3370 - 3375, Replace the identity comparison of Name objects with an equality check: in MustCallConsistencyAnalyzer where you inspect nodes (the block that checks node instanceof AssignmentNode and assigns VariableTree iterVarDecl), change the line that sets isAssignmentOfIterVar using iterVarDecl.getName() == loopVariable.getName() to use equals (or Objects.equals) so the Name comparison uses value equality; update the check around the AssignmentNode/VariableTree handling so isAssignmentOfIterVar = iterVarDecl.getName().equals(loopVariable.getName()) (or Objects.equals(iterVarDecl.getName(), loopVariable.getName())).
572-583:⚠️ Potential issue | 🔴 CriticalCritical: Override
hashCode()when overridingequals().
CollectionObligationoverridesequals()to includemustCallMethodbut does not overridehashCode(). This violates theequals/hashCodecontract and will cause incorrect behavior whenCollectionObligationinstances are used in hash-based collections (theobligationssets throughout this file).🔧 Proposed fix
`@Override` public boolean equals(`@Nullable` Object obj) { if (obj == this) { return true; } if (!(obj instanceof CollectionObligation)) { return false; } return super.equals(obj) && ((CollectionObligation) obj).mustCallMethod.equals(this.mustCallMethod); } + + `@Override` + public int hashCode() { + return Objects.hash(super.hashCode(), mustCallMethod); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 572 - 583, CollectionObligation overrides equals to include the mustCallMethod field but does not override hashCode, violating the equals/hashCode contract and breaking hash-based collections like the obligations sets; add an override of hashCode() in the CollectionObligation class that composes super.hashCode() with the hash of mustCallMethod (handling null safely) so objects equal by equals() produce equal hash codes (e.g., combine using a prime multiplier or Objects.hash(super.hashCode(), mustCallMethod)).
1933-1941:⚠️ Potential issue | 🟠 MajorAdd null and empty checks before accessing
mustCallValues(second occurrence).Similar to the previous issue, line 1938 calls
.get(0)on the result ofgetMustCallValuesOfResourceCollectionComponent()without null or empty checks.🔧 Proposed fix
case OwningCollection: // assignment not allowed + List<String> mustCallValues = + coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); + String methodName = (mustCallValues == null || mustCallValues.isEmpty()) + ? "Unknown" + : mustCallValues.get(0); checker.reportError( node.getTree(), "unfulfilled.collection.obligations", - coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0), + methodName, lhs.getTree(), "Field assignment might overwrite field's current value"); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 1933 - 1941, In the OwningCollection case inside MustCallConsistencyAnalyzer, avoid calling .get(0) directly on coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); instead, first assign the result to a local variable (e.g., mustCallValues), check that it is non-null and not empty, and only then use mustCallValues.get(0) in the checker.reportError call; if the collection is null or empty, handle that branch (e.g., report a different, safe error message or skip the specific value) so no NPE or IndexOutOfBoundsException can occur when executing checker.reportError with that element.
833-839:⚠️ Potential issue | 🔴 CriticalAdd null check for
mustCallValuesto prevent potential NPE.At line 834,
getMustCallValuesOfResourceCollectionComponentis called but there's no null check before iterating at lines 835-838. Per the factory's API documentation, this method can returnnullfor non-resource element types.🔧 Proposed fix
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()); + if (mustCallValues == null) { + // No obligations for non-resource element types + break; + } for (String mustCallMethod : mustCallValues) { obligations.add( CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 833 - 839, The call to coAtf.getMustCallValuesOfResourceCollectionComponent(...) can return null, so guard iteration over mustCallValues to avoid an NPE: after obtaining mustCallValues from coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()), add a null check (e.g., if (mustCallValues != null) ...) before the for loop that calls CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod) and adds to obligations so non-resource elements are safely skipped.
1869-1876:⚠️ Potential issue | 🟠 MajorAdd null and empty checks before accessing
mustCallValues.At line 1873, the code calls
.get(0)on the result ofgetMustCallValuesOfResourceCollectionComponent()without checking if the result isnullor empty. This could cause aNullPointerExceptionorIndexOutOfBoundsException.Based on learnings: "when constructing diagnostics that reference an element method name, fall back to 'Unknown' if the list is null/empty."
🔧 Proposed fix
} else { // is an initialization block. Not supported. + List<String> mustCallValues = + coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); + String methodName = (mustCallValues == null || mustCallValues.isEmpty()) + ? "Unknown" + : mustCallValues.get(0); checker.reportError( node, "unfulfilled.collection.obligations", - coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0), + methodName, lhs.getTree(), "Field assignment outside method or declaration might overwrite field's current value"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 1869 - 1876, The code calls coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0) without guarding for null or empty results; update MustCallConsistencyAnalyzer so you retrieve the list into a local (e.g., mustCallValues), check for null or isEmpty(), and pass a safe fallback (the string "Unknown" or an appropriate placeholder) into checker.reportError when the list is null/empty; ensure you still pass the actual first element when present so that checker.reportError receives either mustCallValues.get(0) or the fallback.
783-788:⚠️ Potential issue | 🔴 CriticalHandle null
mustCallValuesgracefully instead of throwing.At lines 783-788, the code throws
BugInCFwhengetMustCallValuesOfResourceCollectionComponentreturnsnull. Per established patterns for this codebase, anullresult should be treated as "no obligations" and the method should skip creatingCollectionObligations rather than throwing. This can occur for@OwningCollectionover non-resource element types.Based on learnings: "In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as 'no obligations' and skip creating CollectionObligation(s); do not throw."
🔧 Proposed fix
List<String> mustCallValues = coAtf.getMustCallValuesOfResourceCollectionComponent(node.getTree()); - if (mustCallValues == null) { - throw new BugInCF( - "List of MustCall values of component type is null for OwningCollection return value:" - + " " - + node); - } - if (!ResourceLeakUtils.isIterator(node.getType())) { + // 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) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java` around lines 783 - 788, In MustCallConsistencyAnalyzer (method addObligationsForOwningCollectionReturn), replace the throw when getMustCallValuesOfResourceCollectionComponent(...) returns null with logic that treats null as "no obligations": if mustCallValues is null simply skip creating any CollectionObligation(s) and return/continue from the method instead of throwing BugInCF; keep all other handling of non-null mustCallValues intact and continue using CollectionObligation and the existing flow for adding obligations when values are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 1543-1551: The code around the treeForAlias assignment contains
commented-out logic and an uncertain comment about temp vars; either remove the
dead commented block and replace the vague comment with a concise rationale for
always using node.getTree(), or restore and use the original conditional logic
(typeFactory.isTempVar(lhsVar) ? typeFactory.getTreeForTempVar(lhsVar) :
node.getTree()) so loopBodyAnalysis correctly handles temp-var aliases, and add
a brief clarifying comment explaining why the temp-var branch is needed or why
it was removed; look at treeForAlias, typeFactory.isTempVar(lhsVar),
typeFactory.getTreeForTempVar(lhsVar), node.getTree(), and loopBodyAnalysis to
implement the change.
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`:
- Around line 3370-3375: Replace the identity comparison of Name objects with an
equality check: in MustCallConsistencyAnalyzer where you inspect nodes (the
block that checks node instanceof AssignmentNode and assigns VariableTree
iterVarDecl), change the line that sets isAssignmentOfIterVar using
iterVarDecl.getName() == loopVariable.getName() to use equals (or
Objects.equals) so the Name comparison uses value equality; update the check
around the AssignmentNode/VariableTree handling so isAssignmentOfIterVar =
iterVarDecl.getName().equals(loopVariable.getName()) (or
Objects.equals(iterVarDecl.getName(), loopVariable.getName())).
- Around line 572-583: CollectionObligation overrides equals to include the
mustCallMethod field but does not override hashCode, violating the
equals/hashCode contract and breaking hash-based collections like the
obligations sets; add an override of hashCode() in the CollectionObligation
class that composes super.hashCode() with the hash of mustCallMethod (handling
null safely) so objects equal by equals() produce equal hash codes (e.g.,
combine using a prime multiplier or Objects.hash(super.hashCode(),
mustCallMethod)).
- Around line 1933-1941: In the OwningCollection case inside
MustCallConsistencyAnalyzer, avoid calling .get(0) directly on
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()); instead,
first assign the result to a local variable (e.g., mustCallValues), check that
it is non-null and not empty, and only then use mustCallValues.get(0) in the
checker.reportError call; if the collection is null or empty, handle that branch
(e.g., report a different, safe error message or skip the specific value) so no
NPE or IndexOutOfBoundsException can occur when executing checker.reportError
with that element.
- Around line 833-839: The call to
coAtf.getMustCallValuesOfResourceCollectionComponent(...) can return null, so
guard iteration over mustCallValues to avoid an NPE: after obtaining
mustCallValues from
coAtf.getMustCallValuesOfResourceCollectionComponent(receiverNode.getTree()),
add a null check (e.g., if (mustCallValues != null) ...) before the for loop
that calls CollectionObligation.fromTree(receiverNode.getTree(), mustCallMethod)
and adds to obligations so non-resource elements are safely skipped.
- Around line 1869-1876: The code calls
coAtf.getMustCallValuesOfResourceCollectionComponent(lhs.getTree()).get(0)
without guarding for null or empty results; update MustCallConsistencyAnalyzer
so you retrieve the list into a local (e.g., mustCallValues), check for null or
isEmpty(), and pass a safe fallback (the string "Unknown" or an appropriate
placeholder) into checker.reportError when the list is null/empty; ensure you
still pass the actual first element when present so that checker.reportError
receives either mustCallValues.get(0) or the fallback.
- Around line 783-788: In MustCallConsistencyAnalyzer (method
addObligationsForOwningCollectionReturn), replace the throw when
getMustCallValuesOfResourceCollectionComponent(...) returns null with logic that
treats null as "no obligations": if mustCallValues is null simply skip creating
any CollectionObligation(s) and return/continue from the method instead of
throwing BugInCF; keep all other handling of non-null mustCallValues intact and
continue using CollectionObligation and the existing flow for adding obligations
when values are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9e9b486-54a2-411f-9393-f81e12468928
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
Comment on lines
+1543
to
+1551
| // 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(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing or addressing commented code and uncertain comment.
Lines 1544-1550 contain a commented-out code block with a comment expressing uncertainty ("I don't think we need a tempVar here"). If the commented code is not needed, consider removing it to improve clarity. If there's uncertainty about whether temp var handling is needed, this should be verified and documented more clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java`
around lines 1543 - 1551, The code around the treeForAlias assignment contains
commented-out logic and an uncertain comment about temp vars; either remove the
dead commented block and replace the vague comment with a concise rationale for
always using node.getTree(), or restore and use the original conditional logic
(typeFactory.isTempVar(lhsVar) ? typeFactory.getTreeForTempVar(lhsVar) :
node.getTree()) so loopBodyAnalysis correctly handles temp-var aliases, and add
a brief clarifying comment explaining why the temp-var branch is needed or why
it was removed; look at treeForAlias, typeFactory.isTempVar(lhsVar),
typeFactory.getTreeForTempVar(lhsVar), node.getTree(), and loopBodyAnalysis to
implement the change.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (1)
799-805:⚠️ Potential issue | 🔴 CriticalFix the matching logic to require both tree and node to match.
The condition still uses
||(OR) which matches if EITHER the tree OR the node matches. This is inconsistent withIteratedCollectionElement.equals()which requires BOTH fields to match. The current logic could return an incorrectIteratedCollectionElementwhen only one field matches.Apply the previously suggested fix:
🐛 Proposed fix
`@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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 799 - 805, The matching logic in getIteratedCollectionElement currently returns an IteratedCollectionElement if either tree or node matches; change it to require both match. In getIteratedCollectionElement (CFAbstractStore) update the condition that examines each IteratedCollectionElement (from iteratedCollectionElements.keySet()) so it uses a logical AND combining tree-equality and node-equality checks, and perform null-safe comparisons (e.g., (ice.tree == tree || (ice.tree != null && ice.tree.equals(tree))) && (ice.node == node || (ice.node != null && ice.node.equals(node)))) to mirror IteratedCollectionElement.equals() semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 799-805: The matching logic in getIteratedCollectionElement
currently returns an IteratedCollectionElement if either tree or node matches;
change it to require both match. In getIteratedCollectionElement
(CFAbstractStore) update the condition that examines each
IteratedCollectionElement (from iteratedCollectionElements.keySet()) so it uses
a logical AND combining tree-equality and node-equality checks, and perform
null-safe comparisons (e.g., (ice.tree == tree || (ice.tree != null &&
ice.tree.equals(tree))) && (ice.node == node || (ice.node != null &&
ice.node.equals(node)))) to mirror IteratedCollectionElement.equals() semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ea5edb7-230d-4ab5-af98-72591dd27603
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java
Member
|
Closed in favor of #7602, which contains all of this pull request's commits and more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests