ext/dynblock: A more arduous way to find variables required to expand

The previous ForEachVariables method was flawed because it didn't have
enough information to properly analyze child blocks. Since the core HCL
API requires a schema for any body analysis, and since a schema only
describes one level of configuration structure at a time, we must require
callers to drive a recursive walk through their nested block structure so
that the correct schema can be provided at each level.

This API is rather more complex than is ideal, but is the best we can do
with the HCL Body API as currently defined, and it's currently defined
that way in order to properly support ambiguous syntaxes like JSON.

ext/dynblock/README.md

@@ -83,12 +83,61 @@ within the `content` block are evaluated separately and so can be passed a
 separate `EvalContext` if desired, during normal attribute expression
 evaluation.
 
+## Detecting Variables
+
 Some applications dynamically generate an `EvalContext` by analyzing which
-variables are referenced by an expression before evaluating it. This can be
+variables are referenced by an expression before evaluating it.
-achieved for a block that might contain `dynamic` blocks by calling
+
-`ForEachVariables`, which returns the variables required by the `for_each`
+This unfortunately requires some extra effort when this analysis is required
-and `labels` attributes in all `dynamic` blocks within the given body,
+for the context passed to `Expand`: the HCL API requires a schema to be
-including any nested `dynamic` blocks.
+provided in order to do any analysis of the blocks in a body, but the low-level
+schema model provides a description of only one level of nested blocks at
+a time, and thus a new schema must be provided for each additional level of
+nesting.
+
+To make this arduous process as convenient as possbile, this package provides
+a helper function `WalkForEachVariables`, which returns a `WalkVariablesNode`
+instance that can be used to find variables directly in a given body and also
+determine which nested blocks require recursive calls. Using this mechanism
+requires that the caller be able to look up a schema given a nested block type.
+For _simple_ formats where a specific block type name always has the same schema
+regardless of context, a walk can be implemented as follows:
+
+```
+func walkVariables(node dynblock.WalkVariablesNode, schema *hcl.BodySchema) []hcl.Traversal {
+	vars, children := node.Visit(schema)
+
+	for _, child := range children {
+		var childSchema *hcl.BodySchema
+		switch child.BlockTypeName {
+		case "a":
+			childSchema = &hcl.BodySchema{
+				Blocks: []hcl.BlockHeaderSchema{
+					{
+						Type:       "b",
+						LabelNames: []string{"key"},
+					},
+				},
+			}
+		case "b":
+			childSchema = &hcl.BodySchema{
+				Attributes: []hcl.AttributeSchema{
+					{
+						Name:     "val",
+						Required: true,
+					},
+				},
+			}
+		default:
+			// Should never happen, because the above cases should be exhaustive
+			// for the application's configuration format.
+			panic(fmt.Errorf("can't find schema for unknown block type %q", child.BlockTypeName))
+		}
+
+		vars = append(vars, testWalkAndAccumVars(child.Node, childSchema)...)
+	}
+}
+```
 
 # Performance
 

ext/dynblock/variables.go

@@ -2,58 +2,146 @@ package dynblock
 
 import (
 	"github.com/hashicorp/hcl2/hcl"
+	"github.com/zclconf/go-cty/cty"
 )
 
