refactor(ast): converge dispatch tables onto a canonical node catalog (#1243)#1249
Merged
Conversation
…#1243) Introduce AstNodeCatalog as the single source of truth for the AST node set, derived once from the Expr/Stmt records in AST.cs. NodeRegistry's Freeze/FreezeAsync/AutoRegister now read it instead of each re-running its own typeof(Expr).GetNestedTypes reflection, collapsing three duplicated reflection sites to one (behavior-identical predicate/set). Add AstDispatchTests to guard the one remaining hand-maintained table, the AstVisitorBase Visit(Expr/Stmt) switch: every catalog node must dispatch without falling through to the "no dispatch case" default arm, with a self-check proving the probe fires for an out-of-catalog node. A node added to AST.cs now auto-appears in the catalog, so omitting its visitor case fails a test instead of only blowing up at visit time. Evaluated the issue's source-generator option but chose the dependency-free canonical-table route: SharpTS uses no Roslyn today, and only one table is hand-maintained. No new dependencies or projects. xUnit 15419/0, Test262 22/0/4skip, TSConf 31/0. Closes #1243.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #1243 — Converge the AST dispatch tables onto a single canonical node list
Follow-up to #1143 / epic #1094.
Approach
The issue asked to "evaluate a source generator (or a single canonical table)" so a node can't
silently skip a dispatch table. I started down the source-generator path but that means introducing
Roslyn (
Microsoft.CodeAnalysis.CSharp) as a build-time dependency — which this repo does notcurrently use, on a preview SDK. A generator's only real advantage here is turning a missed node into
a compile error rather than a test failure, and only one table is actually hand-maintained
(the
AstVisitorBaseswitch).NodeRegistryalready derives its node set by reflection and validatesexhaustiveness at startup.
So this takes the lighter, dependency-free route: a single canonical table everything converges on.
What changed
New
Parsing/Visitors/AstNodeCatalog.cs— the single source of truth for the AST node set,derived once (reflection) from the
Expr/Stmtrecords inAST.csusing the exact predicate thedispatch tables used before (concrete, directly-nested, inherits the base). Exposes
ExprTypes/StmtTypes.NodeRegistry—Freeze(),FreezeAsync(), andAutoRegister()now read the node set fromAstNodeCataloginstead of each re-running its owntypeof(Expr).GetNestedTypes(...)reflection.Three duplicated reflection sites collapse to one. Behavior is identical (same predicate, same set),
so the interpreter/type-checker startup exhaustiveness checks — and the
InterpreterRegistryasync-handler
FreezeAsynccheck — are unchanged except for their source of truth.New
SharpTS.Tests/RegistryTests/AstDispatchTests.cs— guards the one hand-maintained table.It walks every catalog node through
AstVisitorBase.Visitand asserts none falls through to thedefaultarm (the "no dispatch case"NotSupportedException). A self-check (Probe_detects_an_unhandled_node)proves the discriminator actually fires for an out-of-catalog node. Adding a node to
AST.csnowauto-appears in the catalog, so forgetting its
AstVisitorBasecase fails this test instead of onlyblowing up at visit time.
Result — every dispatch table now converges on
AstNodeCatalogNodeRegistrysync handlers (interp + checker)Freeze()against the catalogNodeRegistryasync expr handlers (interp)FreezeAsync()against the catalogAstVisitorBase.Visit(Expr/Stmt)switchAstDispatchTestsagainst the catalogNo new dependencies; no new projects.
Testing
AstDispatchTests: 4/4 pass.Closes #1243.