You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
vartree=CSharpSyntaxTree.ParseText(source);varcomp=CSharpCompilation.Create("lint",[tree]);// no MetadataReferences neededvarmodel=comp.GetSemanticModel(tree);// "does this identifier bind to the slot declared by that deconstruction?"staticboolResolvesTo(SemanticModelmodel,IdentifierNameSyntaxsite,ISymbolslot)=>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.
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.
Problem
SampleCards_SharedStateDoesNotSpreadintests/Reactor.Tests/Tooling/GallerySampleLintTests.csdecides 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-700and: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:
var (url, setUrl) = UseState(...)is not aLocalDeclarationStatementSyntaxcatch (UriFormatException url)beside a type-levelconst string urlfrom url in,let url =,join … into,into)newthrough type-provided contextThe generalisation, which is what makes this worth fixing structurally rather than patching again:
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),usingdeclarations,foreachvariables, 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/GetDeclaredSymboland compareISymbolidentity, 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. ACSharpCompilationover bareCSharpSyntaxTree.ParseTextfragments — 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:
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:
KnownSharedStatePagesalready 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
:625-640source 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.Confirmation