fix(extractor): curated app-scoped source exemption for the Application subscriber (Closes #228)#232
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -895,6 +895,120 @@ | ||
| && 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<IMethodSymbol>().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<IsPatternExpressionSyntax>().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<AssignmentExpressionSyntax>()) | ||
| if (SymbolEqualityComparer.Default.Equals( | ||
| model.GetSymbolInfo(asg.Left).Symbol, local)) | ||
| return false; | ||
| foreach (var arg in scope.DescendantNodes().OfType<ArgumentSyntax>()) | ||
| 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<IMethodSymbol>().FirstOrDefault(); | ||
| return sym is not null | ||
| && SymbolEqualityComparer.Default.Equals(sym.ContainingType, cls); | ||
PhysShell marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| // 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. | ||
| @@ -3817,7 +3931,7 @@ | ||
| .Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries) | ||
| .Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); | ||
| var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase); | ||
Check warning on line 3934 in frontend/roslyn/OwnSharp.Extractor/Program.cs
| ||
| var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList(); | ||
| // P-004 WPF profile: widen the reference set with assemblies named by the | ||
| // OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref | ||
| @@ -4063,6 +4177,19 @@ | ||
| // 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
Check warning on line 74 in frontend/roslyn/samples/AppScopedSourceSample.cs
| ||
| } | ||
| 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 | ||
Check warning on line 89 in frontend/roslyn/samples/AppScopedSourceSample.cs
| ||
| } | ||
| } | ||
| // 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 | ||
Check warning on line 100 in frontend/roslyn/samples/AppScopedSourceSample.cs
| ||
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| 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 | ||
Check warning on line 118 in frontend/roslyn/samples/AppScopedSourceSample.cs
| ||
| } | ||
| 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 | ||
Check warning on line 137 in frontend/roslyn/samples/AppScopedSourceSample.cs
| ||
| } | ||
| void OnTheme(object? sender, EventArgs e) { } | ||
| } | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.