Skip to content

Commit 40756e3

Browse files
committed
fix(extractor): base-chain bare DisposeAsync + non-generic-IEnumerator regression pin (review r2)
Second review round on #240: - Bare DisposeAsync on a BASE (P2): the belt-and-braces async check used nt.GetMembers() (current type only), so a base with a bare `public ValueTask DisposeAsync()` that does NOT implement IAsyncDisposable slipped its real async cleanup through — AllInterfaces misses it (no interface) and the member scan misses it (inherited). Now walks the whole base chain. New flagged control BareInheritedAsyncEnumerator ('ba'). - Non-generic IEnumerator regression pin (P2): the IEnumerator<T>-only narrowing had no negative fixture. Added LegacyEnumerator (IEnumerator + IDisposable, empty Dispose) -> must stay flagged ('ng'). Flow count 6 -> 8. - Stale docs (P3): the HasEmptyDisposeBody header, the sample-file header still said "generalises to any type" — the exact over-broad framing that produced #238. Rewritten to the actual IEnumerator<T>-only scope. Verified with the real extractor: flow flags exactly s,r,lr,d,ar,ia,ba,ng with the two generic enumerators (e,x) silent; full-sample diff vs main byte-identical. run_tests 276/276, ruff, mypy green. Refs #238 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
1 parent 0d1411c commit 40756e3

3 files changed

