fix(extractor): resolve-aware field disposability (mined ImageSharp FP #1/4)#83
Conversation
…posable (mined ImageSharp) The field-disposable leak detector classified a field's type with a pure NAME heuristic (EndsWith Stream/Reader/Writer/Subscription), so any in-project type whose name happens to end that way was flagged even when it is NOT IDisposable. Mining SixLabors/ImageSharp surfaced the FPs: undisposed `Vp8BitWriter`/`Vp8LBitWriter` (`: BitWriterBase`) and `JpegBitReader` (a struct) fields — none of them IDisposable. IsOwnedDisposableType now asks the RESOLVED type's real IDisposable/IAsyncDisposable interface; it falls back to the name heuristic only when the type does NOT resolve (an unreferenced external assembly — WPF/DevExpress on a Linux runner — which is the whole reason that heuristic exists). This generalises the #79 PipeReader/PipeWriter exclusion: any resolved non-IDisposable type is now excluded, no curated list needed. Regression sample ResolvedDisposableSample.cs: a resolved non-IDisposable `…Writer` field and a non-IDisposable `…Reader` struct stay silent; resolved IDisposable controls (MemoryStream, CancellationTokenSource) still warn. 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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the name-based ChangesResolve-aware IDisposable detection for WPF003
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}
| // Control: resolved IDisposable fields the class new's but never disposes -> must WARN. | ||
| public sealed class HolderWithRealDisposable | ||
| { | ||
| private readonly MemoryStream stream = new(); // MemoryStream IS IDisposable |
| public sealed class HolderWithRealDisposable | ||
| { | ||
| private readonly MemoryStream stream = new(); // MemoryStream IS IDisposable | ||
| private readonly CancellationTokenSource cts = new(); // CancellationTokenSource IS IDisposable |
![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: d2c0f925d8
ℹ️ 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".
| var sym = model.GetTypeInfo(type).Type; | ||
| if (sym is null or IErrorTypeSymbol) | ||
| return IsDisposableType(type.ToString()); | ||
| return ImplementsDisposable(sym); |
There was a problem hiding this comment.
For fields whose type now resolves semantically, this flags every IDisposable implementation, including types the extractor already treats as optional-dispose (Task, ValueTask, DataTable, DataSet, DataView in IsDisposeOptional). A class with private readonly DataTable table = new(); or private readonly Task task = new(...); was intentionally silent under the old syntactic IsDisposableType path but will now emit WPF003 because AllInterfaces contains IDisposable, reintroducing the Task/DataTable false positives documented just above IsDisposeOptional. Consider applying the same optional-dispose filter before returning true here.
Useful? React with 👍 / 👎.
![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 @.github/workflows/ci.yml:
- Around line 213-216: The grep assertion checking for HolderWithRealDisposable
only validates the class name is present, allowing the CI to pass even if the
diagnostic code or resource details regress. Strengthen the grep pattern in the
echo command to also match the specific diagnostic code [OWN001] and ideally the
resource field description (resource: disposable field) to ensure the complete
diagnostic output maintains the intended contract for detecting unresolved
IDisposable fields like MemoryStream or CancellationTokenSource.
🪄 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: ef0d9b93-e96e-420c-a250-0579244fbc53
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/ResolvedDisposableSample.cs
… (Codex); tighten CI assertion (CodeRabbit) Codex P2: the resolve-aware IsOwnedDisposableType flagged EVERY real IDisposable, so dispose-optional types the flat name path skipped for free — Task / ValueTask / DataTable / DataSet / DataView — would re-FP as undisposed fields. Reuse the existing ImplementsIDisposable + IsDisposeOptional helpers (shared with the flow detector): `ImplementsIDisposable(sym) && !IsDisposeOptional(sym)`. Dropped the duplicate ImplementsDisposable/IsDisposableInterface helpers I had added. Sample gains a HolderWithDisposeOptional control (a new'd, undisposed Task + DataTable) that must stay silent. CI: strengthen the positive assertion to OWN001 + the disposable-field resource on ResolvedDisposableSample (CodeRabbit), kept severity-agnostic since the disposable-field leak renders as error, not warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
3 participants
Mined from
SixLabors/ImageSharp— fix 1 of 4Mining ImageSharp (a pool/IDisposable-heavy library) surfaced 4 distinct detector FPs. This is the first and highest-value fix.
The bug
The field-disposable leak detector classified a field's type with a pure name heuristic (
EndsWith Stream/Reader/Writer/Subscription). So any in-project type whose name ends that way was flagged even when it is notIDisposable:Vp8Encoder.bitWriterVp8BitWriter : BitWriterBaseVp8LEncoder.bitWriterVp8LBitWriter : BitWriterBaseArithmeticScanDecoder.scanBufferJpegBitReader(astruct)HuffmanScanDecoder.scanBufferJpegBitReader(astruct)BitWriterBasehas no interface list andJpegBitReaderis astruct— none implementIDisposable. Four false positives.The fix
IsOwnedDisposableTypenow asks the resolved type's realIDisposable/IAsyncDisposableinterface, and only falls back to the name heuristic when the type does not resolve (an unreferenced external assembly — WPF/DevExpress on a Linux runner, which is the whole reason that heuristic exists). This generalises the #79PipeReader/PipeWriterexclusion — any resolved non-IDisposabletype is now excluded, no curated list needed.Regression guard
ResolvedDisposableSample.cs— a resolved non-IDisposable…Writerfield and a non-IDisposable…Readerstructstay silent; resolvedIDisposablecontrols (MemoryStream,CancellationTokenSource) still warn.Coming next (this mining run)
field?.Dispose()not recognised (~9 FPs)Disposenot seen (3 FPs)static classwith static handlers (2 FPs)🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit