From 5179aac2a4d2f83ebc8474fd7010690ee09efdb8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 09:53:06 +0000 Subject: [PATCH 1/2] fix(extractor): two subscription/escape FPs mined from ScreenToGif MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mining NickeManarin/ScreenToGif (WPF) surfaced two false positives in the subscription / region-escape detectors; both are suppressed soundly: - OWN014 on `App`: the WPF application object is a process-lived singleton, so hooking the process-lived AppDomain.CurrentDomain.UnhandledException promotes nothing — its "leaked" lifetime already equals the process. IsProcessLivedApplication recognises it syntactically (base `Application`, or the XAML-split `partial class App` whose `: Application` is in the generated partial we never see) since WPF does not resolve on the Linux runner. (ScreenToGif/App.xaml.cs:52 + Other/Translator/App.xaml.cs:14) - OWN001 on a static helper's subscription: a `+=` inside a static member has no enclosing `this`, so a static-method group or a lambda over locals retains no instance of the type — the "keeps alive" subscriber leak is structurally impossible. IsStaticContext skips it. (static ProcessHelper.RestartAsAdmin:83 does `process.Exited += (s,a) => comp.SetResult(...)` over method-locals) The undisposed `HttpClient` local in ScreenToGif.Test/Util/HttpHelper.cs (OWN001) is a TRUE positive and is intentionally left firing. Regression samples + wpf-extractor CI guards: AppLifetimeSample.cs (both App shapes stay silent), StaticContextSubscription.cs (static-context subs silent, the injected instance control still warns). The existing StaticEventEscapeViewModel (a non-App instance escape) must still raise OWN014, proving the exemptions are scoped and the detectors otherwise intact. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 22 ++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 75 +++++++++++++++++++ frontend/roslyn/samples/AppLifetimeSample.cs | 46 ++++++++++++ .../samples/StaticContextSubscription.cs | 53 +++++++++++++ 4 files changed, 196 insertions(+) create mode 100644 frontend/roslyn/samples/AppLifetimeSample.cs create mode 100644 frontend/roslyn/samples/StaticContextSubscription.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef82cbb7..90bb8b5a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,6 +136,8 @@ jobs: frontend/roslyn/samples/DiCaptiveSample.cs \ frontend/roslyn/samples/SampleTypes.cs \ frontend/roslyn/samples/PipeFieldsSample.cs \ + frontend/roslyn/samples/StaticContextSubscription.cs \ + frontend/roslyn/samples/AppLifetimeSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -283,6 +285,26 @@ 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-context exemption (mined: ScreenToGif ProcessHelper): a `+=` + # inside a STATIC member has no enclosing `this`, so it cannot leak an + # instance of its type. Both the static-class helper and the static method on + # an instance class subscribe `publisher.Fired` and must stay SILENT, while + # the INSTANCE control (`_bus.Fired += OnFired`) still warns. + echo "$out" | grep -qE "StaticContextSubscription\.cs:[0-9]+: warning: \[OWN001\]" \ + || { echo "FAIL: expected the instance-method subscription (control) to warn"; exit 1; } + echo "$out" | grep -q "_bus.Fired" \ + || { echo "FAIL: expected the injected instance subscription (_bus.Fired) to warn"; exit 1; } + if echo "$out" | grep -q "publisher.Fired"; then + echo "FAIL: a static-context subscription was wrongly reported (should be silent)"; 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 + # escape (OWN014) must NOT fire — for both the name-based (`partial class + # App`) and base-based (`: Application`) shapes. + if echo "$out" | grep -q "AppLifetimeSample.cs"; then + echo "FAIL: a process-lived App static-event subscription was wrongly reported (OWN014 FP)"; exit 1 + fi # P-006 DI001 (captive dependency): the registration + constructor graph # extracted from DiCaptiveSample.cs feeds ownlang/di.py. A singleton that # captures a scoped service — directly, transitively through a transient, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index e83ee2b9..ed2c356f 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -189,6 +189,66 @@ static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) => IsHandler(right) && model.GetSymbolInfo(right).Symbol is IMethodSymbol { IsStatic: true }; +// P-004 static-context exemption: a `+=` lexically inside a STATIC member (or a +// `static class`) has no enclosing `this`, so the handler — a static method group, +// or a lambda that can only close over locals/parameters/statics — retains no +// instance of this type. The "keeps alive" subscriber-leak is then +// structurally impossible, however long-lived the source. (Found mining +// ScreenToGif: `static ProcessHelper.RestartAsAdmin` does +// `process.Exited += (s, a) => comp.SetResult(...)` — a static helper's method-local +// `Process` and a lambda over locals; nothing instance-scoped to leak. The detector +// had mis-attributed it to an "injected dependency keeping ProcessHelper alive", +// but ProcessHelper is a static class — there is no instance.) The first enclosing +// member / local-function / type decides; lambdas are walked past so the rule keys +// off the real execution frame, not the closure. A static-source region escape +// (OWN014) is likewise impossible with no instance to promote, so this skips both. +static bool IsStaticContext(SyntaxNode node) +{ + foreach (var anc in node.Ancestors()) + switch (anc) + { + case LocalFunctionStatementSyntax lf: + return lf.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); + case BaseMethodDeclarationSyntax bm: + return bm.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); + case BasePropertyDeclarationSyntax bp: + return bp.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); + case TypeDeclarationSyntax t: + return t.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); + } + return false; +} + +// P-004 process-lived-subscriber exemption: the WPF application object (`App`) is a +// process-lived singleton — exactly one instance, created at startup, alive until +// the process exits. Subscribing it to a process-lived static event +// (`AppDomain.CurrentDomain.UnhandledException`, `SystemEvents.*`) promotes nothing: +// its "leaked" lifetime already equals the process. So a static-source region escape +// (OWN014) raised from inside `App` is a false positive (found mining ScreenToGif +// and its bundled Translator tool, both flagged on the textbook unhandled-exception +// hook). Detected syntactically because WPF does not resolve on the Linux runner: +// either the class derives from `Application` / `System.Windows.Application`, or it +// is the conventional XAML-split `partial class App` (whose `: Application` lives in +// the generated `App.g.cs` partial the extractor never sees). Only the STATIC-source +// escape is suppressed; an instance-field subscription leak inside `App` still fires. +static bool IsProcessLivedApplication(TypeDeclarationSyntax cls) +{ + if (cls.BaseList is { } bl) + foreach (var bt in bl.Types) + { + var n = bt.Type switch + { + IdentifierNameSyntax id => id.Identifier.Text, + QualifiedNameSyntax q => q.Right.Identifier.Text, + _ => null, + }; + if (n is "Application") + return true; + } + return cls.Identifier.Text == "App" + && cls.Modifiers.Any(m => m.IsKind(SyntaxKind.PartialKeyword)); +} + // P-004 severity tiering: of the subscriptions that survive the self-owned and // static-handler exemptions (and are not timers), how long-lived is the event // SOURCE? A static event lives for the whole process, so an undetached handler is @@ -1906,6 +1966,10 @@ or ImplicitObjectCreationExpressionSyntax && IsTemplatePartFetch(a.Right)) selfOwned.Add(tf.Name); + // 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); + var subs = new List(); foreach (var a in assigns) { @@ -1921,6 +1985,12 @@ or ImplicitObjectCreationExpressionSyntax if (leftSymbol is IEventSymbol ev) { var isTimer = IsTimerEvent(a.Left); + // A subscription in a STATIC context has no enclosing `this`, so no + // instance of this type can be retained — neither a subscriber leak + // (OWN001) nor a region escape (OWN014) is possible. (timers excluded: + // a running timer is dispatcher-rooted regardless of context.) + if (!isTimer && IsStaticContext(a)) + continue; // P-004 lifetime exemptions — skip, not a leak (timers excluded: a // running timer is dispatcher-rooted regardless): // - self-owned source (`this`, or a field/local the class @@ -1938,6 +2008,11 @@ 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. + if (source == "static" && clsIsApp) + continue; var released = unsub.Contains($"{a.Left}|{a.Right}") || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); subs.Add(new diff --git a/frontend/roslyn/samples/AppLifetimeSample.cs b/frontend/roslyn/samples/AppLifetimeSample.cs new file mode 100644 index 00000000..a7dbe670 --- /dev/null +++ b/frontend/roslyn/samples/AppLifetimeSample.cs @@ -0,0 +1,46 @@ +using System; +using System.Windows; + +namespace Own.Samples.WpfApp; + +// P-004 process-lived-subscriber exemption (mined from ScreenToGif + its Translator). +// +// The WPF application object (`App`) is a process-lived singleton: subscribing it to +// a process-lived static event (AppDomain.CurrentDomain.UnhandledException) is the +// textbook global-exception hook and promotes nothing — App already lives for the +// whole process. So the static-source region escape (OWN014) must NOT fire on it. +// Two detection shapes, both must stay SILENT: + +// (1) name-based: ScreenToGif's real shape — `partial class App` whose `: Application` +// lives in the generated `App.g.cs` partial the extractor never sees (here the +// only visible base is IDisposable). +public partial class App : IDisposable +{ + private void App_Startup(object sender, EventArgs e) + { + AppDomain.CurrentDomain.UnhandledException += CurrentDomain_UnhandledException; + } + + private void CurrentDomain_UnhandledException(object sender, UnhandledExceptionEventArgs e) + { + Console.WriteLine(e.ExceptionObject); + } + + public void Dispose() { } +} + +// (2) base-based: a custom-named application object deriving from `Application` +// directly in the .cs (the base stays unresolved on the Linux runner, but the +// syntactic base-name detection still applies). +public class BootstrapApp : Application +{ + public void Init() + { + AppDomain.CurrentDomain.UnhandledException += OnUnhandled; + } + + private void OnUnhandled(object sender, UnhandledExceptionEventArgs e) + { + Console.WriteLine(e.ExceptionObject); + } +} diff --git a/frontend/roslyn/samples/StaticContextSubscription.cs b/frontend/roslyn/samples/StaticContextSubscription.cs new file mode 100644 index 00000000..fa6c44ae --- /dev/null +++ b/frontend/roslyn/samples/StaticContextSubscription.cs @@ -0,0 +1,53 @@ +using System; +using System.Threading.Tasks; + +namespace Own.Samples; + +// P-004 static-context exemption (mined from ScreenToGif's ProcessHelper). +// +// A `+=` lexically inside a STATIC member has no enclosing `this`, so the handler — +// a method group or a lambda over locals/parameters/statics — retains no instance of +// the enclosing type. The "keeps alive" subscriber leak is then structurally +// impossible, so these subscriptions must stay SILENT. The negative control +// (`InstanceSetup`) proves the skip does NOT bleed into ordinary instance +// subscriptions to an injected source, which must still WARN (OWN001). +public interface IPublisher +{ + event EventHandler Fired; +} + +// ScreenToGif's `static ProcessHelper.RestartAsAdmin`: a static helper subscribes a +// method-local publisher's event with a lambda capturing only locals. No instance of +// the static class exists -> nothing to leak -> SILENT. +public static class StaticHelperSubscription +{ + public static bool Run(IPublisher publisher) + { + var done = new TaskCompletionSource(); + publisher.Fired += (s, e) => done.SetResult(true); + return done.Task.Result; + } +} + +public class MixedSubscription +{ + private readonly IPublisher _bus; + + public MixedSubscription(IPublisher bus) => _bus = bus; + + // A static method on an INSTANCE class is still a static context (no `this`), + // so this subscription to the parameter's event must also stay SILENT. + public static void StaticSetup(IPublisher publisher) + { + publisher.Fired += (s, e) => Console.WriteLine("static"); + } + + // Control: an INSTANCE method subscribing an instance handler to an INJECTED + // source (a ctor-param bus of unknown lifetime) is the classic leak -> must WARN. + public void InstanceSetup() + { + _bus.Fired += OnFired; + } + + private void OnFired(object sender, EventArgs e) => Console.WriteLine(_bus); +} From 3c46b87810dc599945d5969ce751d1b5c601d92b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 10:03:14 +0000 Subject: [PATCH 2/2] fix(extractor): revert unsound static-context exemption (Codex P2); keep App-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged the static-context skip as unsound (correctly): a `+=` inside a static member has no `this`, but the handler can still retain a NON-`this` instance — `publisher.Fired += subscriber.OnFired`, or a lambda capturing a parameter — which an outliving source leaks. "No enclosing `this`" does not imply "nothing can be retained". The ProcessHelper case is safe only because its source `process` is a FRESH, non-escaping method-local (`Process.Start(info)`), but there is no sound syntactic rule that admits a factory-fresh local while still excluding an aliased long-lived source (`var src = injectedBus; src.X += ...`) — only `new` proves freshness. A known soundness hole is worse than the FP under the zero-FN mandate, so the static-context exemption (helper + loop skip + StaticContextSubscription.cs + its CI assertions) is reverted in full. ProcessHelper's `process.Exited` subscription stays an honest "possible leak" warning; a proper fresh-source fix is a separate, deliberate change. This leaves PR #81 as the clean, uncontested App fix only: OWN014 region-escape is suppressed for the process-lived WPF `App` singleton. Also handle the alias-qualified base type `global::System.Windows.Application` (AliasQualifiedNameSyntax) in IsProcessLivedApplication, per CodeRabbit — without it `clsIsApp` would miss that declaration shape and re-enable the FP. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 13 ----- frontend/roslyn/OwnSharp.Extractor/Program.cs | 37 +------------ .../samples/StaticContextSubscription.cs | 53 ------------------- 3 files changed, 1 insertion(+), 102 deletions(-) delete mode 100644 frontend/roslyn/samples/StaticContextSubscription.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90bb8b5a..52278707 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,7 +136,6 @@ jobs: frontend/roslyn/samples/DiCaptiveSample.cs \ frontend/roslyn/samples/SampleTypes.cs \ frontend/roslyn/samples/PipeFieldsSample.cs \ - frontend/roslyn/samples/StaticContextSubscription.cs \ frontend/roslyn/samples/AppLifetimeSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" @@ -285,18 +284,6 @@ 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-context exemption (mined: ScreenToGif ProcessHelper): a `+=` - # inside a STATIC member has no enclosing `this`, so it cannot leak an - # instance of its type. Both the static-class helper and the static method on - # an instance class subscribe `publisher.Fired` and must stay SILENT, while - # the INSTANCE control (`_bus.Fired += OnFired`) still warns. - echo "$out" | grep -qE "StaticContextSubscription\.cs:[0-9]+: warning: \[OWN001\]" \ - || { echo "FAIL: expected the instance-method subscription (control) to warn"; exit 1; } - echo "$out" | grep -q "_bus.Fired" \ - || { echo "FAIL: expected the injected instance subscription (_bus.Fired) to warn"; exit 1; } - if echo "$out" | grep -q "publisher.Fired"; then - echo "FAIL: a static-context subscription was wrongly reported (should be silent)"; 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 ed2c356f..091baec2 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -189,36 +189,6 @@ static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) => IsHandler(right) && model.GetSymbolInfo(right).Symbol is IMethodSymbol { IsStatic: true }; -// P-004 static-context exemption: a `+=` lexically inside a STATIC member (or a -// `static class`) has no enclosing `this`, so the handler — a static method group, -// or a lambda that can only close over locals/parameters/statics — retains no -// instance of this type. The "keeps alive" subscriber-leak is then -// structurally impossible, however long-lived the source. (Found mining -// ScreenToGif: `static ProcessHelper.RestartAsAdmin` does -// `process.Exited += (s, a) => comp.SetResult(...)` — a static helper's method-local -// `Process` and a lambda over locals; nothing instance-scoped to leak. The detector -// had mis-attributed it to an "injected dependency keeping ProcessHelper alive", -// but ProcessHelper is a static class — there is no instance.) The first enclosing -// member / local-function / type decides; lambdas are walked past so the rule keys -// off the real execution frame, not the closure. A static-source region escape -// (OWN014) is likewise impossible with no instance to promote, so this skips both. -static bool IsStaticContext(SyntaxNode node) -{ - foreach (var anc in node.Ancestors()) - switch (anc) - { - case LocalFunctionStatementSyntax lf: - return lf.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); - case BaseMethodDeclarationSyntax bm: - return bm.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); - case BasePropertyDeclarationSyntax bp: - return bp.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); - case TypeDeclarationSyntax t: - return t.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)); - } - return false; -} - // P-004 process-lived-subscriber exemption: the WPF application object (`App`) is a // process-lived singleton — exactly one instance, created at startup, alive until // the process exits. Subscribing it to a process-lived static event @@ -240,6 +210,7 @@ static bool IsProcessLivedApplication(TypeDeclarationSyntax cls) { IdentifierNameSyntax id => id.Identifier.Text, QualifiedNameSyntax q => q.Right.Identifier.Text, + AliasQualifiedNameSyntax aq => aq.Name.Identifier.Text, _ => null, }; if (n is "Application") @@ -1985,12 +1956,6 @@ or ImplicitObjectCreationExpressionSyntax if (leftSymbol is IEventSymbol ev) { var isTimer = IsTimerEvent(a.Left); - // A subscription in a STATIC context has no enclosing `this`, so no - // instance of this type can be retained — neither a subscriber leak - // (OWN001) nor a region escape (OWN014) is possible. (timers excluded: - // a running timer is dispatcher-rooted regardless of context.) - if (!isTimer && IsStaticContext(a)) - continue; // P-004 lifetime exemptions — skip, not a leak (timers excluded: a // running timer is dispatcher-rooted regardless): // - self-owned source (`this`, or a field/local the class diff --git a/frontend/roslyn/samples/StaticContextSubscription.cs b/frontend/roslyn/samples/StaticContextSubscription.cs deleted file mode 100644 index fa6c44ae..00000000 --- a/frontend/roslyn/samples/StaticContextSubscription.cs +++ /dev/null @@ -1,53 +0,0 @@ -using System; -using System.Threading.Tasks; - -namespace Own.Samples; - -// P-004 static-context exemption (mined from ScreenToGif's ProcessHelper). -// -// A `+=` lexically inside a STATIC member has no enclosing `this`, so the handler — -// a method group or a lambda over locals/parameters/statics — retains no instance of -// the enclosing type. The "keeps alive" subscriber leak is then structurally -// impossible, so these subscriptions must stay SILENT. The negative control -// (`InstanceSetup`) proves the skip does NOT bleed into ordinary instance -// subscriptions to an injected source, which must still WARN (OWN001). -public interface IPublisher -{ - event EventHandler Fired; -} - -// ScreenToGif's `static ProcessHelper.RestartAsAdmin`: a static helper subscribes a -// method-local publisher's event with a lambda capturing only locals. No instance of -// the static class exists -> nothing to leak -> SILENT. -public static class StaticHelperSubscription -{ - public static bool Run(IPublisher publisher) - { - var done = new TaskCompletionSource(); - publisher.Fired += (s, e) => done.SetResult(true); - return done.Task.Result; - } -} - -public class MixedSubscription -{ - private readonly IPublisher _bus; - - public MixedSubscription(IPublisher bus) => _bus = bus; - - // A static method on an INSTANCE class is still a static context (no `this`), - // so this subscription to the parameter's event must also stay SILENT. - public static void StaticSetup(IPublisher publisher) - { - publisher.Fired += (s, e) => Console.WriteLine("static"); - } - - // Control: an INSTANCE method subscribing an instance handler to an INJECTED - // source (a ctor-param bus of unknown lifetime) is the classic leak -> must WARN. - public void InstanceSetup() - { - _bus.Fired += OnFired; - } - - private void OnFired(object sender, EventArgs e) => Console.WriteLine(_bus); -}