feat(extractor): pooled buffers through the flow engine → OWN003/OWN002 (recall 4/9→6/9)#52
Merged
Merged
Conversation
…02 (recall 4/9->6/9)
The pool pass was purely syntactic ("was this buffer Returned anywhere?") — only
POOL001 (rent-without-return). A second Return or a read after Return was invisible,
so arraypool-double-return (OWN003) and arraypool-use-after-return (OWN002) were
benchmark misses. Counting Returns would be UNSOUND (false-positives on
`if (x) Return(b); else Return(b);`), and precision is sacred.
So route pooled buffers through the path-sensitive flow engine that already proves
OWN002/OWN003 for IDisposable locals:
- a `*Pool.Rent(...)` local is an acquire (LowerFlowStmt + candidates);
- `*Pool.Return(buf)` is a release — the buffer is the ARGUMENT, not the receiver
(EmitFlowExpr);
- a read of the buffer, including in a `return` value, is a use (the return
expression is now lowered, so `return BuildResult(buf)` after Return(buf) is the
use-after-return);
- pooled buffers do NOT escape on arg-passing (the ArrayPool convention is the
renter returns it, so Work(buf)/Return(buf) are a borrow/release, not a transfer);
- the syntactic POOL001 is suppressed under --flow-locals so there is no
double-report (it stays for the legacy/no-flow path).
The core's CFG analysis then flags the double-release and the use-after-release
soundly, path-sensitive. Recall is 6/9; --min-recall raised to 6; specificity stays
9/9, 0 FP. Validated end-to-end by the corpus-benchmark CI job (no local .NET SDK).
docs: corpus-benchmark.md + P-012.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…visits them The flow-pool change compiled and didn't regress (wpf-extractor green), but the benchmark stayed 4/9: arraypool-double-return and arraypool-use-after-return declared file-scope `static` methods, which parse as top-level local functions — the extractor's per-class flow pass (cls.Members) never walks them, so the new pool tracking never ran on them (the other corpus cases are classes, hence caught). Wrap each method in a static class (mirrored before/after) + stub the helpers so the reduction is self-contained — exactly the self-containment lesson from the loaded-subscription fixture. Now the flow pass visits them: Rent -> acquire, Return -> release, the read-after-Return / second-Return -> OWN002 / OWN003. case.own is unchanged (test_corpus is unaffected). Targets recall 6/9. 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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR extends the ChangesPooled-buffer flow tracking and corpus recall improvement
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://cdn.github.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: c89b54d2e6
ℹ️ 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".
![coderabbitai[bot]](https://cdn.github.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 `@docs/notes/corpus-benchmark.md`:
- Around line 29-31: The markdown file has a heading level violation in the
ratchet section where the heading "Ratchet → 6/9 (two ratchets)" is marked as H3
(###) and its subheading is marked as H4 (####), which creates an improper level
jump. To fix this, demote the main heading "### Ratchet → 6/9 (two ratchets)" to
H2 by changing it to "## Ratchet → 6/9 (two ratchets)", and adjust the
subheading "#### → 4/9: a fixture was understating us" to H3 by changing it to
"### → 4/9: a fixture was understating us" to maintain proper heading hierarchy.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1157-1162: The POOL001 syntactic pass is being disabled entirely
when flowLocals is enabled, which removes detection for pool leaks in
field-assigned rentals since the flow-analysis only tracks locals in method
bodies. Instead of conditionally skipping the entire member iteration based on
the flowLocals flag, move the conditional check inside the loop over cls.Members
so that POOL001 suppression is applied selectively only to the locals and
methods that are actually flow-analysed (when flowLocals is true), while still
performing the POOL001 pass for other members like field assignments that fall
outside the flow-analysis scope.
- Around line 725-741: The IsPoolRent and PoolReturnBuffer methods use
unreliable text-based checking with Contains("Pool"/"pool") that can miss
aliased receivers and falsely match unrelated APIs. Replace this text-based
approach with semantic model-based type checking to properly identify ArrayPool
and MemoryPool types. The semantic model is already available at line 1276;
thread it as a parameter through the call chain LowerFlowBody → LowerFlowStmt →
EmitFlowExpr → IsPoolRent/PoolReturnBuffer so that these methods can use the
semantic model to check if the receiver type is actually an ArrayPool or
MemoryPool, eliminating the heuristic failures.
🪄 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: 84d7371b-f173-4e9c-a887-c8888502a981
📒 Files selected for processing (8)
.github/workflows/ci.ymlcorpus/real-world/arraypool-double-return/after.cscorpus/real-world/arraypool-double-return/before.cscorpus/real-world/arraypool-use-after-return/after.cscorpus/real-world/arraypool-use-after-return/before.csdocs/notes/corpus-benchmark.mddocs/proposals/P-012-bug-corpus-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…xed result, fix heading Three review fixes on the flow-pool tracking change: - POOL001 field-rent regression (Codex): gating the whole syntactic POOL001 pass on `!flowLocals` dropped coverage for field/assignment rents (`_buf = pool.Rent(n)`), which the flow engine never tracks (it only follows local declarations). Run the pass in both modes; under --flow-locals skip only the local-declaration rents (the flow pass owns those) so field-backed rents still get POOL001 and locals are not double-reported. - use-after-return fixture aliasing (Codex): BuildResult `=> buffer` handed back the pooled array itself, so after.cs's "fixed" result still aliased the buffer it returned to the pool. Return a distinct copy (`(int[])buffer.Clone()`) in both before/after so the only bug under test is the use-after-return ordering. - MD001 (CodeRabbit): the ratchet section jumped H1 -> H3. Promote it to ## and its two subsections to ###. Recall stays 6/9; precision stays absolute (every after.cs silent).
This was referenced Jun 21, 2026
Merged
11 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
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.
What
Routes pooled buffers through the path-sensitive flow engine, so the benchmark's two pool misses are caught soundly — corpus recall 4/9 → 6/9.
The extractor's pool pass was purely syntactic — "was this buffer
Returned anywhere?" → onlyPOOL001(rent-without-return). A secondReturn, or a read afterReturn, was invisible. CountingReturns would be unsound (it false-positives onif (x) Return(b); else Return(b);— exactly one runs), and precision is sacred here. So instead, reuse the flow engine that already provesOWN002/OWN003for IDisposable locals:*Pool.Rent(...)local is an acquire (candidates+LowerFlowStmt);*Pool.Return(buf)is a release — the buffer is the argument, the pool the receiver (unlikeDispose, where the local is the receiver);returnvalue — is a use (the return expression is now lowered, soreturn BuildResult(buf)afterReturn(buf)is the use-after-return);Work(buf)/Return(buf)are a borrow/release, not a transfer);POOL001is suppressed under--flow-locals(no double-report); it stays for the legacy/no-flow path.The core's CFG analysis then flags the double-release (
OWN003) and the use-after-release (OWN002) path-sensitively.The proof (benchmark, CI)
Green at
--min-recall 6:arraypool-double-return→ OWN003,arraypool-use-after-return→ OWN002, both caught; specificity stays 9/9, 0 false positives. Thewpf-extractorjob confirms the C# compiles and the existing flow-locals tests don't regress.Bonus: the benchmark diagnosed its own last gap — the first run stayed 4/9 because those reductions used file-scope
staticmethods (parsed as top-level local functions, which the per-class flow pass doesn't walk). Wrapping each in a class (mirrored before/after, helpers stubbed — the same self-containment lesson as the loaded-subscription fixture) closed it.case.ownunchanged.Files
frontend/roslyn/OwnSharp.Extractor/Program.cs— pool acquire/release recognition, pool-aware escape, return-value lowering,POOL001gated under--flow-locals,IsPoolRent/PoolReturnBufferhelpers.corpus/real-world/arraypool-{double-return,use-after-return}/{before,after}.cs— wrapped in classes..github/workflows/ci.yml— floor--min-recall 6.docs/—corpus-benchmark.md+ P-012 (the two-step ratchet to 6/9).Remaining recall backlog (3): interprocedural ownership-handoff, a cross-method use-after-dispose, a region-escape shape.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
Rent(...)throughReturn(...), improving detection accuracy for double-return and use-after-return scenarios.Bug Fixes
Documentation
Tests
Chores