terraform: refactor nodeModuleVariable and NodeRootVariable EvalTree()s (#26245)

EvalModuleCallArguments is now a method on nodeModuleVariable, it's only
caller, and the other functions have been replaces with straight through
code (or in the case of evalVariableValidations, a standalone function).

I was unable to add tests for nodeModuleVariable.Execute, which requires
fixtures that aren't part of the MockEvalContext (a scope.evalContext is
one); it's not ideal but that function should be well covered by the
context tests so I chose to leave it as-is.

Finally, I removed the unused function hclTypeName. Deleting code is
fun!

terraform/eval_variable.go

@@ -3,138 +3,25 @@ package terraform
 import (
 	"fmt"
 	"log"
-	"reflect"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
-	"github.com/hashicorp/terraform/instances"
 	"github.com/hashicorp/terraform/tfdiags"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/convert"
 )
 
-// EvalSetModuleCallArguments is an EvalNode implementation that sets values
+// evalVariableValidations ensures ta all of the configured custom validations
-// for arguments of a child module call, for later retrieval during
+// for a variable are passing.
-// expression evaluation.
-type EvalSetModuleCallArguments struct {
-	Module addrs.ModuleCallInstance
-	Values map[string]cty.Value
-}
-
-// TODO: test
-func (n *EvalSetModuleCallArguments) Eval(ctx EvalContext) (interface{}, error) {
-	ctx.SetModuleCallArguments(n.Module, n.Values)
-	return nil, nil
-}
-
-// EvalModuleCallArgument is an EvalNode implementation that produces the value
-// for a particular variable as will be used by a child module instance.
-//
-// The result is written into the map given in Values, with its key
-// set to the local name of the variable, disregarding the module instance
-// address. Any existing values in that map are deleted first. This weird
-// interface is a result of trying to be convenient for use with
-// EvalContext.SetModuleCallArguments, which expects a map to merge in with
-// any existing arguments.
-type EvalModuleCallArgument struct {
-	Addr           addrs.InputVariable
-	Config         *configs.Variable
-	Expr           hcl.Expression
-	ModuleInstance addrs.ModuleInstance
-
-	Values map[string]cty.Value
-
-	// validateOnly indicates that this evaluation is only for config
-	// validation, and we will not have any expansion module instance
-	// repetition data.
-	validateOnly bool
-}
-
-func (n *EvalModuleCallArgument) Eval(ctx EvalContext) (interface{}, error) {
-	// Clear out the existing mapping
-	for k := range n.Values {
-		delete(n.Values, k)
-	}
-
-	wantType := n.Config.Type
-	name := n.Addr.Name
-	expr := n.Expr
-
-	if expr == nil {
-		// Should never happen, but we'll bail out early here rather than
-		// crash in case it does. We set no value at all in this case,
-		// making a subsequent call to EvalContext.SetModuleCallArguments
-		// a no-op.
-		log.Printf("[ERROR] attempt to evaluate %s with nil expression", n.Addr.String())
-		return nil, nil
-	}
-
-	var moduleInstanceRepetitionData instances.RepetitionData
-
-	switch {
-	case n.validateOnly:
-		// the instance expander does not track unknown expansion values, so we
-		// have to assume all RepetitionData is unknown.
-		moduleInstanceRepetitionData = instances.RepetitionData{
-			CountIndex: cty.UnknownVal(cty.Number),
-			EachKey:    cty.UnknownVal(cty.String),
-			EachValue:  cty.DynamicVal,
-		}
-
-	default:
-		// Get the repetition data for this module instance,
-		// so we can create the appropriate scope for evaluating our expression
-		moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance)
-	}
-
-	scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData)
-	val, diags := scope.EvalExpr(expr, cty.DynamicPseudoType)
-
-	// We intentionally passed DynamicPseudoType to EvalExpr above because
-	// now we can do our own local type conversion and produce an error message
-	// with better context if it fails.
-	var convErr error
-	val, convErr = convert.Convert(val, wantType)
-	if convErr != nil {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid value for module argument",
-			Detail: fmt.Sprintf(
-				"The given value is not suitable for child module variable %q defined at %s: %s.",
-				name, n.Config.DeclRange.String(), convErr,
-			),
-			Subject: expr.Range().Ptr(),
-		})
-		// We'll return a placeholder unknown value to avoid producing
-		// redundant downstream errors.
-		val = cty.UnknownVal(wantType)
-	}
-
-	n.Values[name] = val
-	return nil, diags.ErrWithWarnings()
-}
-
-// evalVariableValidations is an EvalNode implementation that ensures that
-// all of the configured custom validations for a variable are passing.
 //
 // This must be used only after any side-effects that make the value of the
 // variable available for use in expression evaluation, such as
 // EvalModuleCallArgument for variables in descendent modules.
