zclsyntax: In Variables, don't return traversals in child scopes

The purpose of the Variables function is to tell a calling application
what symbols need to be present in the _root_ scope, so it would be
unhelpful to include child scope traversals. Child scopes are populated
by the nodes that create them, and are thus not interesting to the
calling application (for this purpose, at least).

zcl/zclsyntax/expression.go

@@ -591,12 +591,30 @@ func (e *ForExpr) Value(ctx *zcl.EvalContext) (cty.Value, zcl.Diagnostics) {
 
 func (e *ForExpr) walkChildNodes(w internalWalkFunc) {
 	e.CollExpr = w(e.CollExpr).(Expression)
-	if e.KeyExpr != nil {
+
-		e.KeyExpr = w(e.KeyExpr).(Expression)
+	scopeNames := map[string]struct{}{}
+	if e.KeyVar != "" {
+		scopeNames[e.KeyVar] = struct{}{}
 	}
-	e.ValExpr = w(e.ValExpr).(Expression)
+	if e.ValVar != "" {
+		scopeNames[e.ValVar] = struct{}{}
+	}
+
+	if e.KeyExpr != nil {
+		w(ChildScope{
+			LocalNames: scopeNames,
+			Expr:       &e.KeyExpr,
+		})
+	}
+	w(ChildScope{
+		LocalNames: scopeNames,
+		Expr:       &e.ValExpr,
+	})
 	if e.CondExpr != nil {
-		e.CondExpr = w(e.CondExpr).(Expression)
+		w(ChildScope{
+			LocalNames: scopeNames,
+			Expr:       &e.CondExpr,
+		})
 	}
 }
 

zcl/zclsyntax/variables.go

@@ -11,15 +11,76 @@ import (
 func Variables(expr Expression) []zcl.Traversal {
 	var vars []zcl.Traversal
 
-	// TODO: When traversing into ForExpr, filter out references to
+	walker := &variablesWalker{
-	// the iterator variables, since they are references into the child
+		Callback: func(t zcl.Traversal) {
-	// scope, and thus not interesting to the caller.
+			vars = append(vars, t)
-
+		},
-	VisitAll(expr, func(n Node) zcl.Diagnostics {
-		if ste, ok := n.(*ScopeTraversalExpr); ok {
-			vars = append(vars, ste.Traversal)
 	}
-		return nil
+
-	})
+	Walk(expr, walker)
+
 	return vars
 }
+
+// variablesWalker is a Walker implementation that calls its callback for any
+// root scope traversal found while walking.
+type variablesWalker struct {
+	Callback    func(zcl.Traversal)
+	localScopes []map[string]struct{}
+}
+
+func (w *variablesWalker) Enter(n Node) zcl.Diagnostics {
+	switch tn := n.(type) {
+	case *ScopeTraversalExpr:
+		t := tn.Traversal
+
+		// Check if the given root name appears in any of the active
+		// local scopes. We don't want to return local variables here, since
+		// the goal of walking variables is to tell the calling application
+		// which names it needs to populate in the _root_ scope.
+		name := t.RootName()
+		for _, names := range w.localScopes {
+			if _, localized := names[name]; localized {
+				return nil
+			}
+		}
+
+		w.Callback(t)
+	case ChildScope:
+		w.localScopes = append(w.localScopes, tn.LocalNames)
+	}
+	return nil
+}
+
+func (w *variablesWalker) Exit(n Node) zcl.Diagnostics {
+	switch n.(type) {
+	case ChildScope:
+		// pop the latest local scope, assuming that the walker will
+		// behave symmetrically as promised.
+		w.localScopes = w.localScopes[:len(w.localScopes)-1]
+	}
+	return nil
+}
+
+// ChildScope is a synthetic AST node that is visited during a walk to
+// indicate that its descendent will be evaluated in a child scope, which
+// may mask certain variables from the parent scope as locals.
+//
+// ChildScope nodes don't really exist in the AST, but are rather synthesized
+// on the fly during walk. Therefore it doesn't do any good to transform them;
+// instead, transform either parent node that created a scope or the expression
+// that the child scope struct wraps.
+type ChildScope struct {
+	LocalNames map[string]struct{}
+	Expr       *Expression // pointer because it can be replaced on walk
+}
+
+func (e ChildScope) walkChildNodes(w internalWalkFunc) {
+	*(e.Expr) = w(*(e.Expr)).(Expression)
+}
+
+// Range returns the range of the expression that the ChildScope is
+// encapsulating. It isn't really very useful to call Range on a ChildScope.
+func (e ChildScope) Range() zcl.Range {
+	return (*e.Expr).Range()
+}

zcl/zclsyntax/variables_test.go

@@ -0,0 +1,231 @@
+package zclsyntax
+
+import (
+	"fmt"
+	"testing"
+
+	"reflect"
+
+	"github.com/davecgh/go-spew/spew"
+	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-zcl/zcl"
+)
+
+func TestVariables(t *testing.T) {
+	tests := []struct {
+		Expr Expression
+		Want []zcl.Traversal
+	}{
+		{
+			&LiteralValueExpr{
+				Val: cty.True,
+			},
+			nil,
+		},
+		{
+			&ScopeTraversalExpr{
+				Traversal: zcl.Traversal{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+			},
+			[]zcl.Traversal{
+				{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+			},
+		},
+		{
+			&BinaryOpExpr{
+				LHS: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "foo",
+						},
+					},
+				},
+				Op: OpAdd,
+				RHS: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "bar",
+						},
+					},
+				},
+			},
+			[]zcl.Traversal{
+				{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "bar",
+					},
+				},
+			},
+		},
+		{
+			&UnaryOpExpr{
+				Val: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "foo",
+						},
+					},
+				},
+				Op: OpNegate,
+			},
+			[]zcl.Traversal{
+				{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+			},
+		},
+		{
+			&ConditionalExpr{
+				Condition: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "foo",
+						},
+					},
+				},
+				TrueResult: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "bar",
+						},
+					},
+				},
+				FalseResult: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "baz",
+						},
+					},
+				},
+			},
+			[]zcl.Traversal{
+				{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "bar",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "baz",
+					},
+				},
+			},
+		},
+		{
+			&ForExpr{
+				KeyVar: "k",
+				ValVar: "v",
+
+				CollExpr: &ScopeTraversalExpr{
+					Traversal: zcl.Traversal{
+						zcl.TraverseRoot{
+							Name: "foo",
+						},
+					},
+				},
+				KeyExpr: &BinaryOpExpr{
+					LHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "k",
+							},
+						},
+					},
+					Op: OpAdd,
+					RHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "bar",
+							},
+						},
+					},
+				},
+				ValExpr: &BinaryOpExpr{
+					LHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "v",
+							},
+						},
+					},
+					Op: OpAdd,
+					RHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "baz",
+							},
+						},
+					},
+				},
+				CondExpr: &BinaryOpExpr{
+					LHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "k",
+							},
+						},
+					},
+					Op: OpLessThan,
+					RHS: &ScopeTraversalExpr{
+						Traversal: zcl.Traversal{
+							zcl.TraverseRoot{
+								Name: "limit",
+							},
+						},
+					},
+				},
+			},
+			[]zcl.Traversal{
+				{
+					zcl.TraverseRoot{
+						Name: "foo",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "bar",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "baz",
+					},
+				},
+				{
+					zcl.TraverseRoot{
+						Name: "limit",
+					},
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%#v", test.Expr), func(t *testing.T) {
+			got := Variables(test.Expr)
+
+			if !reflect.DeepEqual(got, test.Want) {
+				t.Errorf("wrong result\ngot:  %s\nwant: %s", spew.Sdump(got), spew.Sdump(test.Want))
+			}
+		})
+	}
+}