From c678ab65c19f3f6567bd21e5bbafb05d68bb13a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 04:49:02 +0000 Subject: [PATCH 1/2] fix(extractor): recognize a field disposed through a local alias (mined Npgsql NpgsqlDataSource) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 13 +++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 24 ++++++++- frontend/roslyn/samples/AliasDisposeSample.cs | 50 +++++++++++++++++++ 3 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 frontend/roslyn/samples/AliasDisposeSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9cf2c8c3..fe9be7da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,6 +143,7 @@ jobs: frontend/roslyn/samples/FieldReleaseSample.cs \ frontend/roslyn/samples/StaticClassEscapeSample.cs \ frontend/roslyn/samples/AppDomainShutdownSample.cs \ + frontend/roslyn/samples/AliasDisposeSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -240,6 +241,18 @@ jobs: fi echo "$out" | grep -q "pooled buffer 'leakedBuf'" \ || { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; } + # field disposed through a local ALIAS (mined: Npgsql NpgsqlDataSource): `var cts = _cts; + # cts.Dispose();` (and the `this._f` / `cts?.Dispose()` shapes) releases the field -> the + # aliased fields must be SILENT. + if echo "$out" | grep -qE "'_aliased'|'_aliasedQ'"; then + echo "FAIL: a field disposed through a local alias was wrongly reported as undisposed"; exit 1 + fi + # controls: an alias that is never disposed, and an alias REBOUND to a new object, must + # both STILL leak (the recognition needs an actual dispose on an un-reassigned alias). + echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_neverDisposed'" \ + || { echo "FAIL: a field aliased but never disposed must still warn"; exit 1; } + echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_rebound'" \ + || { echo "FAIL: a field whose alias was rebound to a new object must still warn"; exit 1; } # WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+ # disposed one stays silent. "ignored" is unique to the WPF004 message. echo "$out" | grep -q "MessengerViewModel.cs" \ diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 163a6944..c5168814 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2199,21 +2199,41 @@ or ImplicitObjectCreationExpressionSyntax // disposes. Owned (not injected) = in `constructed` (computed above); // released = a `.Dispose()` call somewhere in the class. var disposed = new HashSet(); + // A local that ALIASES a field of this class — `var cts = _cts;` / `var cts = this._cts;` — + // and is then disposed THROUGH the alias (`cts.Dispose()`) releases the field's object: the + // alias and the field name the same instance. Map each such alias local -> its field so the + // disposal scan below credits the FIELD. Mined: Npgsql's NpgsqlDataSource disposes its + // CancellationTokenSource via `var cts = _cts; cts.Dispose();`. Three gates keep it sound: + // the 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). + var reassignedLocals = new HashSet(StringComparer.Ordinal); + foreach (var a in assigns) + if (a.Left is IdentifierNameSyntax lid) + reassignedLocals.Add(lid.Identifier.Text); + var aliasToField = new Dictionary(StringComparer.Ordinal); + foreach (var decl in cls.DescendantNodes().OfType()) + if (decl.Initializer?.Value is { } init + && ThisFieldName(init) is { } af + && !reassignedLocals.Contains(decl.Identifier.Text) + && model.GetSymbolInfo(init).Symbol is IFieldSymbol) + aliasToField[decl.Identifier.Text] = af; foreach (var inv in cls.DescendantNodes().OfType()) if (inv.Expression is MemberAccessExpressionSyntax m && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" && FieldName(m.Expression) is { } df) - disposed.Add(df); + disposed.Add(aliasToField.TryGetValue(df, out var ra) ? ra : df); // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose // WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the // dominant disposal shape the match above misses. Mined as an FP across ImageSharp: // `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter) // and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls. + // (The same alias translation applies — `cts?.Dispose()` on a field-aliasing local.) foreach (var cae in cls.DescendantNodes().OfType()) if (FieldName(cae.Expression) is { } cdf && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") - disposed.Add(cdf); + disposed.Add(aliasToField.TryGetValue(cdf, out var rc) ? rc : cdf); foreach (var fd in cls.Members.OfType()) { diff --git a/frontend/roslyn/samples/AliasDisposeSample.cs b/frontend/roslyn/samples/AliasDisposeSample.cs new file mode 100644 index 00000000..0cebbe77 --- /dev/null +++ b/frontend/roslyn/samples/AliasDisposeSample.cs @@ -0,0 +1,50 @@ +using System; +using System.Threading; + +namespace Own.Samples; + +// WPF003/OWN001 field disposed through a local ALIAS (mined: Npgsql NpgsqlDataSource). A field copied to a +// local (`var cts = _cts;`) and disposed through that alias (`cts.Dispose();`) IS released: the +// alias and the field reference the same object. The field-disposable detector must credit the +// FIELD so it is not reported as an undisposed leak. Contrast the controls below. +public sealed class AliasDisposes : IDisposable +{ + private readonly CancellationTokenSource _aliased = new CancellationTokenSource(); + private readonly CancellationTokenSource _aliasedQ = new CancellationTokenSource(); + + public void Dispose() + { + var a = _aliased; // bare-field alias + a.Dispose(); // disposed through the alias -> _aliased released -> SILENT + + var q = this._aliasedQ; // this-qualified alias + q?.Dispose(); // null-conditional dispose through the alias -> _aliasedQ released -> SILENT + } +} + +// Control: a field aliased to a local that is NEVER disposed must STILL leak — the alias +// recognition requires an actual `.Dispose()` on the alias, not merely a copy. +public sealed class AliasNeverDisposes : IDisposable +{ + private readonly CancellationTokenSource _neverDisposed = new CancellationTokenSource(); + + public void Dispose() + { + var a = _neverDisposed; // aliased but never disposed -> _neverDisposed leaks -> WARN OWN001 + _ = a.Token; + } +} + +// Control: a REASSIGNED alias no longer tracks the field — disposing it releases the NEW object, +// 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(); + + public void Dispose() + { + var a = _rebound; // starts as the field... + a = new CancellationTokenSource(); // ...but is rebound to a NEW source + a.Dispose(); // disposes the new one, not _rebound -> WARN OWN001 + } +} From 5a3ba5229364028950c8cc6d8b591a0a7247bf74 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 04:56:44 +0000 Subject: [PATCH 2/2] fix(extractor): key the field-alias map by local SYMBOL, count ref/out rebinds (Codex P2 + CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 7 +++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 46 ++++++++++++------- frontend/roslyn/samples/AliasDisposeSample.cs | 38 +++++++++++++++ 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe9be7da..103d3b81 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -253,6 +253,13 @@ jobs: || { echo "FAIL: a field aliased but never disposed must still warn"; exit 1; } echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_rebound'" \ || { echo "FAIL: a field whose alias was rebound to a new object must still warn"; exit 1; } + # Codex control: an alias rebound through a ref/out ARGUMENT must still leak. + echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_refRebound'" \ + || { echo "FAIL: a field whose alias was rebound via a ref/out argument must still warn"; exit 1; } + # Codex/CodeRabbit control: aliases are symbol-scoped, not name-keyed — an unrelated + # same-named local disposed in another method must NOT credit the field, so it still leaks. + echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_scopedLeak'" \ + || { echo "FAIL: a same-named local in another scope must not be miscredited (symbol-scoped aliases)"; exit 1; } # WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+ # disposed one stays silent. "ignored" is unique to the WPF004 message. echo "$out" | grep -q "MessengerViewModel.cs" \ diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index c5168814..79c0c286 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2201,39 +2201,51 @@ or ImplicitObjectCreationExpressionSyntax var disposed = new HashSet(); // A local that ALIASES a field of this class — `var cts = _cts;` / `var cts = this._cts;` — // and is then disposed THROUGH the alias (`cts.Dispose()`) releases the field's object: the - // alias and the field name the same instance. Map each such alias local -> its field so the - // disposal scan below credits the FIELD. Mined: Npgsql's NpgsqlDataSource disposes its - // CancellationTokenSource via `var cts = _cts; cts.Dispose();`. Three gates keep it sound: - // the 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). - var reassignedLocals = new HashSet(StringComparer.Ordinal); - foreach (var a in assigns) - if (a.Left is IdentifierNameSyntax lid) - reassignedLocals.Add(lid.Identifier.Text); - var aliasToField = new Dictionary(StringComparer.Ordinal); + // alias and the field name the same instance. Map each such alias LOCAL SYMBOL -> its field + // so the disposal scan below credits the FIELD. Mined: Npgsql's NpgsqlDataSource disposes its + // CancellationTokenSource via `var cts = _cts; cts.Dispose();`. Sound by construction: + // - the initializer is a `this`/bare field reference (never `other._f`) that BINDS to a real + // field symbol (not a same-named local); + // - aliases are keyed by the LOCAL SYMBOL, not its name, so a same-named local in another + // method (or a second alias reusing the name) is a DISTINCT entry, never conflated (Codex); + // - an alias REASSIGNED anywhere — by `=`, or rebound through a `ref`/`out` argument — is + // excluded: a rebound local no longer tracks the field, so we decline to credit it (Codex). + var reassignedAliases = new HashSet(SymbolEqualityComparer.Default); + foreach (var asg in assigns) + if (model.GetSymbolInfo(asg.Left).Symbol is ILocalSymbol rls) + reassignedAliases.Add(rls); + foreach (var arg in cls.DescendantNodes().OfType()) + if ((arg.RefKindKeyword.IsKind(SyntaxKind.RefKeyword) || arg.RefKindKeyword.IsKind(SyntaxKind.OutKeyword)) + && model.GetSymbolInfo(arg.Expression).Symbol is ILocalSymbol als) + reassignedAliases.Add(als); + var aliasToField = new Dictionary(SymbolEqualityComparer.Default); foreach (var decl in cls.DescendantNodes().OfType()) if (decl.Initializer?.Value is { } init && ThisFieldName(init) is { } af - && !reassignedLocals.Contains(decl.Identifier.Text) - && model.GetSymbolInfo(init).Symbol is IFieldSymbol) - aliasToField[decl.Identifier.Text] = af; + && model.GetSymbolInfo(init).Symbol is IFieldSymbol + && model.GetDeclaredSymbol(decl) is ILocalSymbol aliasSym + && !reassignedAliases.Contains(aliasSym)) + aliasToField[aliasSym] = af; + // a `.Dispose()`/`.DisposeAsync()` on a field — directly (`_f.Dispose()`) or through an + // alias local (translated by SYMBOL via aliasToField) — releases that field. foreach (var inv in cls.DescendantNodes().OfType()) if (inv.Expression is MemberAccessExpressionSyntax m && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" && FieldName(m.Expression) is { } df) - disposed.Add(aliasToField.TryGetValue(df, out var ra) ? ra : df); + disposed.Add(model.GetSymbolInfo(m.Expression).Symbol is ILocalSymbol ls + && aliasToField.TryGetValue(ls, out var fa) ? fa : df); // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose // WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the // dominant disposal shape the match above misses. Mined as an FP across ImageSharp: // `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter) // and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls. - // (The same alias translation applies — `cts?.Dispose()` on a field-aliasing local.) + // (The same alias-by-symbol translation applies — `cts?.Dispose()` on an aliasing local.) foreach (var cae in cls.DescendantNodes().OfType()) if (FieldName(cae.Expression) is { } cdf && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") - disposed.Add(aliasToField.TryGetValue(cdf, out var rc) ? rc : cdf); + disposed.Add(model.GetSymbolInfo(cae.Expression).Symbol is ILocalSymbol lc + && aliasToField.TryGetValue(lc, out var fc) ? fc : cdf); foreach (var fd in cls.Members.OfType()) { diff --git a/frontend/roslyn/samples/AliasDisposeSample.cs b/frontend/roslyn/samples/AliasDisposeSample.cs index 0cebbe77..8ea4797f 100644 --- a/frontend/roslyn/samples/AliasDisposeSample.cs +++ b/frontend/roslyn/samples/AliasDisposeSample.cs @@ -48,3 +48,41 @@ public void Dispose() a.Dispose(); // disposes the new one, not _rebound -> WARN OWN001 } } + +// Codex control: an alias rebound through a `ref`/`out` ARGUMENT (not just `=`) no longer tracks +// the field — so the field must STILL leak. The reassignment gate counts ref/out args, not only +// assignment expressions. +public sealed class RefReboundAliasLeaks : IDisposable +{ + private readonly CancellationTokenSource _refRebound = new CancellationTokenSource(); + + public void Dispose() + { + var a = _refRebound; // starts as the field... + Swap(ref a); // ...but a helper rebinds it via `ref` + a.Dispose(); // disposes whatever Swap installed, not _refRebound -> WARN OWN001 + } + + private static void Swap(ref CancellationTokenSource c) => c = new CancellationTokenSource(); +} + +// Codex/CodeRabbit control: aliases are scoped by the LOCAL SYMBOL, not its name. Here `Touch` +// aliases the field but never disposes it, while an UNRELATED local also named `c` in `Other` is +// disposed. A class-wide name key would miscredit `Other`'s `c.Dispose()` to the field and hide +// 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(); + + public void Touch() + { + var c = _scopedLeak; // aliases the field but does NOT dispose it + _ = c.Token; + } + + public void Other() + { + var c = new CancellationTokenSource(); // a DIFFERENT local named `c`... + c.Dispose(); // ...disposing it must NOT credit _scopedLeak + } +}