feat(extractor): transitive consume contract — use-after-handoff through a forwarding consumer#68
Merged
Merged
Conversation
…ugh a forwarding consumer Cross-method D4: the consume-handoff was modelled only for a callee that disposes a by-value IDisposable parameter DIRECTLY (#55). The inference deliberately stopped one hop short — a parameter merely forwarded to another call was left ambiguous (ownir.py's `passed` branch: "awaiting ... a later transitive pass"). So a consumer that forwards rather than disposes (`Consume(sink) -> Inner(sink) -> sink.Dispose()`) was NOT seen as a consumer, the handoff was not a release, and a later use of the argument was invisible. The extractor's consumer detection is now transitive: `ConsumesParam` recognises a parameter as consumed if the body disposes it directly OR forwards it to another first-party consumer that consumes it — following the chain, guarded against cycles (keyed per parameter), binding each callee's body with its own tree's SemanticModel so cross-file chains resolve. So `Consume(s)` is still a call-site release and a use after it trips OWN002. Conservative: a parameter handed to an unknown or merely-borrowing callee is not treated as consumed (no false release, no false OWN002). The facts are identical to the direct handoff (a release at the call site), so the core is unchanged. Pinned by a new corpus case `ownership-handoff-use-transitive` (before -> OWN002, after silent), which lifts the benchmark recall floor 10 -> 11. The .own reduction already checks the transitive shape (the front-end's signature inference recurses); this brings it to real C#. Validated locally: case.own -> OWN002, corpus 9/9, the constructed extractor facts -> OWN002. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Roslyn extractor's ChangesTransitive Ownership Handoff Detection
Sequence Diagram(s)sequenceDiagram
participant ConsumeReleaseArgs
participant ConsumesParam
participant ConsumerBody
participant DisposesLocal
ConsumeReleaseArgs->>ConsumesParam: (param, methodBody, visited={})
ConsumesParam->>DisposesLocal: check direct Dispose/Close/DisposeAsync
alt direct disposal found
DisposesLocal-->>ConsumesParam: true
else no direct disposal
ConsumesParam->>ConsumesParam: scan ImmediateInvocations for param reference
ConsumesParam->>ConsumerBody: resolve forwarding callee IMethodSymbol
ConsumerBody-->>ConsumesParam: callee body (or null)
alt callee body available
ConsumesParam->>ConsumesParam: map argument to callee parameter by name/position
ConsumesParam->>ConsumesParam: recurse(mapped param, visited ∪ {param})
else unknown/no body
ConsumesParam-->>ConsumesParam: false (conservative)
end
end
ConsumesParam-->>ConsumeReleaseArgs: consumed → mark release at call site
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 304aaaee67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // live in another file, so bind its calls with that tree's OWN model (a SemanticModel only | ||
| // resolves nodes in its own tree); the Compilation is shared across all parsed inputs. | ||
| var bodyModel = model.Compilation.GetSemanticModel(body.SyntaxTree); | ||
| foreach (var inv in body.DescendantNodesAndSelf().OfType<InvocationExpressionSyntax>()) |
There was a problem hiding this comment.
When a parameter is only captured by a nested lambda/local function that later forwards it to a real consumer, this DescendantNodesAndSelf() scan still sees the inner Inner(s) call and marks the outer method as consuming s immediately. For example, Register(Stream s) { callbacks.Add(() => Inner(s)); } would now release the caller's argument at Register(s) and report OWN002 on any following use, even though the stream has not been handed off or disposed at that call site; the traversal should avoid descending into nested function bodies for this inference.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 896-903: The ConsumerBody method currently only checks for
BaseMethodDeclarationSyntax but misses local functions which are represented as
LocalFunctionStatementSyntax. Add a parallel check in the foreach loop to handle
LocalFunctionStatementSyntax in addition to BaseMethodDeclarationSyntax. Since
LocalFunctionStatementSyntax has the same Body and ExpressionBody properties as
BaseMethodDeclarationSyntax, extract and return the body or expression body from
local functions using the same pattern of checking d.Body ?? d.ExpressionBody to
avoid missing forwarding chains through local functions.
- Around line 930-944: Replace the `DescendantNodesAndSelf()` call in the
foreach loop within `ConsumesParam()` function with the same filtering pattern
used elsewhere in the codebase (referenced at line 396) that excludes lambda and
local-function bodies. Additionally, apply the identical filter to the
`DisposesLocal()` function wherever it uses `DescendantNodesAndSelf()`. This
prevents deferred or conditional code paths within lambdas and local functions
from being incorrectly treated as immediate execution, which is causing false
positive/negative OWN002/OWN001 violations.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 771307e5-e1c5-4183-ac52-50c8b7487420
📒 Files selected for processing (9)
.github/workflows/ci.ymlcorpus/real-world/README.mdcorpus/real-world/ownership-handoff-use-transitive/after.cscorpus/real-world/ownership-handoff-use-transitive/before.cscorpus/real-world/ownership-handoff-use-transitive/case.owncorpus/real-world/ownership-handoff-use-transitive/expected-diagnostics.txtcorpus/real-world/ownership-handoff-use-transitive/notes.mddocs/proposals/P-016-deep-fact-extraction.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…es for consume inference
Codex P2: ConsumesParam's DescendantNodesAndSelf scan (and the pre-existing DisposesLocal)
descended into nested lambdas / local functions, so a parameter disposed or forwarded ONLY
inside a stored callback — `Register(Stream s) { callbacks.Add(() => Inner(s)); }` — was
wrongly treated as consumed at the call site. That would release the caller's argument at
`Register(s)` and report a phantom use-after-handoff (false OWN002), even though the dispose
runs DEFERRED, not at the call boundary.
Both scans now use DescendantNodesAndSelf(descendIntoChildren: not lambda/local-function) —
the same deferred-body rule the flow lowering already follows (PR #57/#59). A dispose/forward
inside a nested function contributes nothing to the consume inference. Conservative: it can
only DROP a consume (a missed handoff = a leak, never a false use-after-release).
Pinned by a flow-locals silent control: DeferredHandoffNoFalsePositive's `defer` is passed to
a deferred-lambda "consumer", used, then disposed normally — it must stay SILENT (it would
trip a false OWN002 without the fix). The transitive corpus case (Inner forwarded directly,
not in a lambda) is unchanged. corpus 9/9, ownir 116/116.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/samples/FlowLocalsSample.cs`:
- Around line 492-496: The DeferredConsumer method currently invokes the
discharge action immediately (discharge()), which causes the Stream parameter to
be disposed synchronously before the method returns. To make this a true
deferred-handoff case as intended, remove the immediate invocation of
discharge() within the DeferredConsumer method. Instead, only create the Action
that wraps the Dispose call, and let the caller decide when or if to invoke it.
This will ensure the parameter is not consumed at the call site and properly
demonstrates the deferred handoff pattern without creating a use-after-dispose
scenario.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e5e73b6-8ea3-4b62-a66b-6f4f8c90d0e3
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/roslyn/OwnSharp.Extractor/Program.cs
…callees, fix deferred control Addresses the CodeRabbit review on the transitive consume slice: - Extract `ImmediateInvocations(body)` — the shared "descendants excluding nested lambda / local-function bodies" scan — and use it in both ConsumesParam and DisposesLocal (the deferred-scope fix from c99cee3, now de-duplicated; CodeRabbit Major, already addressed). - ConsumerBody now also returns a LOCAL FUNCTION's body (LocalFunctionStatementSyntax has the same Body/ExpressionBody): a directly-called local function runs synchronously, so a forwarding chain through one is followed too — closes a transitive false-negative (CodeRabbit). - Fix the deferred-handoff control: DeferredConsumer stored the disposer in a lambda but then INVOKED it synchronously, so the stream was actually disposed at the call (a real bug, not a clean control — CodeRabbit Major). It now only STORES the callback (a registry, not drained), so nothing is disposed at the call site; `defer` is disposed once by its own Dispose() and stays SILENT — the genuine no-false-OWN002 guard. Behaviour unchanged on the catching path: the transitive corpus case (Inner forwarded via a direct method call) still hits recall 11 — c99cee3's benchmark was green at the floor. corpus 9/9, ownir 116/116. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This was referenced Jun 26, 2026
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
2 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.
Cross-method use-after-dispose, through a forwarding consumer
The D4 frontier. The inter-procedural consume-handoff already caught a use-after-handoff when the callee disposes a by-value
IDisposableparameter directly (#55). But the inference deliberately stopped one hop short — a parameter merely forwarded to another call was left ambiguous (thepassedbranch inownir.py: "awaiting … a later transitive pass"). This is that pass.Before this PR,
Consume(which forwards rather than disposes) wasn't seen as a consumer, soConsume(s)wasn't a release and the laters.Lengthwas invisible.How
The extractor's consumer detection (
ConsumesParam) is now transitive: a parameter is consumed if the body disposes it directly or forwards it to another first-party consumer that consumes it — followingConsume → Inner → Disposethrough the chain, cycle-guarded (keyed per parameter), and binding each callee's body with its own tree'sSemanticModelso cross-file chains resolve correctly. SoConsume(s)is still modelled as a call-site release (same shape as poolReturn(buf)), and a use after it trips OWN002.The emitted facts are identical to the direct handoff (a
releaseat the call site), so the core is unchanged — only the extractor's consumer-recognition reaches one+ hops further.Precision (0 FP): a parameter handed to an unknown or merely-borrowing callee is not treated as consumed — no false release, no false OWN002 (the conservative default, gated absolutely by the corpus benchmark's 0-FP rule on every
after.cs).Pinned
New corpus case
ownership-handoff-use-transitive(before.cs → OWN002,after.cssilent), lifting the benchmark recall floor 10 → 11 — the ratchet that asserts the catch (if the transitive logic didn't work, recall stays 10 < 11 and CI fails).Validated locally:
case.own → OWN002(the.ownfront-end already does transitive consume inference), corpus 9/9, and the constructed extractor facts (acquire s; release s @ handoff; use s) → OWN002.Remaining backlog (now narrower): a field-mediated cross-method use-after-dispose (dispose in one method, use in another via shared state) — genuinely whole-program, out of the narrow-frontend scope for now.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
defer) paths as non-reporting.Tests
Documentation
Chores