Skip to content

Commit d089065

Browse files
authored
Merge pull request #233 from PhysShell/claude/empty-dispose-225
fix(extractor): exempt locals of user types with a provably-empty Dispose() (Closes #225)
2 parents 3ca4bde + 447dee4 commit d089065

6 files changed

Lines changed: 225 additions & 6 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ jobs:
211211
frontend/roslyn/samples/SelfDetachingHandlerSample.cs \
212212
frontend/roslyn/samples/UsingFieldAcquisitionSample.cs \
213213
frontend/roslyn/samples/TemplatePartLocalCaptureSample.cs \
214+
frontend/roslyn/samples/EmptyDisposeSample.cs \
214215
-o "$RUNNER_TEMP/facts.json"
215216
cat "$RUNNER_TEMP/facts.json"
216217
- name: Check facts through the core
@@ -835,7 +836,20 @@ jobs:
835836
# ...while the same-named local in the OTHER method must still warn.
836837
echo "$out" | grep -qE "TemplatePartLocalCaptureSample\.cs:[0-9]+: warning: \[OWN001\].*'SameNameDifferentScopeSubscriber'" \
837838
|| { echo "FAIL: expected OWN001 on the same-named-but-unrelated local in a different method"; exit 1; }
838-
echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) at the C# location"
839+
# issue #225 — a user-defined type whose Dispose() body is provably EMPTY holds no resource,
840+
# so a LOCAL of it never disposed is not a leak (extends the named-BCL no-op exemption to any
841+
# type). Here the flat (non-flow) name path: a `*Reader`-named empty-Dispose local (ScratchReader)
842+
# is silent, while a non-empty `*Reader` local (LeakyReader) still leaks. (The semantic enumerator
843+
# form is exercised in the --flow-locals step below.)
844+
if echo "$out" | grep -q "'ScratchReader'"; then
845+
echo "FAIL: a local of a user type with a provably-empty Dispose() body was wrongly flagged (#225)"; exit 1
846+
fi
847+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr'.*'LeakyReader'" \
848+
|| { echo "FAIL: a non-empty-Dispose local (LeakyReader) must still leak (#225 stays scoped)"; exit 1; }
849+
# Codex P2: an empty SYNC Dispose but a real DisposeAsync (a `*Reader` here) must still leak.
850+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ar'.*'AsyncReader'" \
851+
|| { echo "FAIL: a type with a real DisposeAsync (empty sync Dispose) must still leak (#225)"; exit 1; }
852+
echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) at the C# location"
839853
- name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2)
840854
run: |
841855
# Path-sensitive flow analysis of local IDisposables — bugs the flat D1
@@ -844,7 +858,8 @@ jobs:
844858
frontend/roslyn/samples/FlowLocalsSample.cs \
845859
frontend/roslyn/samples/MemoryOwnerEscapeSample.cs \
846860
frontend/roslyn/samples/FactoryLeakSample.cs \
847-
frontend/roslyn/samples/OverloadSigSample.cs --flow-locals -o "$RUNNER_TEMP/flow.json"
861+
frontend/roslyn/samples/OverloadSigSample.cs \
862+
frontend/roslyn/samples/EmptyDisposeSample.cs --flow-locals -o "$RUNNER_TEMP/flow.json"
848863
out=$(python -m ownlang ownir "$RUNNER_TEMP/flow.json" || true)
849864
echo "$out"
850865
echo "$out" | grep -q "OWN002" || { echo "FAIL: expected OWN002 (use-after-dispose)"; exit 1; }
@@ -1066,7 +1081,26 @@ jobs:
10661081
for ok in opened probe; do
10671082
if echo "$out" | grep -q "'$ok'"; then echo "FAIL: stage-2 silent case '$ok' was reported"; exit 1; fi
10681083
done
1069-
echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)"
1084+
# issue #225 (semantic flow path — the real ClosedXML Slice.Enumerator shape): a LOCAL of a
1085+
# user type whose Dispose() body is provably EMPTY holds no resource, so never disposing it
1086+
# cannot leak. The three controls STILL leak: a real IDisposable ('r'), a non-empty *Reader
1087+
# ('lr'), and an empty override over a base that owns a real Dispose ('d').
1088+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'r' is never disposed" \
1089+
|| { echo "FAIL: #225 flow: a real IDisposable local must still leak"; exit 1; }
1090+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr' is never disposed" \
1091+
|| { echo "FAIL: #225 flow: a non-empty *Reader local must still leak"; exit 1; }
1092+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'d' is never disposed" \
1093+
|| { echo "FAIL: #225 flow: an empty override over a real base Dispose must still leak"; exit 1; }
1094+
# Codex P2: an empty SYNC Dispose() but a real DisposeAsync() (IDisposable + IAsyncDisposable)
1095+
# must NOT be exempted — the real cleanup is async and the flow detector treats it as a release.
1096+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ar' is never disposed" \
1097+
|| { echo "FAIL: #225 flow: a type with a real DisposeAsync (empty sync Dispose) must still leak"; exit 1; }
1098+
# exactly 4 EmptyDisposeSample findings -> the two provably-empty-Dispose locals (the enumerator
1099+
# and ScratchReader) are SILENT; any leak of them would push the count past 4.
1100+
n225=$(echo "$out" | grep -cE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN00")
1101+
[ "$n225" = "4" ] \
1102+
|| { echo "FAIL: #225 flow: expected exactly 4 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
1103+
echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, #225 empty-Dispose local exempt, beyond flat)"
10701104
- name: Gallery C#-native bad/ok pairs (examples/gallery/cs/)
10711105
run: |
10721106
# C#-native mirror of examples/gallery/*.own, run through the real extractor ->

‎docs/notes/field-notes-patterns.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,24 @@ type names, a `Dispose()` method whose body is provably empty (no statements)
644644
is safe to skip regardless of whose type it is — first-party or third-party,
645645
named allowlist or not.**
646646

647+
**Shipped (issue #225).** `HasEmptyDisposeBody` (extractor) recognises a type
648+
whose parameterless `Dispose()` is declared in source with a **block body of zero
649+
statements** *and* whose base chain carries no `Dispose` to cascade to (base is
650+
`object` / not `IDisposable`), so nothing real is skipped. A **LOCAL** of such a
651+
type never disposed is then dropped from both the flat (`IsDisposableType`) and the
652+
`--flow-locals` (`ImplementsIDisposable`) local-disposable paths. Deliberately
653+
**scoped to locals**: a FIELD keeps its own disposal contract (so the
654+
`OwnIgnoreSample` `Handle` field stand-in and other field fixtures are untouched).
655+
Precision guards, each pinned by `EmptyDisposeSample.cs`: a **non-empty** body
656+
(`RealResource`, `LeakyReader`) still leaks; an **empty override over a base whose
657+
`Dispose` does real work** (`EmptyOverrideOverRealBase`) still leaks; an
658+
**expression-bodied / metadata-only** `Dispose` we cannot read as empty is not
659+
exempted (never a guessed drop). The empty enumerator (`EmptyDisposeEnumerator`,
660+
the ClosedXML shape) and an empty `*Reader` (`ScratchReader`) stay silent. Two
661+
existing fixtures that had used an empty `Dispose()` as a modelling shortcut
662+
(`UnitOfWork`, `PixelOwner`) were made faithful (a real Dispose body) so they
663+
remain genuine leak fixtures.
664+
647665
---
648666

649667
## The through-line

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

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1306,6 +1306,38 @@ static bool IsNoOpDisposeWrapper(BaseObjectCreationExpressionSyntax oce, Semanti
13061306
return arg0 is not null && IsInMemoryDisposableBacking(model.GetTypeInfo(arg0).Type);
13071307
}
13081308

1309+
// Issue #225 — a user-defined type whose parameterless `Dispose()` body is provably EMPTY holds no
1310+
// resource behind the interface: it implements IDisposable only to satisfy a contract (e.g.
1311+
// `IEnumerator<T>`), so never calling Dispose() cannot leak. Recognised from SOURCE: the type
1312+
// declares `void Dispose()` with a BLOCK body of ZERO statements, and no base in its chain carries a
1313+
// Dispose to cascade to (the base is `object` / not IDisposable), so nothing real is skipped. A body
1314+
// we cannot read (third-party metadata-only, or expression-bodied doing work) is NOT provably empty
1315+
// and stays flagged — never a guessed drop. This generalises the named-BCL IsNoOpDisposeWrapper /
1316+
// IsDisposeOptional idea to any type. Deliberately scoped to LOCAL disposables (the #201-sweep
1317+
// evidence — ClosedXML's `Slice.Enumerator` locals): a FIELD keeps its own disposal contract, so an
1318+
// empty-Dispose field (the OwnIgnoreSample `Handle` stand-in) is unaffected.
1319+
static bool HasEmptyDisposeBody(ITypeSymbol? t)
1320+
{
1321+
if (t is not INamedTypeSymbol nt)
1322+
return false;
1323+
// The base must not own a Dispose we'd be silently skipping — only `object` / a non-IDisposable base.
1324+
for (var b = nt.BaseType; b is not null && b.SpecialType != SpecialType.System_Object; b = b.BaseType)
1325+
if (ImplementsIDisposable(b))
1326+
return false;
1327+
// A non-empty DisposeAsync() can do the REAL cleanup even when the sync Dispose() is an empty
1328+
// compatibility no-op (a type implementing both IDisposable AND IAsyncDisposable). The flow
1329+
// detector already treats DisposeAsync() as a release, so an undisposed local of such a type is a
1330+
// real leak — conservatively refuse to exempt any type that declares its own DisposeAsync (Codex).
1331+
if (nt.GetMembers("DisposeAsync").OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0))
1332+
return false;
1333+
var dispose = nt.GetMembers("Dispose").OfType<IMethodSymbol>().FirstOrDefault(
1334+
m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0);
1335+
if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0)
1336+
return false; // no readable source Dispose (metadata-only) -> cannot prove empty
1337+
return dispose.DeclaringSyntaxReferences.All(
1338+
sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 });
1339+
}
1340+
13091341
// A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
13101342
// base chain). A modeless `Form.Show()` transfers ownership to the framework, which
13111343
// disposes the form on close — EmitFlowExpr models that show as a release.
@@ -4715,6 +4747,11 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol
47154747
};
47164748
if (ctype is null || !IsDisposableType(ctype))
47174749
continue;
4750+
// #225: a user type whose Dispose() body is provably empty holds no resource —
4751+
// never disposing a local of it cannot leak (mirrors the flow path's guard).
4752+
if (v.Initializer?.Value is { } newExpr
4753+
&& HasEmptyDisposeBody(model.GetTypeInfo(newExpr).Type))
4754+
continue;
47184755
subs.Add(new
47194756
{
47204757
@event = name,
@@ -4758,7 +4795,10 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol
47584795
if (v.Initializer is { Value: ObjectCreationExpressionSyntax
47594796
or ImplicitObjectCreationExpressionSyntax } init
47604797
&& model.GetTypeInfo(init.Value).Type is { } dt
4761-
&& ImplementsIDisposable(dt) && !IsDisposeOptional(dt))
4798+
&& ImplementsIDisposable(dt) && !IsDisposeOptional(dt)
4799+
// #225: a user type with a provably-empty Dispose() body holds no
4800+
// resource -> never disposing a local of it cannot leak.
4801+
&& !HasEmptyDisposeBody(dt))
47624802
{
47634803
candidates.Add(v.Identifier.Text);
47644804
newedDisposables.Add(v.Identifier.Text);
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
using System;
2+
using System.Collections;
3+
using System.Collections.Generic;
4+
5+
namespace Own.Samples;
6+
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.
13+
14+
// EMPTY Dispose — implements IDisposable only because IEnumerator<T> requires it (the ClosedXML
15+
// shape). Its Dispose does literally nothing.
16+
public sealed class EmptyDisposeEnumerator : IEnumerator<int>
17+
{
18+
private int _i;
19+
public int Current => _i;
20+
object IEnumerator.Current => Current;
21+
public bool MoveNext() => ++_i <= 3;
22+
public void Reset() => _i = 0;
23+
public void Dispose() { } // literally empty -> no resource
24+
}
25+
26+
// EMPTY Dispose on a name that ALSO matches the flat (non-flow) name heuristic (`*Reader`), so the
27+
// non-flow local-disposable path exercises the same exemption.
28+
public sealed class ScratchReader : IDisposable
29+
{
30+
public int Read() => -1;
31+
public void Dispose() { } // empty -> no resource
32+
}
33+
34+
// NON-empty Dispose — a real owned resource released in Dispose. A local never disposed LEAKS.
35+
public sealed class RealResource : IDisposable
36+
{
37+
private bool _closed;
38+
public void Touch() { }
39+
public void Dispose() { _closed = true; } // has a statement -> not provably empty
40+
}
41+
42+
// NON-empty Dispose, name matches the flat heuristic — the flat-path control.
43+
public sealed class LeakyReader : IDisposable
44+
{
45+
public int Read() => 0;
46+
public void Dispose() { GC.SuppressFinalize(this); } // real work -> stays flagged
47+
}
48+
49+
// Empty-bodied OVERRIDE, but the BASE owns a real Dispose — skipping this type's empty Dispose would
50+
// skip the base's cascade, so a local of it must STAY flagged.
51+
public class BaseWithRealDispose : IDisposable
52+
{
53+
public void Ping() { }
54+
public virtual void Dispose() { GC.SuppressFinalize(this); }
55+
}
56+
public sealed class EmptyOverrideOverRealBase : BaseWithRealDispose
57+
{
58+
public override void Dispose() { } // empty, but base.Dispose does real work
59+
}
60+
61+
// NON-empty cleanup via DisposeAsync, with the sync Dispose() an empty COMPATIBILITY no-op (a type
62+
// implementing both IDisposable and IAsyncDisposable). The empty sync body must NOT exempt it — the
63+
// real cleanup lives in DisposeAsync, which the flow detector treats as a release, so an undisposed
64+
// local still leaks (Codex P2). Name ends `Reader` so both detector paths see it.
65+
public sealed class AsyncReader : IDisposable, IAsyncDisposable
66+
{
67+
private readonly System.Threading.CancellationTokenSource _cts = new();
68+
public int Read() => 0;
69+
public void Dispose() { } // empty sync compat no-op
70+
public System.Threading.Tasks.ValueTask DisposeAsync() // REAL cleanup
71+
{
72+
_cts.Dispose();
73+
return default;
74+
}
75+
}
76+
77+
public sealed class EmptyDisposeConsumers
78+
{
79+
// SILENT: an empty-Dispose enumerator local, iterated and never disposed (the ClosedXML shape).
80+
public int CountEmpty()
81+
{
82+
var e = new EmptyDisposeEnumerator();
83+
var n = 0;
84+
while (e.MoveNext()) n++; // never disposed -> Dispose is empty -> SILENT
85+
return n;
86+
}
87+
88+
// SILENT: an empty-Dispose `*Reader` local (flat-path name match) never disposed.
89+
public int UseScratch()
90+
{
91+
var s = new ScratchReader();
92+
return s.Read(); // never disposed -> Dispose is empty -> SILENT
93+
}
94+
95+
// FLAGGED (control): a real IDisposable local never disposed -> OWN001.
96+
public void LeakReal()
97+
{
98+
var r = new RealResource();
99+
r.Touch(); // used, never disposed -> LEAK
100+
}
101+
102+
// FLAGGED (control): a non-empty `*Reader` local never disposed -> OWN001 (flat path).
103+
public int LeakReader()
104+
{
105+
var lr = new LeakyReader();
106+
return lr.Read(); // never disposed -> LEAK
107+
}
108+
109+
// FLAGGED (control): empty override, but the base Dispose does real work -> OWN001.
110+
public void LeakDerived()
111+
{
112+
var d = new EmptyOverrideOverRealBase();
113+
d.Ping(); // base.Dispose() is real -> LEAK
114+
}
115+
116+
// FLAGGED (control, Codex P2): empty SYNC Dispose but a real DisposeAsync -> never disposing
117+
// (sync or async) leaks; the empty sync body must not exempt it.
118+
public int LeakAsync()
119+
{
120+
var ar = new AsyncReader();
121+
return ar.Read(); // never disposed (sync or async) -> LEAK
122+
}
123+
}

‎frontend/roslyn/samples/MemoryOwnerEscapeSample.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ internal sealed class PixelOwner : IMemoryOwner<byte>
1616

1717
public Memory<byte> Memory => this.data;
1818

19-
public void Dispose() { }
19+
public void Dispose() { Array.Clear(this.data, 0, this.data.Length); } // real work (#225): not a no-op
2020
}
2121

2222
internal static class MemoryOwnerEscape

‎frontend/roslyn/samples/UnitOfWorkFlowSample.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,15 @@ public interface IUnitOfWork : IDisposable
9999
// optional) — which is what makes a leaked local matter.
100100
public sealed class UnitOfWork : IUnitOfWork
101101
{
102+
// Owns a real IDisposable (a DbContext-like resource) released in Dispose, so a leaked
103+
// local genuinely leaks — its Dispose() is NOT a no-op (keeps this fixture faithful under
104+
// the #225 empty-Dispose exemption, which only skips a provably-empty body).
105+
private readonly System.Threading.CancellationTokenSource _cts = new();
102106
public UnitOfWork(bool isCatalogs) { }
103107
public IQueryable<ProductCatalog> ProductCatalogs => Enumerable.Empty<ProductCatalog>().AsQueryable();
104108
public IQueryable<TempProduct> TempProducts => Enumerable.Empty<TempProduct>().AsQueryable();
105109
public IGenericRepository<T> GenericRepository<T>() => new GenericRepository<T>();
106-
public void Dispose() { }
110+
public void Dispose() { _cts.Dispose(); }
107111
}
108112

109113
public interface IGenericRepository<T>

0 commit comments

Comments
 (0)