Lines changed: 91 additions & 21 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,16 +1128,24 @@ jobs:
11281128
# the same silent-false-negative class #238 exists to close.
11291129
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ia' is never disposed" \
11301130
|| { echo "FAIL: #240 flow: an inherited-IAsyncDisposable enumerator must still leak"; exit 1; }
1131+
# #240 review round 2: a BASE with a bare DisposeAsync (no IAsyncDisposable interface) must
1132+
# be caught by the base-chain scan -> still leak.
1133+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ba' is never disposed" \
1134+
|| { echo "FAIL: #240 flow: a base-chain bare DisposeAsync enumerator must still leak"; exit 1; }
1135+
# #240 review round 2: a NON-generic IEnumerator (does not force IDisposable) must stay
1136+
# flagged -> pins the IEnumerator<T>-only narrowing.
1137+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ng' is never disposed" \
1138+
|| { echo "FAIL: #240 flow: a non-generic-IEnumerator empty-Dispose local must still leak"; exit 1; }
11311139
# #238 soundness: ScratchReader (empty source Dispose, NOT an enumerator) must stay flagged
11321140
# in the flow path too — a weaver can rewrite a non-enumerator Dispose at build time.
11331141
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'s' is never disposed" \
11341142
|| { echo "FAIL: #238 flow: a NON-enumerator empty-source-Dispose local must stay flagged"; exit 1; }
11351143
# exactly 5 EmptyDisposeSample findings -> the two provably-empty-Dispose ENUMERATOR locals
11361144
# ('e' plain, 'x' explicit-interface — the #238 coverage fix) are SILENT; any leak of them
1137-
# would push the count past 6.
1145+
# would push the count past 8 (controls: s,r,lr,d,ar,ia,ba,ng).
11381146
n225=$(echo "$out" | grep -cE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN00")
1139-
[ "$n225" = "6" ] \
1140-
|| { echo "FAIL: #225/#238 flow: expected exactly 6 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
1147+
[ "$n225" = "8" ] \
1148+
|| { echo "FAIL: #225/#238 flow: expected exactly 8 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
11411149
# #238 weaver kill-switch: a directory carrying FodyWeavers.xml disables the exemption even
11421150
# for a perfect enumerator shape — the weaved fixture's local must be FLAGGED.
11431151
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \

‎frontend/roslyn/OwnSharp.Extractor/Program.cs‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,14 +1431,17 @@ static bool IsNoOpDisposeWrapper(BaseObjectCreationExpressionSyntax oce, Semanti
14311431
return arg0 is not null && IsInMemoryDisposableBacking(model.GetTypeInfo(arg0).Type);
14321432
}
14331433

1434-
// Issue #225 — a user-defined type whose parameterless `Dispose()` body is provably EMPTY holds no
1435-
// resource behind the interface: it implements IDisposable only to satisfy a contract (e.g.
1436-
// `IEnumerator<T>`), so never calling Dispose() cannot leak. Recognised from SOURCE: the type
1437-
// declares `void Dispose()` with a BLOCK body of ZERO statements, and no base in its chain carries a
1438-
// Dispose to cascade to (the base is `object` / not IDisposable), so nothing real is skipped. A body
1439-
// we cannot read (third-party metadata-only, or expression-bodied doing work) is NOT provably empty
1440-
// and stays flagged — never a guessed drop. This generalises the named-BCL IsNoOpDisposeWrapper /
1441-
// IsDisposeOptional idea to any type. Deliberately scoped to LOCAL disposables (the #201-sweep
1434+
// Issue #225 (narrowed by #238) — a user-defined `IEnumerator<T>` whose parameterless `Dispose()`
1435+
// body is provably EMPTY holds no resource behind the interface: `IEnumerator<T>` is the one
1436+
// interface that FORCES IDisposable onto types that usually hold nothing, so never calling its
1437+
// empty Dispose() cannot leak. Recognised from SOURCE: the type declares `void Dispose()` (plain or
1438+
// explicit-interface) with a BLOCK body of ZERO statements, and no base in its chain carries a
1439+
// Dispose to cascade to, no IAsyncDisposable / bare DisposeAsync anywhere in the type or its bases,
1440+
// and no FodyWeavers.xml above it (an IL weaver could rewrite the "empty" body at build time — the
1441+
// #238 soundness lesson). A body we cannot read (metadata-only, or expression-bodied doing work) is
1442+
// NOT provably empty and stays flagged — never a guessed drop. Deliberately NOT "any type": that
1443+
// over-broad framing was the #238 regression (263 silently-swallowed ClosedXML findings). Scoped to
1444+
// LOCAL disposables (the #201-sweep
14421445
// evidence — ClosedXML's `Slice.Enumerator` locals): a FIELD keeps its own disposal contract, so an
14431446
// empty-Dispose field (the OwnIgnoreSample `Handle` stand-in) is unaffected.
14441447
static bool HasEmptyDisposeBody(ITypeSymbol? t)
@@ -1478,10 +1481,14 @@ static bool HasEmptyDisposeBody(ITypeSymbol? t)
14781481
return false;
14791482
// Belt-and-braces for a bare `DisposeAsync()` method that the flow detector may treat as a
14801483
// release without the type formally implementing IAsyncDisposable (declared or explicit-impl).
1481-
if (nt.GetMembers().OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0
1482-
&& (m.Name == "DisposeAsync"
1483-
|| m.ExplicitInterfaceImplementations.Any(x => x.Name == "DisposeAsync"))))
1484-
return false;
1484+
// Walk the ENTIRE base chain (review #240 round 2): GetMembers() returns only members declared
1485+
// on the current type, so a base with a bare DisposeAsync (no IAsyncDisposable, so AllInterfaces
1486+
// above misses it) would otherwise slip its real async cleanup through as an exemption.
1487+
for (var cur = nt; cur is not null && cur.SpecialType != SpecialType.System_Object; cur = cur.BaseType)
1488+
if (cur.GetMembers().OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0
1489+
&& (m.Name == "DisposeAsync"
1490+
|| m.ExplicitInterfaceImplementations.Any(x => x.Name == "DisposeAsync"))))
1491+
return false;
14851492
// #238 coverage: `void IDisposable.Dispose() { }` (explicit interface implementation — the
14861493
// actual ClosedXML Slice.Enumerator spelling) is a Dispose too; GetMembers("Dispose") misses
14871494
// it because its metadata name is "System.IDisposable.Dispose".

‎frontend/roslyn/samples/EmptyDisposeSample.cs‎

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,13 @@
44

55
namespace Own.Samples;
66

7-
// Issue #225 — a user-defined type whose Dispose() body is provably EMPTY holds no resource behind
8-
// the interface (it implements IDisposable only to satisfy a contract, e.g. IEnumerator<T>), so a
9-
// LOCAL of it that is never disposed cannot leak. Generalises the named-BCL no-op exemption
10-
// (field-notes entry 9 / IsNoOpDisposeWrapper) to any type. Mirrors ClosedXML's Slice.Enumerator.
11-
// Negative controls prove it does not over-widen. A FIELD keeps its own disposal contract (out of
12-
// scope here), so the OwnIgnoreSample `Handle` field stand-in is unaffected.
7+
// Issue #225 (narrowed by #238) — a user-defined `IEnumerator<T>` whose Dispose() body is provably
8+
// EMPTY holds no resource behind the interface (IEnumerator<T> is the one interface that FORCES
9+
// IDisposable onto a type that usually holds nothing), so a LOCAL of it never disposed cannot leak.
10+
// Mirrors ClosedXML's Slice.Enumerator. The exemption is deliberately NOT "any type" (that was the
11+
// #238 regression) and NOT the non-generic IEnumerator (which does not extend IDisposable). The
12+
// negative controls below pin every edge of the narrowing. A FIELD keeps its own disposal contract
13+
// (out of scope here), so the OwnIgnoreSample `Handle` field stand-in is unaffected.
1314

1415
// EMPTY Dispose — implements IDisposable only because IEnumerator<T> requires it (the ClosedXML
1516
// shape). Its Dispose does literally nothing.
@@ -112,6 +113,40 @@ public sealed class InheritedAsyncEnumerator : AsyncOwnerBase, IEnumerator<int>
112113
public void Dispose() { } // empty sync, but the base owns a real DisposeAsync
113114
}
114115

