From cec7c7e5ac021135667d765ae1d04be8bc6955cf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 11:09:15 +0000 Subject: [PATCH 1/4] fix(extractor): recognize null-conditional dispose + cross-member pool release (mined ImageSharp #2,#3/4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more field-detector precision fixes from the ImageSharp mining run. #2 — null-conditional disposal. The disposed-field set matched only a plain `field.Dispose()`; the dominant `field?.Dispose()` (a ConditionalAccess whose WhenNotNull is the .Dispose() invocation) was missed, so a field disposed that way was reported as a leak. Mined across ImageSharp: `this.memoryStream?.Dispose()` (ZipExrCompressor, DeflateCompressor, IccDataWriter) and the BufferedStreams benchmark's `[GlobalCleanup]`. #3 — cross-member pool release. The per-member pool pass required the Return in the same member as the Rent, but a FIELD buffer is rented in the ctor and returned in Dispose (different members). A field is now also released if `pool.Return(field)` appears ANYWHERE in the class (a field name is class-unique, so no cross-masking; locals keep per-member scoping), or if the buffer is TRANSFERRED into a `new Guard(field)` that the object stores in a field — the #80 escaping-ctor transfer at field level. Mined: BufferedReadStream (returns this.readBuffer in Dispose(bool)) and SharedArrayPoolBuffer (LifetimeGuard). Regression sample FieldReleaseSample.cs: `field?.Dispose()` and the cross-member / guard-transferred pooled fields stay silent; an undisposed field and a rented-never-returned pooled field still warn. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 15 ++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 61 +++++++++++++-- frontend/roslyn/samples/FieldReleaseSample.cs | 75 +++++++++++++++++++ 3 files changed, 143 insertions(+), 8 deletions(-) create mode 100644 frontend/roslyn/samples/FieldReleaseSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0de5280..450544b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,6 +140,7 @@ jobs: frontend/roslyn/samples/ViewOwnsVmSample.xaml.cs \ frontend/roslyn/samples/InjectedDcViewSample.xaml.cs \ frontend/roslyn/samples/ResolvedDisposableSample.cs \ + frontend/roslyn/samples/FieldReleaseSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -223,6 +224,20 @@ jobs: if echo "$out" | grep -q "HolderWithDisposeOptional"; then echo "FAIL: a dispose-optional (Task/DataTable) field was wrongly flagged"; exit 1 fi + # field release recognition (mined: ImageSharp). #2 null-conditional dispose + # `field?.Dispose()` must be recognized -> silent; the undisposed control still warns. + if echo "$out" | grep -q "DisposesViaConditional"; then + echo "FAIL: a field disposed via null-conditional field?.Dispose() was wrongly flagged"; exit 1 + fi + echo "$out" | grep -q "NeverDisposesField" \ + || { echo "FAIL: an undisposed IDisposable field control must still warn"; exit 1; } + # #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) or transferred + # into a field-stored guard must be silent; the rented-never-returned control warns. + if echo "$out" | grep -qE "pooled buffer '(returnedBuf|guardedBuf)'"; then + echo "FAIL: a pooled field returned in Dispose / transferred to a guard was wrongly flagged"; exit 1 + fi + echo "$out" | grep -q "pooled buffer 'leakedBuf'" \ + || { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; } # WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+ # disposed one stays silent. "ignored" is unique to the WPF004 message. echo "$out" | grep -q "MessengerViewModel.cs" \ diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index aea10637..626ff09f 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2128,6 +2128,16 @@ or ImplicitObjectCreationExpressionSyntax && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" && FieldName(m.Expression) is { } df) disposed.Add(df); + // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose + // WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the + // dominant disposal shape the match above misses. Mined as an FP across ImageSharp: + // `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter) + // and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls. + foreach (var cae in cls.DescendantNodes().OfType()) + if (FieldName(cae.Expression) is { } cdf + && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } + && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") + disposed.Add(cdf); foreach (var fd in cls.Members.OfType()) { @@ -2355,25 +2365,58 @@ or ImplicitObjectCreationExpressionSyntax // tracks local declarations, so field/assignment-backed rents still need // this syntactic pass; the local-declaration rents are skipped below to // avoid double-reporting them (Codex). + // A FIELD-backed pooled buffer is legitimately rented in one member (the ctor) and + // released in another, so for FIELDS the release is searched CLASS-WIDE (a field name + // is unique to the class, so this cannot cross-mask — unlike same-named LOCALS in + // different methods, which keep the per-member scoping below). Released either by a + // direct `pool.Return(field)` ANYWHERE (mined: ImageSharp BufferedReadStream returns + // this.readBuffer in Dispose(bool)), or by TRANSFER of the buffer into a `new Guard(field)` + // that this object STORES in a field — a lifetime-owning wrapper that returns it (the #80 + // escaping-ctor transfer at field level; mined: SharedArrayPoolBuffer's LifetimeGuard). + var fieldReleased = new HashSet(); + foreach (var inv in cls.DescendantNodes().OfType()) + if (inv.Expression is MemberAccessExpressionSyntax rm + && rm.Name.Identifier.Text == "Return" + && inv.ArgumentList.Arguments.Count > 0 + && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs) + fieldReleased.Add(rfs.Name); + foreach (var oce in cls.DescendantNodes().OfType()) + { + // the `new Wrapper(field)` must itself be STORED in a field (kept by this object), + // not a throwaway local — mirrors #80's escape requirement, so a non-owning local + // view (`var v = new Span(field)`) is NOT mistaken for a transfer. + var storedInField = + (oce.Parent is AssignmentExpressionSyntax pa + && model.GetSymbolInfo(pa.Left).Symbol is IFieldSymbol) + || oce.Parent is EqualsValueClauseSyntax + { Parent: VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax + { Parent: FieldDeclarationSyntax } } }; + if (!storedInField || oce.ArgumentList is not { } al) + continue; + foreach (var arg in al.Arguments) + if (model.GetSymbolInfo(arg.Expression).Symbol is IFieldSymbol afs) + fieldReleased.Add(afs.Name); + } + foreach (var member in cls.Members) { - var rented = new List<(string Name, int Line)>(); + var rented = new List<(string Name, int Line, bool IsField)>(); foreach (var inv in member.DescendantNodes().OfType()) if (IsPoolRent(inv, model)) { - string? name = inv.Parent switch + (string? name, bool isField) = inv.Parent switch { // a local-declaration rent is the flow pass's job under // --flow-locals; skip it here so it is not double-reported. EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax vd } - => flowLocals ? null : vd.Identifier.Text, + => (flowLocals ? null : vd.Identifier.Text, false), // a field/assignment rent (`_buf = pool.Rent(...)`) is NOT a // flow candidate, so this pass keeps it in both modes. - AssignmentExpressionSyntax asg => FieldName(asg.Left), - _ => null, + AssignmentExpressionSyntax asg => (FieldName(asg.Left), true), + _ => ((string?)null, false), }; if (name != null) - rented.Add((name, LineOf(inv))); + rented.Add((name, LineOf(inv), isField)); } if (rented.Count == 0) continue; @@ -2384,12 +2427,14 @@ or ImplicitObjectCreationExpressionSyntax && inv.ArgumentList.Arguments.Count > 0 && FieldName(inv.ArgumentList.Arguments[0].Expression) is { } rn) returned.Add(rn); - foreach (var (name, line) in rented) + foreach (var (name, line, isField) in rented) subs.Add(new { @event = name, line, - released = returned.Contains(name), + // locals stay per-member; a field is also released if returned/transferred + // anywhere in the class (cross-member ctor-rent + Dispose-return). + released = returned.Contains(name) || (isField && fieldReleased.Contains(name)), resource = "pool", }); } diff --git a/frontend/roslyn/samples/FieldReleaseSample.cs b/frontend/roslyn/samples/FieldReleaseSample.cs new file mode 100644 index 00000000..32457535 --- /dev/null +++ b/frontend/roslyn/samples/FieldReleaseSample.cs @@ -0,0 +1,75 @@ +using System; +using System.Buffers; +using System.IO; + +namespace Own.Samples; + +// Field release recognition (mined: ImageSharp). Two shapes the field detectors missed: +// #2 null-conditional disposal `field?.Dispose()`, and +// #3 a pooled FIELD released in a DIFFERENT member than the rent (ctor rent + Dispose +// return), or transferred into a field-stored guard that owns/returns it. + +// #2: an IDisposable field disposed via the null-conditional `?.Dispose()` -> SILENT. +public sealed class DisposesViaConditional : IDisposable +{ + private readonly MemoryStream stream = new(); + + public void Dispose() => this.stream?.Dispose(); // null-conditional -> now recognized +} + +// #2 control: an IDisposable field the class new's but never disposes -> must WARN. +public sealed class NeverDisposesField +{ + private readonly MemoryStream stream = new(); + + public long Use() => this.stream.Length; +} + +// #3: a pooled buffer FIELD rented in the ctor and Returned in Dispose (cross-member) -> SILENT. +public sealed class PoolFieldReturnedInDispose : IDisposable +{ + private readonly byte[] returnedBuf; + + public PoolFieldReturnedInDispose(int n) => this.returnedBuf = ArrayPool.Shared.Rent(n); + + public void Dispose() => ArrayPool.Shared.Return(this.returnedBuf); + + public byte First() => this.returnedBuf[0]; +} + +// #3: a pooled buffer FIELD transferred into a field-stored guard (#80 transfer at field +// level) — the guard owns and returns it -> SILENT. +public sealed class PoolFieldTransferredToGuard +{ + private readonly byte[] guardedBuf; + private readonly BufferGuard guard; + + public PoolFieldTransferredToGuard(int n) + { + this.guardedBuf = ArrayPool.Shared.Rent(n); + this.guard = new BufferGuard(this.guardedBuf); // ownership handed to the guard + } +} + +public sealed class BufferGuard +{ + private byte[]? held; + + public BufferGuard(byte[] b) => this.held = b; + + public void Release() + { + ArrayPool.Shared.Return(this.held!); + this.held = null; + } +} + +// #3 control: a pooled buffer FIELD rented but NEVER returned/transferred -> must WARN. +public sealed class PoolFieldLeaked +{ + private readonly byte[] leakedBuf; + + public PoolFieldLeaked(int n) => this.leakedBuf = ArrayPool.Shared.Rent(n); + + public byte First() => this.leakedBuf[0]; +} From dc1a1a79480b34fd72d97b903f9b30493ebe7044 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 11:15:07 +0000 Subject: [PATCH 2/4] fix(extractor): drop unsound pool guard-transfer; keep cross-member return only (Codex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2: treating a rented field passed to ANY field-stored `new X(field)` as released wrongly suppresses a real leak for NON-owning wrappers (`_view = new ReadOnlyMemory(_buf)` and other cached views), where the array is never returned. Soundly distinguishing an owning guard (that Returns the buffer) from a non-owning view is not worth it for the single SharedArrayPoolBuffer FP, so the guard-transfer is removed entirely. Fix #3 keeps only the sound, high-value part: a pooled FIELD is released if `pool.Return(field)` appears anywhere in the class (cross-member ctor-rent + Dispose-return — BufferedReadStream). SharedArrayPoolBuffer's indirect release via its LifetimeGuard is left as an honest known limitation. Sample/CI drop the PoolFieldTransferredToGuard case accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 8 ++--- frontend/roslyn/OwnSharp.Extractor/Program.cs | 31 +++++-------------- frontend/roslyn/samples/FieldReleaseSample.cs | 29 +---------------- 3 files changed, 12 insertions(+), 56 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 450544b4..56a91db2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -231,10 +231,10 @@ jobs: fi echo "$out" | grep -q "NeverDisposesField" \ || { echo "FAIL: an undisposed IDisposable field control must still warn"; exit 1; } - # #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) or transferred - # into a field-stored guard must be silent; the rented-never-returned control warns. - if echo "$out" | grep -qE "pooled buffer '(returnedBuf|guardedBuf)'"; then - echo "FAIL: a pooled field returned in Dispose / transferred to a guard was wrongly flagged"; exit 1 + # #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) must be silent; + # the rented-never-returned control still warns. + if echo "$out" | grep -q "pooled buffer 'returnedBuf'"; then + echo "FAIL: a pooled field returned in Dispose was wrongly flagged"; exit 1 fi echo "$out" | grep -q "pooled buffer 'leakedBuf'" \ || { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; } diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 626ff09f..a94e544e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2366,13 +2366,13 @@ or ImplicitObjectCreationExpressionSyntax // this syntactic pass; the local-declaration rents are skipped below to // avoid double-reporting them (Codex). // A FIELD-backed pooled buffer is legitimately rented in one member (the ctor) and - // released in another, so for FIELDS the release is searched CLASS-WIDE (a field name - // is unique to the class, so this cannot cross-mask — unlike same-named LOCALS in - // different methods, which keep the per-member scoping below). Released either by a - // direct `pool.Return(field)` ANYWHERE (mined: ImageSharp BufferedReadStream returns - // this.readBuffer in Dispose(bool)), or by TRANSFER of the buffer into a `new Guard(field)` - // that this object STORES in a field — a lifetime-owning wrapper that returns it (the #80 - // escaping-ctor transfer at field level; mined: SharedArrayPoolBuffer's LifetimeGuard). + // Returned in another (Dispose), so for FIELDS the `pool.Return(field)` is searched + // CLASS-WIDE (a field name is unique to the class, so this cannot cross-mask — unlike + // same-named LOCALS in different methods, which keep the per-member scoping below). + // Mined: ImageSharp BufferedReadStream returns this.readBuffer in Dispose(bool). (An + // INDIRECT release via a lifetime-guard object — SharedArrayPoolBuffer — is left honest: + // a `new X(field)` is NOT assumed to own/return the buffer, since a non-owning view like + // `new ReadOnlyMemory(field)` would otherwise hide a real leak — Codex.) var fieldReleased = new HashSet(); foreach (var inv in cls.DescendantNodes().OfType()) if (inv.Expression is MemberAccessExpressionSyntax rm @@ -2380,23 +2380,6 @@ or ImplicitObjectCreationExpressionSyntax && inv.ArgumentList.Arguments.Count > 0 && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs) fieldReleased.Add(rfs.Name); - foreach (var oce in cls.DescendantNodes().OfType()) - { - // the `new Wrapper(field)` must itself be STORED in a field (kept by this object), - // not a throwaway local — mirrors #80's escape requirement, so a non-owning local - // view (`var v = new Span(field)`) is NOT mistaken for a transfer. - var storedInField = - (oce.Parent is AssignmentExpressionSyntax pa - && model.GetSymbolInfo(pa.Left).Symbol is IFieldSymbol) - || oce.Parent is EqualsValueClauseSyntax - { Parent: VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax - { Parent: FieldDeclarationSyntax } } }; - if (!storedInField || oce.ArgumentList is not { } al) - continue; - foreach (var arg in al.Arguments) - if (model.GetSymbolInfo(arg.Expression).Symbol is IFieldSymbol afs) - fieldReleased.Add(afs.Name); - } foreach (var member in cls.Members) { diff --git a/frontend/roslyn/samples/FieldReleaseSample.cs b/frontend/roslyn/samples/FieldReleaseSample.cs index 32457535..b8509208 100644 --- a/frontend/roslyn/samples/FieldReleaseSample.cs +++ b/frontend/roslyn/samples/FieldReleaseSample.cs @@ -37,34 +37,7 @@ public sealed class PoolFieldReturnedInDispose : IDisposable public byte First() => this.returnedBuf[0]; } -// #3: a pooled buffer FIELD transferred into a field-stored guard (#80 transfer at field -// level) — the guard owns and returns it -> SILENT. -public sealed class PoolFieldTransferredToGuard -{ - private readonly byte[] guardedBuf; - private readonly BufferGuard guard; - - public PoolFieldTransferredToGuard(int n) - { - this.guardedBuf = ArrayPool.Shared.Rent(n); - this.guard = new BufferGuard(this.guardedBuf); // ownership handed to the guard - } -} - -public sealed class BufferGuard -{ - private byte[]? held; - - public BufferGuard(byte[] b) => this.held = b; - - public void Release() - { - ArrayPool.Shared.Return(this.held!); - this.held = null; - } -} - -// #3 control: a pooled buffer FIELD rented but NEVER returned/transferred -> must WARN. +// #3 control: a pooled buffer FIELD rented but NEVER returned -> must WARN. public sealed class PoolFieldLeaked { private readonly byte[] leakedBuf; From 8e80d668ce271c45edc590f21656175a2df52bda Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 11:18:43 +0000 Subject: [PATCH 3/4] fix(extractor): suppress OWN014 region-escape from a static class (mined ImageSharp FP #4/4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `static class` has no instance, so a static-source subscription from it cannot promote an instance to the source's lifetime — the OWN014 region escape is vacuous. Mined: SixLabors/ImageSharp's MemoryAllocatorValidator, a `static class` whose static ctor hooks the static `MemoryDiagnostics.MemoryAllocated`/`MemoryReleased` events, was wrongly reported as "promotes MemoryAllocatorValidator to process lifetime" — there is no instance to promote. The static-source escape skip now also fires when the enclosing type is a static class (next to the existing process-lived `App` case). Scoped to the OWN014 escape only — OWN001 token leaks are untouched — and to non-timers, like the App case. (The two MemoryDiagnosticsTests OWN014s — lambdas in `static void RunTest` local functions of an INSTANCE class — are left honest: no `this` is captured so the named-instance claim is wrong, but the closure is retained, so this is the murky static-context territory, not a clean static-class drop.) Regression sample StaticClassEscapeSample.cs: a static class subscribing a LAMBDA (not covered by the static-method-handler exemption) to a static event stays silent; the existing StaticEventEscapeViewModel proves an INSTANCE class on the same shape still raises OWN014. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 8 +++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 18 +++++++---- .../roslyn/samples/StaticClassEscapeSample.cs | 30 +++++++++++++++++++ 3 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 frontend/roslyn/samples/StaticClassEscapeSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 56a91db2..19f595c4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -141,6 +141,7 @@ jobs: frontend/roslyn/samples/InjectedDcViewSample.xaml.cs \ frontend/roslyn/samples/ResolvedDisposableSample.cs \ frontend/roslyn/samples/FieldReleaseSample.cs \ + frontend/roslyn/samples/StaticClassEscapeSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -321,6 +322,13 @@ jobs: if echo "$out" | grep -q "CleanStaticEventViewModel"; then echo "FAIL: an unsubscribed (released) static-event subscription was wrongly reported"; exit 1 fi + # P-004 static-class escape exemption (mined: ImageSharp MemoryAllocatorValidator): a + # `static class` has no instance, so a static-source subscription (even a lambda) from it + # cannot be a region escape -> OWN014 must NOT fire. (StaticEventEscapeViewModel above + # proves an INSTANCE class on the same shape still escapes, so this stays scoped.) + if echo "$out" | grep -q "StaticAllocationCounter"; then + echo "FAIL: a static-source subscription from a static class was wrongly reported as a region escape"; exit 1 + fi # P-004 process-lived-subscriber exemption (mined: ScreenToGif App + # Translator): the WPF `App` singleton hooking the process-lived # AppDomain.UnhandledException promotes nothing, so the static-source region diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index a94e544e..11530c07 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2043,6 +2043,11 @@ or ImplicitObjectCreationExpressionSyntax // Is this class the process-lived WPF application object? Used to drop the // static-source region escape (OWN014) — `App` cannot be over-promoted. var clsIsApp = IsProcessLivedApplication(cls); + // A `static class` has NO instance, so a static-source subscription from it cannot + // promote an instance to the source's lifetime — the OWN014 escape is vacuous. (Mined: + // ImageSharp MemoryAllocatorValidator, a static class whose static ctor hooks the static + // MemoryDiagnostics events.) Drops only the static-source escape, not OWN001 token leaks. + var clsIsStaticClass = cls.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); var subs = new List(); foreach (var a in assigns) @@ -2076,12 +2081,13 @@ or ImplicitObjectCreationExpressionSyntax : SubscriptionSourceKind(a.Left, ev, model); if (source == "local") continue; - // Process-lived subscriber (the WPF `App` singleton): a static-source - // subscription promotes nothing — `App` already lives for the whole - // process — so the region escape (OWN014) is a false positive. Scoped - // to NON-timers: a timer is forced to source "static" above, but a - // never-stopped timer in `App` is still a real leak (CodeRabbit). - if (!isTimer && source == "static" && clsIsApp) + // A static-source subscription whose SUBSCRIBER cannot be over-promoted is not a + // region escape (OWN014): the process-lived WPF `App` singleton (its lifetime + // already equals the process), or a `static class` (no instance exists at all — + // mined: ImageSharp MemoryAllocatorValidator). Scoped to NON-timers: a timer is + // forced to source "static" above, but a never-stopped timer is still a real + // leak (CodeRabbit). + if (!isTimer && source == "static" && (clsIsApp || clsIsStaticClass)) continue; var released = unsub.Contains($"{a.Left}|{a.Right}") || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); diff --git a/frontend/roslyn/samples/StaticClassEscapeSample.cs b/frontend/roslyn/samples/StaticClassEscapeSample.cs new file mode 100644 index 00000000..732b3449 --- /dev/null +++ b/frontend/roslyn/samples/StaticClassEscapeSample.cs @@ -0,0 +1,30 @@ +using System; + +namespace Own.Samples; + +// P-004 static-class region-escape exemption (mined: ImageSharp MemoryAllocatorValidator). +// +// A `static class` has NO instance, so subscribing to a process-lived STATIC event from its +// static ctor cannot promote an instance to the source's lifetime — OWN014 must NOT fire. A +// LAMBDA handler is used on purpose: it is NOT covered by the static-method-handler exemption, +// so silence here exercises the static-class drop itself, not that exemption. Contrast: +// StaticEventEscapeViewModel (an INSTANCE class on the same shape) must STILL raise OWN014. + +public static class StaticDiagnosticsBus +{ + public static event EventHandler? Allocated; + + public static void Raise() => Allocated?.Invoke(null, EventArgs.Empty); +} + +public static class StaticAllocationCounter +{ + private static int count; + + static StaticAllocationCounter() + { + StaticDiagnosticsBus.Allocated += (_, _) => count++; // lambda + static event + static class -> SILENT + } + + public static int Count => count; +} From 0cc0d5b1110cc944d1917b32792b820426a331f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 11:26:06 +0000 Subject: [PATCH 4/4] fix(extractor): make IsStaticHandler resolve member-group symbols; drop static-class escape suppression (Codex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2: blanket-suppressing OWN014 for ALL static-source subscriptions in a static class is unsound — a CAPTURING lambda still retains its compiler-generated display-class instance for the process, even with no `this`. Codex's pointer is right: the mined case (MemoryAllocatorValidator) uses static-METHOD handlers, which the existing IsStaticHandler is meant to exempt; it missed them because a method group's symbol can surface as a member group (Symbol == null, CandidateSymbols populated). So: make IsStaticHandler fall back to CandidateSymbols (requiring ALL candidates static, so a mixed overload set is not wrongly exempted), and revert the static-class suppression entirely. A static-method handler in a static class is now exempted by IsStaticHandler (null target, no instance), while a capturing lambda there still escapes — sound. Sample now uses a static-METHOD handler on a static event from a static class's static ctor (replicating MemoryAllocatorValidator) and must stay silent; StaticEventEscapeViewModel still proves an instance handler on a static event escapes. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 11 +++--- frontend/roslyn/OwnSharp.Extractor/Program.cs | 39 +++++++++++-------- .../roslyn/samples/StaticClassEscapeSample.cs | 19 +++++---- 3 files changed, 38 insertions(+), 31 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 19f595c4..5474ee1b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -322,12 +322,13 @@ jobs: if echo "$out" | grep -q "CleanStaticEventViewModel"; then echo "FAIL: an unsubscribed (released) static-event subscription was wrongly reported"; exit 1 fi - # P-004 static-class escape exemption (mined: ImageSharp MemoryAllocatorValidator): a - # `static class` has no instance, so a static-source subscription (even a lambda) from it - # cannot be a region escape -> OWN014 must NOT fire. (StaticEventEscapeViewModel above - # proves an INSTANCE class on the same shape still escapes, so this stays scoped.) + # P-004 robust static-handler exemption (mined: ImageSharp MemoryAllocatorValidator): a + # static-METHOD handler on a static event stores a null-target delegate -> no instance is + # retained -> OWN014 must NOT fire, even when the method-group symbol surfaces as a member + # group (now resolved via CandidateSymbols). (StaticEventEscapeViewModel above proves an + # INSTANCE handler on the same static event still escapes, so this stays scoped.) if echo "$out" | grep -q "StaticAllocationCounter"; then - echo "FAIL: a static-source subscription from a static class was wrongly reported as a region escape"; exit 1 + echo "FAIL: a static-method handler on a static event was wrongly reported as a region escape"; exit 1 fi # P-004 process-lived-subscriber exemption (mined: ScreenToGif App + # Translator): the WPF `App` singleton hooking the process-lived diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 11530c07..51fb0c5a 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -185,10 +185,23 @@ BinaryExpressionSyntax b when b.IsKind(SyntaxKind.AsExpression) => b.Left, // Target is null, so no instance is retained — the subscription cannot leak a // subscriber, however long-lived the source. Only method-group handlers // (identifier / member access) are judged; lambdas and delegate-typed values may -// capture state and are left as leak candidates. -static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) => - IsHandler(right) - && model.GetSymbolInfo(right).Symbol is IMethodSymbol { IsStatic: true }; +// capture state and are left as leak candidates. A method group's symbol can surface +// as a MEMBER GROUP (Symbol == null, CandidateSymbols populated) instead of the bound +// method, so fall back to the candidates — else a genuinely static-method handler is +// missed and a static-source subscription that retains NO instance is mis-reported as +// a region escape (mined: ImageSharp MemoryAllocatorValidator's static MemoryDiagnostics +// handlers). When falling back, require ALL candidates static so an overload set that +// mixes a static and an instance method is not wrongly exempted. +static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) +{ + if (!IsHandler(right)) + return false; + var info = model.GetSymbolInfo(right); + if (info.Symbol is { } s) + return s is IMethodSymbol { IsStatic: true }; + var cands = info.CandidateSymbols; + return cands.Length > 0 && cands.All(c => c is IMethodSymbol { IsStatic: true }); +} // P-004 process-lived-subscriber exemption: the WPF application object (`App`) is a // process-lived singleton — exactly one instance, created at startup, alive until @@ -2043,11 +2056,6 @@ or ImplicitObjectCreationExpressionSyntax // Is this class the process-lived WPF application object? Used to drop the // static-source region escape (OWN014) — `App` cannot be over-promoted. var clsIsApp = IsProcessLivedApplication(cls); - // A `static class` has NO instance, so a static-source subscription from it cannot - // promote an instance to the source's lifetime — the OWN014 escape is vacuous. (Mined: - // ImageSharp MemoryAllocatorValidator, a static class whose static ctor hooks the static - // MemoryDiagnostics events.) Drops only the static-source escape, not OWN001 token leaks. - var clsIsStaticClass = cls.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); var subs = new List(); foreach (var a in assigns) @@ -2081,13 +2089,12 @@ or ImplicitObjectCreationExpressionSyntax : SubscriptionSourceKind(a.Left, ev, model); if (source == "local") continue; - // A static-source subscription whose SUBSCRIBER cannot be over-promoted is not a - // region escape (OWN014): the process-lived WPF `App` singleton (its lifetime - // already equals the process), or a `static class` (no instance exists at all — - // mined: ImageSharp MemoryAllocatorValidator). Scoped to NON-timers: a timer is - // forced to source "static" above, but a never-stopped timer is still a real - // leak (CodeRabbit). - if (!isTimer && source == "static" && (clsIsApp || clsIsStaticClass)) + // Process-lived subscriber (the WPF `App` singleton): a static-source + // subscription promotes nothing — `App` already lives for the whole + // process — so the region escape (OWN014) is a false positive. Scoped + // to NON-timers: a timer is forced to source "static" above, but a + // never-stopped timer in `App` is still a real leak (CodeRabbit). + if (!isTimer && source == "static" && clsIsApp) continue; var released = unsub.Contains($"{a.Left}|{a.Right}") || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); diff --git a/frontend/roslyn/samples/StaticClassEscapeSample.cs b/frontend/roslyn/samples/StaticClassEscapeSample.cs index 732b3449..e31cab4f 100644 --- a/frontend/roslyn/samples/StaticClassEscapeSample.cs +++ b/frontend/roslyn/samples/StaticClassEscapeSample.cs @@ -2,13 +2,14 @@ namespace Own.Samples; -// P-004 static-class region-escape exemption (mined: ImageSharp MemoryAllocatorValidator). +// P-004 static-handler exemption, robustly (mined: ImageSharp MemoryAllocatorValidator). // -// A `static class` has NO instance, so subscribing to a process-lived STATIC event from its -// static ctor cannot promote an instance to the source's lifetime — OWN014 must NOT fire. A -// LAMBDA handler is used on purpose: it is NOT covered by the static-method-handler exemption, -// so silence here exercises the static-class drop itself, not that exemption. Contrast: -// StaticEventEscapeViewModel (an INSTANCE class on the same shape) must STILL raise OWN014. +// A static class whose static ctor hooks a process-lived STATIC event with a STATIC METHOD +// handler stores a delegate whose Target is null — no instance is retained, so OWN014 must NOT +// fire. The mined case slipped through because the method-group symbol can surface as a member +// group (Symbol == null), which IsStaticHandler now resolves via CandidateSymbols. Contrast: +// StaticEventEscapeViewModel — an INSTANCE handler on a static event — must still raise OWN014 +// (capturing lambdas in a static class likewise still escape: the closure is retained). public static class StaticDiagnosticsBus { @@ -19,12 +20,10 @@ public static class StaticDiagnosticsBus public static class StaticAllocationCounter { - private static int count; - static StaticAllocationCounter() { - StaticDiagnosticsBus.Allocated += (_, _) => count++; // lambda + static event + static class -> SILENT + StaticDiagnosticsBus.Allocated += OnAllocated; // static-method handler -> null target -> SILENT } - public static int Count => count; + private static void OnAllocated(object? sender, EventArgs e) { } }