cue/ast: don't walk comments twice

mvdan · mvdan · commit fd645501c4b5 · 2024-04-30T12:52:24.000Z
I was surprised by the exported Walk function using the inspector type to then call the unexported walk function, rather than having a single Walk function without any intermediary state. From reading the code, it seems like the reason was to interleave visiting the node comments with the node contents themselves. However, we were also visiting the node comments in their entirety as part of walk, meaning we were visiting each comment twice. Moreover, there weren't any tests for this interleaving of comments, and I'm fairly sure that the logic wasn't sound, as the Token method which is meant to advance relative token positions was never called. On top of that, cue/ast is currently not able to represent comments which follow an entire node on a new line, as it can only represent inline comments or comments after a fixed number of tokens. It seems best to delete this untested and likely broken code for now, and attempt interleaving the visiting of comments again in the future once we have tests and better support for relative comment positions in cue/ast. For the time being, it seems like none of our walk API uses were interested in comments at all, so there are no visible changes. The fact that no user reported the double-walking of comments also seems to indicate that very few users relied on comment walking. Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I7d43f5327dcdfc927d2d98c8978476a6da264c56 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1194083 Reviewed-by: Roger Peppe <rogpeppe@gmail.com> TryBot-Result: CUEcueckoo <cueckoo@cuelang.org> Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
diff --git a/cue/ast/walk.go b/cue/ast/walk.go
@@ -16,23 +16,12 @@ package ast
 
 import (
 	"fmt"
-
-	"cuelang.org/go/cue/token"
 )
 
-// Walk traverses an AST in depth-first order: It starts by calling f(node);
-// node must not be nil. If before returns true, Walk invokes f recursively for
-// each of the non-nil children of node, followed by a call of after. Both
-// functions may be nil. If before is nil, it is assumed to always return true.
-func Walk(node Node, before func(Node) bool, after func(Node)) {
-	v := &inspector{before: before, after: after}
-	walk(node, v.Before, v.After)
-}
-
 // WalkVisitor traverses an AST in depth-first order with a [Visitor].
 func WalkVisitor(node Node, visitor Visitor) {
 	v := &stackVisitor{stack: []Visitor{visitor}}
-	walk(node, v.Before, v.After)
+	Walk(node, v.Before, v.After)
 }
 
 // stackVisitor helps implement Visitor support on top of Walk.
@@ -65,12 +54,16 @@ type Visitor interface {
 
 func walkList[N Node](list []N, before func(Node) bool, after func(Node)) {
 	for _, node := range list {
-		walk(node, before, after)
+		Walk(node, before, after)
 	}
 }
 
-func walk(node Node, before func(Node) bool, after func(Node)) {
-	if !before(node) {
+// Walk traverses an AST in depth-first order: It starts by calling f(node);
+// node must not be nil. If before returns true, Walk invokes f recursively for
+// each of the non-nil children of node, followed by a call of after. Both
+// functions may be nil. If before is nil, it is assumed to always return true.
+func Walk(node Node, before func(Node) bool, after func(Node)) {
+	if before != nil && !before(node) {
 		return
 	}
 
@@ -93,15 +86,15 @@ func walk(node Node, before func(Node) bool, after func(Node)) {
 		// nothing to do
 
 	case *Field:
-		walk(n.Label, before, after)
+		Walk(n.Label, before, after)
 		if n.Value != nil {
-			walk(n.Value, before, after)
+			Walk(n.Value, before, after)
 		}
 		walkList(n.Attrs, before, after)
 
 	case *Func:
 		walkList(n.Args, before, after)
-		walk(n.Ret, before, after)
+		Walk(n.Ret, before, after)
 
 	case *StructLit:
 		walkList(n.Elts, before, after)
@@ -118,46 +111,46 @@ func walk(node Node, before func(Node) bool, after func(Node)) {
 
 	case *Ellipsis:
 		if n.Type != nil {
-			walk(n.Type, before, after)
+			Walk(n.Type, before, after)
 		}
 
 	case *ParenExpr:
-		walk(n.X, before, after)
+		Walk(n.X, before, after)
 
 	case *SelectorExpr:
-		walk(n.X, before, after)
-		walk(n.Sel, before, after)
+		Walk(n.X, before, after)
+		Walk(n.Sel, before, after)
 
 	case *IndexExpr:
-		walk(n.X, before, after)
-		walk(n.Index, before, after)
+		Walk(n.X, before, after)
+		Walk(n.Index, before, after)
 
 	case *SliceExpr:
-		walk(n.X, before, after)
+		Walk(n.X, before, after)
 		if n.Low != nil {
-			walk(n.Low, before, after)
+			Walk(n.Low, before, after)
 		}
 		if n.High != nil {
-			walk(n.High, before, after)
+			Walk(n.High, before, after)
 		}
 
 	case *CallExpr:
-		walk(n.Fun, before, after)
+		Walk(n.Fun, before, after)
 		walkList(n.Args, before, after)
 
 	case *UnaryExpr:
-		walk(n.X, before, after)
+		Walk(n.X, before, after)
 
 	case *BinaryExpr:
-		walk(n.X, before, after)
-		walk(n.Y, before, after)
+		Walk(n.X, before, after)
+		Walk(n.Y, before, after)
 
 	// Declarations
 	case *ImportSpec:
 		if n.Name != nil {
-			walk(n.Name, before, after)
+			Walk(n.Name, before, after)
 		}
-		walk(n.Path, before, after)
+		Walk(n.Path, before, after)
 
 	case *BadDecl:
 		// nothing to do
@@ -166,100 +159,42 @@ func walk(node Node, before func(Node) bool, after func(Node)) {
 		walkList(n.Specs, before, after)
 
 	case *EmbedDecl:
-		walk(n.Expr, before, after)
+		Walk(n.Expr, before, after)
 
 	case *LetClause:
-		walk(n.Ident, before, after)
-		walk(n.Expr, before, after)
+		Walk(n.Ident, before, after)
+		Walk(n.Expr, before, after)
 
 	case *Alias:
-		walk(n.Ident, before, after)
-		walk(n.Expr, before, after)
+		Walk(n.Ident, before, after)
+		Walk(n.Expr, before, after)
 
 	case *Comprehension:
 		walkList(n.Clauses, before, after)
-		walk(n.Value, before, after)
+		Walk(n.Value, before, after)
 
 	// Files and packages
 	case *File:
 		walkList(n.Decls, before, after)
 
 	case *Package:
-		walk(n.Name, before, after)
+		Walk(n.Name, before, after)
 
 	case *ForClause:
 		if n.Key != nil {
-			walk(n.Key, before, after)
+			Walk(n.Key, before, after)
 		}
-		walk(n.Value, before, after)
-		walk(n.Source, before, after)
+		Walk(n.Value, before, after)
+		Walk(n.Source, before, after)
 
 	case *IfClause:
-		walk(n.Condition, before, after)
+		Walk(n.Condition, before, after)
 
 	default:
 		panic(fmt.Sprintf("Walk: unexpected node type %T", n))
 	}
 
-	after(node)
-}
-
-type inspector struct {
-	before func(Node) bool
-	after  func(Node)
-
-	commentStack []commentFrame
-	current      commentFrame
-}
-
-type commentFrame struct {
-	cg  []*CommentGroup
-	pos int8
-}
-
-func (f *inspector) Before(node Node) bool {
-	if f.before == nil || f.before(node) {
-		f.commentStack = append(f.commentStack, f.current)
-		f.current = commentFrame{cg: Comments(node)}
-		f.visitComments(f.current.pos)
-		return true
-	}
-	return false
-}
-
-func (f *inspector) After(node Node) {
-	f.visitComments(127)
-	p := len(f.commentStack) - 1
-	f.current = f.commentStack[p]
-	f.commentStack = f.commentStack[:p]
-	f.current.pos++
-	if f.after != nil {
-		f.after(node)
-	}
-}
-
-func (f *inspector) Token(t token.Token) {
-	f.current.pos++
-}
-
-func (f *inspector) visitComments(pos int8) {
-	c := &f.current
-	for ; len(c.cg) > 0; c.cg = c.cg[1:] {
-		cg := c.cg[0]
-		if cg.Position == pos {
-			continue
-		}
-		if f.before == nil || f.before(cg) {
-			for _, c := range cg.List {
-				if f.before == nil || f.before(c) {
-					if f.after != nil {
-						f.after(c)
-					}
-				}
-			}
-			if f.after != nil {
-				f.after(cg)
-			}
-		}
+	if after != nil {
+		after(node)
 	}
 }
