fix(extractor): recognize a field disposed through a local alias (mined Npgsql NpgsqlDataSource)#90
Merged
Merged
Conversation
…ed Npgsql NpgsqlDataSource) The field-disposable detector credited a field as released only on a DIRECT `_field.Dispose()` / `_field?.Dispose()`. Npgsql's NpgsqlDataSource disposes its CancellationTokenSource through a local copy — `var cts = _cts; cts.Dispose();` — so the field looked undisposed and was reported as an OWN001 leak (false positive). Teach the `disposed` scan to follow a local that aliases a field: map each `var a = _f;` / `var a = this._f;` to its field, then translate a `.Dispose()` / `?.Dispose()` on the alias to the underlying field. Three gates keep it sound: - the alias initializer is a `this`/bare field reference (never `other._f`); - it BINDS to a real field symbol (not a same-named local); and - the alias is never REASSIGNED anywhere — a rebound local no longer tracks the field, so we conservatively decline to credit it (keeps the finding). Direct disposals are unchanged: a field with no alias entry passes through the translation untouched. Regression sample AliasDisposeSample.cs: AliasDisposes (`_aliased` via bare alias, `_aliasedQ` via `this._f` + `?.Dispose()`) is SILENT; the controls still warn — AliasNeverDisposes (`_neverDisposed`, aliased but never disposed) and ReboundAliasLeaks (`_rebound`, alias rebound to a new source). CI asserts all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // recognition requires an actual `.Dispose()` on the alias, not merely a copy. | ||
| public sealed class AliasNeverDisposes : IDisposable | ||
| { | ||
| private readonly CancellationTokenSource _neverDisposed = new CancellationTokenSource(); |
| // not the field, so the field must STILL leak (the reassignment gate declines to credit it). | ||
| public sealed class ReboundAliasLeaks : IDisposable | ||
| { | ||
| private readonly CancellationTokenSource _rebound = new CancellationTokenSource(); |
|
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)
📝 WalkthroughWalkthroughThe WPF003 disposable-field scanner in ChangesAlias Disposal Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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: c678ab65c1
ℹ️ 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".
Comment on lines
+2211
to
+2213
| foreach (var a in assigns) | ||
| if (a.Left is IdentifierNameSyntax lid) | ||
| reassignedLocals.Add(lid.Identifier.Text); |
There was a problem hiding this comment.
This reassignment gate only looks at assignment expressions, so it still credits the field when the alias is rebound through ref/out before disposal, e.g. var a = _cts; Reset(out a); a.Dispose();. In that scenario the helper can replace a with a new object, the original field leaks, and the extractor now suppresses the OWN001 finding because the alias name was never added to reassignedLocals.
Useful? React with 👍 / 👎.
| && ThisFieldName(init) is { } af | ||
| && !reassignedLocals.Contains(decl.Identifier.Text) | ||
| && model.GetSymbolInfo(init).Symbol is IFieldSymbol) | ||
| aliasToField[decl.Identifier.Text] = af; |
There was a problem hiding this comment.
Keying aliases only by identifier for the whole class makes any same-named dispose translate to this field, even outside the alias's scope. For example, if one method contains var cts = _cts; but another method disposes an unrelated local or member named cts, cts.Dispose() is credited as releasing _cts, hiding a real undisposed-field leak; similarly, two methods using the same alias name for different fields overwrite each other in this dictionary.
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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2210-2236: The aliasToField dictionary uses class-wide plain name
strings as keys, causing same-named locals across different scopes to collide
and allowing non-local declarators to be incorrectly included in the mapping.
Refactor the alias tracking to use symbol-based identification instead of plain
strings by examining the ISymbol returned from model.GetSymbolInfo(init) and
only including declarators that represent local variables (check symbol kind or
verify the parent scope context). Then update the disposal detection logic in
both the InvocationExpressionSyntax and ConditionalAccessExpressionSyntax loops
to perform symbol-based alias lookups instead of string-based dictionary lookups
against aliasToField, ensuring disposal credits are correctly attributed to the
right field within the correct scope.
🪄 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: 6c156178-2f45-4bb2-b23d-e4c6119a4e6e
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/AliasDisposeSample.cs
…t rebinds (Codex P2 + CodeRabbit) The first cut keyed the alias map by identifier STRING, scanned class-wide, and gated reassignment only on `=` expressions. Three holes, all flagged in review: - CodeRabbit (major) + Codex #2 — class-wide name keys conflate same-named locals across methods: an unrelated `c.Dispose()` in one method could credit a field aliased by a `c` in another (FN), and two aliases reusing a name overwrite each other. Now keyed by the LOCAL SYMBOL (model.GetDeclaredSymbol(decl)), and the dispose scan resolves the receiver symbol — so each alias is scope-exact. - Codex #1 — an alias rebound through a `ref`/`out` argument (`Swap(ref a)`) was not counted as reassigned, so a disposed-after-rebind alias wrongly credited the field (FN). The reassignment set now includes ref/out argument locals. - Iterating declarators by symbol (is ILocalSymbol) also drops non-local declarators from the map (locals-only, as intended). Direct field disposals are unchanged: a receiver that is not an alias local falls back to FieldName, exactly as before. Regression controls added: RefReboundAliasLeaks (`Swap(ref a)` then dispose -> _refRebound still leaks) and ScopedAliasNameLeak (an unrelated same-named `c` disposed in another method must not credit _scopedLeak -> it still leaks). CI asserts both, alongside the existing aliased-silent and never-disposed/`=`-rebound controls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // assignment expressions. | ||
| public sealed class RefReboundAliasLeaks : IDisposable | ||
| { | ||
| private readonly CancellationTokenSource _refRebound = new CancellationTokenSource(); |
| // the leak; symbol scoping keeps them distinct, so the field STILL leaks (WARN OWN001). | ||
| public sealed class ScopedAliasNameLeak : IDisposable | ||
| { | ||
| private readonly CancellationTokenSource _scopedLeak = new CancellationTokenSource(); |
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
#90 CTS-alias) for the npgsql re-mine
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
…field + alias reads (Codex + CodeRabbit) The first cut keyed the AvailableWaitHandle gate by bare identifier text via ThisFieldName, which (a) missed a read through a field ALIAS — `var s = _sem; s.AvailableWaitHandle` recorded `s`, not `_sem`, so the field stayed wrongly exempt (Codex) — and (b) could conflate a shadowing local/parameter with a same-named field (CodeRabbit). Resolve the receiver by SYMBOL: credit the field's AvailableWaitHandle read only when the receiver binds to a real field symbol via a this/bare access (excludes `other._f` and shadowing locals), OR to a field-alias local (reusing the #90 aliasToField map). Anything else is ignored. Regression: SemaphoreFieldSample gains AliasedWaitHandleSemaphore._aliasedSem (AvailableWaitHandle read through `var s = _aliasedSem`) which must STILL warn — the field is tracked through the alias. The existing controls (_optionalSem silent, _handleSem direct-read warns, _ctsControl type-scope warns) are unchanged. 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.
What
The field-disposable detector credited a field as released only on a direct
_field.Dispose()/_field?.Dispose(). Npgsql'sNpgsqlDataSourcedisposes itsCancellationTokenSourcethrough a local copy:So the field looked undisposed and was reported as an OWN001 leak — a false positive.
How (sound, three gates)
Teach the
disposedscan to follow a local that aliases a field: map eachvar a = _f;/var a = this._f;→ its field, then translate a.Dispose()/?.Dispose()on the alias to the underlying field. The alias is recognized only when:this/bare field reference (neverother._f);Direct disposals are unchanged: a field with no alias entry passes through the translation untouched, so this is purely additive.
Mined
Npgsql.NpgsqlDataSourcedisposes_ctsviavar cts = _cts; cts.Dispose();— the last documented FP class from the Npgsql mine.Regression sample (
AliasDisposeSample.cs)AliasDisposes—_aliased(bare alias +.Dispose()) and_aliasedQ(this._falias +?.Dispose()) → SILENT.AliasNeverDisposes—_neverDisposed, aliased but never disposed → STILL warns (recognition needs an actual dispose).ReboundAliasLeaks—_rebound, alias rebound to a new source before disposal → STILL warns (reassignment gate).CI asserts all three. The corpus benchmark guards against any real-C# recall regression.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Dispose/DisposeAsynccalled on eligible local aliases (including null-conditional calls) as releases of the underlying fields, improving results for undisposed-field warnings.ref/outhelpers or when multiple locals share the same alias name across different scopes.