core and backend: remove redundant handling of default variable values

Previously we had three different layers all thinking they were
responsible for substituting a default value for an unset root module
variable:
 - the local backend, via logic in backend.ParseVariableValues
 - the context.Plan function (and other similar functions) trying to
   preprocess the input variables using
   terraform.mergeDefaultInputVariableValues .
 - the newer prepareFinalInputVariableValue, which aims to centralize all
   of the variable preparation logic so it can be common to both root and
   child module variables.

The second of these was also trying to handle type constraint checking,
which is also the responsibility of the central function and not something
we need to handle so early.

Only the last of these consistently handles both root and child module
variables, and so is the one we ought to keep. The others are now
redundant and are causing prepareFinalInputVariableValue to get a slightly
corrupted view of the caller's chosen variable values.

To rectify that, here we remove the two redundant layers altogether and
have unset root variables pass through as cty.NilVal all the way to the
central prepareFinalInputVariableValue function, which will then handle
them in a suitable way which properly respects the "nullable" setting.

This commit includes some test changes in the terraform package to make
those tests no longer rely on the mergeDefaultInputVariableValues logic
we've removed, and to instead explicitly set cty.NilVal for all unset
variables to comply with our intended contract for PlanOpts.SetVariables,
and similar. (This is so that we can more easily catch bugs in callers
where they _don't_ correctly handle input variables; it allows us to
distinguish between the caller explicitly marking a variable as unset vs.
not describing it at all, where the latter is a bug in the caller.)

internal/backend/unparsed_value.go

@@ -164,13 +164,18 @@ func ParseVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*
 
 	// By this point we should've gathered all of the required root module
 	// variables from one of the many possible sources. We'll now populate
-	// any we haven't gathered as their defaults and fail if any of the
+	// any we haven't gathered as unset placeholders which Terraform Core
-	// missing ones are required.
+	// can then react to.
 	for name, vc := range decls {
 		if isDefinedAny(name, ret, undeclared) {
 			continue
 		}
 
+		// This check is redundant with a check made in Terraform Core when
+		// processing undeclared variables, but allows us to generate a more
+		// specific error message which mentions -var and -var-file command
+		// line options, whereas the one in Terraform Core is more general
+		// due to supporting both root and child module variables.
 		if vc.Required() {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
@@ -189,8 +194,14 @@ func ParseVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*
 				SourceRange: tfdiags.SourceRangeFromHCL(vc.DeclRange),
 			}
 		} else {
+			// We're still required to put an entry for this variable
+			// in the mapping to be explicit to Terraform Core that we
+			// visited it, but its value will be cty.NilVal to represent
+			// that it wasn't set at all at this layer, and so Terraform Core
+			// should substitute a default if available, or generate an error
+			// if not.
 			ret[name] = &terraform.InputValue{
-				Value:       vc.Default,
+				Value:       cty.NilVal,
 				SourceType:  terraform.ValueFromConfig,
 				SourceRange: tfdiags.SourceRangeFromHCL(vc.DeclRange),
 			}

internal/backend/unparsed_value_test.go

@@ -204,7 +204,7 @@ func TestUnparsedValue(t *testing.T) {
 				},
 			},
 			"missing2": {
-				Value:      cty.StringVal("default for missing2"),
+				Value:      cty.NilVal, // Terraform Core handles substituting the default
 				SourceType: terraform.ValueFromConfig,
 				SourceRange: tfdiags.SourceRange{
 					Filename: "fake.tf",

internal/command/jsonplan/plan.go

@@ -118,7 +118,7 @@ func Marshal(
 	output := newPlan()
 	output.TerraformVersion = version.String()
 
-	err := output.marshalPlanVariables(p.VariableValues, schemas)
+	err := output.marshalPlanVariables(p.VariableValues, config.Module.Variables)
 	if err != nil {
 		return nil, fmt.Errorf("error in marshalPlanVariables: %s", err)
 	}
@@ -183,11 +183,7 @@ func Marshal(
 	return ret, err
 }
 
-func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, schemas *terraform.Schemas) error {
+func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, decls map[string]*configs.Variable) error {
-	if len(vars) == 0 {
-		return nil
-	}
-
 	p.Variables = make(variables, len(vars))
 
 	for k, v := range vars {
@@ -203,6 +199,41 @@ func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, schemas
 			Value: valJSON,
 		}
 	}
+
+	// In Terraform v1.1 and earlier we had some confusion about which subsystem
+	// of Terraform was the one responsible for substituting in default values
+	// for unset module variables, with root module variables being handled in
+	// three different places while child module variables were only handled
+	// during the Terraform Core graph walk.
+	//
+	// For Terraform v1.2 and later we rationalized that by having the Terraform
+	// Core graph walk always be responsible for selecting defaults regardless
+	// of root vs. child module, but unfortunately our earlier accidental
+	// misbehavior bled out into the public interface by making the defaults
+	// show up in the "vars" map to this function. Those are now correctly
+	// omitted (so that the plan file only records the variables _actually_
+	// set by the caller) but consumers of the JSON plan format may be depending
+	// on our old behavior and so we'll fake it here just in time so that
+	// outside consumers won't see a behavior change.
+	for name, decl := range decls {
+		if _, ok := p.Variables[name]; ok {
+			continue
+		}
+		if val := decl.Default; val != cty.NilVal {
+			valJSON, err := ctyjson.Marshal(val, val.Type())
+			if err != nil {
+				return err
+			}
+			p.Variables[name] = &variable{
+				Value: valJSON,
+			}
+		}
+	}
+
+	if len(p.Variables) == 0 {
+		p.Variables = nil // omit this property if there are no variables to describe
+	}
+
 	return nil
 }
 

internal/terraform/context_apply.go

@@ -87,6 +87,21 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate
 		return nil, walkApply, diags
 	}
 
+	// The plan.VariableValues field only records variables that were actually
+	// set by the caller in the PlanOpts, so we may need to provide
+	// placeholders for any other variables that the user didn't set, in
+	// which case Terraform will once again use the default value from the
+	// configuration when we visit these variables during the graph walk.
+	for name := range config.Module.Variables {
+		if _, ok := variables[name]; ok {
+			continue
+		}
+		variables[name] = &InputValue{
+			Value:      cty.NilVal,
+			SourceType: ValueFromPlan,
+		}
+	}
+
 	graph, moreDiags := (&ApplyGraphBuilder{
 		Config:             config,
 		Changes:            plan.Changes,

internal/terraform/context_apply2_test.go

@@ -426,7 +426,7 @@ resource "test_resource" "b" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	_, diags = ctx.Apply(plan, m)
@@ -530,14 +530,14 @@ resource "test_object" "y" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
 	assertNoErrors(t, diags)
 
 	// FINAL PLAN:
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	// make sure the same marks are compared in the next plan as well

internal/terraform/context_apply_test.go

@@ -517,7 +517,7 @@ func TestContext2Apply_mapVarBetweenModules(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2262,7 +2262,7 @@ func TestContext2Apply_countVariable(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2288,7 +2288,7 @@ func TestContext2Apply_countVariableRef(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2327,7 +2327,7 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) {
 		Provisioners: provisioners,
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	// We'll marshal and unmarshal the plan here, to ensure that we have
@@ -3682,7 +3682,7 @@ func TestContext2Apply_multiVarOrder(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -3713,7 +3713,7 @@ func TestContext2Apply_multiVarOrderInterp(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -4704,9 +4704,7 @@ func TestContext2Apply_provisionerDestroy(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
-		Mode: plans.DestroyMode,
-	})
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -4753,9 +4751,7 @@ func TestContext2Apply_provisionerDestroyFail(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
-		Mode: plans.DestroyMode,
-	})
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -5908,7 +5904,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) {
 		})
 
 		// First plan and apply a create operation
-		plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+		plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 		assertNoErrors(t, diags)
 
 		state, diags = ctx.Apply(plan, m)
@@ -5929,9 +5925,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) {
 		})
 
 		// First plan and apply a create operation
-		plan, diags := ctx.Plan(m, state, &PlanOpts{
+		plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
-			Mode: plans.DestroyMode,
-		})
 		if diags.HasErrors() {
 			t.Fatalf("destroy plan err: %s", diags.Err())
 		}
@@ -7561,6 +7555,12 @@ func TestContext2Apply_vars(t *testing.T) {
 			Value:      cty.StringVal("us-east-1"),
 			SourceType: ValueFromCaller,
 		},
+		"bar": &InputValue{
+			// This one is not explicitly set but that's okay because it
+			// has a declared default, which Terraform Core will use instead.
+			Value:      cty.NilVal,
+			SourceType: ValueFromCaller,
+		},
 		"test_list": &InputValue{
 			Value: cty.ListVal([]cty.Value{
 				cty.StringVal("Hello"),
@@ -7876,7 +7876,7 @@ func TestContext2Apply_issue7824(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -7932,7 +7932,7 @@ func TestContext2Apply_issue5254(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -7951,7 +7951,7 @@ func TestContext2Apply_issue5254(t *testing.T) {
 		},
 	})
 
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -8845,7 +8845,7 @@ func TestContext2Apply_plannedInterpolatedCount(t *testing.T) {
 		Providers: Providers,
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("plan failed: %s", diags.Err())
 	}
@@ -8904,9 +8904,7 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) {
 		Providers: providers,
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
-		Mode: plans.DestroyMode,
-	})
 	if diags.HasErrors() {
 		t.Fatalf("plan failed: %s", diags.Err())
 	}
@@ -9674,7 +9672,7 @@ func TestContext2Apply_plannedConnectionRefs(t *testing.T) {
 		Hooks:        []Hook{hook},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	diags.HasErrors()
 	if diags.HasErrors() {
 		t.Fatalf("diags: %s", diags.Err())
@@ -11687,7 +11685,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -11702,7 +11700,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -11720,6 +11718,7 @@ resource "test_resource" "foo" {
 	plan, diags = ctx.Plan(m, state, &PlanOpts{
 		Mode: plans.NormalMode,
 		SetVariables: InputValues{
+			"sensitive_id": &InputValue{Value: cty.NilVal},
 			"sensitive_var": &InputValue{
 				Value: cty.StringVal("bar"),
 			},
@@ -11759,7 +11758,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("plan errors: %s", diags.Err())
 	}
@@ -11904,7 +11903,7 @@ resource "test_resource" "foo" {
 		)
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	addr := mustResourceInstanceAddr("test_resource.foo")
@@ -11954,7 +11953,7 @@ resource "test_resource" "foo" {
 	// but this seems rather suspicious and we should ideally figure out what
 	// this test was originally intending to do and make it do that.
 	oldPlan := plan
-	_, diags = ctx2.Plan(m2, state, DefaultPlanOpts)
+	_, diags = ctx2.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 	stateWithoutSensitive, diags := ctx.Apply(oldPlan, m)
 	assertNoErrors(t, diags)
@@ -12206,7 +12205,7 @@ func TestContext2Apply_dataSensitive(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("diags: %s", diags.Err())
 	} else {

internal/terraform/context_eval.go

@@ -45,7 +45,7 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a
 	state = state.DeepCopy()
 	var walker *ContextGraphWalker
 
-	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
+	variables := opts.SetVariables
 
 	// By the time we get here, we should have values defined for all of
 	// the root module variables, even if some of them are "unknown". It's the

internal/terraform/context_eval_test.go

@@ -54,7 +54,9 @@ func TestContextEval(t *testing.T) {
 		},
 	})
 
-	scope, diags := ctx.Eval(m, states.NewState(), addrs.RootModuleInstance, &EvalOpts{})
+	scope, diags := ctx.Eval(m, states.NewState(), addrs.RootModuleInstance, &EvalOpts{
+		SetVariables: testInputValuesUnset(m.Module.Variables),
+	})
 	if diags.HasErrors() {
 		t.Fatalf("Eval errors: %s", diags.Err())
 	}

internal/terraform/context_import.go

@@ -53,7 +53,7 @@ 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)
+	variables := opts.SetVariables
 
 	// Initialize our graph builder
 	builder := &ImportGraphBuilder{

internal/terraform/context_plan.go

@@ -21,10 +21,42 @@ import (
 // PlanOpts are the various options that affect the details of how Terraform
 // will build a plan.
 type PlanOpts struct {
-	Mode         plans.Mode
+	// Mode defines what variety of plan the caller wishes to create.
-	SkipRefresh  bool
+	// Refer to the documentation of the plans.Mode type and its values
+	// for more information.
+	Mode plans.Mode
+
+	// SkipRefresh specifies to trust that the current values for managed
+	// resource instances in the prior state are accurate and to therefore
+	// disable the usual step of fetching updated values for each resource
+	// instance using its corresponding provider.
+	SkipRefresh bool
+
+	// SetVariables are the raw values for root module variables as provided
+	// by the user who is requesting the run, prior to any normalization or
+	// substitution of defaults. See the documentation for the InputValue
+	// type for more information on how to correctly populate this.
 	SetVariables InputValues
-	Targets      []addrs.Targetable
+
+	// If Targets has a non-zero length then it activates targeted planning
+	// mode, where Terraform will take actions only for resource instances
+	// mentioned in this set and any other objects those resource instances
+	// depend on.
+	//
+	// Targeted planning mode is intended for exceptional use only,
+	// and so populating this field will cause Terraform to generate extra
+	// warnings as part of the planning result.
+	Targets []addrs.Targetable
+
+	// ForceReplace is a set of resource instance addresses whose corresponding
+	// objects should be forced planned for replacement if the provider's
+	// plan would otherwise have been to either update the object in-place or
+	// to take no action on it at all.
+	//
+	// A typical use of this argument is to ask Terraform to replace an object
+	// which the user has determined is somehow degraded (via information from
+	// outside of Terraform), thereby hopefully replacing it with a
+	// fully-functional new object.
 	ForceReplace []addrs.AbsResourceInstance
 }
 
@@ -99,8 +131,6 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts
 		return nil, diags
 	}
 
-	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
-
 	// By the time we get here, we should have values defined for all of
 	// the root module variables, even if some of them are "unknown". It's the
 	// caller's responsibility to have already handled the decoding of these
@@ -108,7 +138,7 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts
 	// user-friendly error messages if they are not all present, and so
 	// the error message from checkInputVariables should never be seen and
 	// includes language asking the user to report a bug.
-	varDiags := checkInputVariables(config.Module.Variables, variables)
+	varDiags := checkInputVariables(config.Module.Variables, opts.SetVariables)
 	diags = diags.Append(varDiags)
 
 	if len(opts.Targets) > 0 {
@@ -139,8 +169,12 @@ The -target option is not for routine use, and is provided only for exceptional
 	}
 
 	// convert the variables into the format expected for the plan
-	varVals := make(map[string]plans.DynamicValue, len(variables))
+	varVals := make(map[string]plans.DynamicValue, len(opts.SetVariables))
-	for k, iv := range variables {
+	for k, iv := range opts.SetVariables {
+		if iv.Value == cty.NilVal {
+			continue // We only record values that the caller actually set
+		}
+
 		// We use cty.DynamicPseudoType here so that we'll save both the
 		// value _and_ its dynamic type in the plan, so we can recover
 		// exactly the same value later.
@@ -172,6 +206,25 @@ var DefaultPlanOpts = &PlanOpts{
 	Mode: plans.NormalMode,
 }
 
+// SimplePlanOpts is a constructor to help with creating "simple" values of
+// PlanOpts which only specify a mode and input variables.
+//
+// This helper function is primarily intended for use in straightforward
+// tests that don't need any of the more "esoteric" planning options. For
+// handling real user requests to run Terraform, it'd probably be better
+// to construct a *PlanOpts value directly and provide a way for the user
+// to set values for all of its fields.
+//
+// The "mode" and "setVariables" arguments become the values of the "Mode"
+// and "SetVariables" fields in the result. Refer to the PlanOpts type
+// documentation to learn about the meanings of those fields.
+func SimplePlanOpts(mode plans.Mode, setVariables InputValues) *PlanOpts {
+	return &PlanOpts{
+		Mode:         mode,
+		SetVariables: setVariables,
+	}
+}
+
 func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 

internal/terraform/context_plan2_test.go

@@ -205,7 +205,7 @@ data "test_data_source" "foo" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	for _, res := range plan.Changes.Resources {

internal/terraform/context_plan_test.go

@@ -405,7 +405,7 @@ func TestContext2Plan_moduleExpand(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -1175,7 +1175,7 @@ func TestContext2Plan_moduleProviderVar(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -2242,7 +2242,7 @@ func TestContext2Plan_countModuleStatic(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -2295,7 +2295,7 @@ func TestContext2Plan_countModuleStaticGrandchild(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -3938,7 +3938,7 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) {
 			},
 		})
 
-		plan, diags := ctx.Plan(m, state.DeepCopy(), DefaultPlanOpts)
+		plan, diags := ctx.Plan(m, state.DeepCopy(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 		if diags.HasErrors() {
 			t.Fatalf("unexpected errors: %s", diags.Err())
 		}
@@ -5481,7 +5481,7 @@ func TestContext2Plan_variableSensitivity(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -5544,6 +5544,7 @@ func TestContext2Plan_variableSensitivityModule(t *testing.T) {
 	plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
 		Mode: plans.NormalMode,
 		SetVariables: InputValues{
+			"sensitive_var": {Value: cty.NilVal},
 			"another_var": &InputValue{
 				Value:      cty.StringVal("boop"),
 				SourceType: ValueFromCaller,
@@ -6657,7 +6658,7 @@ resource "test_resource" "foo" {
 			},
 		)
 	})
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatal(diags.Err())
 	}

internal/terraform/eval_variable.go

@@ -45,9 +45,11 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 		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
+			// the required _root_ module variables are set, which would
-			// mask this error. We can get here for child module variables,
+			// mask this error with a more specific one that refers to the
-			// though.
+			// CLI features for setting such variables. We can get here for
+			// child module variables, though.
+			log.Printf("[ERROR] prepareFinalInputVariableValue: %s is required but is not set", addr)
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  `Required variable not set`,
@@ -64,6 +66,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 
 	val, err := convert.Convert(given, convertTy)
 	if err != nil {
+		log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n  got:  %s\n  want: %s", addr, given.Type(), convertTy)
 		diags = diags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,
 			Summary:  "Invalid value for module argument",
@@ -93,6 +96,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 		if defaultVal != cty.NilVal {
 			val = defaultVal
 		} else {
+			log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr)
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  `Required variable not set`,

internal/terraform/variables.go

@@ -3,18 +3,50 @@ package terraform
 import (
 	"fmt"
 
-	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty/cty"
-	"github.com/zclconf/go-cty/cty/convert"
 
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
-// InputValue represents a value for a variable in the root module, provided
+// InputValue represents a raw value vor a root module input variable as
-// as part of the definition of an operation.
+// provided by the external caller into a function like terraform.Context.Plan.
+//
+// InputValue should represent as directly as possible what the user set the
+// variable to, without any attempt to convert the value to the variable's
+// type constraint or substitute the configured default values for variables
+// that wasn't set. Those adjustments will be handled by Terraform Core itself
+// as part of performing the requested operation.
+//
+// A Terraform Core caller must provide an InputValue object for each of the
+// variables declared in the root module, even if the end user didn't provide
+// an explicit value for some of them. See the Value field documentation for
+// how to handle that situation.
 type InputValue struct {
-	Value      cty.Value
+	// Value is the raw value as provided by the user as part of the plan
+	// options, or a corresponding similar data structure for non-plan
+	// operations.
+	//
+	// If a particular variable declared in the root module is _not_ set by
+	// the user then the caller must still provide an InputValue for it but
+	// must set Value to cty.NilVal to represent the absense of a value.
+	// This requirement is to help detect situations where the caller isn't
+	// correctly detecting and handling all of the declared variables.
+	//
+	// For historical reasons it's important that callers distinguish the
+	// situation of the value not being set at all (cty.NilVal) from the
+	// situation of it being explicitly set to null (a cty.NullVal result):
+	// for "nullable" input variables that distinction unfortunately decides
+	// whether the final value will be the variable's default or will be
+	// explicitly null.
+	Value cty.Value
+
+	// SourceType is a high-level category for where the value of Value
+	// came from, which Terraform Core uses to tailor some of its error
+	// messages to be more helpful to the user.
+	//
+	// Some SourceType values should be accompanied by a populated SourceRange
+	// value. See that field's documentation below for more information.
 	SourceType ValueSourceType
 
 	// SourceRange provides source location information for values whose
@@ -129,23 +161,6 @@ func (vv InputValues) JustValues() map[string]cty.Value {
 	return ret
 }
 
-// DefaultVariableValues returns an InputValues map representing the default
-// values specified for variables in the given configuration map.
-func DefaultVariableValues(configs map[string]*configs.Variable) InputValues {
-	ret := make(InputValues)
-	for k, c := range configs {
-		if c.Default == cty.NilVal {
-			continue
-		}
-		ret[k] = &InputValue{
-			Value:       c.Default,
-			SourceType:  ValueFromConfig,
-			SourceRange: tfdiags.SourceRangeFromHCL(c.DeclRange),
-		}
-	}
-	return ret
-}
-
 // SameValues returns true if the given InputValues has the same values as
 // the receiever, disregarding the source types and source ranges.
 //
@@ -227,21 +242,15 @@ func (vv InputValues) Identical(other InputValues) bool {
 	return true
 }
 
-func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[string]*configs.Variable) InputValues {
+// checkInputVariables ensures that the caller provided an InputValue
-	var variables InputValues
+// definition for each root module variable declared in the configuration.
-
+// The caller must provide an InputVariables with keys exactly matching
-	// Default variables from the configuration seed our map.
+// the declared variables, though some of them may be marked explicitly
-	variables = DefaultVariableValues(rootVarsConfig)
+// unset by their values being cty.NilVal.
-
+//
-	// Variables provided by the caller (from CLI, environment, etc) can
+// This doesn't perform any type checking, default value substitution, or
-	// override the defaults.
+// validation checks. Those are all handled during a graph walk when we
-	variables = variables.Override(setVals)
+// visit the graph nodes representing each root variable.
-
-	return variables
-}
-
-// checkInputVariables ensures that variable values supplied at the UI conform
-// to their corresponding declarations in configuration.
 //
 // The set of values is considered valid only if the returned diagnostics
 // does not contain errors. A valid set of values may still produce warnings,
@@ -249,11 +258,12 @@ func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[str
 func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
-	for name, vc := range vcs {
+	for name := range vcs {
-		val, isSet := vs[name]
+		_, isSet := vs[name]
 		if !isSet {
-			// Always an error, since the caller should already have included
+			// Always an error, since the caller should have produced an
-			// default values from the configuration in the values map.
+			// item with Value: cty.NilVal to be explicit that it offered
+			// an opportunity to set this variable.
 			diags = diags.Append(tfdiags.Sourceless(
 				tfdiags.Error,
 				"Unassigned variable",
@@ -261,49 +271,6 @@ func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdia
 			))
 			continue
 		}
-
-		// A given value is valid if it can convert to the desired type.
-		_, err := convert.Convert(val.Value, vc.ConstraintType)
-		if err != nil {
-			switch val.SourceType {
-			case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile:
-				// We have source location information for these.
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid value for input variable",
-					Detail:   fmt.Sprintf("The given value is not valid for variable %q: %s.", name, err),
-					Subject:  val.SourceRange.ToHCL().Ptr(),
-				})
-			case ValueFromEnvVar:
-				diags = diags.Append(tfdiags.Sourceless(
-					tfdiags.Error,
-					"Invalid value for input variable",
-					fmt.Sprintf("The environment variable TF_VAR_%s does not contain a valid value for variable %q: %s.", name, name, err),
-				))
-			case ValueFromCLIArg:
-				diags = diags.Append(tfdiags.Sourceless(
-					tfdiags.Error,
-					"Invalid value for input variable",
-					fmt.Sprintf("The argument -var=\"%s=...\" does not contain a valid value for variable %q: %s.", name, name, err),
-				))
-			case ValueFromInput:
-				diags = diags.Append(tfdiags.Sourceless(
-					tfdiags.Error,
-					"Invalid value for input variable",
-					fmt.Sprintf("The value entered for variable %q is not valid: %s.", name, err),
-				))
-			default:
-				// The above gets us good coverage for the situations users
-				// are likely to encounter with their own inputs. The other
-				// cases are generally implementation bugs, so we'll just
-				// use a generic error for these.
-				diags = diags.Append(tfdiags.Sourceless(
-					tfdiags.Error,
-					"Invalid value for input variable",
-					fmt.Sprintf("The value provided for variable %q is not valid: %s.", name, err),
-				))
-			}
-		}
 	}
 
 	// Check for any variables that are assigned without being configured.

internal/terraform/variables_test.go

@@ -3,166 +3,10 @@ package terraform
 import (
 	"testing"
 
-	"github.com/davecgh/go-spew/spew"
+	"github.com/hashicorp/terraform/internal/configs"
-	"github.com/hashicorp/terraform/internal/tfdiags"
-
-	"github.com/go-test/deep"
 	"github.com/zclconf/go-cty/cty"
 )
 
-func TestVariables(t *testing.T) {
-	tests := map[string]struct {
-		Module   string
-		Override map[string]cty.Value
-		Want     InputValues
-	}{
-		"config only": {
-			"vars-basic",
-			nil,
-			InputValues{
-				"a": &InputValue{
-					Value:      cty.StringVal("foo"),
-					SourceType: ValueFromConfig,
-					SourceRange: tfdiags.SourceRange{
-						Filename: "testdata/vars-basic/main.tf",
-						Start:    tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0},
-						End:      tfdiags.SourcePos{Line: 1, Column: 13, Byte: 12},
-					},
-				},
-				"b": &InputValue{
-					Value:      cty.ListValEmpty(cty.String),
-					SourceType: ValueFromConfig,
-					SourceRange: tfdiags.SourceRange{
-						Filename: "testdata/vars-basic/main.tf",
-						Start:    tfdiags.SourcePos{Line: 6, Column: 1, Byte: 55},
-						End:      tfdiags.SourcePos{Line: 6, Column: 13, Byte: 67},
-					},
-				},
-				"c": &InputValue{
-					Value:      cty.MapValEmpty(cty.String),
-					SourceType: ValueFromConfig,
-					SourceRange: tfdiags.SourceRange{
-						Filename: "testdata/vars-basic/main.tf",
-						Start:    tfdiags.SourcePos{Line: 11, Column: 1, Byte: 113},
-						End:      tfdiags.SourcePos{Line: 11, Column: 13, Byte: 125},
-					},
-				},
-			},
-		},
-
-		"override": {
-			"vars-basic",
-			map[string]cty.Value{
-				"a": cty.StringVal("bar"),
-				"b": cty.ListVal([]cty.Value{
-					cty.StringVal("foo"),
-					cty.StringVal("bar"),
-				}),
-				"c": cty.MapVal(map[string]cty.Value{
-					"foo": cty.StringVal("bar"),
-				}),
-			},
-			InputValues{
-				"a": &InputValue{
-					Value:      cty.StringVal("bar"),
-					SourceType: ValueFromCaller,
-				},
-				"b": &InputValue{
-					Value: cty.ListVal([]cty.Value{
-						cty.StringVal("foo"),
-						cty.StringVal("bar"),
-					}),
-					SourceType: ValueFromCaller,
-				},
-				"c": &InputValue{
-					Value: cty.MapVal(map[string]cty.Value{
-						"foo": cty.StringVal("bar"),
-					}),
-					SourceType: ValueFromCaller,
-				},
-			},
-		},
-
-		"bools: config only": {
-			"vars-basic-bool",
-			nil,
-			InputValues{
-				"a": &InputValue{
-					Value:      cty.True,
-					SourceType: ValueFromConfig,
-					SourceRange: tfdiags.SourceRange{
-						Filename: "testdata/vars-basic-bool/main.tf",
-						Start:    tfdiags.SourcePos{Line: 4, Column: 1, Byte: 177},
-						End:      tfdiags.SourcePos{Line: 4, Column: 13, Byte: 189},
-					},
-				},
-				"b": &InputValue{
-					Value:      cty.False,
-					SourceType: ValueFromConfig,
-					SourceRange: tfdiags.SourceRange{
-						Filename: "testdata/vars-basic-bool/main.tf",
-						Start:    tfdiags.SourcePos{Line: 8, Column: 1, Byte: 214},
-						End:      tfdiags.SourcePos{Line: 8, Column: 13, Byte: 226},
-					},
-				},
-			},
-		},
-
-		"bools: override with string": {
-			"vars-basic-bool",
-			map[string]cty.Value{
-				"a": cty.StringVal("foo"),
-				"b": cty.StringVal("bar"),
-			},
-			InputValues{
-				"a": &InputValue{
-					Value:      cty.StringVal("foo"),
-					SourceType: ValueFromCaller,
-				},
-				"b": &InputValue{
-					Value:      cty.StringVal("bar"),
-					SourceType: ValueFromCaller,
-				},
-			},
-		},
-
-		"bools: override with bool": {
-			"vars-basic-bool",
-			map[string]cty.Value{
-				"a": cty.False,
-				"b": cty.True,
-			},
-			InputValues{
-				"a": &InputValue{
-					Value:      cty.False,
-					SourceType: ValueFromCaller,
-				},
-				"b": &InputValue{
-					Value:      cty.True,
-					SourceType: ValueFromCaller,
-				},
-			},
-		},
-	}
-
-	for name, test := range tests {
-		// Wrapped in a func so we can get defers to work
-		t.Run(name, func(t *testing.T) {
-			m := testModule(t, test.Module)
-			fromConfig := DefaultVariableValues(m.Module.Variables)
-			overrides := InputValuesFromCaller(test.Override)
-			got := fromConfig.Override(overrides)
-
-			if !got.Identical(test.Want) {
-				t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(test.Want))
-			}
-			for _, problem := range deep.Equal(got, test.Want) {
-				t.Errorf(problem)
-			}
-		})
-	}
-}
-
 func TestCheckInputVariables(t *testing.T) {
 	c := testModule(t, "input-variables")
 
@@ -280,3 +124,25 @@ func TestCheckInputVariables(t *testing.T) {
 		}
 	})
 }
+
+// testInputValuesUnset is a helper for constructing InputValues values for
+// situations where all of the root module variables are optional and a
+// test case intends to just use those default values and not override them
+// at all.
+//
+// In other words, this constructs an InputValues with one entry per given
+// input variable declaration where all of them are declared as unset.
+func testInputValuesUnset(decls map[string]*configs.Variable) InputValues {
+	if len(decls) == 0 {
+		return nil
+	}
+
+	ret := make(InputValues, len(decls))
+	for name := range decls {
+		ret[name] = &InputValue{
+			Value:      cty.NilVal,
+			SourceType: ValueFromUnknown,
+		}
+	}
+	return ret
+}