fix(extractor): PipeReader/PipeWriter are not IDisposable — stop flagging them as leaks (mined FP)#79
Conversation
…ging them as leaks (mined FP)
Mining Pipelines.Sockets.Unofficial surfaced a systematic false positive: the
field-disposable name heuristic in IsDisposableType matches anything ending in
"Reader"/"Writer", so `System.IO.Pipelines` PipeReader/PipeWriter fields
(SocketConnection._input/_output, assigned `new WrappedReader/WrappedWriter`) were
reported as undisposed-IDisposable leaks (OWN001). But PipeReader/PipeWriter are
NOT IDisposable — they finish via Complete(), not Dispose() — so this is a false
alarm, and it would fire on essentially any code using System.IO.Pipelines.
New IsNonDisposableReaderWriter excludes PipeReader/PipeWriter (matched on the
simple name, so a qualified `System.IO.Pipelines.PipeReader` is covered too) from
the EndsWith("Reader"/"Writer") heuristic. The curated IDisposable names and the
…Stream/…Subscription families are unchanged; no existing corpus/sample uses these
types, so behaviour elsewhere is unaffected.
Regression guard: frontend/roslyn/samples/PipeFieldsSample.cs (a class that
constructs PipeReader/PipeWriter fields and never disposes them) is added to the
wpf-extractor CI job, which asserts it produces NO finding.
Found by `scripts/mine.sh` over a real repo; not reproducible without a .NET SDK
locally, so validated by CI + a re-mine.
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)
📝 WalkthroughWalkthrough
ChangesPipeReader/PipeWriter false-positive fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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://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: 7a607a7aa0
ℹ️ 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".
| static bool IsNonDisposableReaderWriter(string t) => | ||
| (t.Contains('.') ? t[(t.LastIndexOf('.') + 1)..] : t) is "PipeReader" or "PipeWriter"; |
There was a problem hiding this comment.
This helper strips the namespace and suppresses every type whose simple name is PipeReader or PipeWriter, not just System.IO.Pipelines.PipeReader/PipeWriter. In a project that owns a disposable MyLib.PipeReader field/local (or a third-party disposable type with that simple name), the EndsWith("Reader"/"Writer") heuristic on lines 1415-1417 no longer emits the disposable fact, so WPF003/D1/flow-locals silently miss a leak that was previously reported. Please constrain the exemption to the actual System.IO.Pipelines symbols/qualified names rather than all simple-name matches.
Useful? React with 👍 / 👎.
…der/PipeWriter (Codex) Codex: stripping the namespace excluded any type whose simple name is PipeReader/PipeWriter, so a project's own disposable `MyLib.PipeReader` would be silently dropped (a false negative). Match the exact bare or `System.IO.Pipelines`-qualified spelling instead; the bare spelling (the mined FP and the regression sample) stays excluded, a differently-namespaced type does not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
Mined false positive
Running
scripts/mine.shover Pipelines.Sockets.Unofficial (a pool/pipe-heavy real repo) surfaced a systematic false positive.SocketConnection._input/_output— declaredPipeReader/PipeWriterand assignednew WrappedReader(...)/new WrappedWriter(...)— were reported as OWN001 "IDisposable field never disposed (leak)".But
System.IO.Pipelines.PipeReader/PipeWriterare notIDisposable— they finish viaComplete(), notDispose()(the wrapper'sComplete()calls the inner reader'sComplete()). The cause is the field-disposable name heuristic inIsDisposableType, which matches anything ending inReader/Writer. This would fire on essentially any code usingSystem.IO.Pipelines(ubiquitous in modern .NET networking), so it's worth a precise fix.Fix
New
IsNonDisposableReaderWriterexcludesPipeReader/PipeWriterfrom theEndsWith("Reader"/"Writer")branch, matched on the simple name so a qualifiedSystem.IO.Pipelines.PipeReaderis covered too. The curated IDisposable names and the…Stream/…Subscriptionfamilies are unchanged; no existing corpus or sample uses these types, so behaviour elsewhere is unaffected (the recall floor is unchanged).Regression guard
frontend/roslyn/samples/PipeFieldsSample.cs— a class that constructsPipeReader/PipeWriterfields and never disposes them — is added to thewpf-extractorCI job, which asserts it produces no finding.Validation
.owncorpus + core are unaffected by this C# change).wpf-extractorassertion (PipeFieldsSample.cs→ no finding) fails on the pre-fix heuristic and passes with the exclusion. I can't run the extractor without a local SDK, so CI is the check.Out of scope (noted)
The same mine run surfaced a second, harder FP — a pooled buffer passed to an ownership-taking constructor and returned (
ArrayPoolBufferWriter.CreateNewSegment:var array = pool.Rent(...); return new ArrayPoolRefCountedSegment(pool, array, prev);). The intra-procedural escape analysis treats a pool buffer passed as an argument as a borrow and expects a same-methodReturn, missing the ownership transfer through the constructor. That needs a deeper change and is a separate follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
PipeReader/PipeWriterfields: these are now correctly treated as completing viaComplete()(notDispose()), preventing false undisposed-disposable leak reports.PipeReader/PipeWritersample to guard against regressions.