Skip to content

Commit 8630b4d

Browse files
committed
fix(extractor): exempt locals of user types with a provably-empty Dispose() (#225)
A local of a user-defined type whose `Dispose()` body is literally empty was false-flagged as an OWN001 leak. Such a type (e.g. an `IEnumerator<T>` enumerator) implements IDisposable only to satisfy an interface, not because it holds a resource — never calling its empty Dispose cannot leak anything. The existing no-op exemption (IsNoOpDisposeWrapper / IsDisposeOptional) only reached a handful of NAMED BCL types. Confirmed FP in ClosedXML, 5 sites, single root cause (Slice.Enumerator locals); issue #201 oracle sweep, field-notes entry 19. Fix (extractor-only, no OwnIR change): HasEmptyDisposeBody recognises from SOURCE a type whose parameterless `Dispose()` is a block body of zero statements and whose base chain owns no Dispose to cascade to, and drops a LOCAL of it from both the flat (IsDisposableType) and --flow-locals (ImplementsIDisposable) paths. Deliberately scoped to LOCALS: a FIELD keeps its own disposal contract, so the OwnIgnoreSample `Handle` field stand-in (an empty-Dispose type used as a leak control in #209) is unaffected. Precision guards, pinned by EmptyDisposeSample.cs in both the wpf-extractor (flat) and flow-locals CI jobs: a non-empty Dispose (RealResource, LeakyReader) still leaks; an empty override over a base whose Dispose does real work (EmptyOverrideOverRealBase) still leaks; an expression-bodied / metadata-only Dispose we cannot read is not exempted (never a guessed drop). The empty enumerator (the ClosedXML shape) and an empty *Reader stay silent. Two existing fixtures had used an empty `Dispose()` as a modelling shortcut while their comments claimed a real resource; made faithful (a real Dispose body) so they stay genuine leak fixtures: UnitOfWork now owns+disposes a CancellationTokenSource; PixelOwner clears its backing. This keeps their existing by-name CI assertions ('uow'/'leakedOwner' still leak). Verified locally with the real extractor (.NET 8): EmptyDisposeSample goes silent on the two empty-Dispose locals, flags the three controls, in both flat and flow modes; a full existing-sample diff (both modes) is byte-identical after the fixture-fidelity fix (zero regression). Gates: run_tests.py exit 0, test_ownir 276/276, ruff clean, mypy --strict green. Closes #225 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
1 parent 3ca4bde commit 8630b4d

6 files changed

Lines changed: 188 additions & 6 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 30 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,17 @@ 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+
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"
839850
- name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2)
840851
run: |
841852
# Path-sensitive flow analysis of local IDisposables — bugs the flat D1
@@ -844,7 +855,8 @@ jobs:
844855
frontend/roslyn/samples/FlowLocalsSample.cs \
845856
frontend/roslyn/samples/MemoryOwnerEscapeSample.cs \
846857
frontend/roslyn/samples/FactoryLeakSample.cs \
847-
frontend/roslyn/samples/OverloadSigSample.cs --flow-locals -o "$RUNNER_TEMP/flow.json"
858+
frontend/roslyn/samples/OverloadSigSample.cs \
859+
frontend/roslyn/samples/EmptyDisposeSample.cs --flow-locals -o "$RUNNER_TEMP/flow.json"
848860
out=$(python -m ownlang ownir "$RUNNER_TEMP/flow.json" || true)
849861
echo "$out"
850862
echo "$out" | grep -q "OWN002" || { echo "FAIL: expected OWN002 (use-after-dispose)"; exit 1; }
@@ -1066,7 +1078,22 @@ jobs:
10661078
for ok in opened probe; do
10671079
if echo "$out" | grep -q "'$ok'"; then echo "FAIL: stage-2 silent case '$ok' was reported"; exit 1; fi
10681080
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)"
1081+
# issue #225 (semantic flow path — the real ClosedXML Slice.Enumerator shape): a LOCAL of a
1082+
# user type whose Dispose() body is provably EMPTY holds no resource, so never disposing it
1083+
# cannot leak. The three controls STILL leak: a real IDisposable ('r'), a non-empty *Reader
1084+
# ('lr'), and an empty override over a base that owns a real Dispose ('d').
1085+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'r' is never disposed" \
1086+
|| { echo "FAIL: #225 flow: a real IDisposable local must still leak"; exit 1; }
1087+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr' is never disposed" \
1088+
|| { echo "FAIL: #225 flow: a non-empty *Reader local must still leak"; exit 1; }
1089+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'d' is never disposed" \
1090+
|| { echo "FAIL: #225 flow: an empty override over a real base Dispose must still leak"; exit 1; }
1091+
# exactly 3 EmptyDisposeSample findings -> the two empty-Dispose locals (the enumerator and
1092+
# ScratchReader) are SILENT; any leak would push the count past 3.
1093+
n225=$(echo "$out" | grep -cE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN00")
1094+
[ "$n225" = "3" ] \
1095+
|| { echo "FAIL: #225 flow: expected exactly 3 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
1096+
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)"
10701097
- name: Gallery C#-native bad/ok pairs (examples/gallery/cs/)
10711098
run: |
10721099
# 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: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1306,6 +1306,32 @@ 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+
var dispose = nt.GetMembers("Dispose").OfType<IMethodSymbol>().FirstOrDefault(
1328+
m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0);
1329+
if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0)
1330+
return false; // no readable source Dispose (metadata-only) -> cannot prove empty
1331+
return dispose.DeclaringSyntaxReferences.All(
1332+
sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 });
1333+
}
1334+
13091335
// A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
13101336
// base chain). A modeless `Form.Show()` transfers ownership to the framework, which
13111337
// disposes the form on close — EmitFlowExpr models that show as a release.
@@ -4715,6 +4741,11 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol
47154741
};
47164742
if (ctype is null || !IsDisposableType(ctype))
47174743
continue;
4744+
// #225: a user type whose Dispose() body is provably empty holds no resource —
4745+
// never disposing a local of it cannot leak (mirrors the flow path's guard).
4746+
if (v.Initializer?.Value is { } newExpr
4747+
&& HasEmptyDisposeBody(model.GetTypeInfo(newExpr).Type))
4748+
continue;
47184749
subs.Add(new
47194750
{
47204751
@event = name,
@@ -4758,7 +4789,10 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol
47584789
if (v.Initializer is { Value: ObjectCreationExpressionSyntax
47594790
or ImplicitObjectCreationExpressionSyntax } init
47604791
&& model.GetTypeInfo(init.Value).Type is { } dt
4761-
&& ImplementsIDisposable(dt) && !IsDisposeOptional(dt))
4792+
&& ImplementsIDisposable(dt) && !IsDisposeOptional(dt)
4793+
// #225: a user type with a provably-empty Dispose() body holds no
4794+
// resource -> never disposing a local of it cannot leak.
4795+
&& !HasEmptyDisposeBody(dt))
47624796
{
47634797
candidates.Add(v.Identifier.Text);
47644798
newedDisposables.Add(v.Identifier.Text);
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
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+
public sealed class EmptyDisposeConsumers
62+
{
63+
// SILENT: an empty-Dispose enumerator local, iterated and never disposed (the ClosedXML shape).
64+
public int CountEmpty()
65+
{
66+
var e = new EmptyDisposeEnumerator();
67+
var n = 0;
68+
while (e.MoveNext()) n++; // never disposed -> Dispose is empty -> SILENT
69+
return n;
70+
}
71+
72+
// SILENT: an empty-Dispose `*Reader` local (flat-path name match) never disposed.
73+
public int UseScratch()
74+
{
75+
var s = new ScratchReader();
76+
return s.Read(); // never disposed -> Dispose is empty -> SILENT
77+
}
78+
79+
// FLAGGED (control): a real IDisposable local never disposed -> OWN001.
80+
public void LeakReal()
81+
{
82+
var r = new RealResource();
83+
r.Touch(); // used, never disposed -> LEAK
84+
}
85+
86+
// FLAGGED (control): a non-empty `*Reader` local never disposed -> OWN001 (flat path).
87+
public int LeakReader()
88+
{
89+
var lr = new LeakyReader();
90+
return lr.Read(); // never disposed -> LEAK
91+
}
92+
93+
// FLAGGED (control): empty override, but the base Dispose does real work -> OWN001.
94+
public void LeakDerived()
95+
{
96+
var d = new EmptyOverrideOverRealBase();
97+
d.Ping(); // base.Dispose() is real -> LEAK
98+
}
99+
}

‎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)