core: Handle root and child module input variables consistently

Previously we had a significant discrepancy between these two situations:
we wrote the raw root module variables directly into the EvalContext and
then applied type conversions only at expression evaluation time, while
for child modules we converted and validated the values while visiting
the variable graph node and wrote only the _final_ value into the
EvalContext.

This confusion seems to have been the root cause for #29899, where
validation rules for root module variables were being applied at the wrong
point in the process, prior to type conversion.

To fix that bug and also make similar mistakes less likely in the future,
I've made the root module variable handling more like the child module
variable handling in the following ways:
 - The "raw value" (exactly as given by the user) lives only in the graph
   node representing the variable, which mirrors how the _expression_
   for a child module variable lives in its graph node. This means that
   the flow for the two is the same except that there's no expression
   evaluation step for root module variables, because they arrive as
   constant values from the caller.
 - The set of variable values in the EvalContext is always only "final"
   values, after type conversion is complete. That in turn means we no
   longer need to do "just in time" conversion in
   evaluationStateData.GetInputVariable, and can just return the value
   exactly as stored, which is consistent with how we handle all other
   references between objects.

This diff is noisier than I'd like because of how much it takes to wire
a new argument (the raw variable values) through to the plan graph builder,
but those changes are pretty mechanical and the interesting logic lives
inside the plan graph builder itself, in NodeRootVariable, and
the shared helper functions in eval_variable.go.

While here I also took the opportunity to fix a historical API wart in
EvalContext, where SetModuleCallArguments was built to take a set of
variable values all at once but our current caller always calls with only
one at a time. That is now just SetModuleCallArgument singular, to match
with the new SetRootModuleArgument to deal with root module variables.

internal/terraform/context_apply.go

@@ -30,30 +30,11 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State
 		return nil, diags
 	}
 
-	variables := InputValues{}
-	for name, dyVal := range plan.VariableValues {
-		val, err := dyVal.Decode(cty.DynamicPseudoType)
-		if err != nil {
-			diags = diags.Append(tfdiags.Sourceless(
-				tfdiags.Error,
-				"Invalid variable value in plan",
-				fmt.Sprintf("Invalid value for variable %q recorded in plan file: %s.", name, err),
-			))
-			continue
-		}
-
-		variables[name] = &InputValue{
-			Value:      val,
-			SourceType: ValueFromPlan,
-		}
-	}
-
 	workingState := plan.PriorState.DeepCopy()
 	walker, walkDiags := c.walk(graph, operation, &graphWalkOpts{
-		Config:             config,
+		Config:     config,
-		InputState:         workingState,
+		InputState: workingState,
-		Changes:            plan.Changes,
+		Changes:    plan.Changes,
-		RootVariableValues: variables,
 	})
 	diags = diags.Append(walker.NonFatalDiagnostics)
 	diags = diags.Append(walkDiags)
@@ -83,15 +64,43 @@ Note that the -target option is not suitable for routine use, and is provided on
 }
 
 func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate bool) (*Graph, walkOperation, tfdiags.Diagnostics) {
-	graph, diags := (&ApplyGraphBuilder{
+	var diags tfdiags.Diagnostics
-		Config:       config,
+
-		Changes:      plan.Changes,
+	variables := InputValues{}
-		State:        plan.PriorState,
+	for name, dyVal := range plan.VariableValues {
-		Plugins:      c.plugins,
+		val, err := dyVal.Decode(cty.DynamicPseudoType)
-		Targets:      plan.TargetAddrs,
+		if err != nil {
-		ForceReplace: plan.ForceReplaceAddrs,
+			diags = diags.Append(tfdiags.Sourceless(
-		Validate:     validate,
+				tfdiags.Error,
+				"Invalid variable value in plan",
+				fmt.Sprintf("Invalid value for variable %q recorded in plan file: %s.", name, err),
+			))
+			continue
+		}
+
+		variables[name] = &InputValue{
+			Value:      val,
+			SourceType: ValueFromPlan,
+		}
+	}
+	if diags.HasErrors() {
+		return nil, walkApply, diags
+	}
+
+	graph, moreDiags := (&ApplyGraphBuilder{
+		Config:             config,
+		Changes:            plan.Changes,
+		State:              plan.PriorState,
+		RootVariableValues: variables,
+		Plugins:            c.plugins,
+		Targets:            plan.TargetAddrs,
+		ForceReplace:       plan.ForceReplaceAddrs,
+		Validate:           validate,
 	}).Build(addrs.RootModuleInstance)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
+		return nil, walkApply, diags
+	}
 
 	operation := walkApply
 	if plan.UIMode == plans.DestroyMode {

internal/terraform/context_eval.go

@@ -60,9 +60,10 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a
 	log.Printf("[DEBUG] Building and walking 'eval' graph")
 
 	graph, moreDiags := (&EvalGraphBuilder{
-		Config:  config,
+		Config:             config,
-		State:   state,
+		State:              state,
-		Plugins: c.plugins,
+		RootVariableValues: variables,
+		Plugins:            c.plugins,
 	}).Build(addrs.RootModuleInstance)
 	diags = diags.Append(moreDiags)
 	if moreDiags.HasErrors() {
@@ -70,9 +71,8 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a
 	}
 
 	walkOpts := &graphWalkOpts{
-		InputState:         state,
+		InputState: state,
-		Config:             config,
+		Config:     config,
-		RootVariableValues: variables,
 	}
 
 	walker, moreDiags = c.walk(graph, walkEval, walkOpts)

internal/terraform/context_import.go

@@ -53,11 +53,14 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt
 
 	log.Printf("[DEBUG] Building and walking import graph")
 
+	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
+
 	// Initialize our graph builder
 	builder := &ImportGraphBuilder{
-		ImportTargets: opts.Targets,
+		ImportTargets:      opts.Targets,
-		Config:        config,
+		Config:             config,
-		Plugins:       c.plugins,
+		RootVariableValues: variables,
+		Plugins:            c.plugins,
 	}
 
 	// Build the graph
@@ -67,13 +70,10 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt
 		return state, diags
 	}
 
-	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
-
 	// Walk it
 	walker, walkDiags := c.walk(graph, walkImport, &graphWalkOpts{
-		Config:             config,
+		Config:     config,
-		InputState:         state,
+		InputState: state,
-		RootVariableValues: variables,
 	})
 	diags = diags.Append(walkDiags)
 	if walkDiags.HasErrors() {

internal/terraform/context_plan.go

@@ -125,11 +125,11 @@ The -target option is not for routine use, and is provided only for exceptional
 	var planDiags tfdiags.Diagnostics
 	switch opts.Mode {
 	case plans.NormalMode:
-		plan, planDiags = c.plan(config, prevRunState, variables, opts)
+		plan, planDiags = c.plan(config, prevRunState, opts)
 	case plans.DestroyMode:
-		plan, planDiags = c.destroyPlan(config, prevRunState, variables, opts)
+		plan, planDiags = c.destroyPlan(config, prevRunState, opts)
 	case plans.RefreshOnlyMode:
-		plan, planDiags = c.refreshOnlyPlan(config, prevRunState, variables, opts)
+		plan, planDiags = c.refreshOnlyPlan(config, prevRunState, opts)
 	default:
 		panic(fmt.Sprintf("unsupported plan mode %s", opts.Mode))
 	}
@@ -172,14 +172,14 @@ var DefaultPlanOpts = &PlanOpts{
 	Mode: plans.NormalMode,
 }
 
-func (c *Context) plan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
+func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	if opts.Mode != plans.NormalMode {
 		panic(fmt.Sprintf("called Context.plan with %s", opts.Mode))
 	}
 
-	plan, walkDiags := c.planWalk(config, prevRunState, rootVariables, opts)
+	plan, walkDiags := c.planWalk(config, prevRunState, opts)
 	diags = diags.Append(walkDiags)
 	if diags.HasErrors() {
 		return nil, diags
@@ -194,14 +194,14 @@ func (c *Context) plan(config *configs.Config, prevRunState *states.State, rootV
 	return plan, diags
 }
 
-func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
+func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	if opts.Mode != plans.RefreshOnlyMode {
 		panic(fmt.Sprintf("called Context.refreshOnlyPlan with %s", opts.Mode))
 	}
 
-	plan, walkDiags := c.planWalk(config, prevRunState, rootVariables, opts)
+	plan, walkDiags := c.planWalk(config, prevRunState, opts)
 	diags = diags.Append(walkDiags)
 	if diags.HasErrors() {
 		return nil, diags
@@ -235,7 +235,7 @@ func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.S
 	return plan, diags
 }
 
-func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
+func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 	pendingPlan := &plans.Plan{}
 
@@ -260,7 +260,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State
 		log.Printf("[TRACE] Context.destroyPlan: calling Context.plan to get the effect of refreshing the prior state")
 		normalOpts := *opts
 		normalOpts.Mode = plans.NormalMode
-		refreshPlan, refreshDiags := c.plan(config, prevRunState, rootVariables, &normalOpts)
+		refreshPlan, refreshDiags := c.plan(config, prevRunState, &normalOpts)
 		if refreshDiags.HasErrors() {
 			// NOTE: Normally we'd append diagnostics regardless of whether
 			// there are errors, just in case there are warnings we'd want to
@@ -291,7 +291,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State
 		priorState = pendingPlan.PriorState
 	}
 
-	destroyPlan, walkDiags := c.planWalk(config, priorState, rootVariables, opts)
+	destroyPlan, walkDiags := c.planWalk(config, priorState, opts)
 	diags = diags.Append(walkDiags)
 	if walkDiags.HasErrors() {
 		return nil, diags
@@ -392,7 +392,7 @@ func (c *Context) postPlanValidateMoves(config *configs.Config, stmts []refactor
 	return refactoring.ValidateMoves(stmts, config, allInsts)
 }
 
-func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
+func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 	log.Printf("[DEBUG] Building and walking plan graph for %s", opts.Mode)
 
@@ -419,11 +419,10 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r
 	// we can now walk.
 	changes := plans.NewChanges()
 	walker, walkDiags := c.walk(graph, walkOp, &graphWalkOpts{
-		Config:             config,
+		Config:      config,
-		InputState:         prevRunState,
+		InputState:  prevRunState,
-		Changes:            changes,
+		Changes:     changes,
-		MoveResults:        moveResults,
+		MoveResults: moveResults,
-		RootVariableValues: rootVariables,
 	})
 	diags = diags.Append(walker.NonFatalDiagnostics)
 	diags = diags.Append(walkDiags)
@@ -469,34 +468,37 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
 	switch mode := opts.Mode; mode {
 	case plans.NormalMode:
 		graph, diags := (&PlanGraphBuilder{
-			Config:       config,
+			Config:             config,
-			State:        prevRunState,
+			State:              prevRunState,
-			Plugins:      c.plugins,
+			RootVariableValues: opts.SetVariables,
-			Targets:      opts.Targets,
+			Plugins:            c.plugins,
-			ForceReplace: opts.ForceReplace,
+			Targets:            opts.Targets,
-			Validate:     validate,
+			ForceReplace:       opts.ForceReplace,
-			skipRefresh:  opts.SkipRefresh,
+			Validate:           validate,
+			skipRefresh:        opts.SkipRefresh,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlan, diags
 	case plans.RefreshOnlyMode:
 		graph, diags := (&PlanGraphBuilder{
-			Config:          config,
+			Config:             config,
-			State:           prevRunState,
+			State:              prevRunState,
-			Plugins:         c.plugins,
+			RootVariableValues: opts.SetVariables,
-			Targets:         opts.Targets,
+			Plugins:            c.plugins,
-			Validate:        validate,
+			Targets:            opts.Targets,
-			skipRefresh:     opts.SkipRefresh,
+			Validate:           validate,
-			skipPlanChanges: true, // this activates "refresh only" mode.
+			skipRefresh:        opts.SkipRefresh,
+			skipPlanChanges:    true, // this activates "refresh only" mode.
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlan, diags
 	case plans.DestroyMode:
 		graph, diags := (&DestroyPlanGraphBuilder{
-			Config:      config,
+			Config:             config,
-			State:       prevRunState,
+			State:              prevRunState,
-			Plugins:     c.plugins,
+			RootVariableValues: opts.SetVariables,
-			Targets:     opts.Targets,
+			Plugins:            c.plugins,
-			Validate:    validate,
+			Targets:            opts.Targets,
-			skipRefresh: opts.SkipRefresh,
+			Validate:           validate,
+			skipRefresh:        opts.SkipRefresh,
 		}).Build(addrs.RootModuleInstance)
 		return graph, walkPlanDestroy, diags
 	default:

internal/terraform/context_validate.go

@@ -37,17 +37,6 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics {
 
 	log.Printf("[DEBUG] Building and walking validate graph")
 
-	graph, moreDiags := ValidateGraphBuilder(&PlanGraphBuilder{
-		Config:   config,
-		Plugins:  c.plugins,
-		Validate: true,
-		State:    states.NewState(),
-	}).Build(addrs.RootModuleInstance)
-	diags = diags.Append(moreDiags)
-	if moreDiags.HasErrors() {
-		return diags
-	}
-
 	// Validate is to check if the given module is valid regardless of
 	// input values, current state, etc. Therefore we populate all of the
 	// input values with unknown values of the expected type, allowing us
@@ -66,9 +55,20 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics {
 		}
 	}
 
-	walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{
+	graph, moreDiags := ValidateGraphBuilder(&PlanGraphBuilder{
 		Config:             config,
+		Plugins:            c.plugins,
+		Validate:           true,
+		State:              states.NewState(),
 		RootVariableValues: varValues,
+	}).Build(addrs.RootModuleInstance)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
+		return diags
+	}
+
+	walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{
+		Config: config,
 	})
 	diags = diags.Append(walker.NonFatalDiagnostics)
 	diags = diags.Append(walkDiags)

internal/terraform/context_walk.go

@@ -23,8 +23,7 @@ type graphWalkOpts struct {
 	Changes    *plans.Changes
 	Config     *configs.Config
 
-	RootVariableValues InputValues
+	MoveResults refactoring.MoveResults
-	MoveResults        refactoring.MoveResults
 }
 
 func (c *Context) walk(graph *Graph, operation walkOperation, opts *graphWalkOpts) (*ContextGraphWalker, tfdiags.Diagnostics) {
@@ -98,16 +97,15 @@ func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *Con
 	}
 
 	return &ContextGraphWalker{
-		Context:            c,
+		Context:          c,
-		State:              state,
+		State:            state,
-		Config:             opts.Config,
+		Config:           opts.Config,
-		RefreshState:       refreshState,
+		RefreshState:     refreshState,
-		PrevRunState:       prevRunState,
+		PrevRunState:     prevRunState,
-		Changes:            changes.SyncWrapper(),
+		Changes:          changes.SyncWrapper(),
-		InstanceExpander:   instances.NewExpander(),
+		InstanceExpander: instances.NewExpander(),
-		MoveResults:        opts.MoveResults,
+		MoveResults:      opts.MoveResults,
-		Operation:          operation,
+		Operation:        operation,
-		StopContext:        c.runContext,
+		StopContext:      c.runContext,
-		RootVariableValues: opts.RootVariableValues,
 	}
 }

internal/terraform/eval_context.go

@@ -121,12 +121,24 @@ type EvalContext interface {
 	// addresses in this context.
 	EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope
 
-	// SetModuleCallArguments defines values for the variables of a particular
+	// SetRootModuleArgument defines the value for one variable of the root
-	// child module call.
+	// module. The caller must ensure that given value is a suitable
+	// "final value" for the variable, which means that it's already converted
+	// and validated to match any configured constraints and validation rules.
 	//
-	// Calling this function multiple times has merging behavior, keeping any
+	// Calling this function multiple times with the same variable address
-	// previously-set keys that are not present in the new map.
+	// will silently overwrite the value provided by a previous call.
-	SetModuleCallArguments(addrs.ModuleCallInstance, map[string]cty.Value)
+	SetRootModuleArgument(addrs.InputVariable, cty.Value)
+
+	// SetModuleCallArgument defines the value for one input variable of a
+	// particular child module call. The caller must ensure that the given
+	// value is a suitable "final value" for the variable, which means that
+	// it's already converted and validated to match any configured
+	// constraints and validation rules.
+	//
+	// Calling this function multiple times with the same variable address
+	// will silently overwrite the value provided by a previous call.
+	SetModuleCallArgument(addrs.ModuleCallInstance, addrs.InputVariable, cty.Value)
 
 	// GetVariableValue returns the value provided for the input variable with
 	// the given address, or cty.DynamicVal if the variable hasn't been assigned

internal/terraform/eval_context_builtin.go

@@ -313,7 +313,21 @@ func (ctx *BuiltinEvalContext) Path() addrs.ModuleInstance {
 	return ctx.PathValue
 }
 
-func (ctx *BuiltinEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance, vals map[string]cty.Value) {
+func (ctx *BuiltinEvalContext) SetRootModuleArgument(addr addrs.InputVariable, v cty.Value) {
+	ctx.VariableValuesLock.Lock()
+	defer ctx.VariableValuesLock.Unlock()
+
+	log.Printf("[TRACE] BuiltinEvalContext: Storing final value for variable %s", addr.Absolute(addrs.RootModuleInstance))
+	key := addrs.RootModuleInstance.String()
+	args := ctx.VariableValues[key]
+	if args == nil {
+		args = make(map[string]cty.Value)
+		ctx.VariableValues[key] = args
+	}
+	args[addr.Name] = v
+}
+
+func (ctx *BuiltinEvalContext) SetModuleCallArgument(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value) {
 	ctx.VariableValuesLock.Lock()
 	defer ctx.VariableValuesLock.Unlock()
 
@@ -321,18 +335,15 @@ func (ctx *BuiltinEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance
 		panic("context path not set")
 	}
 
-	childPath := n.ModuleInstance(ctx.PathValue)
+	childPath := callAddr.ModuleInstance(ctx.PathValue)
+	log.Printf("[TRACE] BuiltinEvalContext: Storing final value for variable %s", varAddr.Absolute(childPath))
 	key := childPath.String()
-
 	args := ctx.VariableValues[key]
 	if args == nil {
-		ctx.VariableValues[key] = vals
+		args = make(map[string]cty.Value)
-		return
+		ctx.VariableValues[key] = args
-	}
-
-	for k, v := range vals {
-		args[k] = v
 	}
+	args[varAddr.Name] = v
 }
 
 func (ctx *BuiltinEvalContext) GetVariableValue(addr addrs.AbsInputVariableInstance) cty.Value {

internal/terraform/eval_context_mock.go

@@ -111,13 +111,21 @@ type MockEvalContext struct {
 	PathCalled bool
 	PathPath   addrs.ModuleInstance
 
-	SetModuleCallArgumentsCalled bool
+	SetRootModuleArgumentCalled bool
-	SetModuleCallArgumentsModule addrs.ModuleCallInstance
+	SetRootModuleArgumentAddr   addrs.InputVariable
-	SetModuleCallArgumentsValues map[string]cty.Value
+	SetRootModuleArgumentValue  cty.Value
+	SetRootModuleArgumentFunc   func(addr addrs.InputVariable, v cty.Value)
+
+	SetModuleCallArgumentCalled     bool
+	SetModuleCallArgumentModuleCall addrs.ModuleCallInstance
+	SetModuleCallArgumentVariable   addrs.InputVariable
+	SetModuleCallArgumentValue      cty.Value
+	SetModuleCallArgumentFunc       func(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value)
 
 	GetVariableValueCalled bool
 	GetVariableValueAddr   addrs.AbsInputVariableInstance
 	GetVariableValueValue  cty.Value
+	GetVariableValueFunc   func(addr addrs.AbsInputVariableInstance) cty.Value // supersedes GetVariableValueValue
 
 	ChangesCalled  bool
 	ChangesChanges *plans.ChangesSync
@@ -321,15 +329,31 @@ func (c *MockEvalContext) Path() addrs.ModuleInstance {
 	return c.PathPath
 }
 
-func (c *MockEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance, values map[string]cty.Value) {
+func (c *MockEvalContext) SetRootModuleArgument(addr addrs.InputVariable, v cty.Value) {
-	c.SetModuleCallArgumentsCalled = true
+	c.SetRootModuleArgumentCalled = true
-	c.SetModuleCallArgumentsModule = n
+	c.SetRootModuleArgumentAddr = addr
-	c.SetModuleCallArgumentsValues = values
+	c.SetRootModuleArgumentValue = v
+	if c.SetRootModuleArgumentFunc != nil {
+		c.SetRootModuleArgumentFunc(addr, v)
+	}
+}
+
+func (c *MockEvalContext) SetModuleCallArgument(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value) {
+	c.SetModuleCallArgumentCalled = true
+	c.SetModuleCallArgumentModuleCall = callAddr
+	c.SetModuleCallArgumentVariable = varAddr
+	c.SetModuleCallArgumentValue = v
+	if c.SetModuleCallArgumentFunc != nil {
+		c.SetModuleCallArgumentFunc(callAddr, varAddr, v)
+	}
 }
 
 func (c *MockEvalContext) GetVariableValue(addr addrs.AbsInputVariableInstance) cty.Value {
 	c.GetVariableValueCalled = true
 	c.GetVariableValueAddr = addr
+	if c.GetVariableValueFunc != nil {
+		return c.GetVariableValueFunc(addr)
+	}
 	return c.GetVariableValueValue
 }
 

internal/terraform/eval_variable.go

@@ -12,6 +12,102 @@ import (
 	"github.com/zclconf/go-cty/cty/convert"
 )
 
+func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given cty.Value, valRange tfdiags.SourceRange, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) {
+	var diags tfdiags.Diagnostics
+
+	convertTy := cfg.ConstraintType
+	log.Printf("[TRACE] prepareFinalInputVariableValue: preparing %s", addr)
+
+	var defaultVal cty.Value
+	if cfg.Default != cty.NilVal {
+		log.Printf("[TRACE] prepareFinalInputVariableValue: %s has a default value", addr)
+		var err error
+		defaultVal, err = convert.Convert(cfg.Default, convertTy)
+		if err != nil {
+			// Validation of the declaration should typically catch this,
+			// but we'll check it here too to be robust.
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid default value for module argument",
+				Detail: fmt.Sprintf(
+					"The default value for variable %q is incompatible with its type constraint: %s.",
+					cfg.Name, err,
+				),
+				Subject: &cfg.DeclRange,
+			})
+			// We'll return a placeholder unknown value to avoid producing
+			// redundant downstream errors.
+			return cty.UnknownVal(cfg.Type), diags
+		}
+	}
+
+	if given == cty.NilVal { // The variable wasn't set at all (even to null)
+		log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr)
+		if cfg.Required() {
+			// NOTE: The CLI layer typically checks for itself whether all of
+			// the required _root_ module variables are not set, which would
+			// mask this error. We can get here for child module variables,
+			// though.
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  `Required variable not set`,
+				Detail:   fmt.Sprintf(`The variable %q is required, but is not set.`, addr.Variable.Name),
+				Subject:  valRange.ToHCL().Ptr(),
+			})
+			// We'll return a placeholder unknown value to avoid producing
+			// redundant downstream errors.
+			return cty.UnknownVal(cfg.Type), diags
+		}
+
+		given = defaultVal // must be set, because we checked above that the variable isn't required
+	}
+
+	val, err := convert.Convert(given, convertTy)
+	if err != 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.",
+				cfg.Name, cfg.DeclRange.String(), err,
+			),
+			Subject: valRange.ToHCL().Ptr(),
+		})
+		// We'll return a placeholder unknown value to avoid producing
+		// redundant downstream errors.
+		return cty.UnknownVal(cfg.Type), diags
+	}
+
+	// By the time we get here, we know:
+	// - val matches the variable's type constraint
+	// - val is definitely not cty.NilVal, but might be a null value if the given was already null.
+	//
+	// That means we just need to handle the case where the value is null,
+	// which might mean we need to use the default value, or produce an error.
+	//
+	// For historical reasons we do this only for a "non-nullable" variable.
+	// Nullable variables just appear as null if they were set to null,
+	// regardless of any default value.
+	if val.IsNull() && !cfg.Nullable {
+		log.Printf("[TRACE] prepareFinalInputVariableValue: %s is defined as null", addr)
+		if defaultVal != cty.NilVal {
+			val = defaultVal
+		} else {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  `Required variable not set`,
+				Detail:   fmt.Sprintf(`The variable %q is required, but the given value is null.`, addr.Variable.Name),
+				Subject:  valRange.ToHCL().Ptr(),
+			})
+			// Stub out our return value so that the semantic checker doesn't
+			// produce redundant downstream errors.
+			val = cty.UnknownVal(cfg.Type)
+		}
+	}
+
+	return val, diags
+}
+
 // evalVariableValidations ensures that all of the configured custom validations
 // for a variable are passing.
 //
@@ -20,9 +116,10 @@ import (
 // EvalModuleCallArgument for variables in descendent modules.
 func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *configs.Variable, expr hcl.Expression, ctx EvalContext) (diags tfdiags.Diagnostics) {
 	if config == nil || len(config.Validations) == 0 {
-		log.Printf("[TRACE] evalVariableValidations: not active for %s, so skipping", addr)
+		log.Printf("[TRACE] evalVariableValidations: no validation rules declared for %s, so skipping", addr)
 		return nil
 	}
+	log.Printf("[TRACE] evalVariableValidations: validating %s", addr)
 
 	// Variable nodes evaluate in the parent module to where they were declared
 	// because the value expression (n.Expr, if set) comes from the calling
@@ -34,6 +131,14 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config
 	// 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(addr)
+	if val == cty.NilVal {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  "No final value for variable",
+			Detail:   fmt.Sprintf("Terraform doesn't have a final value for %s during validation. This is a bug in Terraform; please report it!", addr),
+		})
+		return diags
+	}
 	hclCtx := &hcl.EvalContext{
 		Variables: map[string]cty.Value{
 			"var": cty.ObjectVal(map[string]cty.Value{

internal/terraform/eval_variable_test.go

@@ -0,0 +1,426 @@
+package terraform
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/zclconf/go-cty/cty"
+
+	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/tfdiags"
+)
+
+func TestPrepareFinalInputVariableValue(t *testing.T) {
+	// This is just a concise way to define a bunch of *configs.Variable
+	// objects to use in our tests below. We're only going to decode this
+	// config, not fully evaluate it.
+	cfgSrc := `
+		variable "nullable_required" {
+		}
+		variable "nullable_optional_default_string" {
+			default = "hello"
+		}
+		variable "nullable_optional_default_null" {
+			default = null
+		}
+		variable "constrained_string_nullable_required" {
+			type = string
+		}
+		variable "constrained_string_nullable_optional_default_string" {
+			type    = string
+			default = "hello"
+		}
+		variable "constrained_string_nullable_optional_default_bool" {
+			type    = string
+			default = true
+		}
+		variable "constrained_string_nullable_optional_default_null" {
+			type    = string
+			default = null
+		}
+		variable "required" {
+			nullable = false
+		}
+		variable "optional_default_string" {
+			nullable = false
+			default  = "hello"
+		}
+		variable "constrained_string_required" {
+			nullable = false
+			type     = string
+		}
+		variable "constrained_string_optional_default_string" {
+			nullable = false
+			type     = string
+			default  = "hello"
+		}
+		variable "constrained_string_optional_default_bool" {
+			nullable = false
+			type     = string
+			default  = true
+		}
+	`
+	cfg := testModuleInline(t, map[string]string{
+		"main.tf": cfgSrc,
+	})
+	variableConfigs := cfg.Module.Variables
+
+	tests := []struct {
+		varName string
+		given   cty.Value
+		want    cty.Value
+		wantErr string
+	}{
+		// nullable_required
+		{
+			"nullable_required",
+			cty.NilVal,
+			cty.UnknownVal(cty.DynamicPseudoType),
+			`Required variable not set: The variable "nullable_required" is required, but is not set.`,
+		},
+		{
+			"nullable_required",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.DynamicPseudoType),
+			``, // "required" for a nullable variable means only that it must be set, even if it's set to null
+		},
+		{
+			"nullable_required",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"nullable_required",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// nullable_optional_default_string
+		{
+			"nullable_optional_default_string",
+			cty.NilVal,
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"nullable_optional_default_string",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.DynamicPseudoType), // nullable variables can be really set to null, masking the default
+			``,
+		},
+		{
+			"nullable_optional_default_string",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"nullable_optional_default_string",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// nullable_optional_default_null
+		{
+			"nullable_optional_default_null",
+			cty.NilVal,
+			cty.NullVal(cty.DynamicPseudoType), // the declared default value
+			``,
+		},
+		{
+			"nullable_optional_default_null",
+			cty.NullVal(cty.String),
+			cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default
+			``,
+		},
+		{
+			"nullable_optional_default_null",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"nullable_optional_default_null",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_nullable_required
+		{
+			"constrained_string_nullable_required",
+			cty.NilVal,
+			cty.UnknownVal(cty.String),
+			`Required variable not set: The variable "constrained_string_nullable_required" is required, but is not set.`,
+		},
+		{
+			"constrained_string_nullable_required",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.String), // the null value still gets converted to match the type constraint
+			``,                      // "required" for a nullable variable means only that it must be set, even if it's set to null
+		},
+		{
+			"constrained_string_nullable_required",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_nullable_required",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_nullable_optional_default_string
+		{
+			"constrained_string_nullable_optional_default_string",
+			cty.NilVal,
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_string",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_string",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_string",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_nullable_optional_default_bool
+		{
+			"constrained_string_nullable_optional_default_bool",
+			cty.NilVal,
+			cty.StringVal("true"), // the declared default value, automatically converted to match type constraint
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_bool",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_bool",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_bool",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_nullable_optional_default_null
+		{
+			"constrained_string_nullable_optional_default_null",
+			cty.NilVal,
+			cty.NullVal(cty.String),
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_null",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.NullVal(cty.String),
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_null",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_nullable_optional_default_null",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// required
+		{
+			"required",
+			cty.NilVal,
+			cty.UnknownVal(cty.DynamicPseudoType),
+			`Required variable not set: The variable "required" is required, but is not set.`,
+		},
+		{
+			"required",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.UnknownVal(cty.DynamicPseudoType),
+			`Required variable not set: The variable "required" is required, but the given value is null.`,
+		},
+		{
+			"required",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"required",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// optional_default_string
+		{
+			"optional_default_string",
+			cty.NilVal,
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"optional_default_string",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"optional_default_string",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"optional_default_string",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_required
+		{
+			"constrained_string_required",
+			cty.NilVal,
+			cty.UnknownVal(cty.String),
+			`Required variable not set: The variable "constrained_string_required" is required, but is not set.`,
+		},
+		{
+			"constrained_string_required",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.UnknownVal(cty.String),
+			`Required variable not set: The variable "constrained_string_required" is required, but the given value is null.`,
+		},
+		{
+			"constrained_string_required",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_required",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_optional_default_string
+		{
+			"constrained_string_optional_default_string",
+			cty.NilVal,
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"constrained_string_optional_default_string",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.StringVal("hello"), // the declared default value
+			``,
+		},
+		{
+			"constrained_string_optional_default_string",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_optional_default_string",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+
+		// constrained_string_optional_default_bool
+		{
+			"constrained_string_optional_default_bool",
+			cty.NilVal,
+			cty.StringVal("true"), // the declared default value, automatically converted to match type constraint
+			``,
+		},
+		{
+			"constrained_string_optional_default_bool",
+			cty.NullVal(cty.DynamicPseudoType),
+			cty.StringVal("true"), // the declared default value, automatically converted to match type constraint
+			``,
+		},
+		{
+			"constrained_string_optional_default_bool",
+			cty.StringVal("ahoy"),
+			cty.StringVal("ahoy"),
+			``,
+		},
+		{
+			"constrained_string_optional_default_bool",
+			cty.UnknownVal(cty.String),
+			cty.UnknownVal(cty.String),
+			``,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%s %#v", test.varName, test.given), func(t *testing.T) {
+			varAddr := addrs.InputVariable{Name: test.varName}.Absolute(addrs.RootModuleInstance)
+			varCfg := variableConfigs[test.varName]
+			if varCfg == nil {
+				t.Fatalf("invalid variable name %q", test.varName)
+			}
+
+			t.Logf(
+				"test case\nvariable:    %s\nconstraint:  %#v\ndefault:     %#v\nnullable:    %#v\ngiven value: %#v",
+				varAddr,
+				varCfg.Type,
+				varCfg.Default,
+				varCfg.Nullable,
+				test.given,
+			)
+
+			got, diags := prepareFinalInputVariableValue(
+				varAddr, test.given, tfdiags.SourceRangeFromHCL(varCfg.DeclRange), varCfg,
+			)
+
+			if test.wantErr != "" {
+				if !diags.HasErrors() {
+					t.Errorf("unexpected success\nwant error: %s", test.wantErr)
+				} else if got, want := diags.Err().Error(), test.wantErr; got != want {
+					t.Errorf("wrong error\ngot:  %s\nwant: %s", got, want)
+				}
+			} else {
+				if diags.HasErrors() {
+					t.Errorf("unexpected error\ngot: %s", diags.Err().Error())
+				}
+			}
+
+			// NOTE: should still have returned some reasonable value even if there was an error
+			if !test.want.RawEquals(got) {
+				t.Fatalf("wrong result\ngot:  %#v\nwant: %#v", got, test.want)
+			}
+		})
+	}
+}

internal/terraform/evaluate.go

@@ -10,7 +10,6 @@ import (
 	"github.com/agext/levenshtein"
 	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty/cty"
-	"github.com/zclconf/go-cty/cty/convert"
 
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
@@ -248,7 +247,7 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd
 	// This is important because otherwise the validation walk will tend to be
 	// overly strict, requiring expressions throughout the configuration to
 	// be complicated to accommodate all possible inputs, whereas returning
-	// known here allows for simpler patterns like using input values as
+	// unknown here allows for simpler patterns like using input values as
 	// guards to broadly enable/disable resources, avoid processing things
 	// that are disabled, etc. Terraform's static validation leans towards
 	// being liberal in what it accepts because the subsequent plan walk has
@@ -267,28 +266,27 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd
 		return cty.UnknownVal(config.Type), diags
 	}
 
+	// d.Evaluator.VariableValues should always contain valid "final values"
+	// for variables, which is to say that they have already had type
+	// conversions, validations, and default value handling applied to them.
+	// Those are the responsibility of the graph notes representing the
+	// variable declarations. Therefore here we just trust that we already
+	// have a correct value.
+
 	val, isSet := vals[addr.Name]
-	switch {
+	if !isSet {
-	case !isSet:
+		// We should not be able to get here without having a valid value
-		// The config loader will ensure there is a default if the value is not
+		// for every variable, so this always indicates a bug in either
-		// set at all.
+		// the graph builder (not including all the needed nodes) or in
-		val = config.Default
+		// the graph nodes representing variables.
-
-	case val.IsNull() && !config.Nullable && config.Default != cty.NilVal:
-		// If nullable=false a null value will use the configured default.
-		val = config.Default
-	}
-
-	var err error
-	val, err = convert.Convert(val, config.ConstraintType)
-	if err != nil {
-		// We should never get here because this problem should've been caught
-		// during earlier validation, but we'll do something reasonable anyway.
 		diags = diags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,
-			Summary:  `Incorrect variable type`,
+			Summary:  `Reference to unresolved input variable`,
-			Detail:   fmt.Sprintf(`The resolved value of variable %q is not appropriate: %s.`, addr.Name, err),
+			Detail: fmt.Sprintf(
-			Subject:  &config.DeclRange,
+				`The final value for %s is missing in Terraform's evaluation context. This is a bug in Terraform; please report it!`,
+				addr.Absolute(d.ModulePath),
+			),
+			Subject: rng.ToHCL().Ptr(),
 		})
 		val = cty.UnknownVal(config.Type)
 	}

internal/terraform/graph_builder_apply.go

@@ -26,6 +26,11 @@ type ApplyGraphBuilder struct {
 	// State is the current state
 	State *states.State
 
+	// RootVariableValues are the root module input variables captured as
+	// part of the plan object, which we must reproduce in the apply step
+	// to get a consistent result.
+	RootVariableValues InputValues
+
 	// Plugins is a library of the plug-in components (providers and
 	// provisioners) available for use.
 	Plugins *contextPlugins
@@ -88,7 +93,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{Config: b.Config, Changes: b.Changes},

internal/terraform/graph_builder_destroy_plan.go

@@ -23,6 +23,11 @@ type DestroyPlanGraphBuilder struct {
 	// State is the current state
 	State *states.State
 
+	// RootVariableValues are the raw input values for root input variables
+	// given by the caller, which we'll resolve into final values as part
+	// of the plan walk.
+	RootVariableValues InputValues
+
 	// Plugins is a library of plug-in components (providers and
 	// provisioners) available for use.
 	Plugins *contextPlugins

internal/terraform/graph_builder_eval.go

@@ -30,6 +30,11 @@ type EvalGraphBuilder struct {
 	// State is the current state
 	State *states.State
 
+	// RootVariableValues are the raw input values for root input variables
+	// given by the caller, which we'll resolve into final values as part
+	// of the plan walk.
+	RootVariableValues InputValues
+
 	// Plugins is a library of plug-in components (providers and
 	// provisioners) available for use.
 	Plugins *contextPlugins
@@ -60,7 +65,7 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer {
 		},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{Config: b.Config},

internal/terraform/graph_builder_import.go

@@ -17,6 +17,11 @@ type ImportGraphBuilder struct {
 	// Module is a configuration to build the graph from. See ImportOpts.Config.
 	Config *configs.Config
 
+	// RootVariableValues are the raw input values for root input variables
+	// given by the caller, which we'll resolve into final values as part
+	// of the plan walk.
+	RootVariableValues InputValues
+
 	// Plugins is a library of plug-in components (providers and
 	// provisioners) available for use.
 	Plugins *contextPlugins
@@ -53,7 +58,7 @@ func (b *ImportGraphBuilder) Steps() []GraphTransformer {
 		&ConfigTransformer{Config: config},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{Config: b.Config},

internal/terraform/graph_builder_plan.go

@@ -28,6 +28,11 @@ type PlanGraphBuilder struct {
 	// State is the current state
 	State *states.State
 
+	// RootVariableValues are the raw input values for root input variables
+	// given by the caller, which we'll resolve into final values as part
+	// of the plan walk.
+	RootVariableValues InputValues
+
 	// Plugins is a library of plug-in components (providers and
 	// provisioners) available for use.
 	Plugins *contextPlugins
@@ -95,7 +100,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		},
 
 		// Add dynamic values
-		&RootVariableTransformer{Config: b.Config},
+		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
 		&ModuleVariableTransformer{Config: b.Config},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{Config: b.Config},

internal/terraform/node_module_variable.go

@@ -12,7 +12,6 @@ import (
 	"github.com/hashicorp/terraform/internal/lang"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 	"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
@@ -143,35 +142,27 @@ func (n *nodeModuleVariable) ModulePath() addrs.Module {
 
 // GraphNodeExecutable
 func (n *nodeModuleVariable) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
-	// If we have no value, do nothing
+	log.Printf("[TRACE] nodeModuleVariable: evaluating %s", n.Addr)
-	if n.Expr == nil {
-		return nil
-	}
 
-	// Otherwise, interpolate the value of this variable and set it
+	var val cty.Value
-	// within the variables mapping.
-	var vals map[string]cty.Value
 	var err error
 
 	switch op {
 	case walkValidate:
-		vals, err = n.evalModuleCallArgument(ctx, true)
+		val, err = n.evalModuleCallArgument(ctx, true)
 		diags = diags.Append(err)
-		if diags.HasErrors() {
-			return diags
-		}
 	default:
-		vals, err = n.evalModuleCallArgument(ctx, false)
+		val, err = n.evalModuleCallArgument(ctx, false)
 		diags = diags.Append(err)
-		if diags.HasErrors() {
+	}
-			return diags
+	if diags.HasErrors() {
-		}
+		return diags
 	}
 
 	// Set values for arguments of a child module call, for later retrieval
 	// during expression evaluation.
 	_, call := n.Addr.Module.CallInstance()
-	ctx.SetModuleCallArguments(call, vals)
+	ctx.SetModuleCallArgument(call, n.Addr.Variable, val)
 
 	return evalVariableValidations(n.Addr, n.Config, n.Expr, ctx)
 }
@@ -199,77 +190,45 @@ func (n *nodeModuleVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNod
 // 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) {
+func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnly bool) (cty.Value, error) {
-	name := n.Addr.Variable.Name
+	var diags tfdiags.Diagnostics
-	expr := n.Expr
+	var givenVal cty.Value
+	var errSourceRange tfdiags.SourceRange
+	if expr := n.Expr; expr != nil {
+		var moduleInstanceRepetitionData instances.RepetitionData
 
-	if expr == nil {
+		switch {
-		// Should never happen, but we'll bail out early here rather than
+		case validateOnly:
-		// crash in case it does. We set no value at all in this case,
+			// the instance expander does not track unknown expansion values, so we
-		// making a subsequent call to EvalContext.SetModuleCallArguments
+			// have to assume all RepetitionData is unknown.
-		// a no-op.
+			moduleInstanceRepetitionData = instances.RepetitionData{
-		log.Printf("[ERROR] attempt to evaluate %s with nil expression", n.Addr.String())
+				CountIndex: cty.UnknownVal(cty.Number),
-		return nil, nil
+				EachKey:    cty.UnknownVal(cty.String),
-	}
+				EachValue:  cty.DynamicVal,
+			}
 
-	var moduleInstanceRepetitionData instances.RepetitionData
+		default:
-
+			// Get the repetition data for this module instance,
-	switch {
+			// so we can create the appropriate scope for evaluating our expression
-	case validateOnly:
+			moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance)
-		// 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:
+		scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData)
-		// Get the repetition data for this module instance,
+		val, moreDiags := scope.EvalExpr(expr, cty.DynamicPseudoType)
-		// so we can create the appropriate scope for evaluating our expression
+		diags = diags.Append(moreDiags)
-		moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance)
+		if moreDiags.HasErrors() {
+			return cty.DynamicVal, diags.ErrWithWarnings()
+		}
+		givenVal = val
+		errSourceRange = tfdiags.SourceRangeFromHCL(expr.Range())
+	} else {
+		// We'll use cty.NilVal to represent the variable not being set at all.
+		givenVal = cty.NilVal
+		errSourceRange = tfdiags.SourceRangeFromHCL(n.Config.DeclRange) // we use the declaration range as a fallback for an undefined variable
 	}
 
-	scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData)
+	finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, givenVal, errSourceRange, n.Config)
-	val, diags := scope.EvalExpr(expr, cty.DynamicPseudoType)
+	diags = diags.Append(moreDiags)
 
-	// We intentionally passed DynamicPseudoType to EvalExpr above because
+	return finalVal, diags.ErrWithWarnings()
-	// 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, n.Config.ConstraintType)
-	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(n.Config.Type)
-	}
-
-	// If there is no default, we have to ensure that a null value is allowed
-	// for this variable.
-	if n.Config.Default == cty.NilVal && !n.Config.Nullable && val.IsNull() {
-		// The value cannot be null, and there is no configured default.
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  `Invalid variable value`,
-			Detail:   fmt.Sprintf(`The variable %q is required, but the given value is null.`, n.Addr),
-			Subject:  &n.Config.DeclRange,
-		})
-		// Stub out our return value so that the semantic checker doesn't
-		// produce redundant downstream errors.
-		val = cty.UnknownVal(n.Config.Type)
-	}
-
-	vals := make(map[string]cty.Value)
-	vals[name] = val
-
-	return vals, diags.ErrWithWarnings()
 }

internal/terraform/node_root_variable.go

@@ -1,16 +1,26 @@
 package terraform
 
 import (
+	"log"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/dag"
 	"github.com/hashicorp/terraform/internal/tfdiags"
+	"github.com/zclconf/go-cty/cty"
 )
 
 // NodeRootVariable represents a root variable input.
 type NodeRootVariable struct {
 	Addr   addrs.InputVariable
 	Config *configs.Variable
+
+	// RawValue is the value for the variable set from outside Terraform
+	// Core, such as on the command line, or from an environment variable,
+	// or similar. This is the raw value that was provided, not yet
+	// converted or validated, and can be nil for a variable that isn't
+	// set at all.
+	RawValue *InputValue
 }
 
 var (
@@ -38,21 +48,56 @@ func (n *NodeRootVariable) ReferenceableAddrs() []addrs.Referenceable {
 
 // GraphNodeExecutable
 func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics {
-	// We don't actually need to _evaluate_ a root module variable, because
+	// Root module variables are special in that they are provided directly
-	// its value is always constant and already stashed away in our EvalContext.
+	// by the caller (usually, the CLI layer) and so we don't really need to
-	// However, we might need to run some user-defined validation rules against
+	// evaluate them in the usual sense, but we do need to process the raw
-	// the value.
+	// values given by the caller to match what the module is expecting, and
+	// make sure the values are valid.
+	var diags tfdiags.Diagnostics
 
-	if n.Config == nil || len(n.Config.Validations) == 0 {
+	addr := addrs.RootModuleInstance.InputVariable(n.Addr.Name)
-		return nil // nothing to do
+	log.Printf("[TRACE] NodeRootVariable: evaluating %s", addr)
+
+	if n.Config == nil {
+		// Because we build NodeRootVariable from configuration in the normal
+		// case it's strange to get here, but we tolerate it to allow for
+		// tests that might not populate the inputs fully.
+		return nil
 	}
 
-	return evalVariableValidations(
+	var givenVal cty.Value
+	if n.RawValue != nil {
+		givenVal = n.RawValue.Value
+	} else {
+		// We'll use cty.NilVal to represent the variable not being set at
+		// all, which for historical reasons is unfortunately different than
+		// explicitly setting it to null in some cases.
+		givenVal = cty.NilVal
+	}
+
+	finalVal, moreDiags := prepareFinalInputVariableValue(
+		addr,
+		givenVal,
+		tfdiags.SourceRangeFromHCL(n.Config.DeclRange),
+		n.Config,
+	)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
+		// No point in proceeding to validations then, because they'll
+		// probably fail trying to work with a value of the wrong type.
+		return diags
+	}
+
+	ctx.SetRootModuleArgument(addr.Variable, finalVal)
+
+	moreDiags = evalVariableValidations(
 		addrs.RootModuleInstance.InputVariable(n.Addr.Name),
 		n.Config,
 		nil, // not set for root module variables
 		ctx,
 	)
+	diags = diags.Append(moreDiags)
+	return diags
 }
 
 // dag.GraphNodeDotter impl.

internal/terraform/node_root_variable_test.go

@@ -3,26 +3,164 @@ package terraform
 import (
 	"testing"
 
+	"github.com/hashicorp/hcl/v2"
+	"github.com/zclconf/go-cty/cty"
+
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
-	"github.com/zclconf/go-cty/cty"
+	"github.com/hashicorp/terraform/internal/lang"
 )
 
 func TestNodeRootVariableExecute(t *testing.T) {
-	ctx := new(MockEvalContext)
+	t.Run("type conversion", func(t *testing.T) {
+		ctx := new(MockEvalContext)
 
-	n := &NodeRootVariable{
+		n := &NodeRootVariable{
-		Addr: addrs.InputVariable{Name: "foo"},
+			Addr: addrs.InputVariable{Name: "foo"},
-		Config: &configs.Variable{
+			Config: &configs.Variable{
-			Name:           "foo",
+				Name:           "foo",
-			Type:           cty.String,
+				Type:           cty.String,
-			ConstraintType: cty.String,
+				ConstraintType: cty.String,
-		},
+			},
-	}
+			RawValue: &InputValue{
+				Value:      cty.True,
+				SourceType: ValueFromUnknown,
+			},
+		}
 
-	diags := n.Execute(ctx, walkApply)
+		diags := n.Execute(ctx, walkApply)
-	if diags.HasErrors() {
+		if diags.HasErrors() {
-		t.Fatalf("unexpected error: %s", diags.Err())
+			t.Fatalf("unexpected error: %s", diags.Err())
-	}
+		}
 
+		if !ctx.SetRootModuleArgumentCalled {
+			t.Fatalf("ctx.SetRootModuleArgument wasn't called")
+		}
+		if got, want := ctx.SetRootModuleArgumentAddr.String(), "var.foo"; got != want {
+			t.Errorf("wrong address for ctx.SetRootModuleArgument\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := ctx.SetRootModuleArgumentValue, cty.StringVal("true"); !want.RawEquals(got) {
+			// NOTE: The given value was cty.Bool but the type constraint was
+			// cty.String, so it was NodeRootVariable's responsibility to convert
+			// as part of preparing the "final value".
+			t.Errorf("wrong value for ctx.SetRootModuleArgument\ngot:  %#v\nwant: %#v", got, want)
+		}
+	})
+	t.Run("validation", func(t *testing.T) {
+		ctx := new(MockEvalContext)
+
+		// The variable validation function gets called with Terraform's
+		// built-in functions available, so we need a minimal scope just for
+		// it to get the functions from.
+		ctx.EvaluationScopeScope = &lang.Scope{}
+
+		// We need to reimplement a _little_ bit of EvalContextBuiltin logic
+		// here to get a similar effect with EvalContextMock just to get the
+		// value to flow through here in a realistic way that'll make this test
+		// useful.
+		var finalVal cty.Value
+		ctx.SetRootModuleArgumentFunc = func(addr addrs.InputVariable, v cty.Value) {
+			if addr.Name == "foo" {
+				t.Logf("set %s to %#v", addr.String(), v)
+				finalVal = v
+			}
+		}
+		ctx.GetVariableValueFunc = func(addr addrs.AbsInputVariableInstance) cty.Value {
+			if addr.String() != "var.foo" {
+				return cty.NilVal
+			}
+			t.Logf("reading final val for %s (%#v)", addr.String(), finalVal)
+			return finalVal
+		}
+
+		n := &NodeRootVariable{
+			Addr: addrs.InputVariable{Name: "foo"},
+			Config: &configs.Variable{
+				Name:           "foo",
+				Type:           cty.Number,
+				ConstraintType: cty.Number,
+				Validations: []*configs.VariableValidation{
+					{
+						Condition: fakeHCLExpressionFunc(func(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
+							// This returns true only if the given variable value
+							// is exactly cty.Number, which allows us to verify
+							// that we were given the value _after_ type
+							// conversion.
+							// This had previously not been handled correctly,
+							// as reported in:
+							//     https://github.com/hashicorp/terraform/issues/29899
+							vars := ctx.Variables["var"]
+							if vars == cty.NilVal || !vars.Type().IsObjectType() || !vars.Type().HasAttribute("foo") {
+								t.Logf("var.foo isn't available")
+								return cty.False, nil
+							}
+							val := vars.GetAttr("foo")
+							if val == cty.NilVal || val.Type() != cty.Number {
+								t.Logf("var.foo is %#v; want a number", val)
+								return cty.False, nil
+							}
+							return cty.True, nil
+						}),
+						ErrorMessage: "Must be a number.",
+					},
+				},
+			},
+			RawValue: &InputValue{
+				// Note: This is a string, but the variable's type constraint
+				// is number so it should be converted before use.
+				Value:      cty.StringVal("5"),
+				SourceType: ValueFromUnknown,
+			},
+		}
+
+		diags := n.Execute(ctx, walkApply)
+		if diags.HasErrors() {
+			t.Fatalf("unexpected error: %s", diags.Err())
+		}
+
+		if !ctx.SetRootModuleArgumentCalled {
+			t.Fatalf("ctx.SetRootModuleArgument wasn't called")
+		}
+		if got, want := ctx.SetRootModuleArgumentAddr.String(), "var.foo"; got != want {
+			t.Errorf("wrong address for ctx.SetRootModuleArgument\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := ctx.SetRootModuleArgumentValue, cty.NumberIntVal(5); !want.RawEquals(got) {
+			// NOTE: The given value was cty.Bool but the type constraint was
+			// cty.String, so it was NodeRootVariable's responsibility to convert
+			// as part of preparing the "final value".
+			t.Errorf("wrong value for ctx.SetRootModuleArgument\ngot:  %#v\nwant: %#v", got, want)
+		}
+	})
+}
+
+// fakeHCLExpressionFunc is a fake implementation of hcl.Expression that just
+// directly produces a value with direct Go code.
+//
+// An expression of this type has no references and so it cannot access any
+// variables from the EvalContext unless something else arranges for them
+// to be guaranteed available. For example, custom variable validations just
+// unconditionally have access to the variable they are validating regardless
+// of references.
+type fakeHCLExpressionFunc func(*hcl.EvalContext) (cty.Value, hcl.Diagnostics)
+
+var _ hcl.Expression = fakeHCLExpressionFunc(nil)
+
+func (f fakeHCLExpressionFunc) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
+	return f(ctx)
+}
+
+func (f fakeHCLExpressionFunc) Variables() []hcl.Traversal {
+	return nil
+}
+
+func (f fakeHCLExpressionFunc) Range() hcl.Range {
+	return hcl.Range{
+		Filename: "fake",
+		Start:    hcl.InitialPos,
+		End:      hcl.InitialPos,
+	}
+}
+
+func (f fakeHCLExpressionFunc) StartRange() hcl.Range {
+	return f.Range()
 }

internal/terraform/transform_variable.go

@@ -13,6 +13,8 @@ import (
 // reach them.
 type RootVariableTransformer struct {
 	Config *configs.Config
+
+	RawValues InputValues
 }
 
 func (t *RootVariableTransformer) Transform(g *Graph) error {
@@ -31,7 +33,8 @@ func (t *RootVariableTransformer) Transform(g *Graph) error {
 			Addr: addrs.InputVariable{
 				Name: v.Name,
 			},
-			Config: v,
+			Config:   v,
+			RawValue: t.RawValues[v.Name],
 		}
 		g.Add(node)
 	}