116+
// #240 review round 2 P2 — the base owns a BARE DisposeAsync() and does NOT implement
117+
// IAsyncDisposable, so neither AllInterfaces nor a current-type-only member scan catches it; only
118+
// walking the whole base chain does. An empty sync Dispose here must STILL leak.
119+
public abstract class BareAsyncBase
120+
{
121+
private readonly System.Threading.CancellationTokenSource _cts = new();
122+
public System.Threading.Tasks.ValueTask DisposeAsync() // bare, no interface, on the BASE
123+
{
124+
_cts.Dispose();
125+
return default;
126+
}
127+
}
128+
public sealed class BareInheritedAsyncEnumerator : BareAsyncBase, IEnumerator<int>
129+
{
130+
private int _i;
131+
public int Current => _i;
132+
object IEnumerator.Current => Current;
133+
public bool MoveNext() => ++_i <= 3;
134+
public void Reset() => _i = 0;
135+
public void Dispose() { } // empty sync, but the base owns a bare real DisposeAsync
136+
}
137+
138+
// #240 review round 2 P2 — a NON-generic System.Collections.IEnumerator does NOT extend IDisposable,
139+
// so pairing it with IDisposable is author-chosen, not forced: the motivating proof does not apply
140+
// and an empty-Dispose local must STAY flagged. Pins the IEnumerator<T>-only narrowing.
141+
public sealed class LegacyEnumerator : IEnumerator, IDisposable
142+
{
143+
private int _i;
144+
public object Current => _i;
145+
public bool MoveNext() => ++_i <= 3;
146+
public void Reset() => _i = 0;
147+
public void Dispose() { } // empty, but the type was not FORCED to implement IDisposable
148+
}
149+
115150
public sealed class EmptyDisposeConsumers
116151
{
117152
// SILENT: an empty-Dispose enumerator local, iterated and never disposed (the ClosedXML shape).
@@ -178,4 +213,24 @@ public int LeakInheritedAsync()
178213
while (ia.MoveNext()) n++; // base DisposeAsync is real -> LEAK
179214
return n;
180215
}
216+
217+
// FLAGGED (control, #240 review round 2): base owns a BARE DisposeAsync (no interface) -> the
218+
// base-chain scan must catch it -> LEAK.
219+
public int LeakBareInheritedAsync()
220+
{
221+
var ba = new BareInheritedAsyncEnumerator();
222+
var n = 0;
223+
while (ba.MoveNext()) n++; // base bare DisposeAsync is real -> LEAK
224+
return n;
225+
}
226+
227+
// FLAGGED (control, #240 review round 2): non-generic IEnumerator + IDisposable is not forced ->
228+
// the IEnumerator<T>-only narrowing must keep it flagged -> LEAK.
229+
public int LeakLegacy()
230+
{
231+
var ng = new LegacyEnumerator();
232+
var n = 0;
233+
while (ng.MoveNext()) n++; // non-generic enumerator, author-coupled IDisposable -> LEAK
234+
return n;
235+
}
181236
}

0 commit comments

Comments
 (0)