diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f63a411..0e6f423 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -212,6 +212,7 @@ jobs: frontend/roslyn/samples/UsingFieldAcquisitionSample.cs \ frontend/roslyn/samples/TemplatePartLocalCaptureSample.cs \ frontend/roslyn/samples/EmptyDisposeSample.cs \ + frontend/roslyn/samples/AppScopedSourceSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -849,7 +850,35 @@ jobs: # Codex P2: an empty SYNC Dispose but a real DisposeAsync (a `*Reader` here) must still leak. echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ar'.*'AsyncReader'" \ || { echo "FAIL: a type with a real DisposeAsync (empty sync Dispose) must still leak (#225)"; exit 1; } - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) at the C# location" + # issue #228 — an Application-derived subscriber on a CURATED app-scoped + # resolver result (PaletteHelper.GetThemeManager) with a method-group handler + # of the App class itself must be SILENT: the source is process-lived (bound + # to the app's own state), so nothing is promoted. Both receiver forms — an + # `is`-pattern local (App) and the direct invocation (DirectApp). + if echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+:.*('App'|'DirectApp')"; then + echo "FAIL: a curated app-scoped subscription inside the Application was wrongly reported (#228)"; exit 1 + fi + # ...and the exemption must NOT over-widen — three controls STAY flagged: + # (1) the SAME curated shape from a NON-Application class (the subscriber + # gate stays clsIsApp, byte-for-byte); + echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'NotAnApp'" \ + || { echo "FAIL: expected OWN001 on the non-Application subscriber (curated source alone must not exempt)"; exit 1; } + # (2) App + curated source, but a LAMBDA handler capturing a local — the + # exact hole that sank the rejected clsIsStatic broadening; + echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'LambdaApp'" \ + || { echo "FAIL: expected OWN001 on the lambda-handler subscription inside the App (capture hole)"; exit 1; } + # (3) App + method-group handler, but a NON-curated resolver; + echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'CuratedOnlyApp'" \ + || { echo "FAIL: expected OWN001 on the non-curated resolver source inside the App"; exit 1; } + # (4, Codex P2) the method group is qualified with ANOTHER instance of the + # same App type — right ContainingType, wrong delegate target; + echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'TwinApp'" \ + || { echo "FAIL: expected OWN001 when the handler target is another instance, not this"; exit 1; } + # (5, Codex P2) the local is REASSIGNED to an injected publisher after the + # curated initializer — the stale declaration binding must not exempt. + echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'ReassignApp'" \ + || { echo "FAIL: expected OWN001 when the resolver-bound local is reassigned before the +="; exit 1; } + echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | # Path-sensitive flow analysis of local IDisposables — bugs the flat D1 diff --git a/docs/notes/oracle-known-fps.md b/docs/notes/oracle-known-fps.md index dfaa27d..cf034b5 100644 --- a/docs/notes/oracle-known-fps.md +++ b/docs/notes/oracle-known-fps.md @@ -308,6 +308,27 @@ which we have no reliable signal for. So those two findings stay in extractor rule. An in-code `ANTI-PATTERN` comment at the exemption site warns against re-adding `|| clsIsStatic`. +### What DID ship next to it (issue #228) — and why it is not the same thing + +The #201 sweep's MaterialDesign `App.xaml.cs` FP (an `App : Application` subscribing +to `PaletteHelper().GetThemeManager()`, an app-scoped instance that is process-lived +without being a literal `static` member) is cleared by a narrowing that inverts every +property that made `clsIsStatic` unsound: + +- the **subscriber** gate is unchanged — byte-for-byte the existing `clsIsApp` + ("the subscriber IS the process-lived object", the property `clsIsStatic` lacked); +- the **source** check loosens only to a **curated allowlist** of resolver methods + whose result is verified app-scoped (`PaletteHelper.GetThemeManager`, verified at + `PaletteHelper.cs:22-26`) — grown one confirmed sibling at a time, like the #223 + weak-event list, never inferred; +- the **handler** must be a method group of the App class itself — a lambda is + rejected outright, so the "captures a shorter-lived local and pins it" hole that + sank `clsIsStatic` (and would NOT have been closed by it anyway) cannot pass. + +Pinned by `AppScopedSourceSample.cs`: two silent positives (pattern-local and direct +receiver) and three flagged controls (non-App subscriber, lambda handler, non-curated +resolver), each edge of the gate. + ## How the baseline stays honest - **Matched by name, not line** — `(repo, file-basename, OWN code, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 0361adb..56ab828 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -895,6 +895,120 @@ static bool IsProcessLivedApplication(TypeDeclarationSyntax cls) && cls.Modifiers.Any(m => m.IsKind(SyntaxKind.PartialKeyword)); } +// P-004 / issue #228: the CURATED resolver allowlist whose call RESULT is app-scoped +// (bound to Application-owned state, hence process-lived) without being a literal +// `static` member. One entry per CONFIRMED real-world sibling — same policy as the +// #223 weak-event allowlist, never an inference. Matched by (containing-type simple +// name, method name), mirroring the [OwnIgnore] simple-name precedent: the declaring +// package usually does not resolve on the Linux runner. +// - MaterialDesign PaletteHelper.GetThemeManager(): returns the IThemeManager bound +// to the app's own merged ResourceDictionary (PaletteHelper.cs:22-26, verified in +// the issue #201 sweep — MahMaterialDragablzMashUp App.xaml.cs FP). +static bool IsAppScopedResolver(IMethodSymbol sym) => + (sym.ContainingType?.Name, sym.Name) is ("PaletteHelper", "GetThemeManager"); + +// #228: does this `+=` receiver resolve (directly, or through a local bound by a +// `var x = ...` initializer or an `is`-pattern designation) to the RESULT of a curated +// app-scoped resolver call? Any unprovable step returns false — the subscription then +// keeps today's honest "injected" warning, never the other way around. +static bool IsCuratedAppScopedSource(ExpressionSyntax left, SemanticModel model) + => left is MemberAccessExpressionSyntax m + && ResolvesToAppScopedCall(m.Expression, model, depth: 0); + +static bool ResolvesToAppScopedCall(ExpressionSyntax expr, SemanticModel model, int depth) +{ + if (depth > 3) + return false; + expr = StripCasts(expr); + if (expr is InvocationExpressionSyntax inv) + { + var info = model.GetSymbolInfo(inv); + var sym = info.Symbol as IMethodSymbol + ?? info.CandidateSymbols.OfType().FirstOrDefault(); + return sym is not null && IsAppScopedResolver(sym); + } + if (model.GetSymbolInfo(expr).Symbol is not ILocalSymbol local) + return false; + // The binding is only trustworthy if the local still HOLDS it at the `+=`: + // a reassignment anywhere in the enclosing member (or a ref/out escape that + // could rebind it) makes the initializer stale — no exemption (Codex P2). + if (!IsNeverReassigned(local, model)) + return false; + foreach (var r in local.DeclaringSyntaxReferences) + switch (r.GetSyntax()) + { + // var tm = helper.GetThemeManager(); + case VariableDeclaratorSyntax { Initializer.Value: { } init } + when ResolvesToAppScopedCall(init, model, depth + 1): + return true; + // helper.GetThemeManager() is { } tm / is ThemeManagerLike tm + case SingleVariableDesignationSyntax des + when des.Ancestors().OfType().FirstOrDefault() + is { Expression: { } scrutinee } + && ResolvesToAppScopedCall(scrutinee, model, depth + 1): + return true; + } + return false; +} + +// #228 (Codex P2): the declaration-site binding proves the local's value at the +// `+=` only if nothing rebinds it. Conservative whole-member scan: ANY assignment +// whose LHS is this local, or ANY `ref`/`out` argument passing it, kills the proof +// (even one after the subscription — cheaper than flow order and precision-safe: +// the worst case is keeping today's honest warning). +static bool IsNeverReassigned(ILocalSymbol local, SemanticModel model) +{ + foreach (var r in local.DeclaringSyntaxReferences) + { + var scope = r.GetSyntax().Ancestors().FirstOrDefault(n => + n is MethodDeclarationSyntax or ConstructorDeclarationSyntax + or AccessorDeclarationSyntax or LocalFunctionStatementSyntax + or AnonymousFunctionExpressionSyntax); + if (scope is null) + return false; + foreach (var asg in scope.DescendantNodes().OfType()) + if (SymbolEqualityComparer.Default.Equals( + model.GetSymbolInfo(asg.Left).Symbol, local)) + return false; + foreach (var arg in scope.DescendantNodes().OfType()) + if (!arg.RefOrOutKeyword.IsKind(SyntaxKind.None) + && SymbolEqualityComparer.Default.Equals( + model.GetSymbolInfo(arg.Expression).Symbol, local)) + return false; + } + return true; +} + +// #228: the handler must be a METHOD GROUP declared on the subscribing class itself — +// its delegate target is then the App singleton (already process-lived, nothing to +// promote). A lambda/anonymous method is rejected outright: it may capture an +// enclosing LOCAL, and pinning that local to the app's lifetime is exactly the leak +// the rejected `clsIsStatic` broadening would have swallowed (oracle-known-fps.md, +// "Rejected approaches"). +static bool IsOwnMethodGroupHandler(ExpressionSyntax right, SemanticModel model, + INamedTypeSymbol? cls) +{ + if (cls is null) + return false; + // The delegate TARGET must be `this` — a bare identifier (implicit this) or an + // explicit `this.X`. A method group qualified with ANOTHER instance of the same + // App-derived type (`otherApp.OnTheme`) has the right ContainingType but pins + // that other instance, not the process-lived singleton (Codex P2). + var receiverIsThis = right switch + { + IdentifierNameSyntax => true, + MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax } => true, + _ => false, + }; + if (!receiverIsThis) + return false; + var info = model.GetSymbolInfo(right); + var sym = info.Symbol as IMethodSymbol + ?? info.CandidateSymbols.OfType().FirstOrDefault(); + return sym is not null + && SymbolEqualityComparer.Default.Equals(sym.ContainingType, cls); +} + // P-004 WPF MVVM ownership: a field read from `this.DataContext`, optionally through // an `as`/cast (`DataContext as VM`, `(VM)DataContext`). Combined with a view whose // own XAML CONSTRUCTS its DataContext, such a field is the view's owned view-model. @@ -4063,6 +4177,19 @@ or ImplicitObjectCreationExpressionSyntax // write-up: docs/notes/oracle-known-fps.md → "Rejected approaches". if (!isTimer && source == "static" && clsIsApp) continue; + // P-004 / issue #228: the same `clsIsApp` subscriber, but the source is an + // APP-SCOPED RESOLVER RESULT (curated: PaletteHelper.GetThemeManager) rather + // than a literal static member — genuinely process-lived, just reached through + // a call, so SubscriptionSourceKind honestly says "injected". This loosens + // ONLY the source check; the subscriber gate stays byte-for-byte `clsIsApp`, + // and the handler must be a METHOD GROUP of the App class itself — a lambda + // is rejected because it may capture an enclosing local, the exact hole that + // sank the `clsIsStatic` broadening above (see that comment + the "Rejected + // approaches" write-up; this narrowing is documented alongside it). + if (!isTimer && source == "injected" && clsIsApp + && IsOwnMethodGroupHandler(a.Right, model, clsSymbol) + && IsCuratedAppScopedSource(a.Left, model)) + 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 diff --git a/frontend/roslyn/samples/AppScopedSourceSample.cs b/frontend/roslyn/samples/AppScopedSourceSample.cs new file mode 100644 index 0000000..37f8adf --- /dev/null +++ b/frontend/roslyn/samples/AppScopedSourceSample.cs @@ -0,0 +1,142 @@ +// issue #228 — an Application-derived subscriber whose event source is an APP-SCOPED +// RESOLVER RESULT (curated: PaletteHelper.GetThemeManager), not a literal static member. +// The source is process-lived (bound to the app's own state), so the subscription +// promotes nothing — but SubscriptionSourceKind honestly tiers a call-result receiver +// "injected", which used to warn. Real-world shape: MaterialDesignInXamlToolkit +// MahMaterialDragablzMashUp App.xaml.cs:10,22 (found by the issue #201 oracle sweep). +// +// The exemption is deliberately NARROW (see the rejected `clsIsStatic` broadening in +// docs/notes/oracle-known-fps.md): subscriber must be the Application class itself, +// the resolver must be on the curated list, and the handler must be a METHOD GROUP of +// that class — three negative controls below pin each edge. +using System; + +namespace OwnSamples.AppScoped +{ + // Stand-in for System.Windows.Application: IsProcessLivedApplication matches the + // base NAME syntactically (WPF assemblies do not resolve on the Linux runner). + public class Application { } + + public class ThemeManagerLike + { + public event EventHandler? ThemeChanged; + public void Raise() => ThemeChanged?.Invoke(this, EventArgs.Empty); + } + + // Stand-in for MaterialDesignThemes.Wpf.PaletteHelper — the curated resolver, + // matched by (containing-type, method) = (PaletteHelper, GetThemeManager). + public class PaletteHelper + { + static readonly ThemeManagerLike Manager = new(); + public ThemeManagerLike? GetThemeManager() => Manager; + } + + // Same shape, NON-curated name -> never exempted. + public class OtherHelper + { + static readonly ThemeManagerLike Manager = new(); + public ThemeManagerLike? GetOtherService() => Manager; + } + + // POSITIVE (silent): App + curated resolver via `is`-pattern local + method-group + // handler of the App class — the exact MaterialDesign shape. + public class App : Application + { + public void OnStartup() + { + var helper = new PaletteHelper(); + if (helper.GetThemeManager() is { } themeManager) + themeManager.ThemeChanged += ThemeManager_ThemeChanged; // silent (#228) + } + + void ThemeManager_ThemeChanged(object? sender, EventArgs e) { } + } + + // POSITIVE (silent): the direct-invocation receiver form, no intermediate local. + public class DirectApp : Application + { + public void OnStartup() + { + new PaletteHelper().GetThemeManager()!.ThemeChanged += OnTheme; // silent (#228) + } + + void OnTheme(object? sender, EventArgs e) { } + } + + // CONTROL 1 (flagged): the SAME curated shape from a class that is NOT the + // Application — the subscriber gate must stay `clsIsApp`, byte-for-byte. + public class NotAnApp + { + public void Wire() + { + var helper = new PaletteHelper(); + if (helper.GetThemeManager() is { } themeManager) + themeManager.ThemeChanged += OnTheme; // OWN001 warning + } + + void OnTheme(object? sender, EventArgs e) { } + } + + // CONTROL 2 (flagged): App + curated source, but the handler is a LAMBDA capturing + // an enclosing local — pinning that local to app lifetime is the exact hole that + // sank the `clsIsStatic` broadening; a lambda must never pass the handler gate. + public class LambdaApp : Application + { + public void OnStartup() + { + var counter = new int[1]; + if (new PaletteHelper().GetThemeManager() is { } themeManager) + themeManager.ThemeChanged += (s, e) => counter[0]++; // OWN001 warning + } + } + + // CONTROL 3 (flagged): App + method-group handler, but a NON-curated resolver — + // membership is a curated allowlist, not "any call result inside App". + public class CuratedOnlyApp : Application + { + public void OnStartup() + { + if (new OtherHelper().GetOtherService() is { } service) + service.ThemeChanged += OnTheme; // OWN001 warning + } + + void OnTheme(object? sender, EventArgs e) { } + } + + // CONTROL 4 (flagged, Codex P2): the method group is qualified with ANOTHER + // instance of the same App-derived type — right ContainingType, wrong delegate + // TARGET: the process-lived source pins `_twin`, not `this`. + public class TwinApp : Application + { + readonly TwinApp? _twin; + + public TwinApp(TwinApp? twin) => _twin = twin; + + public void OnStartup() + { + if (new PaletteHelper().GetThemeManager() is { } themeManager) + themeManager.ThemeChanged += _twin!.OnTheme; // OWN001 warning + } + + public void OnTheme(object? sender, EventArgs e) { } + } + + // CONTROL 5 (flagged, Codex P2): the local STARTS as the curated resolver result + // but is REASSIGNED to an injected publisher before the `+=` — the declaration-site + // binding is stale, so the exemption must not trust it. + public class ReassignApp : Application + { + readonly ThemeManagerLike _injectedBus; + + public ReassignApp(ThemeManagerLike bus) => _injectedBus = bus; + + public void OnStartup() + { + var tm = new PaletteHelper().GetThemeManager()!; + tm = _injectedBus; + tm.ThemeChanged += OnTheme; // OWN001 warning + } + + void OnTheme(object? sender, EventArgs e) { } + } +}