diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9cf2c8c3..103d3b81 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,25 @@ 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; } + # 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 163a6944..79c0c286 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2199,21 +2199,53 @@ 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 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 + && 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(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-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(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 new file mode 100644 index 00000000..8ea4797f --- /dev/null +++ b/frontend/roslyn/samples/AliasDisposeSample.cs @@ -0,0 +1,88 @@ +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 + } +} + +// 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 + } +}