Harden two false positives found by mining Dapper (DisposeAsync, static fields)#20
Merged
Conversation
Mining DapperLib/Dapper surfaced 2 FP patterns in the extractor (zero real leaks in Dapper itself — the precision result we want): - `await x.DisposeAsync()` was not recognised as a release in the flow-locals detector (only Dispose()/Close()), so an async-disposed local read as a leak. EmitFlowExpr now looks through `await` and treats DisposeAsync() as disposal. (Reduced from Dapper's WrappedReaderTests *_DisposeAsync_* tests.) - a `static` IDisposable field (a process-lifetime singleton — a shared HttpClient, or a sentinel like Dapper's DisposedReader.Instance) was flagged as an owned-and-leaked field. The disposable-field detector now skips static fields. Regressions (validated in CI): FlowLocalsSample.DisposedAsync (await DisposeAsync -> silent) and DisposableFieldViewModel.SharedTokenHolder (static field -> silent), plus CI assertions. Python suite / ruff unaffected (extractor + samples are dotnet/CI). https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTwo false-positive suppressions are added to the OwnSharp extractor: ChangesAsync Disposal and Static Field Suppression
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 268-276: The code currently unwraps await expressions to expose
the inner disposal call, but it doesn't handle the common pattern of `await
x.DisposeAsync().ConfigureAwait(false)`. After unwrapping the
AwaitExpressionSyntax by assigning `expr = awaited.Expression`, add an
additional check to see if the resulting expr is an InvocationExpressionSyntax
with the method name "ConfigureAwait". If it is, extract the Expression from
that ConfigureAwait invocation to get to the underlying DisposeAsync call. This
ensures the subsequent disposal check can properly match both the direct `await
x.DisposeAsync()` pattern and the chained `await
x.DisposeAsync().ConfigureAwait(false)` pattern.
🪄 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: dc53a755-871d-46ab-988b-9b44ad4a4faf
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/DisposableFieldViewModel.csfrontend/roslyn/samples/FlowLocalsSample.cs
… a release EmitFlowExpr unwrapped `await` but not a trailing `.ConfigureAwait(false)` — the library-idiomatic chained form awaits the ConfigureAwait call, so the inner DisposeAsync was missed and the local read as a leak. Unwrap ConfigureAwait to the inner invocation before the disposal check. Addresses a CodeRabbit review comment; pinned by FlowLocalsSample.DisposedAsyncConfigured (silent) + a CI assertion. https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This was referenced Jun 21, 2026
feat(extractor): inter-procedural consume contract → use-after-handoff OWN002 (recall 8/10→9/11)
#55
Merged
10 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.
Hardening from the first mine (Dapper)
The first corpus mine (
DapperLib/Dapper, via #19'smine.yml) found zero real leaks in Dapper — the precision result we want from a well-disciplined library — and surfaced two false-positive patterns in our extractor. This fixes both, with regressions reduced from the exact Dapper shapes.FP A —
await x.DisposeAsync()not recognised as a releaseThe flow-locals detector only treated
Dispose()/Close()as disposal, so a local released viaawait reader.DisposeAsync()read as a leak. Dapper'sWrappedReaderTests*_DisposeAsync_*tests tripped this (the syncreader.Dispose()tests in the same file were correctly silent — confirming the gap was async-specific).EmitFlowExprnow looks throughawait(AwaitExpressionSyntax) and treatsDisposeAsync()as a release. The flat detectors already knewDisposeAsync; the hole was only in the flow path.FP B —
staticIDisposable field flagged as an owned leakA
staticIDisposable field is a process-lifetime singleton (a sharedHttpClient, or a sentinel like Dapper'sstatic readonly DisposedReader Instance = new()with a no-opDispose) — intentionally never disposed, not an owned leak. The disposable-field detector now skipsstaticfields.Not fixed (correctly)
The
BenchmarkBase._connectionfinding is aprotectedfield of anabstractbase in benchmark code — the flat field detector is class-local and can't see subclass disposal; a weak/low-value signal, and a reminder to point mining at productionsrc/(left as--pathsguidance, not an auto-skip).Regressions (validated in CI)
FlowLocalsSample.DisposedAsync—await x.DisposeAsync()→ silent.DisposableFieldViewModel.SharedTokenHolder— astaticIDisposable field → silent.ReportViewModel._cts) and sync-dispose cases still fire/stay-silent as before.The extractor + samples are dotnet/CI-validated (no local .NET here); the Python suite and
ruffare unaffected and green.The loop, closed
mine → triage → fixes in the extractor— exactly the cycle the miner was built for. First real codebase, two precision wins.https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
await …DisposeAsync()(includingConfigureAwait(false)chaining) is treated as proper cleanup.IDisposablesingleton fields are no longer reported as owned resource leak issues.