diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6361baa..2b62d4bd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -145,6 +145,7 @@ jobs: frontend/roslyn/samples/EventSourceCountersSample.cs \ frontend/roslyn/samples/AppDomainShutdownSample.cs \ frontend/roslyn/samples/AliasDisposeSample.cs \ + frontend/roslyn/samples/CloseReleaseSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -261,6 +262,19 @@ jobs: # 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; } + # a field released via `.Close()` (direct and null-conditional) must be SILENT — mirrors the + # local detector's Dispose/Close/DisposeAsync set (mined: Npgsql ReplicationConnection._npgsqlConnection). + if echo "$out" | grep -qE "'_closedConn'|'_closedConnQ'"; then + echo "FAIL: a field released via .Close() was wrongly reported as undisposed"; exit 1 + fi + # control: a connection-like field NEITHER closed NOR disposed must STILL warn. + echo "$out" | grep -qE "CloseReleaseSample\.cs:[0-9]+:.*\[OWN001\].*'_leakedConn'" \ + || { echo "FAIL: a field that is never closed/disposed must still warn (Close-as-release stays scoped to an actual Close call)"; exit 1; } + # Codex/CodeRabbit control: Close() credits THIS instance's field only — closing ANOTHER instance + # of the same class's same-named field must NOT suppress this object's leak (ThisFieldName, not a + # receiver-stripping name match that a same-class ContainingType check would also miss). + echo "$out" | grep -qE "CloseReleaseSample\.cs:[0-9]+:.*\[OWN001\].*'_xconn'" \ + || { echo "FAIL: other-instance .Close() must not credit this field (receiver-scoped to this/alias)"; 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 3f37f7e0..bd17fd9b 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2260,12 +2260,20 @@ or ImplicitObjectCreationExpressionSyntax && 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. + // a `.Dispose()`/`.DisposeAsync()`/`.Close()` on a field — directly (`_f.Dispose()` / `this._f.…`) + // or through an alias local (translated by SYMBOL via aliasToField) — releases that field. + // `Close()` counts as a release here exactly as it already does for LOCAL disposables (DisposesLocal + // and the flow detector both accept Dispose/Close/DisposeAsync); a Stream / DbConnection-style field + // released by Close is not a leak. Mined: Npgsql ReplicationConnection disposes its NpgsqlConnection + // field via `await _npgsqlConnection.Close(async: true)`. ThisFieldName (not FieldName) scopes the + // credit to THIS instance's field / a validated alias: `other._f.Close()` on ANOTHER instance of + // the same class must NOT mark this object's `_f` released (Codex/CodeRabbit) — and a field-symbol + // ContainingType check would NOT catch that (same class -> same ContainingType), so key on the + // `this`/bare receiver syntactically. 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) + && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close" + && ThisFieldName(m.Expression) is { } 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 @@ -2275,9 +2283,9 @@ or ImplicitObjectCreationExpressionSyntax // 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 + if (ThisFieldName(cae.Expression) is { } cdf // this-instance field / alias only (not `other._f?.Close()`) && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } - && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") + && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close") disposed.Add(model.GetSymbolInfo(cae.Expression).Symbol is ILocalSymbol lc && aliasToField.TryGetValue(lc, out var fc) ? fc : cdf); diff --git a/frontend/roslyn/samples/CloseReleaseSample.cs b/frontend/roslyn/samples/CloseReleaseSample.cs new file mode 100644 index 00000000..c5ac84d0 --- /dev/null +++ b/frontend/roslyn/samples/CloseReleaseSample.cs @@ -0,0 +1,52 @@ +using System; + +namespace Own.Samples; + +// `.Close()` releases a field, exactly as it already does for LOCAL disposables (DisposesLocal and the +// flow detector both accept Dispose/Close/DisposeAsync): a Stream / DbConnection-style field cleaned up +// by Close() is not a leak. Mined: Npgsql's ReplicationConnection releases its NpgsqlConnection field +// via `await _npgsqlConnection.Close(async: true)`. + +// a field released via `.Close()` — direct and null-conditional — must be SILENT. +public sealed class ReleasesViaClose : IDisposable +{ + private readonly FakeConnection _closedConn = new FakeConnection(); + private readonly FakeConnection _closedConnQ = new FakeConnection(); + + public void Dispose() + { + _closedConn.Close(); // Close() releases the field -> SILENT + _closedConnQ?.Close(); // null-conditional Close() -> SILENT + } +} + +// control: a field NEITHER closed NOR disposed must STILL warn OWN001. +public sealed class LeaksUnclosed : IDisposable +{ + private readonly FakeConnection _leakedConn = new FakeConnection(); + + public void Dispose() { } // never closed/disposed -> WARN +} + +// Codex/CodeRabbit control: Close() must target THIS instance's field. Closing ANOTHER instance of the +// SAME class's same-named private field must NOT credit this object — and note a field-symbol +// ContainingType check could not tell them apart (same class), so this leans on the `this`/bare receiver. +// This object's own _xconn is never closed, so it STILL leaks. +public sealed class ClosesOtherInstanceField : IDisposable +{ + private readonly FakeConnection _xconn = new FakeConnection(); + + public void CloseOther(ClosesOtherInstanceField other) + { + other._xconn.Close(); // closes ANOTHER instance's field -> must NOT credit this._xconn + } + + public void Dispose() { } // this._xconn is never closed -> WARN OWN001 +} + +// A connection-like resource whose release is Close() (the DbConnection.Close() / Stream.Close() shape). +internal sealed class FakeConnection : IDisposable +{ + public void Close() { } + public void Dispose() { } +}