fix(ownir): drop pooled-view provenance on reassignment (POOL002 FP)#98
Conversation
`ViewOwnerOf` read a Span/Memory view local's owner from its declaration initializer only, ignoring later reassignments. So after `v = otherBuf.AsSpan()`, every later reference to `v` was still attributed to the original buffer — a false OWN002 (use-after-return) once that original was Returned, including the reassignment's own LHS. Fix, entirely extractor-side (the core sees no provenance): - A simple-assignment target / `ref`/`out` arg is a DEF, not a use, so it is excluded from the use scan (`IsWriteTarget`). An element/member write (`v[0] = …`) still reads the variable. - A reference to a view local past a reassignment of that same symbol drops the declared owner and goes silent (`ReassignedBetween`, source-order, symbol-matched, intraprocedural). Both edits only ever REMOVE a `use`, so the change is monotonic in findings — it cannot manufacture a false positive; its only risk is over-suppression (an FN), guarded by the fixture. Full flow-sensitive per-path provenance (branch-merge, loop back-edges) is left for later. Pinned by `FlowLocalsSample.ReassignedView`: the pre-reassignment read of a returned buffer stays a real OWN002 (exactly one, on `bufA`); the post-reassignment read of the live `bufB` is silent. Resolves P-007 OQ#1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdphCUFvstTdkYmYTkAH46
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // use-after-return -> exactly ONE OWN002 on `bufA`; `bufB` is returned after its last read -> silent. | ||
| public void ReassignedView(int n, int m) | ||
| { | ||
| byte[] bufA = ArrayPool<byte>.Shared.Rent(n); |
|
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 (5)
📝 WalkthroughWalkthroughUpdates Span view ownership tracking for reassigned locals, adds sample cases for post-return and ref/out use, and extends CI and documentation to cover the new behavior. ChangesSpan View Reassignment Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ 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: 62e7e7df60
ℹ️ 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".
| if (id.SpanStart > lo && id.SpanStart < hi && IsWriteTarget(id) | ||
| && SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(id).Symbol, sym)) | ||
| return true; |
There was a problem hiding this comment.
For a RHS reference in an assignment to the same view local, the LHS identifier has an earlier SpanStart, so this loop treats the assignment itself as a prior reassignment. After ArrayPool<byte>.Shared.Return(buf); v = v.Slice(1);, the RHS v is the stale view that should still emit OWN002, but ViewOwnerOf returns null; subsequent uses are also silenced even though Slice preserves the same owner. The rebind check needs to ignore writes from the assignment containing the current use, or compare at statement boundaries.
Useful? React with 👍 / 👎.
| && (arg.RefKindKeyword.IsKind(SyntaxKind.OutKeyword) | ||
| || arg.RefKindKeyword.IsKind(SyntaxKind.RefKeyword)) |
There was a problem hiding this comment.
ref arguments are not pure definitions: the current value is passed to the callee and can be read before any assignment, unlike out. With this condition, EmitFlowExpr skips the identifier entirely and ReassignedBetween treats it as a rebind, so cases such as Return(buf); Touch(ref v); no longer emit a use-after-return for the view, and later uses are dropped even if the callee only read it. Keep the pure-write exemption to out, or model ref as both a use and a possible write.
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: 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/OwnSharp.Extractor/Program.cs`:
- Around line 1534-1541: Adjust ReassignedBetween in OwnSharp.Extractor.Program
so the reassignment scan does not descend into nested deferred bodies like
lambdas or local functions. The current use of scope.DescendantNodes() over
BaseMethodDeclarationSyntax, AccessorDeclarationSyntax, and
LocalFunctionStatementSyntax can see writes inside deferred bodies and
incorrectly block outer uses. Limit the traversal to the immediate body of the
current scope or explicitly skip nested deferred bodies while checking
IdentifierNameSyntax writes against sym.
🪄 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: 021edd92-2846-469d-8856-d63841d4b5f1
📒 Files selected for processing (5)
.github/workflows/ci.ymlcorpus/real-world/arraypool-span-view-after-return/notes.mddocs/proposals/P-007-arraypool-span.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.cs
Addresses three over-suppression cases the Codex/CodeRabbit review of #98 found in the b' reassignment-FP fix: - ref vs out: a `ref` argument passes the current value (a read), unlike `out`. Split `IsPureWrite` (`=` LHS + `out`, excluded from the use scan) from `IsRebind` (`+ ref`, suppresses later refs), so `Return(buf); Touch(ref v);` again emits the use-after-return it had started dropping. - same-assignment LHS: in `v = v.Slice(1)` the RHS reads the still-stale view, so the assignment's own LHS must not count as a prior rebind of its own RHS — the reslice keeps tripping OWN002. - deferred bodies: `ReassignedBetween` no longer treats a write inside a nested lambda / local function as a straight-line rebind (it runs on invoke, not at its textual position). Each fix only RESTORES a true-positive use that the first cut over-suppressed; none can add a false positive. New fixtures `SliceReassignAfterReturn` and `RefArgUseAfterReturn` pin the two Codex cases (OWN002 on 'sb' / 'rb'); the deferred-body guard is defensive (exotic to fixture) and covered by the exact-count flow-locals asserts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdphCUFvstTdkYmYTkAH46
| // deferred re-slice gap, so this method has exactly the one RHS finding.) | ||
| public void SliceReassignAfterReturn(int n) | ||
| { | ||
| byte[] sb = ArrayPool<byte>.Shared.Rent(n); |
| // use-after-return -> OWN002 on 'rb'. Only `out` is the pure def that is exempt from the use scan. | ||
| public void RefArgUseAfterReturn(int n) | ||
| { | ||
| byte[] rb = ArrayPool<byte>.Shared.Rent(n); |
|
@CodeRabbit @codex review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the main changed files
echo "=== Program.cs ==="
cat -n frontend/roslyn/OwnSharp.Extractor/Program.cs
echo ""
echo "=== FlowLocalsSample.cs ==="
cat -n frontend/roslyn/samples/FlowLocalsSample.cs
echo ""
echo "=== CI workflow ==="
cat -n .github/workflows/ci.yml
echo ""
echo "=== Proposal doc ==="
cat -n docs/proposals/P-007-arraypool-span.md
echo ""
echo "=== Corpus notes ==="
cat -n corpus/real-world/arraypool-span-view-after-return/notes.mdLength of output: 175645 🏁 Script executed: #!/bin/bash
# Find the new helpers added to Program.cs in this PR
echo "=== IsPureWrite / IsRebind / ReassignedBetween / InDeferredBody ==="
grep -n "IsPureWrite\|IsRebind\|ReassignedBetween\|InDeferredBody\|ViewOwnerOf" \
frontend/roslyn/OwnSharp.Extractor/Program.cs | head -80
echo ""
echo "=== Surrounding implementation (search for IsPureWrite definition) ==="
# find the line of IsPureWrite definition
line=$(grep -n "bool IsPureWrite" frontend/roslyn/OwnSharp.Extractor/Program.cs | head -1 | cut -d: -f1)
echo "IsPureWrite defined at line $line"
[ -n "$line" ] && sed -n "$((line-2)),$((line+80))p" frontend/roslyn/OwnSharp.Extractor/Program.cs
echo ""
echo "=== ReassignedBetween definition ==="
line2=$(grep -n "bool ReassignedBetween\|ReassignedBetween(" frontend/roslyn/OwnSharp.Extractor/Program.cs | head -1 | cut -d: -f1)
echo "ReassignedBetween at line $line2"
[ -n "$line2" ] && sed -n "$((line2-2)),$((line2+60))p" frontend/roslyn/OwnSharp.Extractor/Program.cs
echo ""
echo "=== ViewOwnerOf definition and reassignment check ==="
line3=$(grep -n "string? ViewOwnerOf\|ViewOwnerOf(" frontend/roslyn/OwnSharp.Extractor/Program.cs | head -1 | cut -d: -f1)
echo "ViewOwnerOf at line $line3"
[ -n "$line3" ] && sed -n "$((line3-2)),$((line3+60))p" frontend/roslyn/OwnSharp.Extractor/Program.cs
echo ""
echo "=== EmitFlowExpr use-scan call-site (where IsPureWrite gates the use) ==="
grep -n "IsPureWrite\|IsRebind" frontend/roslyn/OwnSharp.Extractor/Program.csLength of output: 16145
Review of PR
|
| Guard | Rationale | Verdict |
|---|---|---|
id.SpanStart <= lo || id.SpanStart >= hi |
Stay within the decl→use window | ✅ |
!IsRebind(id) |
Only pure writes and ref args reset provenance | ✅ |
id.FirstAncestorOrSelf<AssignmentExpressionSyntax>() == useAssign |
sliced = sliced.Slice(1) — the LHS rebind is in the same assignment as the RHS read |
✅ |
InDeferredBody(id, scope) |
A write inside a lambda/local function runs on invoke, not at its textual position | ✅ |
SymbolEqualityComparer.Default.Equals(...) |
Symbol-based matching prevents same-named locals in other scopes from being conflated | ✅ |
The source-order-approximation trade-off (a rebind on a non-taken branch suppresses conservatively, causing a possible miss rather than a false positive) is explicitly acknowledged in comments and in the proposal doc. It's the right engineering call for the b′ slice.
InDeferredBody (Line 1569)
static bool InDeferredBody(SyntaxNode node, SyntaxNode scope)
{
for (var p = node.Parent; p is not null && p != scope; p = p.Parent)
if (p is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax)
return true;
return false;
}✅ Correct. Walks from the candidate node up to (but not including) scope. The pattern union AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax covers lambdas, anonymous methods, and local functions — the complete deferred-body set in Roslyn.
ViewOwnerOf reassignment check (Line 1528)
if (ReassignedBetween(sym, decl, idn, model))
return null;
return ViewOwner(init, model);✅ Correct placement. The check sits after the symbol is resolved but before the owner string is returned, so every call-site that asks for the view's owner for the purpose of emitting a use op gets null for post-reassignment references — causing the use to be silently dropped rather than attributed to the stale owner.
Fixtures — FlowLocalsSample.cs
All three new methods have clear, detailed comments explaining exactly which line should (and shouldn't) fire and why.
ReassignedView (Line 608) — pins the main scenario: exactly one OWN002 on bufA (the v[0] = 1 pre-reassignment read), silent thereafter. The comments label each line (i) / (ii) which directly matches the CI assertion language.
SliceReassignAfterReturn (Line 624) — the same-assignment RHS guard test. sliced = sliced.Slice(1) reads the stale view on the RHS, and the LHS must not suppress that read. One OWN002 on sb. Exactly the fixture needed to prevent the useAssign guard from over-suppressing.
RefArgUseAfterReturn (Line 635) + Touch(ref Span<byte> s) (Line 643) — confirms ref is a use, not a pure write. The static Touch helper is appropriately scoped and its single-expression body (s = s.Slice(0)) makes the rebinding intent unambiguous.
CI assertions — .github/workflows/ci.yml
The three new assertion blocks (Lines 658–678) use exact-count guards where appropriate:
nrv=$(echo "$out" | grep -cE "OWN002.*'bufA'")
[ "$nrv" = "1" ] || { echo "FAIL: expected exactly 1 OWN002 on 'bufA' ..."; exit 1; }This is the right pattern — a plain grep -q would pass even if a regression re-introduced extra false OWN002s. The bufB negative assertion is also explicit. The sb and rb assertions correctly use grep -qE (single findings, no duplicate risk).
Documentation
P-007-arraypool-span.md — Open question #1 is now well-resolved: the precise description of the gap (reassignment → false attribution), the fix (drop declared owner after rebind), and the monotonicity guarantee are all stated clearly.
corpus/real-world/arraypool-span-view-after-return/notes.md — Updated to mention Memory<T> and view reassignment. Clean and accurate.
One optional observation
There is currently no fixture for the out-argument shape — e.g.:
Span<byte> v = bufA.AsSpan(0, n);
ArrayPool<byte>.Shared.Return(bufA);
F(out v); // IsPureWrite → excluded from use scan; IsRebind → suppresses later refs
v[0] = 1; // should be silent (owner dropped after the out-rebind)The ref case is covered by RefArgUseAfterReturn, but out follows a different code path (IsPureWrite rather than IsRebind-only). Since out args exclude the view from the use scan entirely and then suppress later attribution, the combined behaviour is correct by construction — but a fixture would add confidence and document the contract. This is not blocking (the analysis is correct without it), but worth noting for a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4ecfe4be9
ℹ️ 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".
| // reference, as dropping the declared owner. A `ref` is also a USE (it is not an IsPureWrite), so it | ||
| // still emits a use of the current owner at its own position and only suppresses LATER references. | ||
| static bool IsRebind(IdentifierNameSyntax idn) => | ||
| IsPureWrite(idn) |
There was a problem hiding this comment.
Treating out as a prior rebind purely by source order drops real stale-view reads in later arguments of the same invocation. For example, after Return(buf), a call like Reinit(out v, v[0]) still evaluates v[0] before the callee assigns the out parameter, but ReassignedBetween sees the earlier out v, returns null from ViewOwnerOf, and emits no OWN002 for the returned buffer. Please ignore out/ref rebinds that are in the same invocation as the use, or otherwise model that the rebind only takes effect after argument evaluation.
Useful? React with 👍 / 👎.
… use — arguments evaluate before the callee writes the param, so Reinit(out v, v[0]) keeps tripping OWN002
| // sibling argument of the same call (it is not yet in effect during argument evaluation). | ||
| public void OutArgSiblingUseAfterReturn(int n) | ||
| { | ||
| byte[] ob = ArrayPool<byte>.Shared.Rent(n); |
|
@codex @CodeRabbit re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94d6a7f734
ℹ️ 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".
| int lo = decl.SpanStart, hi = use.SpanStart; | ||
| foreach (var id in scope.DescendantNodes().OfType<IdentifierNameSyntax>()) | ||
| { | ||
| if (id.SpanStart <= lo || id.SpanStart >= hi || !IsRebind(id)) |
There was a problem hiding this comment.
When the rebind is in a for incrementor, this source-order test treats it as occurring before a use in the loop body because the incrementor appears in the header, but C# executes it after the body. For example, after Return(buf), for (; ; v = default) { v[0] = 1; } still reads the stale view on the first iteration; this check makes ReassignedBetween return true and ViewOwnerOf drops the owner, hiding the OWN002 that was previously reported. Please ignore incrementor rebinds for uses inside the same loop body, or otherwise account for runtime order.
Useful? React with 👍 / 👎.
| // suppress a use in the loop body. | ||
| public void ForIncrementorBodyUseAfterReturn(int n) | ||
| { | ||
| byte[] fb = ArrayPool<byte>.Shared.Rent(n); |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
679-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse exact OWN002 counts for
obandfbassertions.These are currently presence checks; switching to
= 1(likebufA) will catch duplicate-regression noise too.Suggested diff
- echo "$out" | grep -qE "OWN002.*'ob'" \ - || { echo "FAIL: expected OWN002 on 'ob' (sibling-arg read before the out-write)"; exit 1; } + nob=$(echo "$out" | grep -cE "OWN002.*'ob'") + [ "$nob" = "1" ] \ + || { echo "FAIL: expected exactly 1 OWN002 on 'ob' (sibling-arg read before the out-write), got $nob"; exit 1; } - echo "$out" | grep -qE "OWN002.*'fb'" \ - || { echo "FAIL: expected OWN002 on 'fb' (for-incrementor must not suppress the body use)"; exit 1; } + nfb=$(echo "$out" | grep -cE "OWN002.*'fb'") + [ "$nfb" = "1" ] \ + || { echo "FAIL: expected exactly 1 OWN002 on 'fb' (for-incrementor must not suppress the body use), got $nfb"; exit 1; }🤖 Prompt for 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. In @.github/workflows/ci.yml around lines 679 - 688, Update the OWN002 assertions in the CI workflow to check exact match counts for the `ob` and `fb` cases instead of only verifying presence. In the test block that exercises the `Reinit(out ov, ov[0])` sibling-argument case and the for-incrementor `fv[0]` case, change the existing grep-based checks so they assert a single OWN002 occurrence for each symbol, matching the stricter pattern already used for `bufA`. This keeps the validation anchored to the existing test names and symbols while catching duplicate-regression noise.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 679-688: Update the OWN002 assertions in the CI workflow to check
exact match counts for the `ob` and `fb` cases instead of only verifying
presence. In the test block that exercises the `Reinit(out ov, ov[0])`
sibling-argument case and the for-incrementor `fv[0]` case, change the existing
grep-based checks so they assert a single OWN002 occurrence for each symbol,
matching the stricter pattern already used for `bufA`. This keeps the validation
anchored to the existing test names and symbols while catching
duplicate-regression noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eec5283c-cf42-4394-abe1-5e368e8c942f
📒 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
|
@CodeRabbit @codex re-re-review |
|
✅ Action performedFull review finished. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
ViewOwnerOfread a Span/Memory view local's owner from its declaration initializer only, ignoring later reassignments. So afterv = otherBuf.AsSpan(), every later reference tovwas still attributed to the original buffer — a false OWN002 (use-after-return) once that original was Returned, including the reassignment's own LHS.Fix, entirely extractor-side (the core sees no provenance):
ref/outarg is a DEF, not a use, so it is excluded from the use scan (IsWriteTarget). An element/member write (v[0] = …) still reads the variable.ReassignedBetween, source-order, symbol-matched, intraprocedural).Both edits only ever REMOVE a
use, so the change is monotonic in findings — it cannot manufacture a false positive; its only risk is over-suppression (an FN), guarded by the fixture. Full flow-sensitive per-path provenance (branch-merge, loop back-edges) is left for later.Pinned by
FlowLocalsSample.ReassignedView: the pre-reassignment read of a returned buffer stays a real OWN002 (exactly one, onbufA); the post-reassignment read of the livebufBis silent. Resolves P-007 OQ#1.Claude-Session: https://claude.ai/code/session_01XdphCUFvstTdkYmYTkAH46
Summary by CodeRabbit
New Features
Spanview scenarios in samples for reassignment, reslicing, and staleref/outusage.Bug Fixes
ref/outrebinding timing.Tests
OWN002assertions and negative controls.Documentation