feat(pool): MemoryPool using-owned view escape — using owner; return owner.Memory → OWN002 (POOL004)#74
Conversation
… owner.Memory` → OWN002 (POOL004) The follow-up Codex flagged on #73. `using IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n); return owner.Memory;` is sugar for `try { return owner.Memory; } finally { owner.Dispose(); }` — the implicit scope-exit dispose hands the caller a Memory backed by a buffer already returned to the pool: a dangling borrow. Previously silent because the flow-locals candidate pass skips `using` locals (auto-disposed, non-leak), so the owner never entered `tracked`. Fix (extractor only, reuses #70's onReturn / ReturnedViewOwners machinery): a new LowerFlowStatements desugars a TRACKED `using IMemoryOwner = MemoryPool.Rent(...)` declaration into `acquire; try { rest } finally { release }` — the using-dispose is threaded onto the rest's returns/throws, so a returned view of the owner is read after the release -> OWN002. Routed from LowerFlowBody, the BlockSyntax case, and the try-body. With no such `using` declaration, LowerFlowStatements lowers each statement exactly as the old loop, so every existing shape (incl. #73's `using owner; Consume(owner.Memory.Span)` after.cs) is unchanged. The candidate pass now tracks `using` MemoryPool owners (gated to IsMemoryPoolRent). Corpus: memorypool-using-view-escape (before: `using owner; return owner.Memory` -> OWN002; after: return the IMemoryOwner, ownership transferred -> silent) — the MemoryPool twin of arraypool-memory-view-escape (#70). Benchmark recall floor 17 -> 18. P-007 status updated. 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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends the ChangesMemoryPool using-view escape detection (OWN002)
Sequence Diagram(s)sequenceDiagram
participant Extractor as Roslyn Extractor (--flow-locals)
participant LowerFlowBody
participant LowerFlowStatements
participant OwnLang as OwnLang Emitter
participant Checker as OWN002 Checker
Extractor->>LowerFlowBody: process method body with continuations
LowerFlowBody->>LowerFlowStatements: lower statement list (canEscape, onThrow, onReturn)
LowerFlowStatements->>LowerFlowStatements: detect `using` MemoryPool.Rent (declaration or statement)
LowerFlowStatements->>OwnLang: emit acquire(owner)
LowerFlowStatements->>OwnLang: inject release(owner) onto return/throw paths (try/finally wiring)
OwnLang->>Checker: submit flow graph with owner.Memory read after release
Checker-->>Extractor: report OWN002 (use-after-release on returned Memory view)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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: 609fdbd9da
ℹ️ 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 (IsMemoryPoolRent(v.Initializer?.Value, model)) | ||
| candidates.Add(v.Identifier.Text); |
There was a problem hiding this comment.
When a using MemoryPool owner is returned directly (for example using var owner = MemoryPool<byte>.Shared.Rent(n); return owner;), this line adds it as a candidate, but the later escape pass still removes direct ReturnStatementSyntax references as ownership transfers. In C#, the using declaration disposes the owner as the method returns, so the caller receives an already-disposed IMemoryOwner<T>; the new using-desugaring never runs because the local has been untracked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and distinct from what this PR catches. #74 targets the dangling view escape (return owner.Memory), where the owner stays tracked. Returning the owner itself (using owner; return owner;) is a different shape: the escape pass untracks any directly-returned local as an ownership transfer — correct for a non-using owner, but wrong for a using owner (which is disposed at scope exit, so the caller gets an already-disposed IMemoryOwner).
Catching it needs the escape pass to treat a using owner's direct return as a dangling rather than a transfer (and the return handler to model the caller's use after the scope-exit release) — a separate slice from the view escape. Scoped as a follow-up and noted in the corpus notes.md; not folding it into this PR to keep the view-escape change focused. Thanks for the catch.
Generated by Claude Code
![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 431-449: The LowerFlowStatements function currently only applies
MemoryPool owner tracking and acquire/release desugaring to using declarations
(LocalDeclarationStatementSyntax with UsingKeyword), but not to the equivalent
using statement form (UsingStatementSyntax). Both syntactic forms can contain
MemoryPool owners requiring the same scope-exit disposal handling. Add logic to
detect UsingStatementSyntax in addition to the existing
LocalDeclarationStatementSyntax check, apply the same IsMemoryPoolRent
validation to the statement's resource declaration, and apply the same
acquire/release desugaring pattern. Additionally, update the candidate selection
logic (around lines 1983-1993) to also scan for UsingStatementSyntax forms when
building the tracked set, so statement-form MemoryPool owners are properly
tracked throughout the lowering process.
🪄 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: 75967709-7219-4743-bfc4-192a5e9b4912
📒 Files selected for processing (8)
.github/workflows/ci.ymlcorpus/real-world/memorypool-using-view-escape/after.cscorpus/real-world/memorypool-using-view-escape/before.cscorpus/real-world/memorypool-using-view-escape/case.owncorpus/real-world/memorypool-using-view-escape/expected-diagnostics.txtcorpus/real-world/memorypool-using-view-escape/notes.mddocs/proposals/P-007-arraypool-span.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…ool view escape (CodeRabbit) #74's using-desugar only matched the `using` DECLARATION form (LocalDeclarationStatementSyntax with UsingKeyword). The equivalent `using (IMemoryOwner owner = MemoryPool.Rent(n)) { return owner.Memory; }` STATEMENT form has the same scope-exit dispose, so the returned view dangles the same way — but UsingStatementSyntax only lowered its body and the candidate pass only scanned LocalDeclarationStatementSyntax, so the statement-form owner was untracked and silent. Extend both: the candidate pass now scans UsingStatementSyntax declarations for IsMemoryPoolRent, and the UsingStatementSyntax flow case desugars a tracked MemoryPool owner into `acquire; try { body } finally { release }` exactly like the declaration form. Corpus: memorypool-using-statement-view-escape (-> OWN002; after.cs returns the owner -> silent). Benchmark recall floor 18 -> 19. Not in scope (distinct follow-up, Codex review): `using owner; return owner;` — returning the OWNER itself (not a view) stays silent because the escape pass untracks directly-returned owners as ownership transfers; catching it needs the escape pass to treat a using-owner's direct return as a dangling rather than a transfer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
What
The follow-up Codex flagged on #73. The idiomatic MemoryPool dangle:
using owner; return owner.Memory;is exactlytry { return owner.Memory; } finally { owner.Dispose(); }— the MemoryPool twin of the ArrayPool try/finallyMemoryescape (#70). It was silent because the flow-locals candidate pass skipsusinglocals (auto-disposed → not leak candidates), so the owner never enteredtracked.How
A new
LowerFlowStatementsdesugars a trackedusing IMemoryOwner = MemoryPool.Rent(...)declaration intoacquire; try { rest } finally { release }: the implicit scope-exit dispose is threaded onto the rest's returns/throws (reusing #70'sonReturn+ReturnedViewOwnersmachinery), so the returned view's caller-use lands after the release → OWN002.LowerFlowBody, theBlockSyntaxcase, and the try-body.usingdeclaration,LowerFlowStatementslowers each statement exactly as the old loop — so every existing shape is unchanged (verified: feat(pool): MemoryPool view borrow — owner.Memory[.Span] used after Dispose → OWN002 (POOL002) #73'susing owner; Consume(owner.Memory.Span)after.csstays silent — acquire→use→release, balanced).usingMemoryPool owners (gated toIsMemoryPoolRent); areturn ownerdirectly still escapes/untracks as before.Extractor + corpus only — reuses OWN002 and the borrow lattice.
Validation
memorypool-using-view-escape(before → OWN002; after, ownership-transfer → silent) — the MemoryPool twin ofarraypool-memory-view-escape.Scope
Completes the MemoryPool escape story (use-after-explicit-Dispose from #73 + the
using-owned return-view dangle here). The remaining P-007 backlog item is a POOL005 view stored in a FIELD.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
usingdeclaration andusing (...) { }statement patterns, including more consistent flow handling.OWN002.