diff --git a/Parsing/Visitors/AstNodeCatalog.cs b/Parsing/Visitors/AstNodeCatalog.cs new file mode 100644 index 00000000..9abd53c4 --- /dev/null +++ b/Parsing/Visitors/AstNodeCatalog.cs @@ -0,0 +1,40 @@ +using System.Reflection; + +namespace SharpTS.Parsing.Visitors; + +/// +/// The single canonical list of concrete AST node types, derived once from the +/// and record definitions in Parsing/AST.cs. +/// +/// +/// +/// This is the convergence point for the AST dispatch tables (issue #1243, epic #1094). Every +/// consumer that needs "the set of AST node kinds" reads it from here instead of re-deriving it: +/// uses it for exhaustiveness +/// checks (Freeze/FreezeAsync) and handler auto-registration (AutoRegister), +/// and the visitor-dispatch guard test validates 's switch against it. +/// A new node added to AST.cs automatically appears here, so it cannot be silently omitted +/// from a dispatch table without a loud failure (a startup exception from the registry, or a +/// failing guard test for the visitor switch). +/// +/// +/// "Node type" is defined exactly as the dispatch tables historically defined it: a directly +/// nested type of / that is concrete and inherits from the +/// base. This excludes helper records nested under the bases that are not themselves dispatched +/// nodes — Expr.PropertyKey/Property, Stmt.Parameter, the interface-member +/// records, and so on. +/// +/// +public static class AstNodeCatalog +{ + /// Every concrete node type, in reflection order. + public static readonly IReadOnlyList ExprTypes = CollectConcreteNodes(typeof(Expr)); + + /// Every concrete node type, in reflection order. + public static readonly IReadOnlyList StmtTypes = CollectConcreteNodes(typeof(Stmt)); + + private static IReadOnlyList CollectConcreteNodes(Type baseType) => + baseType.GetNestedTypes(BindingFlags.Public) + .Where(t => baseType.IsAssignableFrom(t) && !t.IsAbstract && t != baseType) + .ToArray(); +} diff --git a/Parsing/Visitors/AstVisitorBase.cs b/Parsing/Visitors/AstVisitorBase.cs index 6d2168d7..c87e46f6 100644 --- a/Parsing/Visitors/AstVisitorBase.cs +++ b/Parsing/Visitors/AstVisitorBase.cs @@ -74,7 +74,8 @@ public virtual void Visit(Expr expr) // Fail loud on an unhandled node rather than silently skipping its subtree. A newly // added Expr kind that nobody wired here would otherwise be invisible to every analyzer // built on AstVisitorBase (closure/suspension/variable analysis) — a latent miscompile, - // not just missing coverage (#1107). + // not just missing coverage (#1107). AstDispatchTests validates this switch against the + // canonical AstNodeCatalog so an omission fails a test, not just at visit time (#1243). default: throw new NotSupportedException( $"AstVisitorBase has no dispatch case for expression node '{expr.GetType().Name}'. " + @@ -130,6 +131,7 @@ public virtual void Visit(Stmt stmt) case Stmt.DeclareGlobal s: VisitDeclareGlobal(s); break; case Stmt.Using s: VisitUsing(s); break; // Fail loud on an unhandled node rather than silently skipping its subtree (#1107). + // AstDispatchTests validates this switch against the canonical AstNodeCatalog (#1243). default: throw new NotSupportedException( $"AstVisitorBase has no dispatch case for statement node '{stmt.GetType().Name}'. " + diff --git a/Parsing/Visitors/NodeRegistry.cs b/Parsing/Visitors/NodeRegistry.cs index 0d07993d..b7edaf63 100644 --- a/Parsing/Visitors/NodeRegistry.cs +++ b/Parsing/Visitors/NodeRegistry.cs @@ -227,12 +227,9 @@ public NodeRegistry Freeze() var missingTypes = new List(); - // Get all concrete Expr types (nested types in Expr that inherit from Expr) - var allExprTypes = typeof(Expr).GetNestedTypes(BindingFlags.Public) - .Where(t => typeof(Expr).IsAssignableFrom(t) && !t.IsAbstract && t != typeof(Expr)) - .ToList(); - - foreach (var exprType in allExprTypes) + // The canonical node lists come from AstNodeCatalog so this check, AutoRegister, and the + // AstVisitorBase dispatch switch all agree on the node set by construction (#1243). + foreach (var exprType in AstNodeCatalog.ExprTypes) { if (!_exprHandlers.ContainsKey(exprType)) { @@ -240,12 +237,7 @@ public NodeRegistry Freeze() } } - // Get all concrete Stmt types (nested types in Stmt that inherit from Stmt) - var allStmtTypes = typeof(Stmt).GetNestedTypes(BindingFlags.Public) - .Where(t => typeof(Stmt).IsAssignableFrom(t) && !t.IsAbstract && t != typeof(Stmt)) - .ToList(); - - foreach (var stmtType in allStmtTypes) + foreach (var stmtType in AstNodeCatalog.StmtTypes) { if (!_stmtHandlers.ContainsKey(stmtType)) { @@ -282,11 +274,7 @@ public NodeRegistry FreezeAsync() var missingAsyncTypes = new List(); - var allExprTypes = typeof(Expr).GetNestedTypes(BindingFlags.Public) - .Where(t => typeof(Expr).IsAssignableFrom(t) && !t.IsAbstract && t != typeof(Expr)) - .ToList(); - - foreach (var exprType in allExprTypes) + foreach (var exprType in AstNodeCatalog.ExprTypes) { if (!_asyncExprHandlers.ContainsKey(exprType)) { @@ -351,12 +339,8 @@ public NodeRegistry AutoRegister() var contextType = typeof(TContext); var missingMethods = new List(); - // Register all expression types - var allExprTypes = typeof(Expr).GetNestedTypes(BindingFlags.Public) - .Where(t => typeof(Expr).IsAssignableFrom(t) && !t.IsAbstract && t != typeof(Expr)) - .ToList(); - - foreach (var exprType in allExprTypes) + // Register all expression types (node list from AstNodeCatalog — see Freeze). #1243 + foreach (var exprType in AstNodeCatalog.ExprTypes) { var methodName = $"Visit{exprType.Name}"; var method = contextType.GetMethod(methodName, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance, [exprType]); @@ -372,11 +356,7 @@ public NodeRegistry AutoRegister() } // Register all statement types - var allStmtTypes = typeof(Stmt).GetNestedTypes(BindingFlags.Public) - .Where(t => typeof(Stmt).IsAssignableFrom(t) && !t.IsAbstract && t != typeof(Stmt)) - .ToList(); - - foreach (var stmtType in allStmtTypes) + foreach (var stmtType in AstNodeCatalog.StmtTypes) { var methodName = $"Visit{stmtType.Name}"; var method = contextType.GetMethod(methodName, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance, [stmtType]); diff --git a/SharpTS.Tests/RegistryTests/AstDispatchTests.cs b/SharpTS.Tests/RegistryTests/AstDispatchTests.cs new file mode 100644 index 00000000..f9b59f7c --- /dev/null +++ b/SharpTS.Tests/RegistryTests/AstDispatchTests.cs @@ -0,0 +1,111 @@ +using System.Runtime.CompilerServices; +using SharpTS.Parsing; +using SharpTS.Parsing.Visitors; +using Xunit; + +namespace SharpTS.Tests.RegistryTests; + +/// +/// Guards the convergence of the AST dispatch tables onto (issue #1243, +/// epic #1094). The catalog is the single source of truth for the AST node set; these tests ensure +/// the hand-maintained dispatch switch cannot silently omit a node the +/// catalog knows about. 's own +/// exhaustiveness (interpreter + type-checker handlers) is already enforced at startup via the same +/// catalog, so its static initializer running under any test transitively covers that table. +/// +public class AstDispatchTests +{ + /// A concrete visitor that keeps AstVisitorBase's default child-traversal behavior. + private sealed class ProbeVisitor : AstVisitorBase; + + // Expr/Stmt subtypes deliberately absent from the catalog, used to prove the dispatch switch's + // default arm actually throws for an unhandled node — i.e. that the probe below distinguishes + // "handled" from "unhandled" rather than passing vacuously. + private sealed record UnknownExpr : Expr; + private sealed record UnknownStmt : Stmt; + + [Fact] + public void Catalog_is_populated_and_excludes_non_node_records() + { + Assert.NotEmpty(AstNodeCatalog.ExprTypes); + Assert.NotEmpty(AstNodeCatalog.StmtTypes); + + // Representative nodes are present. + Assert.Contains(typeof(Expr.Binary), AstNodeCatalog.ExprTypes); + Assert.Contains(typeof(Stmt.If), AstNodeCatalog.StmtTypes); + + // Every entry is a concrete subtype of its base. + Assert.All(AstNodeCatalog.ExprTypes, t => + { + Assert.True(typeof(Expr).IsAssignableFrom(t)); + Assert.False(t.IsAbstract); + }); + Assert.All(AstNodeCatalog.StmtTypes, t => + { + Assert.True(typeof(Stmt).IsAssignableFrom(t)); + Assert.False(t.IsAbstract); + }); + + // Helper records nested under the bases that are not dispatched nodes stay out. + Assert.DoesNotContain(typeof(Expr.Property), AstNodeCatalog.ExprTypes); + Assert.DoesNotContain(typeof(Stmt.Parameter), AstNodeCatalog.StmtTypes); + } + + [Fact] + public void AstVisitorBase_dispatches_every_Expr_node() + { + foreach (var type in AstNodeCatalog.ExprTypes) + { + var node = (Expr)RuntimeHelpers.GetUninitializedObject(type); + Assert.False( + HitsDefaultArm(() => new ProbeVisitor().Visit(node)), + $"AstVisitorBase.Visit(Expr) has no dispatch case for Expr.{type.Name}; " + + $"add a case that calls a Visit{type.Name} method."); + } + } + + [Fact] + public void AstVisitorBase_dispatches_every_Stmt_node() + { + foreach (var type in AstNodeCatalog.StmtTypes) + { + var node = (Stmt)RuntimeHelpers.GetUninitializedObject(type); + Assert.False( + HitsDefaultArm(() => new ProbeVisitor().Visit(node)), + $"AstVisitorBase.Visit(Stmt) has no dispatch case for Stmt.{type.Name}; " + + $"add a case that calls a Visit{type.Name} method."); + } + } + + [Fact] + public void Probe_detects_an_unhandled_node() + { + // Sanity check on the discriminator itself: an out-of-catalog node DOES hit the default arm. + Assert.True(HitsDefaultArm(() => new ProbeVisitor().Visit(new UnknownExpr()))); + Assert.True(HitsDefaultArm(() => new ProbeVisitor().Visit(new UnknownStmt()))); + } + + /// + /// Runs against an uninitialized node and reports whether dispatch fell + /// through to 's default arm. That arm is the ONLY source of + /// in the visitor: a handled node either returns cleanly (a + /// leaf) or throws as its traversal walks the uninitialized + /// node's null children — neither of which is a NotSupportedException. + /// + private static bool HitsDefaultArm(Action visit) + { + try + { + visit(); + return false; + } + catch (NotSupportedException) + { + return true; + } + catch + { + return false; + } + } +}