Fix RLC unsoundness with explicit @MustCallUnknown annotation#6994
Fix RLC unsoundness with explicit @MustCallUnknown annotation#6994kelloggm wants to merge 19 commits intotypetools:masterfrom
@MustCallUnknown annotation#6994Conversation
|
@mernst I think you should review this again to make sure that the new Also, this change could introduce user-facing behavior in the form of new warnings (as shown by the need to add those annotations). Does that warrant a changelog entry? If so, I don't mind adding one. |
|
@kelloggm CI is not passing. Could you fix that? Then I will review. |
|
@kelloggm Ping. |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes an RLC (Resource Leak Checker) unsoundness issue by adding explicit @MustCallUnknown annotations to prevent incorrect behavior when handling owning parameters with unknown must-call obligations.
- Adds proper error detection for
@Owningparameters with@MustCallUnknownannotations - Improves must-call type resolution by using hierarchy-aware annotation lookup
- Adds type annotations to initialization classes to ensure proper type checking
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| DropOwning.java | New test case demonstrating the fix for issue #6990 with expected error |
| CrashForTempVar.java | Adds suppression annotation to focus testing on crash detection |
| MustCallConsistencyAnalyzer.java | Updates annotation lookup logic and fixes typo in comment |
| InitializationTransfer.java | Adds @MustCall type annotations to class declaration |
| InitializationStore.java | Adds @MustCall type annotations and improves class documentation |
…ion/InitializationStore.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds Checker Framework suppression annotations for Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/initialization/InitializationTransfer.java`:
- Around line 59-62: The generic type V in InitializationTransfer is bound as
CFAbstractValue<V> but InitializationStore is parameterized with `@MustCall`({})
V, causing an annotated type-argument mismatch; change V's bound to
CFAbstractValue<@MustCall({}) V> so the value type annotation matches
InitializationStore's value parameter (update the type parameter declaration on
class InitializationTransfer accordingly, keeping T and S as-is and preserving
references to InitializationStore and `@MustCall`).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65fb28ac-d3eb-4c99-8f99-9320be6dabc6
📒 Files selected for processing (7)
annotation-file-utilities/src/main/java/org/checkerframework/afu/annotator/Main.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/io/ASTPath.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/LinkedHashKeyedSet.javaannotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/coll/VivifyingMap.javachecker/src/main/java/org/checkerframework/checker/initialization/InitializationStore.javachecker/src/main/java/org/checkerframework/checker/initialization/InitializationTransfer.javachecker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java
| public class InitializationTransfer< | ||
| V extends CFAbstractValue<V>, | ||
| T extends InitializationTransfer<V, T, S>, | ||
| S extends InitializationStore<V, S>> | ||
| S extends InitializationStore<@MustCall({}) V, @MustCall({}) S>> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show the recursive value bounds in InitializationStore and InitializationTransfer.
# Expectation after the fix: both value bounds use CFAbstractValue<@MustCall({}) V>.
rg -n -C 3 'class Initialization(Store|Transfer)|V extends CFAbstractValue' \
--iglob 'InitializationStore.java' \
--iglob 'InitializationTransfer.java'Repository: typetools/checker-framework
Length of output: 2234
Align V's bound with InitializationStore.
Line 62 passes @MustCall({}) V to InitializationStore, but line 60 binds V as CFAbstractValue<V> instead of CFAbstractValue<@MustCall({}) V>. Mirror InitializationStore's value bound to resolve the annotated type-argument mismatch.
Proposed fix
public class InitializationTransfer<
- V extends CFAbstractValue<V>,
+ V extends CFAbstractValue<@MustCall({}) V>,
T extends InitializationTransfer<V, T, S>,
S extends InitializationStore<@MustCall({}) V, `@MustCall`({}) S>>📝 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 class InitializationTransfer< | |
| V extends CFAbstractValue<V>, | |
| T extends InitializationTransfer<V, T, S>, | |
| S extends InitializationStore<V, S>> | |
| S extends InitializationStore<@MustCall({}) V, @MustCall({}) S>> | |
| public class InitializationTransfer< | |
| V extends CFAbstractValue<@MustCall({}) V>, | |
| T extends InitializationTransfer<V, T, S>, | |
| S extends InitializationStore<@MustCall({}) V, `@MustCall`({}) 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/initialization/InitializationTransfer.java`
around lines 59 - 62, The generic type V in InitializationTransfer is bound as
CFAbstractValue<V> but InitializationStore is parameterized with `@MustCall`({})
V, causing an annotated type-argument mismatch; change V's bound to
CFAbstractValue<@MustCall({}) V> so the value type annotation matches
InitializationStore's value parameter (update the type parameter declaration on
class InitializationTransfer accordingly, keeping T and S as-is and preserving
references to InitializationStore and `@MustCall`).
fixes #6990