fix(report): label a flow-path ArrayPool rent a "pooled buffer", not "disposable"#97
Merged
Merged
Conversation
…"disposable"
The --body-throw-edges Npgsql capstone surfaced our own reporting bug: an ArrayPool
Rent leaked/misused on the path-sensitive flow path (e.g. Returned on the normal path
but not the throw edge — CompositeBuilder/BitStringConverters) was reported as
"IDisposable local '…' … [resource: disposable]" instead of "pooled buffer '…' …
[resource: pooled buffer]". The flat pool detector (rkind=="pool") already worded
never-returned rents correctly; only the FLOW path mislabelled them, because the
acquire fact carried no resource kind, so the bridge defaulted every flow-local to
"disposable".
Fix: the extractor stamps the flow acquire's `kind` ("pool" for an ArrayPool Rent,
else "disposable"); the bridge carries it onto the handle and words/tags the finding
accordingly — OWN001 reads "rented but never returned" / "may not be returned to the
pool on every path", OWN002/003/009 read in Return terms, tag [resource: pooled
buffer]. Plain disposables are unchanged.
Pinned by tests/fixtures/ownir/flow_pool_partial.facts.json + a test_ownir bridge
check (118 now) and, end-to-end, by FlowLocalsSample.PoolReturnedOnOnePath + a CI
assertion (extractor stamps kind -> bridge labels pooled buffer).
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe flow-local analysis pipeline gains pooled-buffer awareness: the Roslyn extractor now emits a ChangesPool-aware flow-local labeling
Sequence DiagramssequenceDiagram
participant Roslyn as Roslyn Extractor
participant OwnIR as OwnIR Lowering
participant Finding as Finding Mapper
Roslyn->>Roslyn: Detect ArrayPool<T>.Rent initialization
Roslyn->>OwnIR: Emit acquire {kind: 'pool'}
OwnIR->>OwnIR: Store pool=True on synthetic handle
OwnIR->>Finding: Route to flow-local OWN001 formatter
Finding->>Finding: Check handle.pool flag
Finding->>Finding: Format: "pooled buffer may not be returned to the pool"
Finding->>Finding: Set Finding.kind = 'pooled buffer'
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Npgsql capstone on CompositeBuilder/BitStringConverters ArrayPool rents). | ||
| public void PoolReturnedOnOnePath(bool c) | ||
| { | ||
| var partialBuf = ArrayPool<byte>.Shared.Rent(16); |
Pure line-wrapping of the new pooled-buffer flow messages via implicit string concatenation — the rendered strings are byte-identical (test_ownir 118/118, the CI flow assertion unchanged). No logic change. 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.
🧹 Nitpick comments (1)
tests/test_ownir.py (1)
454-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten this regression check to fail on unexpected extra findings.
The loop validates expected events, but this block still passes if additional unrelated findings appear. Add an exact event-set/size assertion before per-event checks.
♻️ Suggested test hardening
ppfindings = check_facts(ppfacts) by = {x.event: x for x in ppfindings} checks += 1 + expected_events = {"buf", "nbuf", "d"} + if set(by.keys()) != expected_events: + fails.append(f"pool-label: expected events {sorted(expected_events)}, got " + f"{sorted(by.keys())}") + want = [ ("buf", "pooled buffer", "may not be returned to the pool on every path", "pooled buffer"), ("nbuf", "pooled buffer", "rented but never returned to the pool", "pooled buffer"), ("d", "disposable", "may not be disposed on every path", "disposable"), ]🤖 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 `@tests/test_ownir.py` around lines 454 - 471, The test currently validates that expected events exist with correct properties, but does not fail if unexpected additional findings are present in the ppfindings. Before the loop that iterates through the want list and checks each event, add an assertion to verify the exact event-set or size of findings matches expectations. Compare the keys from the by dictionary (which maps x.event to x for all ppfindings) against the expected events extracted from the want list to ensure no extra findings are present that would otherwise be silently ignored.
🤖 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 `@tests/test_ownir.py`:
- Around line 454-471: The test currently validates that expected events exist
with correct properties, but does not fail if unexpected additional findings are
present in the ppfindings. Before the loop that iterates through the want list
and checks each event, add an assertion to verify the exact event-set or size of
findings matches expectations. Compare the keys from the by dictionary (which
maps x.event to x for all ppfindings) against the expected events extracted from
the want list to ensure no extra findings are present that would otherwise be
silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1883d757-fd6d-4175-9a6f-3724bf2a1eb6
📒 Files selected for processing (6)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FlowLocalsSample.csownlang/ownir.pytests/fixtures/ownir/flow_pool_partial.facts.jsontests/test_ownir.py
…ngs (CodeRabbit) Harden the flow-pool-label regression: fail on any extra/duplicate finding (set of events + count), so a future spurious pool/disposable finding can't slip past the per-event checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
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
3 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.
Why
The
--body-throw-edgesNpgsql capstone triage surfaced a bug in our own reporting. AnArrayPool<T>.Shared.Rent(...)buffer leaked/misused on the path-sensitive flow path — e.g.Returned on the normal path but not on a throw edge (CompositeBuilder.cs,BitStringConverters.cs) — was reported as:instead of the correct:
The flat pool detector (
rkind=="pool") already worded never-returned rents correctly; only the flow path mislabelled them, because the flowacquirefact carried no resource kind, so the bridge defaulted every flow-local to"disposable".Fix
acquire'skind—"pool"for anArrayPoolRent (IsPoolRent), else"disposable".ownir._lower_flow) carriespoolonto the handle and words/tags the finding:[resource: pooled buffer].kindabsent /"disposable"→ existing wording).Tests
tests/fixtures/ownir/flow_pool_partial.facts.json+test_ownir[resource: …]tagFlowLocalsSample.PoolReturnedOnOnePath+ CI assertionArrayPool.RentReturned on one path only → the extractor stampskindand the bridge labelspooled buffer 'partialBuf' may not be returned to the pool on every pathValidated locally:
test_ownir118/118,run_testscorpus/wpf/lifetimes/loops/spec all green.Scope: ArrayPool (
IsPoolRent) only — MemoryPoolIMemoryOwnerrents areDispose-released and keep the disposable wording. Backward-compatible: an acquire fact withoutkindstill reads as a disposable.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
ArrayPoolreturns on only some control-flow paths.Bug Fixes
[resource: pooled buffer]tagging.Tests
Chores