fix(extractor): exempt locals of user types with a provably-empty Dispose() (Closes #225) #233
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| using System; | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace Own.Samples; | ||
|
|
||
| // Issue #225 — a user-defined type whose Dispose() body is provably EMPTY holds no resource behind | ||
| // the interface (it implements IDisposable only to satisfy a contract, e.g. IEnumerator<T>), so a | ||
| // LOCAL of it that is never disposed cannot leak. Generalises the named-BCL no-op exemption | ||
| // (field-notes entry 9 / IsNoOpDisposeWrapper) to any type. Mirrors ClosedXML's Slice.Enumerator. | ||
| // Negative controls prove it does not over-widen. A FIELD keeps its own disposal contract (out of | ||
| // scope here), so the OwnIgnoreSample `Handle` field stand-in is unaffected. | ||
|
|
||
| // EMPTY Dispose — implements IDisposable only because IEnumerator<T> requires it (the ClosedXML | ||
| // shape). Its Dispose does literally nothing. | ||
| public sealed class EmptyDisposeEnumerator : IEnumerator<int> | ||
| { | ||
| private int _i; | ||
| public int Current => _i; | ||
| object IEnumerator.Current => Current; | ||
| public bool MoveNext() => ++_i <= 3; | ||
| public void Reset() => _i = 0; | ||
| public void Dispose() { } // literally empty -> no resource | ||
| } | ||
|
|
||
| // EMPTY Dispose on a name that ALSO matches the flat (non-flow) name heuristic (`*Reader`), so the | ||
| // non-flow local-disposable path exercises the same exemption. | ||
| public sealed class ScratchReader : IDisposable | ||
| { | ||
| public int Read() => -1; | ||
| public void Dispose() { } // empty -> no resource | ||
| } | ||
|
|
||
| // NON-empty Dispose — a real owned resource released in Dispose. A local never disposed LEAKS. | ||
| public sealed class RealResource : IDisposable | ||
| { | ||
| private bool _closed; | ||
| public void Touch() { } | ||
| public void Dispose() { _closed = true; } // has a statement -> not provably empty | ||
| } | ||
|
|
||
| // NON-empty Dispose, name matches the flat heuristic — the flat-path control. | ||
| public sealed class LeakyReader : IDisposable | ||
| { | ||
| public int Read() => 0; | ||
| public void Dispose() { GC.SuppressFinalize(this); } // real work -> stays flagged | ||
| } | ||
|
|
||
| // Empty-bodied OVERRIDE, but the BASE owns a real Dispose — skipping this type's empty Dispose would | ||
| // skip the base's cascade, so a local of it must STAY flagged. | ||
| public class BaseWithRealDispose : IDisposable | ||
| { | ||
| public void Ping() { } | ||
| public virtual void Dispose() { GC.SuppressFinalize(this); } | ||
| } | ||
| public sealed class EmptyOverrideOverRealBase : BaseWithRealDispose | ||
| { | ||
| public override void Dispose() { } // empty, but base.Dispose does real work | ||
| } | ||
|
|
||
| // NON-empty cleanup via DisposeAsync, with the sync Dispose() an empty COMPATIBILITY no-op (a type | ||
| // implementing both IDisposable and IAsyncDisposable). The empty sync body must NOT exempt it — the | ||
| // real cleanup lives in DisposeAsync, which the flow detector treats as a release, so an undisposed | ||
| // local still leaks (Codex P2). Name ends `Reader` so both detector paths see it. | ||
| public sealed class AsyncReader : IDisposable, IAsyncDisposable | ||
| { | ||
| private readonly System.Threading.CancellationTokenSource _cts = new(); | ||
| public int Read() => 0; | ||
| public void Dispose() { } // empty sync compat no-op | ||
| public System.Threading.Tasks.ValueTask DisposeAsync() // REAL cleanup | ||
| { | ||
| _cts.Dispose(); | ||
| return default; | ||
| } | ||
| } | ||
|
|
||
| public sealed class EmptyDisposeConsumers | ||
| { | ||
| // SILENT: an empty-Dispose enumerator local, iterated and never disposed (the ClosedXML shape). | ||
| public int CountEmpty() | ||
| { | ||
| var e = new EmptyDisposeEnumerator(); | ||
| var n = 0; | ||
| while (e.MoveNext()) n++; // never disposed -> Dispose is empty -> SILENT | ||
| return n; | ||
| } | ||
|
|
||
| // SILENT: an empty-Dispose `*Reader` local (flat-path name match) never disposed. | ||
| public int UseScratch() | ||
| { | ||
| var s = new ScratchReader(); | ||
| return s.Read(); // never disposed -> Dispose is empty -> SILENT | ||
| } | ||
|
|
||
| // FLAGGED (control): a real IDisposable local never disposed -> OWN001. | ||
| public void LeakReal() | ||
| { | ||
| var r = new RealResource(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning |
||
|
|
||
| r.Touch(); // used, never disposed -> LEAK | ||
| } | ||
|
|
||
| // FLAGGED (control): a non-empty `*Reader` local never disposed -> OWN001 (flat path). | ||
| public int LeakReader() | ||
| { | ||
| var lr = new LeakyReader(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning |
||
|
|
||
| return lr.Read(); // never disposed -> LEAK | ||
| } | ||
|
|
||
| // FLAGGED (control): empty override, but the base Dispose does real work -> OWN001. | ||
| public void LeakDerived() | ||
| { | ||
| var d = new EmptyOverrideOverRealBase(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning |
||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| d.Ping(); // base.Dispose() is real -> LEAK | ||
| } | ||
|
|
||
| // FLAGGED (control, Codex P2): empty SYNC Dispose but a real DisposeAsync -> never disposing | ||
| // (sync or async) leaks; the empty sync body must not exempt it. | ||
| public int LeakAsync() | ||
| { | ||
| var ar = new AsyncReader(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning |
||
|
|
||
| return ar.Read(); // never disposed (sync or async) -> LEAK | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exemption only proves that synchronous
Dispose()is empty; it does not rule out a non-emptyDisposeAsync()on the same source-defined type. For a type that implements bothIDisposableandIAsyncDisposablewithDispose()as a compatibility no-op andDisposeAsync()doing the real cleanup, a local such asvar r = new AsyncReader();is now dropped from the local-disposable candidates in both detector paths, so failing to call either dispose method becomes silent even though the existing flow logic already treatsDisposeAsync()as a release.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed in 447dee4. You're right:
HasEmptyDisposeBodyonly proved the syncDispose()was empty, so a type withIDisposable(empty compat no-op) +IAsyncDisposable(realDisposeAsync) would have been silenced even though the real cleanup is async and the flow detector already treatsDisposeAsync()as a release.The guard now conservatively refuses to exempt any type that declares its own parameterless
DisposeAsync. Pinned by a newAsyncReadercontrol (empty syncDispose, realDisposeAsync) — flagged in both the flat and--flow-localsCI jobs, while the two provably-empty-Dispose locals stay silent. The guard only ever flags more, so the existing-sample diff stays byte-identical (zero regression); gates green.Generated by Claude Code