Skip to content

Commit 447dee4

Browse files
committed
fix(extractor): don't exempt an empty sync Dispose when the type has a real DisposeAsync (#225)
Codex review catch. HasEmptyDisposeBody proved only that the SYNCHRONOUS Dispose() body is empty; a type implementing both IDisposable and IAsyncDisposable can have an empty sync Dispose() as a compatibility no-op while DisposeAsync() does the real cleanup. The flow detector already treats DisposeAsync() as a release, so silencing such a local would drop a real leak. Now conservatively refuse to exempt any type that declares its own parameterless DisposeAsync. Pinned by a new AsyncReader control in EmptyDisposeSample.cs (empty sync Dispose, real DisposeAsync) — flagged in both the flat and --flow-locals CI jobs; the two provably-empty-Dispose locals stay silent. Verified locally: existing-sample diff still byte-identical (the guard only ever flags MORE, never fewer); gates green (test_ownir 276/276, ruff, mypy). Refs #225 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
1 parent 8630b4d commit 447dee4

3 files changed

Lines changed: 41 additions & 4 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -846,6 +846,9 @@ jobs:
846846
fi
847847
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr'.*'LeakyReader'" \
848848
|| { 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; }
849852
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"
850853
- name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2)
851854
run: |
@@ -1088,11 +1091,15 @@ jobs:
10881091
|| { echo "FAIL: #225 flow: a non-empty *Reader local must still leak"; exit 1; }
10891092
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'d' is never disposed" \
10901093
|| { 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.
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.
10931100
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; }
1101+
[ "$n225" = "4" ] \
1102+
|| { echo "FAIL: #225 flow: expected exactly 4 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
10961103
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)"
10971104
- name: Gallery C#-native bad/ok pairs (examples/gallery/cs/)
10981105
run: |

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1324,6 +1324,12 @@ static bool HasEmptyDisposeBody(ITypeSymbol? t)
13241324
for (var b = nt.BaseType; b is not null && b.SpecialType != SpecialType.System_Object; b = b.BaseType)
13251325
if (ImplementsIDisposable(b))
13261326
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;
13271333
var dispose = nt.GetMembers("Dispose").OfType<IMethodSymbol>().FirstOrDefault(
13281334
m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0);
13291335
if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0)

‎frontend/roslyn/samples/EmptyDisposeSample.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,22 @@ public sealed class EmptyOverrideOverRealBase : BaseWithRealDispose
5858
public override void Dispose() { } // empty, but base.Dispose does real work
5959
}
6060

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+
6177
public sealed class EmptyDisposeConsumers
6278
{
6379
// SILENT: an empty-Dispose enumerator local, iterated and never disposed (the ClosedXML shape).
@@ -96,4 +112,12 @@ public void LeakDerived()
96112
var d = new EmptyOverrideOverRealBase();
97113
d.Ping(); // base.Dispose() is real -> LEAK
98114
}
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+
}
99123
}

0 commit comments

Comments
 (0)