feat(d5.4): extractor emits alias_join for verified ctor-adopt (step 2, first slice)#137
Merged
Conversation
…2, first slice) D5.4 step 2 — the Roslyn extractor learns to EMIT the `alias_join` flow op (steps 0/1 built the core primitive + bridge; this fires it on real C#). First slice: constructor adoption at the construction site — `var w = new W(x)` where W is a first-party wrapper that adopts the tracked local x into an owning field. The adopt is PROVEN, never guessed (precision-first / §11 must-only): - DisposedOwningFields(W): the set of owning fields W disposes UNCONDITIONALLY in its Dispose() — a top-level `_f.Dispose()` / `_f?.Dispose()`; conditional or nested disposes are excluded (a sometimes-dispose can't justify an alias). - TryAdoptedArgIndex(new W(...)): an adopt iff W disposes EXACTLY ONE owning field and a ctor assigns that field DIRECTLY from a single parameter (`_f = p;`), with the call positional up to that slot. Any ambiguity -> no claim (false adopt would fabricate a double-dispose, the one thing we must never do). Escape-pass interaction (the hazard): today x passed to `new W(x)` escapes (untracked). IsAdoptedArgOfBoundedWrapper keeps the adopted arg tracked ONLY when the wrapper is a non-`using` local candidate that does not itself escape (LocalEscapesSyntactically, deliberately over-approximating so we DECLINE rather than fabricate). This avoids the false-OWN001 traps (`using var w`, returned/ stored wrapper). Then the construction site emits `alias_join var=w src=x` instead of an acquire, and the per-RID core does the rest. Validated end-to-end on real C# by FactoryAdoptSample.cs (new CI step): adopt- clean and dispose-inner-clean stay silent, dropping both leaks the inner ONCE (OWN001), disposing both is OWN003, and a NON-adopting holder makes no claim so disposing both is not a false double-dispose. The bridge half + the exact rendered findings were pre-verified locally against simulated facts; the extractor emission is validated only in CI (no local dotnet). Deferred to a later slice: shape (a) return-alias / caller-side propagation, the field-store-to-`this` adopt (Polly BulkheadPolicy), and the declined using/ escaping-wrapper cases. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe extractor now recognizes a bounded adopting-wrapper pattern, keeps the adopted constructor argument tracked, emits ChangesAdopting wrapper ownership transfer
Estimated 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Dropping BOTH leaks the one underlying resource ONCE (OWN001 on the inner). | ||
| public static void AdoptLeak() | ||
| { | ||
| var adoptInnerLeak = new MemoryStream(); |
| // Disposing BOTH aliases is a double-dispose (OWN003). | ||
| public static void AdoptDouble() | ||
| { | ||
| var adoptInnerDbl = new MemoryStream(); |
![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: c958ad0c92
ℹ️ 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".
Comment on lines
942
to
+944
| if (tracked.Contains(v.Identifier.Text) | ||
| && v.Initializer?.Value is ObjectCreationExpressionSyntax adoptOce | ||
| && AdoptedArg(adoptOce, model) is IdentifierNameSyntax adoptedId |
There was a problem hiding this comment.
When the adopting wrapper is created with target-typed new, e.g. StreamAdopter w = new(inner); inner.Dispose();, this check does not run because the initializer is an ImplicitObjectCreationExpressionSyntax. The matching escape exception also misses that shape, so the inner is treated as escaped while w falls through to an independent acquire; direct disposal of the inner then produces a false OWN001 on the wrapper even though the new adopt model is supposed to let either alias discharge the shared obligation. Please cover implicit object creation in the adopt/alias path as well.
Useful? React with 👍 / 👎.
Codex P2 on #137: the adopt check and its escape exception were typed to ObjectCreationExpressionSyntax, so target-typed `StreamAdopter w = new(inner)` (an ImplicitObjectCreationExpressionSyntax) was missed — the wrapper fell through to an independent acquire and disposing the inner directly produced a false OWN001 on the wrapper, contradicting the adopt model (either alias should discharge the shared obligation). Fix: widen TryAdoptedArgIndex / AdoptedArg / IsAdoptedArgOfBoundedWrapper and the construction-site check from ObjectCreationExpressionSyntax to the common base BaseObjectCreationExpressionSyntax, which covers both `new W(x)` and `new(x)` and still resolves the ctor + ArgumentList from the symbol. The candidate detection already handled ImplicitObjectCreationExpressionSyntax. Sample/CI: FactoryAdoptSample gains AdoptTargetTyped (`StreamAdopter w = new(s); s.Dispose();`) and its locals are added to the silence assertion — dispose-inner on a target-typed adopter must stay clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 714-722: The wrapper-escape check in Program.cs only inspects the
immediate parent in the classification block around idn, so wrapped forms like
casts or binary expressions are missed. Update the ancestor walk around the
existing return/assignment/argument checks to treat any ancestor that ultimately
sits inside a return, assignment right-hand side, or argument position as
escaping, using the same wrapper-escape logic in the idn/mbody traversal.
- Around line 636-638: Reject static fields from the adopt proof by updating the
field checks in the symbol-collection logic around the recv handling and the
later matching path in Program’s adopt analysis. In the relevant IFieldSymbol
checks (the one using GetSymbolInfo(recv) and the one in the later W field
scan), require that the field belongs to W and is not static before adding it to
the result, so only instance-bounded wrapper storage can contribute to
alias_join/OWN003.
- Around line 671-686: The constructor adoption check in the top-level statement
scan is too permissive because it accepts a matching field assignment even when
the ctor body contains additional sibling statements that can rebind or mutate
ownership. Tighten the logic in the method that iterates `cbody.Statements` so
it only returns true for a pure direct `_f = p` constructor pattern, and
otherwise declines when any extra statement exists beyond the single matching
assignment. Keep the existing symbol checks with `cm.GetSymbolInfo`,
`IFieldSymbol`, `IParameterSymbol`, and the `matched` ordinal guard, but add an
explicit rejection for any non-matching sibling statement in the ctor body
before allowing `alias_join`.
🪄 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: 0fc755d1-f1bb-498a-9cba-e0121d89a266
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/notes/d5-ownership-transfer.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FactoryAdoptSample.cs
…estor escape walk (CodeRabbit)
Three CodeRabbit Major findings on the adopt verification (a false adopt would
fabricate a double-dispose, so the gate must only ever under-claim):
1. Reject static fields. DisposedOwningFields and the ctor LHS check now require
the owning field to be an INSTANCE field (!IsStatic) — a static field is shared
across all instances, not per-wrapper ownership, so aliasing on it could fake
an OWN003 across unrelated wrappers.
2. Decline a ctor that does more than the direct `_f = p`. The scan accepted a
matching assignment even amid sibling statements that could rebind the field or
mutate the param (`_f = p; Rebind(other);`). v1 now accepts ONLY a pure
single-assignment adopter ctor `{ _f = p; }`; any multi-statement ctor is
declined (deferred with the rest of step 2's remainder).
3. Walk ancestors when classifying wrapper escapes. LocalEscapesSyntactically
only inspected the direct parent, so `return (IDisposable)w;`, `x = w ?? f;`,
`Foo((object)w)` slipped through and kept the adopted arg tracked even though
the wrapper escaped — a potential false OWN001. It now climbs value-wrapping
expressions (casts/coalesce/parens/conditionals) to where the value lands (a
return, an argument, or an assignment RHS), while a bare `w.Dispose()` receiver
stays a use, not an escape.
Sample unaffected (StreamAdopter is a single-statement, instance-field, non-
escaping wrapper); validated on CI.
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 27, 2026
…lias_join Dev-loop only (revert before any merge to main): point the push-triggered oracle at App-vNext/Polly (src/) and repoint the oracle.yml push branch to this dev branch, to measure whether the new ctor-adopt alias_join (PR #137) moves any cross-tool bucket on real Polly code vs Infer# / CodeQL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
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.
D5.4 step 2 — the extractor fires
alias_joinon real C#Steps 0/1 built the RID core + the
alias_joinprimitive + the OwnIR flow op (synthetic-tested). This slice makes the Roslyn extractor emit it on real C# — the first time T4 wrap/adopt fires on actual source, not hand-authored facts.First slice: constructor adoption at the construction site —
var w = new W(x)whereWis a first-party wrapper that adopts the tracked localxinto an owning field.The adopt is proven, never guessed (§11 must-only)
A false adopt would fabricate a double-dispose — the one thing precision-first must never do — so the gate only ever under-claims:
DisposedOwningFields(W)— the owning fieldsWdisposes unconditionally inDispose()(a top-level_f.Dispose()/_f?.Dispose(); conditional/nested excluded).TryAdoptedArgIndex(new W(...))— an adopt iffWdisposes exactly one owning field and a ctor assigns it directly from a single parameter (_f = p;), with the call positional up to that slot. Any ambiguity → no claim.The escape-pass hazard (and how it's gated)
Today
xpassed tonew W(x)escapes (untracked). To make the alias meaningfulxmust stay tracked — but only safely.IsAdoptedArgOfBoundedWrapperkeeps the adopted arg tracked only when the wrapper is a non-usinglocal candidate that does not itself escape (LocalEscapesSyntactically, deliberately over-approximating so we decline rather than fabricate). This sidesteps the false-OWN001traps (using var w, a returned/stored wrapper). Then the construction site emitsalias_join var=w src=xinstead of an acquire, and the per-RID core (steps 0/1) does the rest.Validation
FactoryAdoptSample.cs:Deferred to a later slice
Shape (a) return-alias / caller-side propagation (
var r = Create(reader)→aliasOf:reader); the field-store-to-thisadopt (PollyBulkheadPolicy(factory())); and theusing/escaping-wrapper cases the v1 gate declines. These need return-skeletonaliasOf:iinference and/or cross-method field analysis.Refs
docs/notes/d5-ownership-transfer.md§7, §11 (D5.4 step 2).🤖 Generated with Claude Code
https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
Generated by Claude Code
Summary by CodeRabbit