fix(extractor): pooled buffer passed to an escaping constructor is a transfer, not a leak (mined FP)#80
Conversation
…an ownership transfer, not a leak (mined FP) Second false positive from mining Pipelines.Sockets.Unofficial: ArrayPoolBufferWriter.CreateNewSegment does `var array = pool.Rent(size); return new ArrayPoolRefCountedSegment(pool, array, prev);`. The flow-locals escape analysis treats a pooled buffer passed as an argument as a BORROW (so `pool.Return(buf); Work(buf)` still trips use-after-return) and expects a same-method Return — missing that the buffer's ownership is TRANSFERRED to the constructed object (which Returns it on its own teardown) and leaves the method inside that returned object. Result: a false OWN001 "never disposed/returned". New PassedToEscapingCtor: a pooled buffer handed to a `new` whose result is the direct return value or a field-assignment RHS escapes (untracked), like a returned local. One level only — a `new` buried in a local or another expression stays a borrow (an honest limitation). Purely syntactic, so it holds for unresolved wrapper types. Only the --flow-locals path is affected (the syntactic D1 path already escapes all arg-passed locals); a plain borrow `Work(buf)` still leaks if unreturned. Regression guard: FlowLocalsSample.PooledIntoReturnedCtor (`ctorMoved` handed to a returned PooledHolder) added to the --flow-locals job's must-stay-silent list. No corpus case has this shape, so the recall floor is unchanged. 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: 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)
📝 WalkthroughWalkthroughAdds a ChangesEscaping-constructor ownership-transfer pattern
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: 735d59fba6
ℹ️ 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".
| idn.Parent is ArgumentSyntax { Parent: ArgumentListSyntax | ||
| { Parent: BaseObjectCreationExpressionSyntax oce } } | ||
| && (oce.Parent is ReturnStatementSyntax | ||
| || (oce.Parent is AssignmentExpressionSyntax a && a.Right == oce && FieldName(a.Left) is not null)); |
There was a problem hiding this comment.
When the constructed wrapper is assigned to an already-declared local, e.g. PooledHolder h; h = new PooledHolder(buf);, this condition still returns true because FieldName returns a name for any bare identifier, not just fields. The caller at line 2419 then removes buf from tracked, so a wrapper that never leaves the method and never returns the ArrayPool buffer is reported as silent instead of the expected missing-Return leak; this contradicts the intended one-level-only behavior where a constructor result stored in a local stays a borrow.
Useful? React with 👍 / 👎.
…d assignment (Codex) Codex: FieldName(a.Left) returns a name for any bare identifier, so a constructor result assigned to a LOCAL (`PooledHolder h; h = new PooledHolder(buf);`) was mistaken for a field-store and escaped the buffer — silencing a real missing-Return leak when that local never leaves the method. Gate the field-assignment case on the LHS resolving to an IFieldSymbol (symbol-based); the return-value case (the mined FP and the FlowLocalsSample regression) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![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 1047-1051: The PassedToEscapingCtor method is too permissive in
determining whether an identifier escapes through a constructor call. The
condition on line 1051 uses FieldName(a.Left) is not null to check if an
assignment is to a field, but FieldName also matches local identifiers, causing
local variable assignments like tmp = new Wrapper(buf) to be incorrectly treated
as field escapes. This leads to buf being untracked at line 2419, hiding real
OWN001 leaks. Modify the condition in PassedToEscapingCtor to properly
distinguish between field assignments and local variable assignments, ensuring
that only actual field escapes are considered escaping constructors, not
temporary local variable assignments.
🪄 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: 248d07ac-e609-4d90-8233-fc84c1a4d6cf
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.cs
2 participants
Mined false positive (the second one)
The same mine run on Pipelines.Sockets.Unofficial that surfaced the PipeReader/PipeWriter FP (#79) also flagged
ArrayPoolBufferWriter.CreateNewSegment:The
--flow-localsescape analysis treats a pooled buffer passed as an argument as a borrow (deliberately — sopool.Return(buf); Work(buf)still trips use-after-return), and so expects a same-methodReturn. It misses that the buffer's ownership is transferred to the constructed object (ArrayPoolRefCountedSegmentreturns it inReleaseImpl) and leaves the method inside that returned object → a false OWN001 "never returned".Fix
New
PassedToEscapingCtor: a pooled buffer handed to anew …(…, buf, …)whose result is the direct return value or a field-assignment RHS escapes (untracked), exactly like a returned local. Purely syntactic, so it holds even when the wrapper type doesn't resolve.Only the
--flow-localspath is touched (the syntactic D1 path already escapes all arg-passed locals); a plain borrowWork(buf)still leaks if unreturned.Precision / scope
newburied in a local (var w = new X(buf); return w;) or another expression stays a borrow (an honest limitation, noted).return new X(pooledBuf)shape, so the recall floor is unchanged; no existing sample/corpus behaviour changes.Regression guard
FlowLocalsSample.PooledIntoReturnedCtor(ctorMovedhanded to a returnedPooledHolder) is added to the--flow-localsjob's must-stay-silent list — it fails on the pre-fix analysis and passes with the escape.Validation
return new X(Rent…)pattern).With #79 (PipeReader/PipeWriter) this closes both false positives mined on Pipelines.Sockets.Unofficial (3 findings → 0).
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Tests
Chores
Bug Fixes