core: More accurate error message for invalid variable values

In earlier Terraform versions we had an extra validation step prior to
the graph walk which tried to partially validate root module input
variable values (just checking their type constraints) and then return
error messages which specified as accurately as possible where the value
had originally come from.

We're now handling that sort of validation exclusively during the graph
walk so that we can share the main logic between both root module and
child module variable values, but previously that shared code wasn't
able to generate such specific information about where the values had
originated, because it was adapted from code originally written to only
deal with child module variables.

Here then we restore a similar level of detail as before, when we're
processing root module variables. For child module variables, we use
synthetic InputValue objects which state that the value was declared
in the configuration, thus causing us to produce a similar sort of error
message as we would've before which includes a source range covering
the argument expression in the calling module block.

internal/terraform/eval_variable.go

@@ -12,7 +12,7 @@ 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) {
+func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *InputValue, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	convertTy := cfg.ConstraintType
@@ -41,6 +41,29 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 		}
 	}
 
+	var sourceRange tfdiags.SourceRange
+	var nonFileSource string
+	if raw.HasSourceRange() {
+		sourceRange = raw.SourceRange
+	} else {
+		// If the value came from a place that isn't a file and thus doesn't
+		// have its own source range, we'll use the declaration range as
+		// our source range and generate some slightly different error
+		// messages.
+		sourceRange = tfdiags.SourceRangeFromHCL(cfg.DeclRange)
+		switch raw.SourceType {
+		case ValueFromCLIArg:
+			nonFileSource = fmt.Sprintf("set using -var=\"%s=...\"", addr.Variable.Name)
+		case ValueFromEnvVar:
+			nonFileSource = fmt.Sprintf("set using the TF_VAR_%s environment variable", addr.Variable.Name)
+		case ValueFromInput:
+			nonFileSource = "set using an interactive prompt"
+		default:
+			nonFileSource = "set from outside of the configuration"
+		}
+	}
+
+	given := raw.Value
 	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() {
@@ -54,7 +77,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 				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(),
+				Subject:  cfg.DeclRange.Ptr(),
 			})
 			// We'll return a placeholder unknown value to avoid producing
 			// redundant downstream errors.
@@ -67,15 +90,27 @@ 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)
+		if nonFileSource != "" {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
-			Summary:  "Invalid value for module argument",
+				Summary:  "Invalid value for input variable",
 				Detail: fmt.Sprintf(
-				"The given value is not suitable for child module variable %q defined at %s: %s.",
+					"Unsuitable value for %s %s: %s.",
-				cfg.Name, cfg.DeclRange.String(), err,
+					addr, nonFileSource, err,
 				),
-			Subject: valRange.ToHCL().Ptr(),
+				Subject: cfg.DeclRange.Ptr(),
 			})
+		} else {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid value for input variable",
+				Detail: fmt.Sprintf(
+					"The given value is not suitable for %s declared at %s: %s.",
+					addr, cfg.DeclRange.String(), err,
+				),
+				Subject: sourceRange.ToHCL().Ptr(),
+			})
+		}
 		// We'll return a placeholder unknown value to avoid producing
 		// redundant downstream errors.
 		return cty.UnknownVal(cfg.Type), diags
@@ -97,12 +132,27 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c
 			val = defaultVal
 		} else {
 			log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr)
+			if nonFileSource != "" {
 				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),
+					Detail: fmt.Sprintf(
-				Subject:  valRange.ToHCL().Ptr(),
+						"Unsuitable value for %s %s: required variable may not be set to null.",
+						addr, nonFileSource,
+					),
+					Subject: cfg.DeclRange.Ptr(),
 				})
+			} else {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  `Required variable not set`,
+					Detail: fmt.Sprintf(
+						"The given value is not suitable for %s defined at %s: required variable may not be set to null.",
+						addr, cfg.DeclRange.String(),
+					),
+					Subject: sourceRange.ToHCL().Ptr(),
+				})
+			}
 			// Stub out our return value so that the semantic checker doesn't
 			// produce redundant downstream errors.
 			val = cty.UnknownVal(cfg.Type)

internal/terraform/eval_variable_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/addrs"
@@ -65,6 +66,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
 	})
 	variableConfigs := cfg.Module.Variables
 