-type evalVariableValidations struct {
+func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *configs.Variable, expr hcl.Expression, ctx EvalContext) error {
-	Addr   addrs.AbsInputVariableInstance
+	if config == nil || len(config.Validations) == 0 {
-	Config *configs.Variable
+		log.Printf("[TRACE] evalVariableValidations: not active for %s, so skipping", addr)
-
+		return nil
-	// Expr is the expression that provided the value for the variable, if any.
-	// This will be nil for root module variables, because their values come
-	// from outside the configuration.
-	Expr hcl.Expression
-}
-
-func (n *evalVariableValidations) Eval(ctx EvalContext) (interface{}, error) {
-	if n.Config == nil || len(n.Config.Validations) == 0 {
-		log.Printf("[TRACE] evalVariableValidations: not active for %s, so skipping", n.Addr)
-		return nil, nil
 	}
 
 	var diags tfdiags.Diagnostics
@@ -148,27 +35,27 @@ func (n *evalVariableValidations) Eval(ctx EvalContext) (interface{}, error) {
 	// bypass our usual evaluation machinery here and just produce a minimal
 	// evaluation context containing just the required value, and thus avoid
 	// the problem that ctx's evaluation functions refer to the wrong module.
-	val := ctx.GetVariableValue(n.Addr)
+	val := ctx.GetVariableValue(addr)
 	hclCtx := &hcl.EvalContext{
 		Variables: map[string]cty.Value{
 			"var": cty.ObjectVal(map[string]cty.Value{
-				n.Config.Name: val,
+				config.Name: val,
 			}),
 		},
 		Functions: ctx.EvaluationScope(nil, EvalDataForNoInstanceKey).Functions(),
 	}
 
-	for _, validation := range n.Config.Validations {
+	for _, validation := range config.Validations {
 		const errInvalidCondition = "Invalid variable validation result"
 		const errInvalidValue = "Invalid value for variable"
 
 		result, moreDiags := validation.Condition.Value(hclCtx)
 		diags = diags.Append(moreDiags)
 		if moreDiags.HasErrors() {
-			log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", n.Addr, validation.DeclRange, diags.Err().Error())
+			log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", addr, validation.DeclRange, diags.Err().Error())
 		}
 		if !result.IsKnown() {
-			log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", n.Addr, validation.DeclRange)
+			log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", addr, validation.DeclRange)
 			continue // We'll wait until we've learned more, then.
 		}
 		if result.IsNull() {
@@ -197,12 +84,12 @@ func (n *evalVariableValidations) Eval(ctx EvalContext) (interface{}, error) {
 		}
 
 		if result.False() {
-			if n.Expr != nil {
+			if expr != nil {
 				diags = diags.Append(&hcl.Diagnostic{
 					Severity: hcl.DiagError,
 					Summary:  errInvalidValue,
 					Detail:   fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()),
-					Subject:  n.Expr.Range().Ptr(),
+					Subject:  expr.Range().Ptr(),
 				})
 			} else {
 				// Since we don't have a source expression for a root module
@@ -212,34 +99,11 @@ func (n *evalVariableValidations) Eval(ctx EvalContext) (interface{}, error) {
 					Severity: hcl.DiagError,
 					Summary:  errInvalidValue,
 					Detail:   fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()),
-					Subject:  n.Config.DeclRange.Ptr(),
+					Subject:  config.DeclRange.Ptr(),
 				})
 			}
 		}
 	}
 
-	return nil, diags.ErrWithWarnings()
+	return diags.ErrWithWarnings()
-}
-
-// hclTypeName returns the name of the type that would represent this value in
-// a config file, or falls back to the Go type name if there's no corresponding
-// HCL type. This is used for formatted output, not for comparing types.
-func hclTypeName(i interface{}) string {
-	switch k := reflect.Indirect(reflect.ValueOf(i)).Kind(); k {
-	case reflect.Bool:
-		return "boolean"
-	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
-		reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32,
-		reflect.Uint64, reflect.Uintptr, reflect.Float32, reflect.Float64:
-		return "number"
-	case reflect.Array, reflect.Slice:
-		return "list"
-	case reflect.Map:
-		return "map"
-	case reflect.String:
-		return "string"
-	default:
-		// fall back to the Go type if there's no match
-		return k.String()
-	}
 }

terraform/node_module_variable.go

@@ -2,13 +2,16 @@ package terraform
 
 import (
 	"fmt"
+	"log"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/instances"
 	"github.com/hashicorp/terraform/lang"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/convert"
 )
 
 // nodeExpandModuleVariable is the placeholder for an variable that has not yet had
@@ -112,7 +115,7 @@ type nodeModuleVariable struct {
 // implementing.
 var (
 	_ GraphNodeModuleInstance = (*nodeModuleVariable)(nil)
-	_ GraphNodeEvalable       = (*nodeModuleVariable)(nil)
+	_ GraphNodeExecutable     = (*nodeModuleVariable)(nil)
 	_ graphNodeTemporaryValue = (*nodeModuleVariable)(nil)
 	_ dag.GraphNodeDotter     = (*nodeModuleVariable)(nil)
 )
@@ -137,57 +140,37 @@ func (n *nodeModuleVariable) ModulePath() addrs.Module {
 	return n.Addr.Module.Module()
 }
 
-// GraphNodeEvalable
+// GraphNodeExecutable
-func (n *nodeModuleVariable) EvalTree() EvalNode {
+func (n *nodeModuleVariable) Execute(ctx EvalContext, op walkOperation) error {
 	// If we have no value, do nothing
 	if n.Expr == nil {
-		return &EvalNoop{}
+		return nil
 	}
 
 	// Otherwise, interpolate the value of this variable and set it
 	// within the variables mapping.
-	vals := make(map[string]cty.Value)
+	var vals map[string]cty.Value
+	var err error
 
-	_, call := n.Addr.Module.CallInstance()
+	switch op {
-
+	case walkRefresh, walkPlan, walkApply, walkDestroy, walkImport:
-	return &EvalSequence{
+		vals, err = n.EvalModuleCallArgument(ctx, false)
-		Nodes: []EvalNode{
+		if err != nil {
-			&EvalOpFilter{
+			return err
-				Ops: []walkOperation{walkRefresh, walkPlan, walkApply,
+		}
-					walkDestroy, walkImport},
+	case walkValidate:
-				Node: &EvalModuleCallArgument{
+		vals, err = n.EvalModuleCallArgument(ctx, true)
-					Addr:           n.Addr.Variable,
+		if err != nil {
-					Config:         n.Config,
+			return err
-					Expr:           n.Expr,
+		}
-					ModuleInstance: n.ModuleInstance,
-					Values:         vals,
-				},
-			},
-
-			&EvalOpFilter{
-				Ops: []walkOperation{walkValidate},
-				Node: &EvalModuleCallArgument{
-					Addr:           n.Addr.Variable,
-					Config:         n.Config,
-					Expr:           n.Expr,
-					ModuleInstance: n.ModuleInstance,
-					Values:         vals,
-					validateOnly:   true,
-				},
-			},
-
-			&EvalSetModuleCallArguments{
-				Module: call,
-				Values: vals,
-			},
-
-			&evalVariableValidations{
-				Addr:   n.Addr,
-				Config: n.Config,
-				Expr:   n.Expr,
-			},
-		},
 	}
+
+	// Set values for arguments of a child module call, for later retrieval
+	// during expression evaluation.
+	_, call := n.Addr.Module.CallInstance()
+	ctx.SetModuleCallArguments(call, vals)
+
+	return evalVariableValidations(n.Addr, n.Config, n.Expr, ctx)
 }
 
 // dag.GraphNodeDotter impl.
@@ -200,3 +183,75 @@ func (n *nodeModuleVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNod
 		},
 	}
 }
+
+// EvalModuleCallArgument produces the value for a particular variable as will
+// be used by a child module instance.
+//
+// The result is written into a map, with its key set to the local name of the
+// variable, disregarding the module instance address. A map is returned instead
+// of a single value as a result of trying to be convenient for use with
+// EvalContext.SetModuleCallArguments, which expects a map to merge in with any
+// existing arguments.
+//
+// validateOnly indicates that this evaluation is only for config
+// validation, and we will not have any expansion module instance
+// repetition data.
+func (n *nodeModuleVariable) EvalModuleCallArgument(ctx EvalContext, validateOnly bool) (map[string]cty.Value, error) {
+	wantType := n.Config.Type
+	name := n.Addr.Variable.Name
+	expr := n.Expr
+
+	if expr == nil {
+		// Should never happen, but we'll bail out early here rather than
+		// crash in case it does. We set no value at all in this case,
+		// making a subsequent call to EvalContext.SetModuleCallArguments
+		// a no-op.
+		log.Printf("[ERROR] attempt to evaluate %s with nil expression", n.Addr.String())
+		return nil, nil
+	}
+
+	var moduleInstanceRepetitionData instances.RepetitionData
+
+	switch {
+	case validateOnly:
+		// the instance expander does not track unknown expansion values, so we
+		// have to assume all RepetitionData is unknown.
+		moduleInstanceRepetitionData = instances.RepetitionData{
+			CountIndex: cty.UnknownVal(cty.Number),
+			EachKey:    cty.UnknownVal(cty.String),
+			EachValue:  cty.DynamicVal,
+		}
+
+	default:
+		// Get the repetition data for this module instance,
+		// so we can create the appropriate scope for evaluating our expression
+		moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance)
+	}
+
+	scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData)
+	val, diags := scope.EvalExpr(expr, cty.DynamicPseudoType)
+
+	// We intentionally passed DynamicPseudoType to EvalExpr above because
+	// now we can do our own local type conversion and produce an error message
+	// with better context if it fails.
+	var convErr error
+	val, convErr = convert.Convert(val, wantType)
+	if convErr != nil {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  "Invalid value for module argument",
+			Detail: fmt.Sprintf(
+				"The given value is not suitable for child module variable %q defined at %s: %s.",
+				name, n.Config.DeclRange.String(), convErr,
+			),
+			Subject: expr.Range().Ptr(),
+		})
+		// We'll return a placeholder unknown value to avoid producing
+		// redundant downstream errors.
+		val = cty.UnknownVal(wantType)
+	}
+
+	vals := make(map[string]cty.Value)
+	vals[name] = val
+	return vals, diags.ErrWithWarnings()
+}

terraform/node_root_variable.go

@@ -35,22 +35,23 @@ func (n *NodeRootVariable) ReferenceableAddrs() []addrs.Referenceable {
 	return []addrs.Referenceable{n.Addr}
 }
 
-// GraphNodeEvalable
+// GraphNodeExecutable
-func (n *NodeRootVariable) EvalTree() EvalNode {
+func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) error {
 	// We don't actually need to _evaluate_ a root module variable, because
 	// its value is always constant and already stashed away in our EvalContext.
 	// However, we might need to run some user-defined validation rules against
 	// the value.
 
 	if n.Config == nil || len(n.Config.Validations) == 0 {
-		return &EvalSequence{} // nothing to do
+		return nil // nothing to do
 	}
 
-	return &evalVariableValidations{
+	return evalVariableValidations(
-		Addr:   addrs.RootModuleInstance.InputVariable(n.Addr.Name),
+		addrs.RootModuleInstance.InputVariable(n.Addr.Name),
-		Config: n.Config,
+		n.Config,
-		Expr:   nil, // not set for root module variables
+		nil, // not set for root module variables
-	}
+		ctx,
+	)
 }
 
 // dag.GraphNodeDotter impl.

terraform/node_root_variable_test.go

@@ -0,0 +1,25 @@
+package terraform
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/configs"
+)
+
+func TestNodeRootVariableExecute(t *testing.T) {
+	ctx := new(MockEvalContext)
+
+	n := &NodeRootVariable{
+		Addr: addrs.InputVariable{Name: "foo"},
+		Config: &configs.Variable{
+			Name: "foo",
+		},
+	}
+
+	err := n.Execute(ctx, walkApply)
+	if err != nil {
+		t.Fatalf("unexpected error: %s", err.Error())
+	}
+
+}