checker-framework icon indicating copy to clipboard operation
checker-framework copied to clipboard

Extend RLC to collections of resources

Open skehrli opened this issue 5 months ago • 2 comments

Summary by CodeRabbit

  • New Features

    • Introduces collection-ownership checking integrated with the Resource Leak Checker, with new annotations (e.g., owning/non-owning collections and field destructors).
    • Defaults and ownership transfer for parameters, returns, fields, assignments, and new allocations.
    • Loop-aware analysis (enhanced-for and index loops) to verify element must-call obligations.
    • Improved diagnostics and dataflow visualization for iterated collection elements.
  • Bug Fixes

    • Fewer false positives: iterator and map access now reflect non-owning returns, reducing spurious errors.
  • Documentation

    • Manual updated with collection-ownership model, defaults, and usage guidance.
  • Tests

    • Extensive new test suites for collections, fields, loops, and defaults.

skehrli avatar Jul 23 '25 15:07 skehrli

@skehrli If you make CI pass, then CI will be more effective in warning us of regressions.

mernst avatar Jul 24 '25 23:07 mernst

📝 Walkthrough

Walkthrough

Adds a Collection Ownership type system and integrates it into the Resource Leak Checker. New ownership qualifiers and annotations (OwningCollection, NotOwningCollection, PolyOwningCollection, OwningCollectionWithoutObligation, OwningCollectionBottom, CollectionFieldDestructor, CreatesCollectionObligation) are introduced. Implements checker components (Checker, AnnotatedTypeFactory, Analysis, Transfer, Store, Visitor), augments MustCall/RLCCalledMethods flows and dataflow to track iterated collection elements (IteratedCollectionElement, CFAbstractStore), and updates many APIs with ownership/nullness annotations, messages, docs, and tests.

Possibly related PRs

  • typetools/checker-framework#7318: Edits ownership annotations and collection utilities (e.g., LinkedHashKeyedSet, ASTPath iterator signatures), overlapping code-level ownership-annotation changes in this PR.

Suggested reviewers

  • smillst

Pre-merge checks and finishing touches

✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0faec762f4d0347697f9cbd477f1118e13e3f276 and 4c56f0cf91adc388c713356e52f00da0c46b14e5.

📒 Files selected for processing (1)
  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java (2 hunks)
🧰 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: 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:

  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java
📚 Learning: 2025-10-22T20:41:01.112Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java:128-136
Timestamp: 2025-10-22T20:41:01.112Z
Learning: In checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java, prefer instance fields over static state for analysis caches (e.g., maps keyed by Tree/Block). There is at most one ATF per running checker, so static mutable fields are discouraged.

Applied to files:

  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java
📚 Learning: 2025-10-22T20:43:32.957Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java:767-798
Timestamp: 2025-10-22T20:43:32.957Z
Learning: In MustCallConsistencyAnalyzer.addObligationsForOwningCollectionReturn, treat a null result from CollectionOwnershipAnnotatedTypeFactory.getMustCallValuesOfResourceCollectionComponent(...) as “no obligations” and skip creating CollectionObligation(s); do not throw. This can occur for OwningCollection over non-resource element types.

Applied to files:

  • checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java (2)
checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipAnnotatedTypeFactory.java (1)
  • CollectionOwnershipAnnotatedTypeFactory (64-745)
checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakUtils.java (1)
  • ResourceLeakUtils (40-468)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 09 '25 01:09 coderabbitai[bot]