diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91c72c29..a4fef14b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -199,6 +199,7 @@ jobs: frontend/roslyn/samples/StaticClassEscapeSample.cs \ frontend/roslyn/samples/EventSourceCountersSample.cs \ frontend/roslyn/samples/AppDomainShutdownSample.cs \ + frontend/roslyn/samples/LambdaTiersSample.cs \ frontend/roslyn/samples/AliasDisposeSample.cs \ frontend/roslyn/samples/CloseReleaseSample.cs \ frontend/roslyn/samples/SemaphoreFieldSample.cs \ @@ -453,12 +454,43 @@ jobs: if echo "$out" | grep -qE "OWN014.*'ShutdownCleanup'"; then echo "FAIL: ShutdownCleanup's non-capturing AppDomain subscriptions were wrongly reported as OWN014"; exit 1 fi - # scope guards: a lambda on a NON-AppDomain static event still escapes; and an AppDomain - # handler that CAPTURES instance state is still pinned to the process -> still OWN014 (Codex). - echo "$out" | grep -qE "OWN014.*NonAppDomainSubscriber" \ - || { echo "FAIL: a lambda on a non-AppDomain static event must still raise OWN014 (exemption stays scoped)"; exit 1; } - echo "$out" | grep -qE "OWN014.*CapturingShutdownSubscriber" \ + # issue #199 — capture-aware static tier: the region escape keys off whether the handler + # RETAINS an instance. A CAPTURING lambda on a non-AppDomain static event (NonAppDomain- + # Subscriber captures the ctor's `cts` — the CsvHelper cts/resetEvent shape Codex defended) + # still pins it -> OWN014; an AppDomain handler that captures instance state (Capturing- + # ShutdownSubscriber) is still pinned -> OWN014. Both must stay flagged. + echo "$out" | grep -qE "\[OWN014\].*NonAppDomainSubscriber" \ + || { echo "FAIL: a CAPTURING lambda on a non-AppDomain static event must still raise OWN014"; exit 1; } + echo "$out" | grep -qE "\[OWN014\].*CapturingShutdownSubscriber" \ || { echo "FAIL: an instance-capturing AppDomain handler must still raise OWN014 (exemption is non-capturing only)"; exit 1; } + # issue #199 FP FIX: a NON-CAPTURING lambda on a static event retains no instance (the + # closure analog of the static-METHOD exemption / StaticHandlerViewModel) -> OWN014's + # premise ("subscriber pinned") fails -> SILENT. Reverses the prior false positive where a + # non-capturing static lambda was flagged (policy: docs/notes/subscription-leaks-and- + # profiles.md — "static + non-retaining handler -> silent; static + retaining -> OWN014"). + if echo "$out" | grep -q "NonCapturingStaticSubscriber"; then + echo "FAIL: a NON-capturing lambda on a static event must be SILENT (retains no instance)"; exit 1 + fi + # issue #199 cosmetic: a LAMBDA handler's OWN014 message spells out the no-'-=' handle, + # like OWN001 does (extractor stamps lambda:true; the bridge folds in the note). + echo "$out" | grep -qE "\[OWN014\].*NonAppDomainSubscriber.*inline lambda it has no" \ + || { echo "FAIL: OWN014 on a lambda handler must carry the inline-lambda no-'-=' note"; exit 1; } + # issue #199 — POSITIVE anchor for LambdaTiersSample.cs: an INJECTED-source lambda + # (publisher handed in as a ctor param, lifetime unknown) is the honest hedge -> OWN001 + # WARNING, and it carries the inline-lambda no-'-=' note like OWN014 does. This is the ONLY + # component the extractor emits from LambdaTiersSample.cs, so it proves the file is actually + # extracted — without it the two silent negatives below would pass VACUOUSLY (CodeRabbit). + echo "$out" | grep -qE "LambdaTiersSample\.cs:[0-9]+: warning: \[OWN001\].*injected dependency.*'InjectedSourceLambda'" \ + || { echo "FAIL: expected OWN001 warning on the injected-source lambda (also the file's extraction anchor)"; exit 1; } + echo "$out" | grep -qE "\[OWN001\].*InjectedSourceLambda.*inline lambda it has no" \ + || { echo "FAIL: OWN001 on a lambda handler must also carry the inline-lambda no-'-=' note"; exit 1; } + # issue #199 — the SILENT lambda tiers (bounded/local + self-owned): a lambda on a + # method-bounded local source, or on a field the class constructs, is not a heap leak + # (the '-=-impossible-but-bounded' shape) -> must be silent. These negatives are NOT + # vacuous: the InjectedSourceLambda anchor above proves the file reached the extractor. + if echo "$out" | grep -qE "LocalBoundedLambda|SelfOwnedFieldLambda"; then + echo "FAIL: a lambda on a bounded/local or self-owned source must stay SILENT"; exit 1 + fi # the unsubscribed variant (a matching `-=`, released capture) is mitigated # -> silent. Must NOT be reported. if echo "$out" | grep -q "CleanStaticEventViewModel"; then diff --git a/docs/notes/field-notes-patterns.md b/docs/notes/field-notes-patterns.md index 27998a2f..3a22a2bb 100644 --- a/docs/notes/field-notes-patterns.md +++ b/docs/notes/field-notes-patterns.md @@ -299,6 +299,41 @@ and the `IsDisposeOptional` allowlist is where that knowledge belongs.** --- +## 10. Invocation-list growth from repeated *non-capturing* subscriptions (candidate, uncovered) + +Recorded as a deliberate non-goal of the issue #199 static-lambda precision fix +(see [`subscription-leaks-and-profiles.md`](subscription-leaks-and-profiles.md)). + +```csharp +static event EventHandler? Pinged; // process-lived static event +sealed class Widget { + public Widget() => Pinged += (_, _) => Log("tick"); // NON-capturing lambda +} +// new Widget(); … many times ⇒ Pinged's invocation list grows without bound +``` + +A **non-capturing** `+=` retains no subscriber instance, so it is *not* a region +escape (OWN014) and is now correctly **silent**. But it still *appends* a delegate to +the event's invocation list, and a hot path that re-subscribes (a `+=` in the +constructor of a short-lived / transient object created repeatedly) grows that list +unbounded — a genuine memory-growth bug of a **different shape**: unbounded *list +length*, not a pinned *instance*. + +**Why the region model doesn't (and shouldn't) cover it.** OWN014 answers "is *this* +subscriber promoted to the source's lifetime?" — a per-instance lifetime question. +Invocation-list growth is a *call-count* question (how often does this `+=` run?), +which needs a different signal: a "subscribe in a hot/repeated constructor without a +matching `-=`" heuristic. It was **never** covered for the static-*method* exemption +either (`X.Event += StaticM` in a loop grows the list identically), so silencing the +non-capturing **lambda** is not a regression relative to that baseline — both are +outside the region model by construction. + +**Analyzer angle:** if pursued, gate on *repetition* (a `+=` reachable from a +type instantiated in a loop / registered transient) + *no `-=`*, and tier it as a +warning — never fold it into OWN014, whose subscriber-pinning premise it does not share. + +--- + ## The through-line Entries 1–6 are the *same lesson from different angles*: **disposal diff --git a/docs/notes/subscription-leaks-and-profiles.md b/docs/notes/subscription-leaks-and-profiles.md index c1e3579b..ed941d1c 100644 --- a/docs/notes/subscription-leaks-and-profiles.md +++ b/docs/notes/subscription-leaks-and-profiles.md @@ -106,7 +106,11 @@ We keep the code `OWN001`. The open behavioural question was: do we always shout lifetime evidence, tier `OWN001`'s *severity* by what the source provably is: ```text -static event + capturing handler -> error process-lifetime: a provable leak +static event + retaining handler -> error process-lifetime: a provable leak — the + (captures `this` or a local) handler pins the subscriber to the source +static event + non-retaining -> silent nothing is pinned: a static method (null + handler (static method / delegate target) or a NON-capturing lambda + non-capturing lambda) (closure analog of the static-method exempt) field / ctor-param / property -> warning lifetime unknown — may leak if the source outlives `this`; an inline lambda has no handle to `-=` at all @@ -116,6 +120,31 @@ local publisher -> drop dies with the scope (today a fals this / constructed field -> exempt self-owned cycle, GC-collectable (P-004) ``` +**Ledger (issue #199): the static tier keys on whether the handler RETAINS an +instance.** OWN014's premise is "the strong subscription pins the subscriber to the +source's (process) lifetime." A handler that retains **nothing** — a static method +(null delegate `Target`) or a **non-capturing lambda** — pins no subscriber, so the +premise fails and it is **silent**, the closure analog of the long-standing +static-method exemption (`StaticHandlerViewModel`). A handler that captures `this` +**or an enclosing local** (the CsvHelper `ConsoleHost` `cts`/`resetEvent` shape) does +pin state and stays **OWN014**. This is gated strictly through the extractor's +conservative `HandlerRetainsNoInstance` (Program.cs): any handler it cannot *prove* +non-retaining — a captured local, an opaque delegate-typed value — is treated as +retaining and still flags. It never widens the exemption syntactically (no +`clsIsStatic`, no "all lambdas"). Pinned by `AppDomainShutdownSample` +(`NonAppDomainSubscriber` capturing → OWN014, `NonCapturingStaticSubscriber` → silent). + +**Deliberate non-goal — invocation-list growth from repeated non-capturing +subscriptions.** A non-capturing `+=` still *appends* a delegate to the event's +invocation list, and a hot path that re-subscribes (e.g. `+=` in the ctor of a +short-lived / transient object created many times) grows that list unbounded — a real +but *different* memory-growth shape (unbounded list length, not a pinned subscriber +instance) that the OWN014 region-escape model does not express. It was never covered +for the static-method exemption either, so silencing the non-capturing **lambda** is +*not* a regression relative to that. Recorded as a candidate in +[`field-notes-patterns.md`](field-notes-patterns.md); it would need its own signal +(a subscribe-in-a-hot-constructor heuristic), not the region model. + The punchline ties the two halves together: **the WPF profile is exactly the thing that turns that `warning` back into an `error`.** When the profile (or an explicit `lifetime` region) resolves the source to App-lifetime over a ViewModel subscriber, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index c625e621..c7857fdc 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -3550,6 +3550,23 @@ or ImplicitObjectCreationExpressionSyntax // write-up: docs/notes/oracle-known-fps.md → "Rejected approaches". if (!isTimer && source == "static" && clsIsApp) continue; + // P-004 / issue #199: a NON-RETAINING handler on a static (process-lived) + // source promotes nothing, so OWN014's premise ("the subscriber is pinned + // for the source's life") does not hold -> silent. A static METHOD handler + // is already dropped above (IsStaticHandler: null delegate target); a + // NON-CAPTURING lambda is its closure analog — it captures neither `this` + // nor an enclosing local, so no subscriber instance is retained. Gated + // STRICTLY through HandlerRetainsNoInstance, which is CONSERVATIVE: a lambda + // that captures `this` OR any enclosing local (the CsvHelper cts/resetEvent + // shape), or any delegate-typed value it cannot prove, is treated as + // RETAINING and STILL raises OWN014. This never widens the exemption + // syntactically (no `clsIsStatic`, no "all lambdas"): only what is provably + // non-retaining is dropped, exactly like the static-method exemption. Policy: + // docs/notes/subscription-leaks-and-profiles.md ("static + non-retaining + // handler (static method / non-capturing lambda) -> silent; static + + // retaining -> OWN014"). + if (!isTimer && source == "static" && HandlerRetainsNoInstance(a.Right, model)) + continue; var released = unsub.Contains($"{a.Left}|{NormalizeHandler(a.Right)}") || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); subs.Add(new diff --git a/frontend/roslyn/samples/AppDomainShutdownSample.cs b/frontend/roslyn/samples/AppDomainShutdownSample.cs index a83529b3..55ff2b81 100644 --- a/frontend/roslyn/samples/AppDomainShutdownSample.cs +++ b/frontend/roslyn/samples/AppDomainShutdownSample.cs @@ -42,12 +42,30 @@ public static class SomeBus public static void Raise() => Pinged?.Invoke(null, EventArgs.Empty); } -// Control: a lambda on a NON-AppDomain process-lived (static) event -> region escape -> must WARN. +// Control (the shape Codex defended — CsvHelper ConsoleHost's cts/resetEvent capture): +// a CAPTURING lambda on a NON-AppDomain process-lived (static) event pins the captured +// state (here the ctor's `cts`) for the whole process — a real region escape that must +// STILL raise OWN014. The non-retaining static gate (issue #199) must NOT clear this: +// HandlerRetainsNoInstance returns false because an enclosing local/parameter is captured. public sealed class NonAppDomainSubscriber { - public NonAppDomainSubscriber() + public NonAppDomainSubscriber(System.Threading.CancellationTokenSource cts) { - SomeBus.Pinged += (_, _) => Handle(); // static event, lambda, not AppDomain -> OWN014 + SomeBus.Pinged += (_, _) => cts.Cancel(); // captures `cts` -> OWN014 + } +} + +// CONTRAST (issue #199): a NON-CAPTURING lambda on the SAME non-AppDomain static event +// captures neither `this` nor any enclosing local (a bare static call) -> retains no +// instance -> the closure analog of the static-METHOD exemption (StaticHandlerViewModel) +// -> SILENT. This is the false positive the non-retaining static gate removes: a +// non-retaining handler cannot pin a subscriber, so OWN014's premise does not hold. +// Policy: docs/notes/subscription-leaks-and-profiles.md (static + non-retaining -> silent). +public sealed class NonCapturingStaticSubscriber +{ + public NonCapturingStaticSubscriber() + { + SomeBus.Pinged += (_, _) => Handle(); // no capture -> silent } private static void Handle() { } diff --git a/frontend/roslyn/samples/LambdaTiersSample.cs b/frontend/roslyn/samples/LambdaTiersSample.cs new file mode 100644 index 00000000..d9e97be3 --- /dev/null +++ b/frontend/roslyn/samples/LambdaTiersSample.cs @@ -0,0 +1,60 @@ +using System; + +namespace Own.Samples; + +// Issue #199 — the SILENT lambda-handler tiers (the static/injected tiers live in +// AppDomainShutdownSample.cs / LambdaHandlerViewModel.cs). Source lifetime decides the +// verdict for a lambda exactly as for a method group; a lambda merely can never be +// `-=`'d (no handle), which matters only when the source actually outlives `this`. + +// BOUNDED / LOCAL source + lambda: the publisher is constructed HERE (a local), so it is +// method-bounded — it dies with the scope and cannot outlive `this`, so the subscription +// is no heap leak -> SILENT (the extractor drops source=="local"). The lambda has no `-=` +// handle, but with a bounded source there is nothing to leak: this IS the +// "`-=`-impossible-but-bounded" shape that must stay silent. +public sealed class LocalBoundedLambda +{ + public void Work() + { + var pub = new Publisher(); + pub.Changed += (s, e) => Console.WriteLine("bounded"); // local source -> SILENT + pub.Raise(); + } +} + +// SELF-OWNED field source + lambda: the class constructs the source (a field it `new`s), +// so the source<->this cycle is collectable together -> SILENT (self-owned exemption), +// whether the handler is a lambda (captures `this`) or a method group. +public sealed class SelfOwnedFieldLambda +{ + private int _n; + private readonly Publisher _pub = new Publisher(); + + public SelfOwnedFieldLambda() + { + _pub.Changed += (s, e) => _n++; // self-owned source -> SILENT + } +} + +// INJECTED source + lambda: the publisher is handed in (a ctor param), so its lifetime is +// unknown — it may outlive `this`, and the inline lambda has no `-=` handle to ever detach -> +// OWN001 WARNING (the injected tier — the honest hedge; a WPF/region profile is what escalates +// it to OWN014). This is also the file's POSITIVE anchor: it is the only class here the extractor +// emits a component for, so the silent negatives above cannot pass vacuously if this file ever +// stops being extracted. +public sealed class InjectedSourceLambda +{ + private int _n; + + public InjectedSourceLambda(Publisher pub) + { + pub.Changed += (s, e) => _n++; // injected source -> OWN001 warning + } +} + +public sealed class Publisher +{ + public event EventHandler? Changed; + + public void Raise() => Changed?.Invoke(this, EventArgs.Empty); +} diff --git a/ownlang/ownir.py b/ownlang/ownir.py index c178e4d8..69861096 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -2502,11 +2502,16 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: "scoped": "a DI scoped service", "transient": "a DI transient service", }.get(life, f"a DI {life} service") + # An inline lambda handler has no `-=` handle, so it could never be + # detached even on purpose — surfaced on OWN014 too (P-004; issue #199), + # matching the OWN001 note below. Empty for a method-group handler. + lam = (" — and being an inline lambda it has no '-=' handle, so it " + "could never be detached") if sub.get("lambda") else "" message = (f"event '{event}' is subscribed (handler '{handler}') to " f"'{st}' — {nice} that outlives '{component}'; the strong " f"subscription promotes '{component}' to the source's " f"lifetime, so it can never be collected — a captive/region " - f"escape (leak, no release path)") + f"escape (leak, no release path{lam})") # reachability slice: the subscribe site -> where the longer-lived source was # registered (its lifetime is why this escapes). The source hop is present only # when the registration site is known from the services graph. @@ -2538,11 +2543,18 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: src = sub.get("source", "?") origin = ("a static (process-lived) event source" if src == "static" else f"a longer-lived source ('{src}')") + # An inline lambda handler has no `-=` handle (issue #199) — surface it on + # the OWN014 region-escape message too, like the OWN001 note below. A + # non-capturing lambda on a static source is exempted upstream (the + # extractor's non-retaining gate), so a lambda that reaches HERE is a + # capturing one — the note is apt. Empty for a method-group handler. + lam = (" — and being an inline lambda it has no '-=' handle, so it " + "could never be detached") if sub.get("lambda") else "" message = (f"event '{event}' is subscribed (handler '{handler}') to " f"{origin} that outlives '{component}'; the strong " f"subscription promotes '{component}' to the source's " f"lifetime, so it can never be collected — a region escape " - f"(leak, no release path)") + f"(leak, no release path{lam})") findings.append(Finding( file=sub["file"], line=int(sub.get("line", 0)), code=d.code, component=component, event=event, handler=handler, diff --git a/tests/test_ownir.py b/tests/test_ownir.py index 3e56bd0f..d226fa26 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -1202,6 +1202,24 @@ def _sub(source: str | None) -> list[Finding]: if any(x.component == "CleanThemeViewModel" for x in cfindings): fails.append("a released (unsubscribed) static capture was wrongly reported") + # issue #199: a static-source capture whose handler is an inline LAMBDA gets the + # same OWN014 region escape, now carrying the "inline lambda — no '-=' handle" note + # (the extractor stamps lambda:true). A NON-capturing static lambda is dropped + # upstream by the extractor's non-retaining gate, so a lambda reaching this branch + # is a capturing one; a method-group capture (lambda absent, above) has no note. + checks += 1 + lamcap = check_facts({"module": "M", "components": [ + {"name": "PingVM", "file": "PingVM.cs", "subscriptions": [ + {"event": "SomeBus.Pinged", "handler": "(_, _) => _n++", "line": 7, + "released": False, "resource": "capture", "source": "static", + "lambda": True}]}]}) + if [(x.component, x.line, x.code) for x in lamcap] != [("PingVM", 7, "OWN014")]: + fails.append(f"a lambda static capture should raise OWN014 (PingVM@7), got " + f"{[(x.component, x.line, x.code) for x in lamcap]}") + elif "inline lambda" not in lamcap[0].message or "-=" not in lamcap[0].message: + fails.append(f"OWN014 lambda-capture message missing the no-'-=' note: " + f"{lamcap[0].message!r}") + # --- P-006 + P-004: DI-sourced region escape. An injected subscription whose # source TYPE resolves (via the `services` graph) to a longer-lived DI # registration than the subscriber is a PROVABLE region escape -> OWN014,