From 51945b4e0cd37be582bfeebea4c2fc3674aad4dd Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 14 Jun 2017 08:03:32 -0700 Subject: [PATCH] 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 | 26 +++- zcl/zclsyntax/variables.go | 79 +++++++++-- zcl/zclsyntax/variables_test.go | 231 ++++++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+), 13 deletions(-) create mode 100644 zcl/zclsyntax/variables_test.go diff --git a/zcl/zclsyntax/expression.go b/zcl/zclsyntax/expression.go index 875bc9f..539004e 100644 --- a/zcl/zclsyntax/expression.go +++ b/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, + }) } } diff --git a/zcl/zclsyntax/variables.go b/zcl/zclsyntax/variables.go index d18d044..bea1178 100644 --- a/zcl/zclsyntax/variables.go +++ b/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 - // the iterator variables, since they are references into the child - // scope, and thus not interesting to the caller. + walker := &variablesWalker{ + Callback: func(t zcl.Traversal) { + vars = append(vars, t) + }, + } + + Walk(expr, walker) - VisitAll(expr, func(n Node) zcl.Diagnostics { - if ste, ok := n.(*ScopeTraversalExpr); ok { - vars = append(vars, ste.Traversal) - } - return nil - }) 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() +} diff --git a/zcl/zclsyntax/variables_test.go b/zcl/zclsyntax/variables_test.go new file mode 100644 index 0000000..21733ab --- /dev/null +++ b/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)) + } + }) + } +}