From db422d9cee798d04be06abb135305f467edb770d Mon Sep 17 00:00:00 2001 From: git-hulk Date: Thu, 11 Jun 2026 10:59:19 +0800 Subject: [PATCH] Add drift test keeping ASTVisitor and Walk traversals in sync Accept/ASTVisitor and Walk/WalkFunc are two independent traversal engines that encode every node's children separately, so a new AST node type added to one can silently be missing from the other. Add a static test that parses the package source and asserts every type with an Accept method has both a Visit method on the ASTVisitor interface and a case in Walk's type switch, and vice versa. The test immediately caught four types missing from Walk - BoolLiteral, CreateNamedCollection, NamedCollectionParam and WindowFrameParam - and their cases are added. Also fix a confirmed traversal bug: JoinTableExpr.Accept returned early after visiting the sample ratio, so VisitJoinTableExpr was never called for table expressions carrying a SAMPLE clause. Add a regression test. Co-Authored-By: Claude Fable 5 --- parser/ast.go | 4 +- parser/traversal_drift_test.go | 137 +++++++++++++++++++++++++++++++++ parser/walk.go | 25 ++++++ 3 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 parser/traversal_drift_test.go diff --git a/parser/ast.go b/parser/ast.go index 90454766..da902fbc 100644 --- a/parser/ast.go +++ b/parser/ast.go @@ -176,7 +176,9 @@ func (j *JoinTableExpr) Accept(visitor ASTVisitor) error { return err } if j.SampleRatio != nil { - return j.SampleRatio.Accept(visitor) + if err := j.SampleRatio.Accept(visitor); err != nil { + return err + } } return visitor.VisitJoinTableExpr(j) } diff --git a/parser/traversal_drift_test.go b/parser/traversal_drift_test.go new file mode 100644 index 00000000..3fc75a0a --- /dev/null +++ b/parser/traversal_drift_test.go @@ -0,0 +1,137 @@ +package parser + +import ( + "go/ast" + goparser "go/parser" + "go/token" + "os" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// The package has two independent traversal engines: Accept/ASTVisitor and +// Walk/WalkFunc. Each encodes every node's children separately, so a new AST +// node type added to one can silently be forgotten in the other. This test +// statically asserts that every type with an Accept method also has a Visit +// method on the ASTVisitor interface and a case in Walk's type switch (and +// vice versa), so the engines cannot drift. +func TestTraversalEnginesCoverSameNodeTypes(t *testing.T) { + entries, err := os.ReadDir(".") + require.NoError(t, err) + + fset := token.NewFileSet() + acceptTypes := map[string]bool{} + visitorTypes := map[string]bool{} + walkTypes := map[string]bool{} + + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := goparser.ParseFile(fset, name, nil, 0) + require.NoError(t, err) + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + switch { + case d.Name.Name == "Accept" && d.Recv != nil && len(d.Recv.List) == 1: + if star, ok := d.Recv.List[0].Type.(*ast.StarExpr); ok { + if ident, ok := star.X.(*ast.Ident); ok { + acceptTypes[ident.Name] = true + } + } + case d.Name.Name == "Walk" && d.Recv == nil: + ast.Inspect(d.Body, func(n ast.Node) bool { + cc, ok := n.(*ast.CaseClause) + if !ok { + return true + } + for _, expr := range cc.List { + if star, ok := expr.(*ast.StarExpr); ok { + if ident, ok := star.X.(*ast.Ident); ok { + walkTypes[ident.Name] = true + } + } + } + return true + }) + } + case *ast.GenDecl: + for _, spec := range d.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok || ts.Name.Name != "ASTVisitor" { + continue + } + iface, ok := ts.Type.(*ast.InterfaceType) + if !ok { + continue + } + for _, method := range iface.Methods.List { + ft, ok := method.Type.(*ast.FuncType) + if !ok || ft.Params == nil || len(ft.Params.List) != 1 { + continue + } + if star, ok := ft.Params.List[0].Type.(*ast.StarExpr); ok { + if ident, ok := star.X.(*ast.Ident); ok { + visitorTypes[ident.Name] = true + } + } + } + } + } + } + } + + require.NotEmpty(t, acceptTypes) + require.NotEmpty(t, visitorTypes) + require.NotEmpty(t, walkTypes) + + require.Empty(t, diffSet(acceptTypes, visitorTypes), + "types with an Accept method but no ASTVisitor Visit method") + require.Empty(t, diffSet(visitorTypes, acceptTypes), + "types with an ASTVisitor Visit method but no Accept method") + require.Empty(t, diffSet(acceptTypes, walkTypes), + "types with an Accept method but no case in Walk's type switch") + require.Empty(t, diffSet(walkTypes, acceptTypes), + "types with a case in Walk's type switch but no Accept method") +} + +// diffSet returns the members of a that are not in b, sorted. +func diffSet(a, b map[string]bool) []string { + var diff []string + for name := range a { + if !b[name] { + diff = append(diff, name) + } + } + sort.Strings(diff) + return diff +} + +// VisitJoinTableExpr must be called even when the table expression carries a +// SAMPLE clause; Accept used to return early after visiting the sample ratio. +func TestVisitJoinTableExprWithSampleRatio(t *testing.T) { + stmts, err := NewParser("SELECT * FROM t SAMPLE 1/10").ParseStmts() + require.NoError(t, err) + require.Len(t, stmts, 1) + + var visitedJoinTable, visitedSample bool + visitor := &DefaultASTVisitor{ + Visit: func(expr Expr) error { + switch expr.(type) { + case *JoinTableExpr: + visitedJoinTable = true + case *SampleClause: + visitedSample = true + } + return nil + }, + } + require.NoError(t, stmts[0].Accept(visitor)) + require.True(t, visitedSample, "SampleClause was not visited") + require.True(t, visitedJoinTable, "JoinTableExpr was not visited") +} diff --git a/parser/walk.go b/parser/walk.go index 0cd7d168..7e782949 100644 --- a/parser/walk.go +++ b/parser/walk.go @@ -122,6 +122,8 @@ func Walk(node Expr, fn WalkFunc) bool { // Leaf node case *StringLiteral: // Leaf node + case *BoolLiteral: + // Leaf node case *NullLiteral: // Leaf node case *NotNullLiteral: @@ -431,6 +433,10 @@ func Walk(node Expr, fn WalkFunc) bool { if !Walk(n.Number, fn) { return false } + case *WindowFrameParam: + if !Walk(n.Param, fn) { + return false + } case *TopClause: if !Walk(n.Number, fn) { return false @@ -740,6 +746,25 @@ func Walk(node Expr, fn WalkFunc) bool { if !Walk(n.Expr, fn) { return false } + case *CreateNamedCollection: + if !Walk(n.Name, fn) { + return false + } + if !Walk(n.OnCluster, fn) { + return false + } + for _, param := range n.Params { + if !Walk(param, fn) { + return false + } + } + case *NamedCollectionParam: + if !Walk(n.Name, fn) { + return false + } + if !Walk(n.Value, fn) { + return false + } case *CreateRole: for _, name := range n.RoleNames { if !Walk(name, fn) {