-// ForEachVariables looks for "dynamic" blocks inside the given body
+// WalkVariables begins the recursive process of walking the variables in the
-// (which should be a body that would be passed to Expand, not the return
+// given body that are needed by any "for_each" or "labels" attributes in
-// value of Expand) and returns any variables that are used within their
+// "dynamic" blocks. The result is a WalkVariablesNode, which can extract
-// "for_each" and "labels" expressions, for use in dynamically constructing a
+// root-level variable traversals and produce a list of child nodes that
-// scope to pass as part of a hcl.EvalContext to Transformer.
+// also need to be processed by calling Visit.
-func ForEachVariables(original hcl.Body) []hcl.Traversal {
+//
-	var traversals []hcl.Traversal
+// This function requires that the caller walk through the nested block
-	container, _, _ := original.PartialContent(variableDetectionContainerSchema)
+// structure in the given body level-by-level so that an appropriate schema
-	if container == nil {
+// can be provided at each level to inform further processing. This workflow
-		return traversals
+// is thus easiest to use for calling applications that have some higher-level
+// schema representation available with which to drive this multi-step
+// process.
+func WalkForEachVariables(body hcl.Body) WalkVariablesNode {
+	return WalkVariablesNode{
+		body: body,
+	}
 }
 
+type WalkVariablesNode struct {
+	body hcl.Body
+	it   *iteration
+}
+
+type WalkVariablesChild struct {
+	BlockTypeName string
+	Node          WalkVariablesNode
+}
+
+// Visit returns the variable traversals required for any "dynamic" blocks
+// directly in the body associated with this node, and also returns any child
+// nodes that must be visited in order to continue the walk.
+//
+// Each child node has its associated block type name given in its BlockTypeName
+// field, which the calling application should use to determine the appropriate
+// schema for the content of each child node and pass it to the child node's
+// own Visit method to continue the walk recursively.
+func (n WalkVariablesNode) Visit(schema *hcl.BodySchema) (vars []hcl.Traversal, children []WalkVariablesChild) {
+	extSchema := n.extendSchema(schema)
+	container, _, _ := n.body.PartialContent(extSchema)
+	if container == nil {
+		return vars, children
+	}
+
+	children = make([]WalkVariablesChild, 0, len(container.Blocks))
+
 	for _, block := range container.Blocks {
+		switch block.Type {
+
+		case "dynamic":
+			blockTypeName := block.Labels[0]
 			inner, _, _ := block.Body.PartialContent(variableDetectionInnerSchema)
 			if inner == nil {
 				continue
 			}
-		iteratorName := block.Labels[0]
+
+			iteratorName := blockTypeName
 			if attr, exists := inner.Attributes["iterator"]; exists {
 				iterTraversal, _ := hcl.AbsTraversalForExpr(attr.Expr)
-			if len(iterTraversal) > 0 {
+				if len(iterTraversal) == 0 {
+					// Ignore this invalid dynamic block, since it'll produce
+					// an error if someone tries to extract content from it
+					// later anyway.
+					continue
+				}
 				iteratorName = iterTraversal.RootName()
 			}
-		}
+			blockIt := n.it.MakeChild(iteratorName, cty.DynamicVal, cty.DynamicVal)
 
 			if attr, exists := inner.Attributes["for_each"]; exists {
-			traversals = append(traversals, attr.Expr.Variables()...)
+				// Filter out iterator names inherited from parent blocks
+				for _, traversal := range attr.Expr.Variables() {
+					if _, inherited := blockIt.Inherited[traversal.RootName()]; !inherited {
+						vars = append(vars, traversal)
+					}
+				}
 			}
 			if attr, exists := inner.Attributes["labels"]; exists {
-			// Filter out our own iterator name, since the caller
+				// Filter out both our own iterator name _and_ those inherited
-			// doesn't need to provide that.
+				// from parent blocks, since we provide _both_ of these to the
+				// label expressions.
 				for _, traversal := range attr.Expr.Variables() {
-				if traversal.RootName() != iteratorName {
+					ours := traversal.RootName() == iteratorName
-					traversals = append(traversals, traversal)
+					_, inherited := blockIt.Inherited[traversal.RootName()]
-				}
+
+					if !(ours || inherited) {
+						vars = append(vars, traversal)
 					}
 				}
 			}
 
-	return traversals
+			for _, contentBlock := range inner.Blocks {
+				// We only request "content" blocks in our schema, so we know
+				// any blocks we find here will be content blocks. We require
+				// exactly one content block for actual expansion, but we'll
+				// be more liberal here so that callers can still collect
+				// variables from erroneous "dynamic" blocks.
+				children = append(children, WalkVariablesChild{
+					BlockTypeName: blockTypeName,
+					Node: WalkVariablesNode{
+						body: contentBlock.Body,
+						it:   blockIt,
+					},
+				})
 			}
 
-// These are more-relaxed schemata than what's in schema.go, since we
+		default:
+			children = append(children, WalkVariablesChild{
+				BlockTypeName: block.Type,
+				Node: WalkVariablesNode{
+					body: block.Body,
+					it:   n.it,
+				},
+			})
+
+		}
+	}
+
+	return vars, children
+}
+
+func (n WalkVariablesNode) extendSchema(schema *hcl.BodySchema) *hcl.BodySchema {
+	// We augment the requested schema to also include our special "dynamic"
+	// block type, since then we'll get instances of it interleaved with
+	// all of the literal child blocks we must also include.
+	extSchema := &hcl.BodySchema{
+		Attributes: schema.Attributes,
+		Blocks:     make([]hcl.BlockHeaderSchema, len(schema.Blocks), len(schema.Blocks)+1),
+	}
+	copy(extSchema.Blocks, schema.Blocks)
+	extSchema.Blocks = append(extSchema.Blocks, dynamicBlockHeaderSchema)
+
+	return extSchema
+}
+
+// This is a more relaxed schema than what's in schema.go, since we
 // want to maximize the amount of variables we can find even if there
 // are erroneous blocks.
-var variableDetectionContainerSchema = &hcl.BodySchema{
-	Blocks: []hcl.BlockHeaderSchema{
-		dynamicBlockHeaderSchema,
-	},
-}
 var variableDetectionInnerSchema = &hcl.BodySchema{
 	Attributes: []hcl.AttributeSchema{
 		{
@@ -69,4 +157,9 @@ var variableDetectionInnerSchema = &hcl.BodySchema{
 			Required: false,
 		},
 	},
+	Blocks: []hcl.BlockHeaderSchema{
+		{
+			Type: "content",
+		},
+	},
 }

ext/dynblock/variables_test.go

@@ -0,0 +1,150 @@
+package dynblock
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	"github.com/davecgh/go-spew/spew"
+
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/hashicorp/hcl2/hcl/hclsyntax"
+)
+
+func TestForEachVariables(t *testing.T) {
+	const src = `
+
+# We have some references to things inside the "val" attribute inside each
+# of our "b" blocks, but since our ForEachVariables walk only considers
+# "for_each" and "labels" within a dynamic block we do _not_ expect these
+# to be in the output.
+
+a {
+  dynamic "b" {
+    for_each = [for i, v in some_list_0: "${i}=${v},${baz}"]
+    labels = ["${b.value} ${something_else_0}"]
+    content {
+      val = "${b.value} ${something_else_1}"
+    }
+  }
+}
+
+dynamic "a" {
+  for_each = some_list_1
+
+  content {
+    b "foo" {
+      val = "${a.value} ${something_else_2}"
+    }
+
+    dynamic "b" {
+      for_each = some_list_2
+      iterator = dyn_b
+      labels = ["${a.value} ${dyn_b.value} ${b} ${something_else_3}"]
+      content {
+        val = "${a.value} ${dyn_b.value} ${something_else_4}"
+      }
+    }
+  }
+}
+
+dynamic "a" {
+  for_each = some_list_3
+  iterator = dyn_a
+
+  content {
+    b "foo" {
+    val = "${dyn_a.value} ${something_else_5}"
+  }
+
+  dynamic "b" {
+    for_each = some_list_4
+      labels = ["${dyn_a.value} ${b.value} ${a} ${something_else_6}"]
+      content {
+        val = "${dyn_a.value} ${b.value} ${something_else_7}"
+      }
+    }
+  }
+}
+`
+
+	f, diags := hclsyntax.ParseConfig([]byte(src), "", hcl.Pos{})
+	if len(diags) != 0 {
+		t.Errorf("unexpected diagnostics during parse")
+		for _, diag := range diags {
+			t.Logf("- %s", diag)
+		}
+		return
+	}
+
+	rootNode := WalkForEachVariables(f.Body)
+	traversals := testWalkAndAccumVars(rootNode, &hcl.BodySchema{
+		Blocks: []hcl.BlockHeaderSchema{
+			{
+				Type: "a",
+			},
+		},
+	})
+
+	got := make([]string, len(traversals))
+	for i, traversal := range traversals {
+		got[i] = traversal.RootName()
+	}
+
+	// The block structure is traversed one level at a time, so the ordering
+	// here is reflecting first a pass of the root, then the first child
+	// under the root, then the first child under that, etc.
+	want := []string{
+		"some_list_1",
+		"some_list_3",
+		"some_list_0",
+		"baz",
+		"something_else_0",
+		"some_list_2",
+		"b", // This is correct because it is referenced in a context where the iterator is overridden to be dyn_b
+		"something_else_3",
+		"some_list_4",
+		"a", // This is correct because it is referenced in a context where the iterator is overridden to be dyn_a
+		"something_else_6",
+	}
+
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want))
+	}
+}
+
+func testWalkAndAccumVars(node WalkVariablesNode, schema *hcl.BodySchema) []hcl.Traversal {
+	vars, children := node.Visit(schema)
+
+	for _, child := range children {
+		var childSchema *hcl.BodySchema
+		switch child.BlockTypeName {
+		case "a":
+			childSchema = &hcl.BodySchema{
+				Blocks: []hcl.BlockHeaderSchema{
+					{
+						Type:       "b",
+						LabelNames: []string{"key"},
+					},
+				},
+			}
+		case "b":
+			childSchema = &hcl.BodySchema{
+				Attributes: []hcl.AttributeSchema{
+					{
+						Name:     "val",
+						Required: true,
+					},
+				},
+			}
+		default:
+			// Should never happen, because we have no other block types
+			// in our test input.
+			panic(fmt.Errorf("can't find schema for unknown block type %q", child.BlockTypeName))
+		}
+
+		vars = append(vars, testWalkAndAccumVars(child.Node, childSchema)...)
+	}
+
+	return vars
+}