diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d17609aa..9cf2c8c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -142,6 +142,7 @@ jobs: frontend/roslyn/samples/ResolvedDisposableSample.cs \ frontend/roslyn/samples/FieldReleaseSample.cs \ frontend/roslyn/samples/StaticClassEscapeSample.cs \ + frontend/roslyn/samples/AppDomainShutdownSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -317,6 +318,20 @@ jobs: || { echo "FAIL: expected OWN014 region escape on the static-event instance subscription"; exit 1; } echo "$out" | grep -q "region escape" \ || { echo "FAIL: expected the region-escape wording on the static-event capture"; exit 1; } + # P-004 process-lifetime AppDomain-event exemption (mined: Npgsql PoolManager): a + # NON-CAPTURING handler on a process-host AppDomain event (ProcessExit/DomainUnload/ + # UnhandledException/FirstChanceException) is a shutdown/diagnostics hook meant to live + # for the process -> NOT a region escape -> silent. (Scoped to ShutdownCleanup, the + # exempt class — CapturingShutdownSubscriber in the same file MUST still raise OWN014.) + 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" \ + || { echo "FAIL: an instance-capturing AppDomain handler must still raise OWN014 (exemption is non-capturing only)"; exit 1; } # the unsubscribed variant (a matching `-=`, released capture) is mitigated # -> silent. Must NOT be reported. if echo "$out" | grep -q "CleanStaticEventViewModel"; then diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index a716e00c..163a6944 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -203,6 +203,61 @@ static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) return cands.Length > 0 && cands.All(c => c is IMethodSymbol { IsStatic: true }); } +// P-004 process-lifetime exemption: a subscription to a PROCESS-HOST `System.AppDomain` +// event — ProcessExit / DomainUnload (shutdown cleanup hooks) or UnhandledException / +// FirstChanceException (process-wide diagnostics) — is never a region escape. The handler +// is MEANT to live for the whole process: it runs at shutdown, or on every unhandled throw, +// and the AppDomain IS the process host. Promoting the subscriber to "the AppDomain's +// lifetime" is therefore the intent, not a leak. Mined: Npgsql's PoolManager static ctor +// `AppDomain.CurrentDomain.{DomainUnload,ProcessExit} += (_,_) => ClearAll()` — a deliberate +// "close idle connectors on appdomain unload (web-app redeployment)" hook (#491). +static bool IsProcessLifetimeAppDomainEvent(IEventSymbol ev) => + ev.Name is "ProcessExit" or "DomainUnload" or "UnhandledException" or "FirstChanceException" + && ev.ContainingType is { Name: "AppDomain" } ct + && IsInNamespace(ct, "System"); + +// Does this handler retain NO subscriber instance? A static method group has a null delegate +// target; a lambda / anonymous method retains nothing only when it captures neither `this` +// (explicit, or implicit via an instance member) nor an enclosing local/parameter. Keeps the +// AppDomain process-lifetime exemption sound — a CAPTURING handler is still pinned to the +// process until shutdown and stays OWN014 (Codex); only a non-capturing one (Npgsql's +// `(_,_) => ClearAll()`, a static call) is safe to drop. +static bool HandlerRetainsNoInstance(ExpressionSyntax right, SemanticModel model) +{ + if (IsStaticHandler(right, model)) + return true; + if (right is not AnonymousFunctionExpressionSyntax lambda) + return false; // a delegate-typed value is opaque -> conservatively assume it captures + foreach (var node in lambda.DescendantNodes()) + { + if (node is ThisExpressionSyntax or BaseExpressionSyntax) + return false; + if (node is not IdentifierNameSyntax id + || (id.Parent is MemberAccessExpressionSyntax m && m.Name == id)) // `x.Member`: name resolved via x + continue; + var sym = model.GetSymbolInfo(id).Symbol; + if (sym is IFieldSymbol { IsStatic: false } or IPropertySymbol { IsStatic: false } + or IEventSymbol { IsStatic: false } + or IMethodSymbol { IsStatic: false, MethodKind: MethodKind.Ordinary }) + return false; // an instance member by SIMPLE name -> implicit `this` capture + if (sym is ILocalSymbol or IParameterSymbol && !DeclaredWithin(sym, lambda)) + return false; // an enclosing local / parameter -> captured + } + return true; +} + +// Is EVERY declaration of `sym` inside `scope`? A lambda's own parameters/locals are; an +// enclosing local/parameter is not — so a reference to the latter is a capture. +static bool DeclaredWithin(ISymbol sym, SyntaxNode scope) +{ + if (sym.DeclaringSyntaxReferences.Length == 0) + return false; + foreach (var r in sym.DeclaringSyntaxReferences) + if (!scope.FullSpan.Contains(r.Span)) + return false; + return true; +} + // P-004 process-lived-subscriber exemption: the WPF application object (`App`) is a // process-lived singleton — exactly one instance, created at startup, alive until // the process exits. Subscribing it to a process-lived static event @@ -2078,8 +2133,16 @@ or ImplicitObjectCreationExpressionSyntax // constructs) — the source<->this cycle is GC-collectable; // - static handler — a static method has a null delegate target, // so no instance is retained and nothing can leak. + // - a process-host AppDomain event (ProcessExit/DomainUnload/Unhandled- + // Exception/FirstChanceException) whose handler retains NO instance — the + // handler is meant to live for the whole process, so the "escape" is the + // intent, not a leak (mined: Npgsql PoolManager's `AppDomain.CurrentDomain. + // ProcessExit += (_,_) => ClearAll()` shutdown hook). A handler that captures + // instance state still pins it to the process, so it stays OWN014 (Codex). if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, selfOwned) - || IsStaticHandler(a.Right, model))) + || IsStaticHandler(a.Right, model) + || (IsProcessLifetimeAppDomainEvent(ev) + && HandlerRetainsNoInstance(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 diff --git a/frontend/roslyn/samples/AppDomainShutdownSample.cs b/frontend/roslyn/samples/AppDomainShutdownSample.cs new file mode 100644 index 00000000..a83529b3 --- /dev/null +++ b/frontend/roslyn/samples/AppDomainShutdownSample.cs @@ -0,0 +1,54 @@ +using System; + +namespace Own.Samples; + +// P-004 process-lifetime AppDomain-event exemption (mined: Npgsql PoolManager). Subscribing to +// AppDomain's process-host events — ProcessExit / DomainUnload (shutdown cleanup hooks) and +// UnhandledException / FirstChanceException (process-wide diagnostics) — is never a region +// escape: the handler is meant to live for the whole process. Must be SILENT. Contrast: +// NonAppDomainSubscriber below — a non-AppDomain static event with a lambda still raises OWN014, +// proving the exemption keys off the AppDomain source, not any process-lived static event. + +public sealed class ShutdownCleanup +{ + public ShutdownCleanup() + { + AppDomain.CurrentDomain.ProcessExit += (_, _) => Cleanup(); // shutdown hook -> SILENT + AppDomain.CurrentDomain.DomainUnload += (_, _) => Cleanup(); // shutdown hook -> SILENT + AppDomain.CurrentDomain.UnhandledException += (_, _) => Cleanup(); // process diagnostics -> SILENT + AppDomain.CurrentDomain.FirstChanceException += (_, _) => Cleanup(); // process diagnostics -> SILENT + } + + private static void Cleanup() { } +} + +// Codex control: a process-host AppDomain event is exempt only when the handler retains NO +// instance. A lambda that CAPTURES instance state (here `_count`) pins this subscriber to the +// process until shutdown — a real region escape that must STILL raise OWN014. +public sealed class CapturingShutdownSubscriber +{ + private int _count; + + public CapturingShutdownSubscriber() + { + AppDomain.CurrentDomain.ProcessExit += (_, _) => _count++; // captures `this` -> OWN014 + } +} + +public static class SomeBus +{ + public static event EventHandler? Pinged; + + public static void Raise() => Pinged?.Invoke(null, EventArgs.Empty); +} + +// Control: a lambda on a NON-AppDomain process-lived (static) event -> region escape -> must WARN. +public sealed class NonAppDomainSubscriber +{ + public NonAppDomainSubscriber() + { + SomeBus.Pinged += (_, _) => Handle(); // static event, lambda, not AppDomain -> OWN014 + } + + private static void Handle() { } +}