feat(extractor): emit fresh-returning factory facts — D5.2 interprocedural leaks on real C##126
Merged
Conversation
… interprocedural leaks) Wire the Roslyn extractor (--flow-locals) to produce the OwnIR the D5.2 fresh-return machinery consumes, so first-party factory leaks are caught on real C# (not just synthetic unit tests): - A new'd IDisposable returned BARE outside a try stays tracked and lowers to acquire + return <var>, so the core classifies the method returnsOwned: fresh (the return discharges it -> the factory itself stays silent). A return inside a try, or a using/pool/owning-factory local, keeps its existing behaviour. - var r = FirstPartyFactory() (a source-visible method returning an owned IDisposable) emits a new call callee=... result=r op. The core mints the acquire only when it proves the callee fresh, so a non-fresh first-party call is never falsely owned. IsFirstPartyDisposableFactory gates on source-declared, non-void, disposable, non-dispose-optional return; overloads resolve to unknown (silent). Adds FactoryLeakSample.cs + a CI assertion: a dropped factory result leaks interprocedurally (OWN001 at the call site), the disposed caller and the factory stay silent. Bridge side already proven by the D5.2 unit tests; this closes the extractor gap so the capability fires on real code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds first-party disposable-factory handling to the Roslyn extractor’s flow-locals pipeline, updates OwnIR call lowering and diagnostics, and adds a sample, CI verification, tests, and documentation for the ownership-transfer behavior. ChangesFirst-party factory ownership flow
Sequence Diagram(s)sequenceDiagram
participant FactoryLeakSample.cs
participant OwnSharp.Extractor
participant ownlang/ownir.py
participant .github/workflows/ci.yml
FactoryLeakSample.cs->>OwnSharp.Extractor: run `--flow-locals`
OwnSharp.Extractor->>ownlang/ownir.py: emit `call` and `return` facts
.github/workflows/ci.yml->>ownlang/ownir.py: assert `OWN001` on `factoryLeak`
.github/workflows/ci.yml->>ownlang/ownir.py: assert silence for `factoryOk` and `made`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 5291fa9f8c
ℹ️ 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".
| return false; // void, or not first-party (no visible body to infer `fresh` from) | ||
| if (!ImplementsIDisposable(m.ReturnType) || IsDisposeOptional(m.ReturnType)) | ||
| return false; | ||
| callee = $"{m.ContainingType.Name}.{m.Name}"; |
There was a problem hiding this comment.
Because this call key drops the namespace and containing-type chain, it can alias a different source method. In a repo with A.StreamFactory.Make() returning a cached Stream (no local candidates, so it emits no functions[] entry) and B.StreamFactory.Make() returning a new stream, a call to A.StreamFactory.Make() is lowered as StreamFactory.Make; the solver can then use B's fresh summary and report a leak on a non-owned singleton result. Use the same fully qualified symbol key for both summaries and calls, or require an exact emitted function, to avoid false OWN001s.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.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 @.github/workflows/ci.yml:
- Around line 825-831: Tighten the D5.2 CI assertion so it verifies the OWN001
finding is anchored to the sample call site, not just any match containing
'factoryLeak'. Update the grep/check in the ci workflow to require the
diagnostic text for FactoryLeakSample.cs and the caller location emitted by the
interprocedural analysis, using the existing D5.2 sample/OWN001 assertion block
as the reference point.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 786-789: The emitted call op for tracked first-party disposable
factories is dropping caller arguments by using Array.Empty<string>(), which
loses ownership/borrow tracking for values like stream or owner. Update the
tracked branch in Program.cs where the anonymous call node is added so it
preserves the initializer call’s arguments instead of clearing them, using the
same argument extraction path already used for normal first-party calls and the
fpCallee returned by IsFirstPartyDisposableFactory.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 809f28bf-93c9-4ee9-acb9-e1f3e719debb
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/notes/d5-ownership-transfer.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FactoryLeakSample.cs
…ll args (CI fix + Codex/CodeRabbit)
The extractor's new call ops surfaced calls to callees not in functions[] (BCL /
extension methods like GetRequiredService), which crashed check_facts with
OWN040. Plus three review items:
- (CI crash) Gate the Call emission on a RESOLVABLE callee (in the MOS or a sink
extern); an unknown callee is dropped (no claim), never lowered to a Call that
raises OWN040. Add OWN040 to the bridge-artifact skip list belt-and-suspenders.
Regression test: a call to an unknown callee makes no claim and does not crash.
- (Codex P2) Fully-qualify the factory summary key on BOTH sides
({Namespace.Type}.{Method} via FlowFunctionName + ContainingType.ToDisplayString)
so two same-named factories across namespaces never alias into a false OWN001.
- (CodeRabbit Major) Preserve the call's tracked identifier args instead of
Array.Empty, so the core can apply the callee's per-argument effects.
- (CodeRabbit Minor) Anchor the CI assertion to FactoryLeakSample.cs:<line> at the
call site, not just any match of 'factoryLeak'.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| // Drops the fresh factory result without disposing -> OWN001 at the call site. | ||
| public static void Leaks() | ||
| { | ||
| var factoryLeak = StreamFactory.Make(); |
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 807-813: The argument collection in OwnSharp.Extractor’s
initializer handling is still using the raw invocation argument order and
ignores named argument mapping, which can misalign positional ownership effects.
Update the logic around the `fpInv.ArgumentList.Arguments` processing to
normalize `NameColon`/named arguments before building `fpArgs`, preserving the
actual parameter order expected by the bridge. Use the `fpInv` and `fpArgs`
extraction path in `Program.cs` to locate the fix.
In `@ownlang/ownir.py`:
- Around line 1538-1545: The `Call` handling in `ownlang/ownir.py` needs to
invalidate any existing ownership binding for `result` before deciding whether a
fresh acquire can be minted. In the `lower_call` path where `summ` may be
unknown and the `Call` can be dropped, make sure `localmap[result]` is cleared
or killed first so overwriting a tracked local does not leave the old handle
alive. Update the logic around `result`, `summ`, `localmap`, and the `Call`
emission check so that a reused local cannot retain its previous ownership state
when no new summary-driven acquire is created.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 999ed7bc-f58c-4fbc-829b-0843ff9a2984
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csownlang/ownir.pytests/test_ownir.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
… call args (CodeRabbit) - (Major, bridge) When a `call` op's result reuses a tracked local, drop the stale localmap binding before any optional fresh acquire. Otherwise `acquire x; x = Unknown(); release x` read as clean while the original x actually leaked (reference lost). A hoisted local keeps its single outer-scope handle. Regression test: the overwritten original now leaks OWN001. - (Minor, extractor) Emit only POSITIONAL identifier args for the factory call op; a named arg (`Wrap(second: s2, first: s1)`) would mis-attribute under the bridge's positional effect application, so named args are dropped (under-claim, never mis-align). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
PhysShell
pushed a commit
that referenced
this pull request
Jun 26, 2026
…l detection on real code Re-point the oracle push-trigger to this dev branch and set the target to Polly's product source (src/). With the D5.2 extractor wiring merged (#126), own-check (--flow-locals) now emits fresh-returning-factory facts, so this run probes whether the new interprocedural capability surfaces anything new on real Polly code, and how it buckets vs Infer# / CodeQL. Dev-branch only; revert before any merge to main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
This was referenced Jun 26, 2026
Merged
7 tasks
7 tasks
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
3 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.
The bridge from machinery to real findings
D5.1b / D5.2 / the branch-scope fix built and unit-tested the interprocedural ownership core, but it was driven only by synthetic OwnIR facts — the Roslyn extractor didn't emit the shapes that trigger it, so it found nothing new on real C#. This PR closes that gap for the fresh-returning factory case (D5.2 / T1).
What the extractor now emits (
--flow-locals)new'dIDisposablereturned bare outside atrystays tracked and lowers toacquire …; return <var>. The core then classifies the methodreturnsOwned: fresh(thereturn <var>discharges it, so the factory itself stays silent). A return inside atry(finally edges not yet modelled), or ausing/pool/owning-factory local, keeps its existing behaviour.var r = FirstPartyFactory()(a call to a source-visible method returning an ownedIDisposable) emits a newcall callee=… result=rop. The core mints the acquire only when it proves the calleefresh, so a non-fresh first-party call is never falsely owned.IsFirstPartyDisposableFactorygates narrowly: source-declared (we can see the body to infer), non-void, return implementsIDisposable, not dispose-optional. Overloads (non-unique names) resolve tounknownin the core → silent. Computed at the emission site, so no param-threading through the flow recursion.Validation
OWN001 @ factoryLeakwith the factory + disposed caller silent.frontend/roslyn/samples/FactoryLeakSample.cs+ a CI assertion in the--flow-localsstep:factoryOk) and the factory's own local (made) → silentThis is the first slice where the accumulated D5 machinery surfaces a genuinely new ownership finding on real code. Next: run the oracle on a real external repo to see it in the wild.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit
IDisposable“fresh” return values, including validation of silent/disposed flows.