+	// Because we loaded our pseudo-module from a temporary file, the
+	// declaration source ranges will have unpredictable filenames. We'll
+	// fix that here just to make things easier below.
+	for _, vc := range variableConfigs {
+		vc.DeclRange.Filename = "main.tf"
+	}
+
 	tests := []struct {
 		varName string
 		given   cty.Value
@@ -264,7 +272,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
 			"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 variable not set: Unsuitable value for var.required set from outside of the configuration: required variable may not be set to null.`,
 		},
 		{
 			"required",
@@ -316,7 +324,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
 			"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.`,
+			`Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`,
 		},
 		{
 			"constrained_string_required",
@@ -401,8 +409,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
 				test.given,
 			)
 
+			rawVal := &InputValue{
+				Value:      test.given,
+				SourceType: ValueFromCaller,
+			}
+
 			got, diags := prepareFinalInputVariableValue(
-				varAddr, test.given, tfdiags.SourceRangeFromHCL(varCfg.DeclRange), varCfg,
+				varAddr, rawVal, varCfg,
 			)
 
 			if test.wantErr != "" {
@@ -423,4 +436,128 @@ func TestPrepareFinalInputVariableValue(t *testing.T) {
 			}
 		})
 	}
+
+	t.Run("SourceType error message variants", func(t *testing.T) {
+		tests := []struct {
+			SourceType  ValueSourceType
+			SourceRange tfdiags.SourceRange
+			WantTypeErr string
+			WantNullErr string
+		}{
+			{
+				ValueFromUnknown,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`,
+			},
+			{
+				ValueFromConfig,
+				tfdiags.SourceRange{
+					Filename: "example.tf",
+					Start:    tfdiags.SourcePos(hcl.InitialPos),
+					End:      tfdiags.SourcePos(hcl.InitialPos),
+				},
+				`Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`,
+				`Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`,
+			},
+			{
+				ValueFromAutoFile,
+				tfdiags.SourceRange{
+					Filename: "example.auto.tfvars",
+					Start:    tfdiags.SourcePos(hcl.InitialPos),
+					End:      tfdiags.SourcePos(hcl.InitialPos),
+				},
+				`Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`,
+				`Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`,
+			},
+			{
+				ValueFromNamedFile,
+				tfdiags.SourceRange{
+					Filename: "example.tfvars",
+					Start:    tfdiags.SourcePos(hcl.InitialPos),
+					End:      tfdiags.SourcePos(hcl.InitialPos),
+				},
+				`Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`,
+				`Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`,
+			},
+			{
+				ValueFromCLIArg,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": required variable may not be set to null.`,
+			},
+			{
+				ValueFromEnvVar,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: required variable may not be set to null.`,
+			},
+			{
+				ValueFromInput,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set using an interactive prompt: string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set using an interactive prompt: required variable may not be set to null.`,
+			},
+			{
+				// NOTE: This isn't actually a realistic case for this particular
+				// function, because if we have a value coming from a plan then
+				// we must be in the apply step, and we shouldn't be able to
+				// get past the plan step if we have invalid variable values,
+				// and during planning we'll always have other source types.
+				ValueFromPlan,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`,
+			},
+			{
+				ValueFromCaller,
+				tfdiags.SourceRange{},
+				`Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`,
+				`Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`,
+			},
+		}
+
+		for _, test := range tests {
+			t.Run(fmt.Sprintf("%s %s", test.SourceType, test.SourceRange.StartString()), func(t *testing.T) {
+				varAddr := addrs.InputVariable{Name: "constrained_string_required"}.Absolute(addrs.RootModuleInstance)
+				varCfg := variableConfigs[varAddr.Variable.Name]
+				t.Run("type error", func(t *testing.T) {
+					rawVal := &InputValue{
+						Value:       cty.EmptyObjectVal,
+						SourceType:  test.SourceType,
+						SourceRange: test.SourceRange,
+					}
+
+					_, diags := prepareFinalInputVariableValue(
+						varAddr, rawVal, varCfg,
+					)
+					if !diags.HasErrors() {
+						t.Fatalf("unexpected success; want error")
+					}
+
+					if got, want := diags.Err().Error(), test.WantTypeErr; got != want {
+						t.Errorf("wrong error\ngot:  %s\nwant: %s", got, want)
+					}
+				})
+				t.Run("null error", func(t *testing.T) {
+					rawVal := &InputValue{
+						Value:       cty.NullVal(cty.DynamicPseudoType),
+						SourceType:  test.SourceType,
+						SourceRange: test.SourceRange,
+					}
+
+					_, diags := prepareFinalInputVariableValue(
+						varAddr, rawVal, varCfg,
+					)
+					if !diags.HasErrors() {
+						t.Fatalf("unexpected success; want error")
+					}
+
+					if got, want := diags.Err().Error(), test.WantNullErr; got != want {
+						t.Errorf("wrong error\ngot:  %s\nwant: %s", got, want)
+					}
+				})
+			})
+		}
+	})
 }

internal/terraform/node_module_variable.go

@@ -227,7 +227,16 @@ func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnl
 		errSourceRange = tfdiags.SourceRangeFromHCL(n.Config.DeclRange) // we use the declaration range as a fallback for an undefined variable
 	}
 
-	finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, givenVal, errSourceRange, n.Config)
+	// We construct a synthetic InputValue here to pretend as if this were
+	// a root module variable set from outside, just as a convenience so we
+	// can reuse the InputValue type for this.
+	rawVal := &InputValue{
+		Value:       givenVal,
+		SourceType:  ValueFromConfig,
+		SourceRange: errSourceRange,
+	}
+
+	finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, rawVal, n.Config)
 	diags = diags.Append(moreDiags)
 
 	return finalVal, diags.ErrWithWarnings()

internal/terraform/node_root_variable.go

@@ -65,20 +65,23 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di
 		return nil
 	}
 
-	var givenVal cty.Value
+	givenVal := n.RawValue
-	if n.RawValue != nil {
+	if givenVal == 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.
+		// explicitly setting it to null in some cases. In normal code we
-		givenVal = cty.NilVal
+		// should never get here because all variables should have raw
+		// values, but we can get here in some historical tests that call
+		// in directly and don't necessarily obey the rules.
+		givenVal = &InputValue{
+			Value:      cty.NilVal,
+			SourceType: ValueFromUnknown,
+		}
 	}
 
 	finalVal, moreDiags := prepareFinalInputVariableValue(
 		addr,
 		givenVal,
-		tfdiags.SourceRangeFromHCL(n.Config.DeclRange),
 		n.Config,
 	)
 	diags = diags.Append(moreDiags)

internal/terraform/variables.go

@@ -9,7 +9,7 @@ import (
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
-// InputValue represents a raw value vor a root module input variable as
+// InputValue represents a raw value for a root module input variable as
 // provided by the external caller into a function like terraform.Context.Plan.
 //
 // InputValue should represent as directly as possible what the user set the
@@ -22,6 +22,11 @@ import (
 // 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.
+//
+// Terraform Core also internally uses InputValue to represent the raw value
+// provided for a variable in a child module call, following the same
+// conventions. However, that's an implementation detail not visible to
+// outside callers.
 type InputValue struct {
 	// 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
@@ -50,8 +55,9 @@ type InputValue struct {
 	SourceType ValueSourceType
 
 	// SourceRange provides source location information for values whose
-	// SourceType is either ValueFromConfig or ValueFromFile. It is not
+	// SourceType is either ValueFromConfig, ValueFromNamedFile, or
-	// populated for other source types, and so should not be used.
+	// ValueForNormalFile. It is not populated for other source types, and so
+	// should not be used.
 	SourceRange tfdiags.SourceRange
 }
 
@@ -106,6 +112,24 @@ func (v *InputValue) GoString() string {
 	}
 }
 
+// HasSourceRange returns true if the reciever has a source type for which
+// we expect the SourceRange field to be populated with a valid range.
+func (v *InputValue) HasSourceRange() bool {
+	return v.SourceType.HasSourceRange()
+}
+
+// HasSourceRange returns true if the reciever is one of the source types
+// that is used along with a valid SourceRange field when appearing inside an
+// InputValue object.
+func (v ValueSourceType) HasSourceRange() bool {
+	switch v {
+	case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile:
+		return true
+	default:
+		return false
+	}
+}
+
 func (v ValueSourceType) GoString() string {
 	return fmt.Sprintf("terraform.%s", v)
 }