Skip to content

[Feature] Resolve gallery-lint shadowing with SemanticModel instead of a syntactic scope walk #1058

Description

Problem

SampleCards_SharedStateDoesNotSpread in tests/Reactor.Tests/Tooling/GallerySampleLintTests.cs decides whether a name at a use-site refers to a given declaration by walking the syntax tree itself — IsShadowedAt / IsShadowedWithin / NamesIntroducedBy / DeclaredLocals / RangeVariablesOf, roughly :615-700 and :1540+. That is a hand-rolled approximation of C# name resolution, and during #1004 it produced a run of defects that all had the same shape.

Instances found and fixed on that branch:

# Binder the walk didn't model Effect
1 var (url, setUrl) = UseState(...) is not a LocalDeclarationStatementSyntax every gallery slot invisible — the rule reported nothing and nothing reddened
2 catch (UriFormatException url) beside a type-level const string url shadow missed → offender whitelisted
3 LINQ range variables (from url in, let url =, join … into, into) shadow missed → offender whitelisted
4 Target-typed new through type-provided context candidate dropped
5 Lifted declarator selection (which declarator a card actually binds) wrong declarator chosen

The generalisation, which is what makes this worth fixing structurally rather than patching again:

Every one of them was a name matched against a declaration without asking whether the site resolves to it — and the cheap approximation for "resolves to" is always some form of drop the candidate, because the alternative requires walking the scope chain. Approximation errors in name resolution are systematically permissive, i.e. they fail open.

That predicts the direction rather than describing it after the fact, and it means the remaining tail is a fail-open tail: local functions, lambda parameters, pattern designations (is T url), using declarations, foreach variables, nested deconstructions, and primary-constructor parameters are all untested today.

Green is uninformative for this bug class. None of the five changed what the tree scan reported — the suite stayed green throughout, because a detector that reports nothing passes every assertion about what it reported. Only synthetic-source mutation separates a working detector from a blind one, and that instrument has to be built and maintained by hand for each binder.

Proposed solution

Replace the syntactic walk with SemanticModel.GetSymbolInfo / GetDeclaredSymbol and compare ISymbol identity, rather than comparing names and then trying to disqualify by scope.

The cost is lower than it looks: shadowing resolution is lexical, so it needs no MetadataReferences. A CSharpCompilation over bare CSharpSyntaxTree.ParseText fragments — which the file already uses (ParseText ×4) — resolves locals, parameters, range variables, catch variables and deconstruction designations fine. Unresolved types would produce diagnostics without references, but those are not the binder questions this rule asks, and type-level diagnostics can simply be ignored.

Sketch:

var tree = CSharpSyntaxTree.ParseText(source);
var comp = CSharpCompilation.Create("lint", [tree]);   // no MetadataReferences needed
var model = comp.GetSemanticModel(tree);

// "does this identifier bind to the slot declared by that deconstruction?"
static bool ResolvesTo(SemanticModel model, IdentifierNameSyntax site, ISymbol slot) =>
    SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(site).Symbol, slot);

This turns the whole class of bug into a non-question: every binder above is one Roslyn already implements, and each fix listed in the table stops being a special case.

Alternatives considered

Keep patching the syntactic walk (the current state, and what #1004 shipped). Defensible in the short term — the five enumerated instances are fixed and pinned by rows that fail if the fix is reverted, and the fail-open direction means a miss whitelists an offender rather than fabricating one. It was the right call for that PR: a detector rewrite inside a converged review loop, to cover the tail of a class that had just been enumerated and covered, is a poor trade. It is a bad long-term position, because the tail is open-ended and each new instance is silent.

Drop the shadowing logic entirely and accept false positives, with an allowlist. Rejected: KnownSharedStatePages already demonstrates how an allowlist decays — it needs a staleness arm to stay honest, and entries flip from load-bearing to defect the instant the underlying page is fixed.

Additional context

  • Filed as follow-up from PR Fix WebView2 gallery page: per-card state and guarded URL parsing #1004 (issue [Bug] WebView2 gallery sample: cards share url state, and an unvalidated new Uri(url) crashes the page on malformed input #982). The coordinator session explicitly requested it be filed rather than left to be re-derived.
  • The :625-640 source comment records the fail-open rationale and the catch-clause / range-variable cases in place, because the finding that produced them arrived as a suppressed review comment with no thread to reply to.
  • Directional note for whoever picks this up: over-reporting a shadow falls through to reporting the offender; under-reporting whitelists a live one. Collecting range variables query-wide rather than per-clause is the correct asymmetry for the same reason.
  • Anyone changing this rule should mutate synthetic source and require the row count to change, not merely run the suite. A vacuous repro — a fixture exercising the wrong binder — fails first and then passes, for the wrong reason both times, which the usual "write the row so it fails first" discipline does not catch.

Confirmation

  • I have searched existing issues and specs for prior discussion of this idea.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions