diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fa78cec6..8da46a6c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -115,6 +115,8 @@ jobs: run: | dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \ frontend/roslyn/samples/CustomerViewModel.cs \ + frontend/roslyn/samples/LambdaHandlerViewModel.cs \ + frontend/roslyn/samples/AliasedSourceViewModel.cs \ frontend/roslyn/samples/OrdersViewModel.cs \ frontend/roslyn/samples/TimerViewModel.cs \ frontend/roslyn/samples/DisposableFieldViewModel.cs \ @@ -130,13 +132,33 @@ jobs: run: | out=$(python -m ownlang ownir "$RUNNER_TEMP/facts.json" || true) echo "$out" - echo "$out" | grep -q "CustomerViewModel.cs" \ - || { echo "FAIL: expected the CustomerViewModel leak"; exit 1; } echo "$out" | grep -q "OWN001" \ || { echo "FAIL: expected OWN001"; exit 1; } + # P-004 tiering: CustomerViewModel subscribes to an INJECTED bus (a ctor + # param of unknown lifetime). We cannot prove it outlives the view model, + # so the leak is reported at WARNING level (an honest "possible leak"), + # not a hard error — until lifetime/ownership modelling lands. + echo "$out" | grep -qE "CustomerViewModel\.cs:[0-9]+: warning: \[OWN001\]" \ + || { echo "FAIL: expected CustomerViewModel as a WARNING (injected source)"; exit 1; } + echo "$out" | grep -q "injected dependency whose lifetime is unknown" \ + || { echo "FAIL: expected the injected-source wording"; exit 1; } if echo "$out" | grep -q "OrdersViewModel.cs"; then echo "FAIL: disposed subscription wrongly reported"; exit 1 fi + # a lambda handler has no stored delegate, so it can NEVER be `-=`'d — the + # finding says so. (Same injected source as Customer -> also a warning.) + echo "$out" | grep -qE "LambdaHandlerViewModel\.cs:[0-9]+: warning: \[OWN001\]" \ + || { echo "FAIL: expected the lambda-handler subscription leak (warning)"; exit 1; } + echo "$out" | grep -q "inline lambda it has no '-=' handle" \ + || { echo "FAIL: expected the lambda no-handle wording"; exit 1; } + # P-004 provenance: a local that ALIASES an injected source (var src = + # _bus) is NOT method-bounded — it must warn, not be silently dropped. A + # local the scope CONSTRUCTS (var owned = new Calc()) IS bounded -> silent. + echo "$out" | grep -qE "AliasedSourceViewModel\.cs:[0-9]+: warning: \[OWN001\]" \ + || { echo "FAIL: aliased-injected local should warn, not be dropped"; exit 1; } + if echo "$out" | grep -q "owned.Changed"; then + echo "FAIL: a locally-constructed publisher must be dropped"; exit 1 + fi # WPF002: the started, never-stopped timer leaks with a [resource: timer] # tag; the timer stopped in Dispose stays silent. echo "$out" | grep -q "TimerViewModel.cs" \ @@ -234,6 +256,30 @@ jobs: if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi done echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach, never-vs-every-path wording, dispose-optional exempt, beyond flat)" + - name: Coverage summary (--stats) + run: | + # --stats prints a flow-locals coverage line to stderr and stamps the same + # counts into the facts JSON: of the methods with a disposable local, how + # many were flow-analysed vs honestly skipped (an unmodelled construct). + cov=$(dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \ + frontend/roslyn/samples/FlowLocalsSample.cs --flow-locals --stats \ + -o "$RUNNER_TEMP/stats.json" 2>&1 >/dev/null) + echo "$cov" + echo "$cov" | grep -qE '^coverage: [0-9]+/[0-9]+ methods .* flow-analysed' \ + || { echo "FAIL: expected a --stats coverage line on stderr"; exit 1; } + # Parse the JSON (not a substring grep): assert the stats object exists, + # all three counters are numbers, and the invariant holds — every method + # with a disposable local is either flow-analysed or honestly skipped. + jq -e '.stats as $s + | ($s.methods_with_local | type == "number") + and ($s.methods_flow_analysed | type == "number") + and ($s.methods_skipped_unmodelled | type == "number") + and ($s.methods_flow_analysed + $s.methods_skipped_unmodelled + == $s.methods_with_local)' \ + "$RUNNER_TEMP/stats.json" >/dev/null \ + || { echo "FAIL: stats object missing / non-numeric / invariant violated"; + cat "$RUNNER_TEMP/stats.json"; exit 1; } + echo "OK: --stats coverage on stderr + valid stats object (invariant holds)" - name: Escape-via-projection leak — GTM UnitOfWork (--flow-locals, P-016 B0b/B2) run: | # A real GTM leak the flat detector misses: a UnitOfWork (IDisposable) used @@ -289,12 +335,18 @@ jobs: || { echo "FAIL: expected the relative path to the Customer leak"; exit 1; } echo "$out" | grep -q "title=OWN001" \ || { echo "FAIL: expected the OWN001 title in the annotation"; exit 1; } - - name: MSBuild diagnostic format over the sample tree + - name: MSBuild diagnostic format over the sample tree (severity tiering) run: | out=$(scripts/own-check.sh --format msbuild -- frontend/roslyn/samples) echo "--- diagnostics ---"; echo "$out"; echo "-------------------" - echo "$out" | grep -qE "CustomerViewModel\.cs\([0-9]+\): error OWN001:" \ - || { echo "FAIL: expected an MSBuild-format error line"; exit 1; } + # P-004 tiering at the default severity, both sides: an injected-source + # subscription (CustomerViewModel's `bus` is a ctor param of unknown + # lifetime) renders as a WARNING, while a provable leak — the started, + # never-stopped timer — stays an ERROR. + echo "$out" | grep -qE "CustomerViewModel\.cs\([0-9]+\): warning OWN001:" \ + || { echo "FAIL: expected CustomerViewModel as a warning (injected source)"; exit 1; } + echo "$out" | grep -qE "TimerViewModel\.cs\([0-9]+\): error OWN001:" \ + || { echo "FAIL: expected the timer leak to stay an error"; exit 1; } - name: --severity warning renders advisory diagnostics run: | out=$(scripts/own-check.sh --format msbuild --severity warning -- frontend/roslyn/samples) diff --git a/docs/notes/subscription-leaks-and-profiles.md b/docs/notes/subscription-leaks-and-profiles.md new file mode 100644 index 00000000..c1e3579b --- /dev/null +++ b/docs/notes/subscription-leaks-and-profiles.md @@ -0,0 +1,149 @@ +# Subscription leaks are a .NET concern, not a WPF one — codes vs. profiles + +Prompted by a good question: should `event += without -=` be a `WPFxxx` error? +Short answer **no** — it is a general .NET lifetime/subscription bug, and the core +already treats it that way (`OWN001` + `[resource: subscription token]`, never a +"WPF" code). Recording the taxonomy so we don't re-open it, and so the *docs* +stop reading as if this were WPF-only when the capability is .NET-wide. + +## What the core actually emits (we already did the right thing) + +`event += without -=` lowers to a resource and comes out as **`OWN001`** with a +domain-neutral `[resource: kind]` tag — see the [README](../../README.md) +"Бизнес-применение" section and [P-001](../proposals/P-001-csharp-extractor.md): + +```text +case.own:16: error: [OWN001] '…' is owned but not released … [resource: subscription token] +``` + +The `[resource: kind]` tag is the **seam**: the WPF profile (and the Roslyn +front-end) key off it without the core knowing a thing about WPF. That is already +in the README ("шов, за который зацепится WPF-профиль, не зная про WPF в ядре"). + +Crucially, the `WPF001..WPF005` in [P-004](../proposals/P-004-wpf-lifetime-profile.md) +are **profile rule mnemonics, not diagnostic codes** — its own table maps each one +to a core verdict: + +```text +WPF001 source.Event += h, no matching -= -> OWN001 [subscription token] +WPF002 Timer Tick/Elapsed, no Stop()/ -= -> OWN001 [timer] +WPF003 owned IDisposable field, never Disposed -> OWN001 [disposable field] +WPF004 ignored Subscribe() IDisposable token -> OWN001 [subscription token] +WPF005 strong capture by a longer-lived source -> OWN014 (region promotion) +``` + +So the core stays neutral; "WPF" is *recognition + lifetime context*, not the +error itself. The critique is correct, and we mostly already shipped it. + +## The naming debt the critique correctly smells + +The capability is general. The *same* `source.Event += h` without `-=` leaks in +**WinForms, Avalonia, MAUI, Unity, an ASP.NET singleton service, a console app +with an event bus** — anywhere a publisher outlives its subscriber. The Rx flavour +is `observable.Subscribe(x => …)` with the `IDisposable` token dropped. None of +that is WPF. + +Yet P-001's subtitle ("the WPF leak spike"), P-004's `WPFxxx` rule names, and the +README's "WPF lifetime-утечки" heading make a .NET-wide analysis *read* as +WPF-only. That is naming a fire "kitchen thermodynamics." The fix is framing, not +the core: **"subscription / lifetime analysis with a WPF profile,"** not "WPF leak +analyzer." + +## Proposed code families (direction, not a now-rename) + +If/when the `[resource]` tag stops carrying enough and we want first-class codes: + +```text +OWN core ownership / borrow / release / lifetime promotion +SUB subscriptions / events / observer tokens +TMR timers +DI dependency-injection lifetimes +POOL ArrayPool / MemoryPool / Span storage +EFF effects / resources +WPF *truly* XAML-model retention (only the things below) +``` + +Today's profile rules would re-home cleanly: `WPF001 -> SUB001`, +`WPF002 -> TMR001`, `WPF004 -> SUB004` (ignored token), `WPF003 -> IDisposable +field (P-005)`, `WPF005 -> OWN014` (already neutral — it is lifetime promotion). + +`WPFxxx` is genuinely *earned* only where the diagnostic needs the XAML object +model and cannot be a generic subscription/timer: + +```text +DataContext retained after a View unloads +Binding / CollectionView keeping its source alive +ResourceDictionary / merged-dictionary retention +DependencyProperty metadata callback capturing an instance +Storyboard / animation / EventTrigger holding its target +WeakEventManager should have been used for a long-lived source +``` + +## OwnIR stays domain-neutral; the profile only adds heuristics + +```text +core facts (neutral — what the extractor emits): + acquire(subscription, loc) release(subscription, loc) + owner(this, subscription) handler(subscription, h) captures(h, this) + source(subscription, publisher) + lifetime(this, ViewModel) lifetime(publisher, App) # when known + +wpf profile (heuristics layered on top — never inside the core): + class *ViewModel / : Window|UserControl|Page -> lifetime ViewModel / UI + Application.Current / singleton service -> lifetime App + Dispose / OnClosed / Unloaded -> cleanup regions + DispatcherTimer / WeakEventManager / … -> WPF-specific sources +``` + +The same facts can later carry `profile = winforms | avalonia | maui | aspnet` +without touching the checker. One core; many profiles. The seam already exists. + +## Severity follows what we can *prove* (the `OWN001` decision) + +We keep the code `OWN001`. The open behavioural question was: do we always shout +`error` at a lambda handler? **No** — that would be "the analyzer named *молодец, +нашёл C#*," which users mute faster than WPF leaks its first ViewModel. Without +lifetime evidence, tier `OWN001`'s *severity* by what the source provably is: + +```text +static event + capturing handler -> error process-lifetime: a provable leak +field / ctor-param / property -> warning lifetime unknown — may leak if the + source outlives `this`; an inline + lambda has no handle to `-=` at all +local publisher -> drop dies with the scope (today a false + positive: `local` is not in the + self-owned set, so it leaks-by-mistake) +this / constructed field -> exempt self-owned cycle, GC-collectable (P-004) +``` + +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, +the hedge becomes the confident verdict the core *already* produces — **`OWN014`** +(`App > ViewModel ⇒ promotion ⇒ leak`, see the README region example). "Warning +without a profile, error with one" is not a cop-out: it is the honest contract, and +the lifetime/region analysis is the upgrade path. Same mechanism, viewed twice. + +This also keeps us consistent with our own line — honest-skip (`--stats`, `OWN050`) +and the oracle precision stance ([oracle.md](oracle.md): we ding Infer# for +ownership-transfer false positives). Erroring on every lambda would be us doing the +exact thing we flag the neighbours for. + +## What to actually change — and what not to + +- **Now (cheap, no code):** reframe the docs — P-001 subtitle, P-004 title and the + table's column header, the README heading, `ROADMAP` — from "WPF leak analyzer / + `WPFxxx` codes" to "subscription / lifetime analysis + a WPF *profile* (rule IDs)." + Keep emitting `OWN001`/`OWN014`; the `[resource: kind]` tag already names the + sub-domain. +- **Later (only on demand):** mint `SUB`/`TMR` as first-class diagnostic codes *if* + the `[resource]` tag ever can't carry a distinction we need. Renaming shipped + behaviour costs goldens, `corpus/wpf/`, `tests/test_wpf.py` — and the codes users + see are already neutral, so there is no rush. +- **Don't:** put any WPF knowledge into the core, or split `event += without -=` out + of `OWN001`. It *is* `OWN001`. WPF is a lens, not the lesson. + +**Verdict:** the core is already domain-neutral and correct. The work is (1) stop +the *docs* over-claiming WPF, and (2) make `OWN001`'s severity honest about source +lifetime, with the WPF/region profile as the evidence that escalates a hedge to a +verdict. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 269bcaa1..9619d269 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -40,11 +40,17 @@ // CFG) so the core checks them path-sensitively (OWN001/002/003). Supersedes the // flat D1 local-disposable detector when on. Default off keeps the shipped surface. bool flowLocals = false; +// --stats (coverage): print a one-line flow-locals coverage summary to stderr +// (of the methods that have a disposable local worth checking, how many were +// flow-analysed vs honestly skipped for an unmodelled construct) and stamp the +// same counts into the facts JSON. Turns "0 findings" into "clean vs didn't-reach". +bool reportStats = false; for (int i = 0; i < args.Length; i++) { if (args[i] == "-o" && i + 1 < args.Length) outPath = args[++i]; else if (args[i] == "--no-event-leaks") emitEvents = false; else if (args[i] == "--flow-locals") flowLocals = true; + else if (args[i] == "--stats") reportStats = true; else rawInputs.Add(args[i]); } @@ -54,6 +60,16 @@ return 2; } +// --stats reports flow-locals coverage; the counters only move inside the +// --flow-locals pass. Without it they would all be zero and the summary would +// read "0/0 methods flow-analysed" — the exact ambiguous zero --stats exists to +// kill (e.g. `own-check.sh --legacy --stats`). Refuse the contradictory combo. +if (reportStats && !flowLocals) +{ + Console.Error.WriteLine("ownsharp-extract: --stats requires --flow-locals"); + return 2; +} + // A path segment we never scan: build output, VCS, and vendored trees. static bool IsSkippedDir(string seg) => seg is "bin" or "obj" or ".git" or ".vs" or "node_modules" or "packages"; @@ -150,6 +166,52 @@ static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) => IsHandler(right) && model.GetSymbolInfo(right).Symbol is IMethodSymbol { IsStatic: true }; +// 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 +// a provable leak -> "static". A local that is CONSTRUCTED right here (`var p = +// new Publisher(); p.X += h`) dies with the scope -> "local" (the caller drops it; +// not a heap leak). But a local that merely ALIASES something else (`var src = +// _bus; src.X += h`) has unknown provenance — it may hold a long-lived injected +// source — so it is NOT dropped. Everything else (an instance field / property / +// injected parameter, or such an aliasing local) has UNKNOWN lifetime -> +// "injected": it MIGHT outlive `this`, but we cannot prove it without ownership +// modelling, so the core renders it a warning (not a hard error) until that lands. +static string SubscriptionSourceKind(ExpressionSyntax left, IEventSymbol ev, + SemanticModel model) +{ + if (ev.IsStatic) + return "static"; + if (left is MemberAccessExpressionSyntax m) + { + var recv = model.GetSymbolInfo(m.Expression).Symbol; + if (recv is ILocalSymbol local) + { + // Method-bounded (droppable) ONLY when the local is the publisher this + // scope constructs (`var p = new Publisher()`), which dies with it. A + // local initialised from anything else (a field, a parameter, a call) + // may alias a long-lived source, so we cannot prove it bounded — fall + // through to "injected" and warn rather than silently drop a real leak. + var constructedHere = local.DeclaringSyntaxReferences + .Select(r => r.GetSyntax()) + .OfType() + .Any(v => v.Initializer?.Value is ObjectCreationExpressionSyntax + or ImplicitObjectCreationExpressionSyntax); + if (constructedHere) + return "local"; + } + if (recv is IFieldSymbol { IsStatic: true } or IPropertySymbol { IsStatic: true }) + return "static"; + } + return "injected"; +} + +// A lambda / anonymous-method handler stores no named delegate, so the +// subscription can NEVER be undone with `-=` (you would have had to cache the +// delegate in a field). A particularly sharp leak shape worth calling out. +static bool IsLambdaHandler(ExpressionSyntax right) => + right is AnonymousFunctionExpressionSyntax; + // --- P-016 B0b/B2: flow lowering for local IDisposables (experimental) --- // A type that implements System.IDisposable (semantic) — the flow lowering tracks @@ -362,6 +424,12 @@ t is "IDisposable" or "IAsyncDisposable" or "CancellationTokenSource" "own", parsed.Select(p => p.tree), references, new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); +// --flow-locals coverage counters (--stats). A method "with a local" here is one +// that has a non-escaping `new` IDisposable worth tracking; of those we either +// flow-analyse it or honestly skip it (an unmodelled for/do/try/switch/async made +// LowerFlowBody bail). methods_with_local == analysed + skipped. +int statMethodsWithLocal = 0, statMethodsAnalysed = 0, statMethodsSkipped = 0; + foreach (var (file, tree) in parsed) { var model = compilation.GetSemanticModel(tree); @@ -425,6 +493,14 @@ or ImplicitObjectCreationExpressionSyntax if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, constructed) || IsStaticHandler(a.Right, model))) continue; + // P-004 tiering: a local-variable source is method-bounded — it + // cannot outlive `this`, so it is not a heap leak; drop it (the same + // spirit as the self-owned drop above). "static"/"injected" ride + // along as a `source` hint so the core can grade the severity. + var source = isTimer ? "static" + : SubscriptionSourceKind(a.Left, ev, model); + if (source == "local") + continue; var released = unsub.Contains($"{a.Left}|{a.Right}") || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); subs.Add(new @@ -434,6 +510,8 @@ or ImplicitObjectCreationExpressionSyntax line = LineOf(a.Left), released, resource = isTimer ? "timer" : "subscription", + source, + lambda = !isTimer && IsLambdaHandler(a.Right), }); } else if (leftSymbol is null && IsHandler(a.Right)) @@ -630,9 +708,14 @@ or ImplicitObjectCreationExpressionSyntax } init tracked.ExceptWith(escapedLocals); if (tracked.Count == 0) continue; + statMethodsWithLocal++; var fbody = LowerFlowBody(mbody, tracked); if (fbody is null || fbody.Count == 0) + { + statMethodsSkipped++; // unmodelled construct -> honestly skipped continue; + } + statMethodsAnalysed++; flowFunctions.Add(new { name = $"{cls.Identifier.Text}.{MethodName(method)}", @@ -648,9 +731,27 @@ or ImplicitObjectCreationExpressionSyntax } init // ownir_version stamps the fact-schema vocabulary; the Python core rejects a // mismatch loudly (ownlang/ownir.py OWNIR_VERSION) rather than mis-reading facts. -var facts = new { ownir_version = 0, module = "Extracted", components, functions = flowFunctions }; +// `stats` is additive coverage metadata — the core's load() ignores unknown keys. +var facts = new +{ + ownir_version = 0, + module = "Extracted", + components, + functions = flowFunctions, + stats = new + { + methods_with_local = statMethodsWithLocal, + methods_flow_analysed = statMethodsAnalysed, + methods_skipped_unmodelled = statMethodsSkipped, + }, +}; var json = JsonSerializer.Serialize(facts, new JsonSerializerOptions { WriteIndented = true }); +if (reportStats) + Console.Error.WriteLine( + $"coverage: {statMethodsAnalysed}/{statMethodsWithLocal} methods with a " + + $"disposable local flow-analysed; {statMethodsSkipped} skipped (unmodelled construct)"); + if (outPath is null) Console.WriteLine(json); else File.WriteAllText(outPath, json); return 0; diff --git a/frontend/roslyn/samples/AliasedSourceViewModel.cs b/frontend/roslyn/samples/AliasedSourceViewModel.cs new file mode 100644 index 00000000..6031f887 --- /dev/null +++ b/frontend/roslyn/samples/AliasedSourceViewModel.cs @@ -0,0 +1,31 @@ +using System; + +// SUBSCRIPTION SOURCE PROVENANCE (P-004): two locals, two verdicts. +// +// (1) `src` ALIASES the injected `_bus` (a ctor dependency of unknown lifetime). +// A local is not automatically method-bounded — this one may hold a +// long-lived source, so the extractor must NOT drop it. It is classified +// `injected`, and the core reports OWN001 at WARNING, like CustomerViewModel. +// +// (2) `owned` is a publisher this scope CONSTRUCTS (`new Calc()`); it dies with +// the constructor, so the subscription cannot outlive `this` — a genuinely +// method-local source. The extractor classifies it `local` and DROPS it (no +// finding), the same spirit as the self-owned-field exemption. +public sealed class AliasedSourceViewModel +{ + private readonly IEventBus _bus; + + public AliasedSourceViewModel(IEventBus bus) + { + _bus = bus; + + var src = _bus; // aliases an injected field + src.CustomerChanged += OnAliased; // unknown lifetime -> WARNING leak + + var owned = new Calc(); // constructed here -> method-bounded + owned.Changed += OnLocal; // dies with the ctor -> dropped + } + + private void OnAliased(object? sender, EventArgs e) { } + private void OnLocal(object? sender, EventArgs e) { } +} diff --git a/frontend/roslyn/samples/CustomerViewModel.cs b/frontend/roslyn/samples/CustomerViewModel.cs index 2eaeb737..21aa5461 100644 --- a/frontend/roslyn/samples/CustomerViewModel.cs +++ b/frontend/roslyn/samples/CustomerViewModel.cs @@ -1,8 +1,13 @@ using System; -// LEAK: subscribes to a (longer-lived) event bus in its constructor and never -// unsubscribes. The extractor emits a subscription with released=false, and the -// core reports OWN001 at the `+=` line. +// SUBSCRIPTION LEAK (injected source): subscribes to an event bus passed into the +// constructor and never unsubscribes. The extractor emits the subscription with +// released=false and source=injected. Because `bus` is an INJECTED dependency we +// cannot prove whether it outlives this view model (it might be a singleton, or +// might not), so for now the core reports OWN001 at WARNING level — an honest +// "possible leak", not a hard error. Once Own.NET models lifetimes/ownership well +// enough to prove the source is long-lived, this escalates to an error (a static +// event, or a proven app-lifetime source, is already a hard error today). public sealed class CustomerViewModel { public CustomerViewModel(IEventBus bus) diff --git a/frontend/roslyn/samples/LambdaHandlerViewModel.cs b/frontend/roslyn/samples/LambdaHandlerViewModel.cs new file mode 100644 index 00000000..b1526d68 --- /dev/null +++ b/frontend/roslyn/samples/LambdaHandlerViewModel.cs @@ -0,0 +1,23 @@ +using System; + +// SUBSCRIPTION LEAK (injected source, lambda handler): subscribes a LAMBDA to an +// injected event bus in the constructor and never unsubscribes. Two things stack: +// (1) the source `bus` is INJECTED, so its lifetime is unknown — like +// CustomerViewModel.cs, the core reports OWN001 at WARNING level (a "possible +// leak") until lifetime/ownership modelling can prove it; (2) a lambda literal has +// no stored delegate, so it can NEVER be removed with `-=` even on purpose (you'd +// have to cache the delegate in a field just to detach it) — the finding spells +// that out. The extractor binds the LHS to the event symbol (a lambda RHS rather +// than a method group doesn't matter) and emits released=false, source=injected, +// lambda=true. Contrast CustomerViewModel.cs (method-group handler, same source). +public sealed class LambdaHandlerViewModel +{ + private int _count; + + public LambdaHandlerViewModel(IEventBus bus) + { + // captures `this` (via _count) -> not a static handler -> not exempt; + // no matching `-=` is even possible -> leak. + bus.CustomerChanged += (s, e) => _count++; + } +} diff --git a/ownlang/__main__.py b/ownlang/__main__.py index 06ac8e33..3880001e 100644 --- a/ownlang/__main__.py +++ b/ownlang/__main__.py @@ -218,7 +218,16 @@ def cmd_ownir(path: str, fmt: str = "human", severity: str = "error", notes = [f for f in findings if f.advisory] shown = leaks if verbosity == "quiet" else findings for f in shown: - print(render_finding(f, fmt, "warning" if f.advisory else severity)) + # Severity is the weaker of the host's --severity and the finding's own + # intrinsic level: an advisory note (OWN050) is always a warning; a global + # `--severity warning` downgrades everything; and a finding the extractor + # could not prove a leak (an injected-source subscription, f.severity == + # "warning") shows as a warning even at the default error level (P-004). + if f.advisory or severity == "warning" or f.severity == "warning": + fsev = "warning" + else: + fsev = severity + print(render_finding(f, fmt, fsev)) if not shown: print(f"{path}: ok — no subscription leaks found", file=summary_to) n = len(leaks) diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 755bffc2..4bd4498a 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -168,6 +168,12 @@ class Finding: # an advisory note (e.g. OWN050 "leakage analysis skipped") rather than a leak # verdict: rendered as a warning and excluded from the exit code. advisory: bool = False + # P-004 tiering: the intrinsic level when the source's lifetime cannot be + # proven — "warning" for a subscription whose event SOURCE is an injected + # dependency of unknown lifetime, None for a provable leak (shown at the host's + # --severity, default error). Still a leak verdict (counts in the exit code); + # only the displayed level differs. + severity: str | None = None def render(self, severity: str = "error") -> str: return (f"{self.file}:{self.line}: {severity}: [{self.code}] " @@ -557,6 +563,10 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: kind="disposable")) continue _, kind = _RESOURCES.get(rkind, _RESOURCES["subscription"]) + # P-004 tiering: only the plain `event += handler` leak (the else branch + # below) grades its severity from the source's proven lifetime; every other + # resource is a provable leak and stays at the host's --severity (error). + fsev: str | None = None if rkind == "timer": message = (f"timer '{event}' (handler '{handler}') is started but " f"never stopped or detached — the running timer keeps " @@ -579,13 +589,29 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: message = (f"pooled buffer '{event}' is rented but never returned " f"to the pool (leak)") else: - message = (f"event '{event}' is subscribed (handler '{handler}') " - f"but never unsubscribed — the source keeps " - f"'{component}' alive (leak)") + # The source's lifetime decides the severity (the extractor stamps + # `source`): a static event is process-lived -> a provable leak (error); + # an injected dependency (ctor param / field / property) has UNKNOWN + # lifetime -> a warning ("may outlive this", honest until ownership + # modelling can prove it). A lambda handler has no `-=` handle, so it + # could never be detached even on purpose — worth spelling out. + lam = (" — and being an inline lambda it has no '-=' handle, so it " + "could never be detached") if sub.get("lambda") else "" + if sub.get("source") == "injected": + fsev = "warning" + message = (f"event '{event}' is subscribed (handler '{handler}') " + f"but never unsubscribed; its source is an injected " + f"dependency whose lifetime is unknown, so it may " + f"outlive and keep '{component}' alive (possible " + f"leak{lam})") + else: + message = (f"event '{event}' is subscribed (handler '{handler}') " + f"but never unsubscribed — the source keeps " + f"'{component}' alive (leak{lam})") findings.append(Finding( file=sub["file"], line=int(sub.get("line", 0)), code=d.code, component=component, event=event, handler=handler, - message=message, kind=kind)) + message=message, kind=kind, severity=fsev)) # DI001 (captive dependency): a separate core analysis over the registration # graph, not the acquire/release model — the bridge just routes the facts to diff --git a/scripts/mine.sh b/scripts/mine.sh index 5c4d782a..ff740b46 100755 --- a/scripts/mine.sh +++ b/scripts/mine.sh @@ -74,15 +74,19 @@ echo "mine: scanning $scan (commit $commit)" >&2 # stderr; keep them apart. Without --fail-on-finding it exits 0 even with leaks; # rc>=2 is a hard error (bad facts) — note it but still report what we captured. set +e -"$root/scripts/own-check.sh" --root "$root" --format "$format" -- "$scan" \ +"$root/scripts/own-check.sh" --root "$root" --format "$format" --stats -- "$scan" \ >"$outdir/findings.txt" 2>"$outdir/extract.log" rc=$? set -e +# --stats writes a one-line flow-locals coverage summary to the extractor's stderr +# (captured in extract.log); surface it in the report so a clean run reads as +# "analysed N, skipped M" rather than an ambiguous zero. +cov="$(grep -m1 '^coverage:' "$outdir/extract.log" 2>/dev/null || true)" [[ "$rc" -ge 2 ]] && echo "mine: own-check hard error (rc=$rc); see $outdir/extract.log" >&2 python "$root/scripts/mine_report.py" "$outdir/findings.txt" \ --repo "$target" --commit "$commit" --json "$outdir/report.json" \ - >"$outdir/report.md" + --coverage "$cov" >"$outdir/report.md" [[ "$keep_src" -eq 1 ]] || rm -rf "$src" diff --git a/scripts/mine_report.py b/scripts/mine_report.py index b6ca30f2..5ea72f66 100644 --- a/scripts/mine_report.py +++ b/scripts/mine_report.py @@ -97,7 +97,7 @@ def aggregate(findings: list[dict[str, Any]]) -> dict[str, Any]: def render_md(findings: list[dict[str, Any]], unparsed: int, repo: str, - commit: str, max_list: int = 60) -> str: + commit: str, coverage: str = "", max_list: int = 60) -> str: """Render the Markdown report (the human-facing miner output).""" agg = aggregate(findings) errors = [f for f in findings if f["severity"] == "error"] @@ -111,14 +111,17 @@ def render_md(findings: list[dict[str, Any]], unparsed: int, repo: str, f"({agg['errors']} error / {agg['advisories']} advisory) " f"across {agg['files_with_findings']} file(s)" + (f"; {unparsed} unparsed line(s)" if unparsed else ""), - "", ] + if coverage: + out.append(f"- {coverage}") + out.append("") if agg["total"] == 0: - out += ["**Clean** — no findings. (A clean run on real code is a precision " - "signal; pair it with the extractor's `--stats` coverage once that " - "lands to know how much was actually analysed vs honestly skipped.)", - ""] + tail = (f" The extractor reports — {coverage}." if coverage else + " Pair it with the extractor's `--stats` (analysed vs " + "honestly-skipped methods) to know how much was actually looked at.") + out += ["**Clean** — no findings. A clean run on real code is a precision " + "signal." + tail, ""] return "\n".join(out) out += ["## By code", "", "| code | n | what |", "|---|---:|---|"] @@ -157,6 +160,8 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument("--commit", default="", help="commit SHA (for the report header)") ap.add_argument("--json", dest="json_out", default="", help="also write the raw aggregates as JSON to this path") + ap.add_argument("--coverage", default="", + help="extractor --stats coverage line to surface in the report") ap.add_argument("--selftest", action="store_true", help="run built-in parser/aggregator checks and exit") args = ap.parse_args(argv) @@ -172,7 +177,7 @@ def main(argv: list[str] | None = None) -> int: json.dump({"repo": args.repo, "commit": args.commit, "unparsed": unparsed, "findings": findings, **aggregate(findings)}, f, indent=2) - print(render_md(findings, unparsed, args.repo, args.commit)) + print(render_md(findings, unparsed, args.repo, args.commit, args.coverage)) return 0 @@ -208,9 +213,12 @@ def _selftest() -> int: # a clean run renders without crashing and says so. if "Clean" not in render_md([], 0, "o/r", "abc123"): fails.append("clean render missing 'Clean'") + # the --stats coverage line is surfaced when supplied (header + clean note). + if "42 methods" not in render_md([], 0, "o/r", "abc123", "coverage: 1/42 methods"): + fails.append("coverage line not rendered") for f in fails: print(f"MINE SELFTEST FAIL: {f}") - print(f"mine_report selftest: {7 - len(fails)}/7 checks passed") + print(f"mine_report selftest: {8 - len(fails)}/8 checks passed") return 1 if fails else 0 diff --git a/scripts/own-check.sh b/scripts/own-check.sh index e51476ed..35c5afa8 100755 --- a/scripts/own-check.sh +++ b/scripts/own-check.sh @@ -12,7 +12,7 @@ # # Usage: # scripts/own-check.sh [--format human|github|msbuild] [--severity error|warning] -# [--fail-on-finding] [--legacy] [--root ] +# [--fail-on-finding] [--legacy] [--stats] [--root ] # [--] [more ...] # # Defaults: --format human, --severity error, scans ".", does not fail the shell @@ -36,6 +36,7 @@ format="human" severity="error" fail_on_finding=0 legacy=0 +stats=0 paths=() while [[ $# -gt 0 ]]; do @@ -51,6 +52,7 @@ while [[ $# -gt 0 ]]; do severity="$2"; shift 2 ;; --fail-on-finding) fail_on_finding=1; shift ;; --legacy) legacy=1; shift ;; + --stats) stats=1; shift ;; --) shift; while [[ $# -gt 0 ]]; do paths+=("$1"); shift; done ;; -h|--help) sed -n '2,30p' "$0"; exit 0 ;; *) paths+=("$1"); shift ;; @@ -75,6 +77,7 @@ trap 'rm -f "$facts"' EXIT # --legacy keeps the flat name-based detector. extractor_args=("${paths[@]}" -o "$facts") [[ "$legacy" -eq 0 ]] && extractor_args+=(--flow-locals) +[[ "$stats" -eq 1 ]] && extractor_args+=(--stats) dotnet run --project "$extractor" -- "${extractor_args[@]}" 1>&2 # Stage 2: the one checker produces the verdict at the C# location. diff --git a/tests/test_ownir.py b/tests/test_ownir.py index 5510a666..b12a5c50 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -119,6 +119,45 @@ def run() -> int: if check_facts({"module": "Empty", "components": []}): fails.append("empty facts produced findings") + # --- P-004 severity tiering: the subscription fact's `source` grades severity. + def _one(source: str, lambda_: bool = False) -> Finding: + """One unreleased subscription with the given source kind -> its Finding.""" + return check_facts({"module": "M", "components": [ + {"name": "Vm", "file": "Vm.cs", "subscriptions": [ + {"event": "bus.X", "handler": "h", "line": 5, "released": False, + "resource": "subscription", "source": source, + "lambda": lambda_}]}]})[0] + + # an injected source (unknown lifetime) is a WARNING-tier leak — not a hard + # error — and says so; it is still a leak verdict (not advisory) so it keeps + # the non-zero exit code. + checks += 1 + inj = _one("injected") + if inj.severity != "warning": + fails.append(f"injected source should be warning-tier, got {inj.severity!r}") + if inj.advisory: + fails.append("injected-source leak must not be advisory (still a leak)") + if "injected dependency" not in inj.message: + fails.append(f"injected message missing wording: {inj.message!r}") + + # a static event source is provably process-lived -> a hard ERROR: its severity + # field is None, so it renders at the host's --severity (default error). (The + # field is the core's verdict; the cmd_ownir render policy is locked in CI.) + checks += 1 + stat = _one("static") + if stat.severity is not None: + fails.append(f"static source should be error-tier (None), got {stat.severity!r}") + if "injected dependency" in stat.message: + fails.append(f"static message must not claim an injected source: {stat.message!r}") + + # a lambda handler additionally calls out that it has no `-=` handle to detach. + checks += 1 + lam = _one("injected", lambda_=True) + if lam.severity != "warning": + fails.append(f"injected lambda should be warning-tier, got {lam.severity!r}") + if "inline lambda" not in lam.message or "-=" not in lam.message: + fails.append(f"lambda handler should note the missing -= handle: {lam.message!r}") + # the fixture carries the current schema version (the contract is stamped). checks += 1 if facts.get("ownir_version") != OWNIR_VERSION: