terraform test: deprecate ambigious variable references in test files (#37218)

* terraform test: deprecate ambigious variable references in test files

* fix panic caused by merge

internal/moduletest/graph/eval_context.go

@@ -151,7 +151,7 @@ func (ec *EvalContext) Verbose() bool {
 	return ec.verbose
 }
 
-func (ec *EvalContext) HclContext(references []*addrs.Reference, variableConfigs map[string]*configs.Variable) (*hcl.EvalContext, tfdiags.Diagnostics) {
+func (ec *EvalContext) HclContext(references []*addrs.Reference) (*hcl.EvalContext, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 
 	runs := make(map[string]cty.Value)
@@ -205,7 +205,7 @@ func (ec *EvalContext) HclContext(references []*addrs.Reference, variableConfigs
 				continue
 			}
 
-			if variable, moreDiags := ec.EvaluateUnparsedVariable(subject.Name, variableConfigs[subject.Name]); variable != nil {
+			if variable, moreDiags := ec.EvaluateUnparsedVariableDeprecated(subject.Name, reference); variable != nil {
 				diags = diags.Append(moreDiags)
 				vars[subject.Name] = variable.Value
 				continue
@@ -404,26 +404,47 @@ func (ec *EvalContext) EvaluateRun(run *moduletest.Run, resultScope *lang.Scope,
 	return status, cty.ObjectVal(outputVals), diags
 }
 
+// EvaluateUnparsedVariable accepts a variable name and a variable definition
+// and checks if we have external unparsed variables that match the given
+// configuration. If no variable was provided, we'll return a nil
+// input value.
 func (ec *EvalContext) EvaluateUnparsedVariable(name string, config *configs.Variable) (*terraform.InputValue, tfdiags.Diagnostics) {
 	variable, exists := ec.unparsedVariables[name]
 	if !exists {
 		return nil, nil
 	}
 
-	if config != nil {
+	value, diags := variable.ParseVariableValue(config.ParsingMode)
+	if diags.HasErrors() {
+		value = &terraform.InputValue{
+			Value: cty.DynamicVal,
+		}
+	}
 
-		// If we have a configuration, then we'll using the parsing mode from
-		// that.
+	return value, diags
+}
 
-		value, diags := variable.ParseVariableValue(config.ParsingMode)
-		if diags.HasErrors() {
-			value = &terraform.InputValue{
-				Value: cty.DynamicVal,
-			}
-		}
-		return value, diags
+// EvaluateUnparsedVariableDeprecated accepts a variable name without a variable
+// definition and attempts to parse it.
+//
+// This function represents deprecated functionality within the testing
+// framework. It is no longer valid to reference external variables without a
+// definition, but we do our best here and provide a warning that this will
+// become completely unsupported in the future.
+func (ec *EvalContext) EvaluateUnparsedVariableDeprecated(name string, ref *addrs.Reference) (*terraform.InputValue, tfdiags.Diagnostics) {
+	variable, exists := ec.unparsedVariables[name]
+	if !exists {
+		return nil, nil
 	}
 
+	var diags tfdiags.Diagnostics
+	diags = diags.Append(&hcl.Diagnostic{
+		Severity: hcl.DiagWarning,
+		Summary:  "Variable referenced without definition",
+		Detail:   fmt.Sprintf("Variable %q was referenced without providing a definition. Referencing undefined variables within Terraform Test files is deprecated, please add a `variable` block into the relevant test file to provide a definition for the variable. This will become required in future versions of Terraform.", name),
+		Subject:  ref.SourceRange.ToHCL().Ptr(),
+	})
+
 	// For backwards-compatibility reasons we do also have to support trying
 	// to parse the global variables without a configuration. We introduced the
 	// file-level variable definitions later, and users were already using
@@ -432,27 +453,19 @@ func (ec *EvalContext) EvaluateUnparsedVariable(name string, config *configs.Var
 	// Otherwise, we have no configuration so we're going to try both parsing
 	// modes.
 
-	value, diags := variable.ParseVariableValue(configs.VariableParseHCL)
-	if !diags.HasErrors() {
+	value, moreDiags := variable.ParseVariableValue(configs.VariableParseHCL)
+	diags = diags.Append(moreDiags)
+	if !moreDiags.HasErrors() {
 		// then good! we can just return these values directly.
 		return value, diags
 	}
 
 	// otherwise, we'll try the other one.
 
-	value, diags = variable.ParseVariableValue(configs.VariableParseLiteral)
-	if diags.HasErrors() {
-
-		// we'll add a warning diagnostic here, just telling the users they
-		// can avoid this by adding a variable definition.
-
-		diags = diags.Append(tfdiags.Sourceless(
-			tfdiags.Warning,
-			"Missing variable definition",
-			fmt.Sprintf("The variable %q could not be parsed. Terraform had no definition block for this variable, this error could be avoided in future by including a definition block for this variable within the Terraform test file.", name)))
-
+	value, moreDiags = variable.ParseVariableValue(configs.VariableParseLiteral)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
 		// as usual make sure we still provide something for this value.
-
 		value = &terraform.InputValue{
 			Value: cty.DynamicVal,
 		}

internal/moduletest/graph/node_provider.go

@@ -71,7 +71,7 @@ func (n *NodeProviderConfigure) Execute(ctx *EvalContext) {
 		return
 	}
 
-	hclContext, moreDiags := ctx.HclContext(references, nil)
+	hclContext, moreDiags := ctx.HclContext(references)
 	n.File.AppendDiagnostics(moreDiags)
 	if moreDiags.HasErrors() {
 		ctx.SetProviderStatus(n.Addr, moduletest.Error)

internal/moduletest/graph/node_variable.go

@@ -58,14 +58,14 @@ func (n *NodeVariableDefinition) Execute(ctx *EvalContext) {
 		}
 	}
 
-	value, moreDiags := terraform.PrepareFinalInputVariableValue(addrs.AbsInputVariableInstance{
+	value, diags := terraform.PrepareFinalInputVariableValue(addrs.AbsInputVariableInstance{
 		Module: addrs.RootModuleInstance,
 		Variable: addrs.InputVariable{
 			Name: n.Address,
 		},
 	}, input, n.Config)
-	n.File.AppendDiagnostics(moreDiags)
-	if moreDiags.HasErrors() {
+	n.File.AppendDiagnostics(diags)
+	if diags.HasErrors() {
 		ctx.SetVariableStatus(n.Address, moduletest.Error)
 		return
 	}
@@ -125,7 +125,7 @@ func (n *NodeVariableExpression) Execute(ctx *EvalContext) {
 		return
 	}
 
-	evalContext, moreDiags := ctx.HclContext(refs, nil)
+	evalContext, moreDiags := ctx.HclContext(refs)
 	n.File.AppendDiagnostics(moreDiags)
 	if moreDiags.HasErrors() {
 		ctx.SetVariableStatus(n.Address, moduletest.Error)

internal/moduletest/graph/variables.go

@@ -32,19 +32,19 @@ func (n *NodeTestRun) GetVariables(ctx *EvalContext, includeWarnings bool) (terr
 	// run block. This is a combination of the variables declared within the
 	// configuration for this run block, and the variables referenced by the
 	// run block assertions.
-	relevantVariables := make(map[string]bool)
+	relevantVariables := make(map[string]*addrs.Reference)
 
 	// First, we'll check to see which variables the run block assertions
 	// reference.
 	for _, reference := range n.References() {
 		if addr, ok := reference.Subject.(addrs.InputVariable); ok {
-			relevantVariables[addr.Name] = true
+			relevantVariables[addr.Name] = reference
 		}
 	}
 
 	// And check to see which variables the run block configuration references.
 	for name := range run.ModuleConfig.Module.Variables {
-		relevantVariables[name] = true
+		relevantVariables[name] = nil
 	}
 
 	// We'll put the parsed values into this map.
@@ -60,7 +60,7 @@ func (n *NodeTestRun) GetVariables(ctx *EvalContext, includeWarnings bool) (terr
 			continue
 		}
 
-		ctx, ctxDiags := ctx.HclContext(refs, run.ModuleConfig.Module.Variables)
+		ctx, ctxDiags := ctx.HclContext(refs)
 		diags = diags.Append(ctxDiags)
 
 		value := cty.DynamicVal
@@ -151,7 +151,7 @@ func (n *NodeTestRun) GetVariables(ctx *EvalContext, includeWarnings bool) (terr
 	// just referenced by parts of this run block. These must have been defined
 	// elsewhere, but we need to include them.
 
-	for variable := range relevantVariables {
+	for variable, reference := range relevantVariables {
 		if _, exists := values[variable]; exists {
 			// Then we've already got a value for this variable.
 			continue
@@ -164,7 +164,20 @@ func (n *NodeTestRun) GetVariables(ctx *EvalContext, includeWarnings bool) (terr
 			values[variable] = value
 			continue
 		}
-		if value, valueDiags := ctx.EvaluateUnparsedVariable(variable, nil); value != nil {
+
+		if reference == nil {
+			// this shouldn't happen, we only put nil references into the
+			// relevantVariables map for values derived from the configuration
+			// and all of these should have been set in previous for loop.
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Missing reference",
+				Detail:   fmt.Sprintf("The variable %q had no point of reference, which should not be possible. This is a bug in Terraform; please report it!", variable),
+			})
+			continue
+		}
+
+		if value, valueDiags := ctx.EvaluateUnparsedVariableDeprecated(variable, reference); value != nil {
 			values[variable] = value
 			diags = diags.Append(valueDiags)
 		}