feat(extractor): borrow checker — Memory<T> view that escapes after Return → OWN002 (POOL004)#70
Merged
Merged
Conversation
…eturn → OWN002 (POOL004) The borrow checker's second bite, and the genuinely dangerous one. `arraypool-span-view-after-return` (#69) caught a `Span` view used after `Return` INSIDE the method — but a `Span` is a ref struct, so the C# compiler already keeps it from escaping. A `Memory<T>` is NOT a ref struct: it can be RETURNED or stored in a field, so a `Memory` view of a pooled buffer can leave the method and dangle in the caller's hands after the buffer was recycled (a silent cross-tenant corruption). That is the escape the borrow checker has to catch, and the compiler will not. The view recognition (`ViewOwner`, renamed from `SpanViewOwner`) is extended from `AsSpan` / `new Span<T>` to `AsMemory` / `new Memory<T>` (and the `ReadOnly*` forms), resolved via the same BCL symbols (`System.MemoryExtensions`, `System.Memory<T>` ctor) — not by name, so a look-alike is not mistaken for a borrow. Because a reference to a view lowers to a use of the owner, and `return view` is such a reference (routed through the return-expression lowering), the escape of a dangling view after `Return(buf)` trips OWN002 at the escape site. Purely extractor-side; the facts are identical to the in-method case (`acquire buf; release buf; use buf`). Conservative (0 FP): only a view of a tracked owner used/returned AFTER the owner's release fires; the fix copies out of the buffer (`AsSpan(..).ToArray()`) and returns the copy, so no view escapes (the `after.cs` arm stays silent). A view used before the return, or of an untracked buffer, adds nothing. Pinned by corpus `arraypool-memory-view-escape` (before → OWN002, after silent), lifting the recall floor 12 → 13. First slice covers the RETURN escape; a view stored in a FIELD and view reassignment are left for later. Validated locally: case.own → OWN002, corpus 11/11, constructed 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Roslyn extractor's view-owner detection helper is generalized from ChangesMemory/ReadOnlyMemory View Escape Detection
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: 25da937830
ℹ️ 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".
| && m.Name.Identifier.Text is "AsSpan" or "AsMemory" | ||
| && m.Expression is IdentifierNameSyntax recv | ||
| && model.GetSymbolInfo(inv).Symbol is IMethodSymbol { ContainingType: { Name: "MemoryExtensions" } mct } | ||
| && IsInNamespace(mct, "System")) |
There was a problem hiding this comment.
When the new AsMemory view is returned from inside a try whose finally returns the pooled buffer, the escape is missed: try { return view; } finally { ArrayPool<byte>.Shared.Return(buf); } evaluates the return expression before the finally, but the caller receives the Memory<T> only after the owner has been released. The current lowering treats the mapped owner reference as a normal use before the onReturn releases, so this common cleanup pattern is considered clean even though it is the same dangling POOL004 escape.
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 851-856: The pattern match in the ViewOwner method currently only
checks for ObjectCreationExpressionSyntax, which misses C# 9+ target-typed
constructor expressions. Replace ObjectCreationExpressionSyntax with
BaseObjectCreationExpressionSyntax in the "if (e is
ObjectCreationExpressionSyntax oc" condition so that the variable oc is typed as
BaseObjectCreationExpressionSyntax instead. This broader base type will match
both explicit object creation syntax and implicit target-typed new expressions,
ensuring borrows are properly linked to their owners for diagnostics.
🪄 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: a58394bd-61be-489c-bd03-f8d1a06bac67
📒 Files selected for processing (9)
.github/workflows/ci.ymlcorpus/real-world/README.mdcorpus/real-world/arraypool-memory-view-escape/after.cscorpus/real-world/arraypool-memory-view-escape/before.cscorpus/real-world/arraypool-memory-view-escape/case.owncorpus/real-world/arraypool-memory-view-escape/expected-diagnostics.txtcorpus/real-world/arraypool-memory-view-escape/notes.mddocs/proposals/P-007-arraypool-span.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…w (Codex/CodeRabbit)
Two review fixes on the Memory-view-escape slice:
- Codex (P2): the escape was missed for THE idiomatic ArrayPool form,
`try { return view; } finally { Return(buf); }`. The `return view` is evaluated before the
finally, but the caller receives the `Memory<T>` only AFTER the finally recycles the buffer — a
dangling escape. The return lowering placed the view's owner-use BEFORE the finally release, so it
looked clean. Now a returned view's owner-use is re-emitted AFTER the finally release(s) — inserted
just before the exit of the `onReturn` chain (`[finally…, exit]`) — so the finally-Return then
trips OWN002. New helper `ReturnedViewOwners`; outside a try the use was already after any earlier
release, so nothing changes there. The corpus `before.cs` is now this idiomatic try/finally form.
Core fact shape validated locally (acquire; use; release; use; exit → OWN002).
- CodeRabbit (Major): `ViewOwner` matched only `ObjectCreationExpressionSyntax`, missing C# 9+
target-typed `Memory<byte> v = new(buf, 0, n)`. Switched to `BaseObjectCreationExpressionSyntax`
(covers explicit + implicit `new`; `ArgumentList`/`GetSymbolInfo` resolve on the base), matching
the codebase idiom.
corpus 11/11, ownir 116/116; case.own → OWN002.
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/OwnSharp.Extractor/Program.cs`:
- Around line 897-905: The ReturnedViewOwners method currently scans all
identifiers in the return expression, incorrectly flagging any tracked owner
identifier as being returned, even when only a property or method result is
returned (like return view.Length returns int, not view). Refactor the method to
only consider expressions where the view itself is being returned by checking
the root expression and its direct structure rather than iterating through all
descendant identifiers. Handle only the cases where the actual returned value is
the view: direct identifier returns, casts, parentheses, method calls that
return views (like AsMemory), and conditional expressions where branches return
views. Change the approach from
DescendantNodesAndSelf().OfType<IdentifierNameSyntax>() iteration to analyzing
the expression structure itself.
🪄 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: ebfe831d-d0c8-47c5-903a-05bcc2f82d68
📒 Files selected for processing (5)
corpus/real-world/arraypool-memory-view-escape/after.cscorpus/real-world/arraypool-memory-view-escape/before.cscorpus/real-world/arraypool-memory-view-escape/case.owncorpus/real-world/arraypool-memory-view-escape/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
✅ Files skipped from review due to trivial changes (1)
- corpus/real-world/arraypool-memory-view-escape/notes.md
🚧 Files skipped from review as they are similar to previous changes (3)
- corpus/real-world/arraypool-memory-view-escape/after.cs
- corpus/real-world/arraypool-memory-view-escape/before.cs
- corpus/real-world/arraypool-memory-view-escape/case.own
…ot every identifier (CodeRabbit)
CodeRabbit (Major): the helper scanned every descendant identifier of the return expression, so
`try { return view.Length; } finally { Return(buf); }` injected a post-finally `use buf` and tripped
a false OWN002 — even though only an `int` (the length) escapes, not the view. The escape happens
only when the RETURNED VALUE itself is the view.
Replaced the descendant scan with a structural visitor that follows only the returned value: a view
local (`return view`), an inline view expression (`return buf.AsMemory(…)`), and through
casts / parentheses / conditional branches. A member or call result of a view is not visited, so it
no longer escapes. The corpus case (`return view`) still fires; `after.cs` (`return …ToArray()`)
stays silent. corpus 11/11, 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 22, 2026
Merged
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.
Диверсия в тыл: the
Memory<T>view that escapes (P-007 POOL004)The borrow checker's second bite — and the genuinely dangerous one the compiler won't catch for you.
arraypool-span-view-after-return(#69) caught aSpanview used afterReturninside the method. But aSpanis a ref struct — the C# compiler already keeps it from escaping. AMemory<T>is not a ref struct: it can be returned or stored in a field, so a view of a pooled buffer can leave the method and dangle in the caller's hands after the buffer was recycled to someone else:How
ViewOwner(renamed fromSpanViewOwner) is extended fromAsSpan/new Span<T>toAsMemory/new Memory<T>(and theReadOnly*forms), resolved via the same BCL symbols (System.MemoryExtensions,System.Memory<T>ctor) — not by name. Because a reference to a view lowers to a use of the owner, andreturn viewis such a reference (routed through the return-expression lowering), the escape of a dangling view afterReturn(buf)trips OWN002 at the escape site.Purely extractor-side — facts identical to the in-method case (
acquire buf; release buf; use buf). The core needs no borrow/escape concept.Precision (0 FP): only a view of a tracked owner used/returned after the owner's release fires. The fix copies out (
AsSpan(..).ToArray()) and returns the copy, so no view escapes —after.csstays silent.Pinned
New corpus case
arraypool-memory-view-escape(before → OWN002,aftersilent), lifting the benchmark recall floor 12 → 13. Validated locally:case.own → OWN002, corpus 11/11, constructed extractor facts → OWN002.First slice covers the return escape; a view stored in a field (object-level escape) and view reassignment are left for later rounds.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
System.Memory<T>/ReadOnlyMemory<T>views are treated as dangling borrows after pooled-bufferReturn, aligning with existingSpan<T>handling.AsMemory()/Memory<T>escape behavior and revised target sequencing.arraypool-memory-view-escape) and updated expected diagnostics.