configs: allow full type constraints for variables

configs/module_merge.go

@@ -1,8 +1,11 @@
 package configs
 
 import (
+	"fmt"
+
 	"github.com/hashicorp/hcl2/hcl"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/convert"
 )
 
 // The methods in this file are used by Module.mergeFile to apply overrides
@@ -52,8 +55,56 @@ func (v *Variable) merge(ov *Variable) hcl.Diagnostics {
 	if ov.Default != cty.NilVal {
 		v.Default = ov.Default
 	}
-	if ov.TypeHint != TypeHintNone {
-		v.TypeHint = ov.TypeHint
+	if ov.Type != cty.NilType {
+		v.Type = ov.Type
+	}
+	if ov.ParsingMode != 0 {
+		v.ParsingMode = ov.ParsingMode
+	}
+
+	// If the override file overrode type without default or vice-versa then
+	// it may have created an invalid situation, which we'll catch now by
+	// attempting to re-convert the value.
+	//
+	// Note that here we may be re-converting an already-converted base value
+	// from the base config. This will be a no-op if the type was not changed,
+	// but in particular might be user-observable in the edge case where the
+	// literal value in config could've been converted to the overridden type
+	// constraint but the converted value cannot. In practice, this situation
+	// should be rare since most of our conversions are interchangable.
+	if v.Default != cty.NilVal {
+		val, err := convert.Convert(v.Default, v.Type)
+		if err != nil {
+			// What exactly we'll say in the error message here depends on whether
+			// it was Default or Type that was overridden here.
+			switch {
+			case ov.Type != cty.NilType && ov.Default == cty.NilVal:
+				// If only the type was overridden
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid default value for variable",
+					Detail:   fmt.Sprintf("Overriding this variable's type constraint has made its default value invalid: %s.", err),
+					Subject:  &ov.DeclRange,
+				})
+			case ov.Type == cty.NilType && ov.Default != cty.NilVal:
+				// Only the default was overridden
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid default value for variable",
+					Detail:   fmt.Sprintf("The overridden default value for this variable is not compatible with the variable's type constraint: %s.", err),
+					Subject:  &ov.DeclRange,
+				})
+			default:
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid default value for variable",
+					Detail:   fmt.Sprintf("This variable's default value is not compatible with its type constraint: %s.", err),
+					Subject:  &ov.DeclRange,
+				})
+			}
+		} else {
+			v.Default = val
+		}
 	}
 
 	return diags

configs/module_merge_test.go

@@ -22,7 +22,8 @@ func TestModuleOverrideVariable(t *testing.T) {
 			Description:    "b_override description",
 			DescriptionSet: true,
 			Default:        cty.StringVal("b_override"),
-			TypeHint:       TypeHintString,
+			Type:           cty.String,
+			ParsingMode:    VariableParseLiteral,
 			DeclRange: hcl.Range{
 				Filename: "test-fixtures/valid-modules/override-variable/primary.tf",
 				Start: hcl.Pos{
@@ -42,7 +43,8 @@ func TestModuleOverrideVariable(t *testing.T) {
 			Description:    "base description",
 			DescriptionSet: true,
 			Default:        cty.StringVal("b_override partial"),
-			TypeHint:       TypeHintString,
+			Type:           cty.String,
+			ParsingMode:    VariableParseLiteral,
 			DeclRange: hcl.Range{
 				Filename: "test-fixtures/valid-modules/override-variable/primary.tf",
 				Start: hcl.Pos{

configs/named_values.go

@@ -3,10 +3,12 @@ package configs
 import (
 	"fmt"
 
+	"github.com/hashicorp/hcl2/ext/typeexpr"
 	"github.com/hashicorp/hcl2/gohcl"
 	"github.com/hashicorp/hcl2/hcl"
 	"github.com/hashicorp/hcl2/hcl/hclsyntax"
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/convert"
 )
 
 // A consistent detail message for all "not a valid identifier" diagnostics.
@@ -17,19 +19,29 @@ type Variable struct {
 	Name        string
 	Description string
 	Default     cty.Value
-	TypeHint    VariableTypeHint
+	Type        cty.Type
+	ParsingMode VariableParsingMode
 
 	DescriptionSet bool
 
 	DeclRange hcl.Range
 }
 
-func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) {
+func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagnostics) {
 	v := &Variable{
 		Name:      block.Labels[0],
 		DeclRange: block.DefRange,
 	}
 
+	// Unless we're building an override, we'll set some defaults
+	// which we might override with attributes below. We leave these
+	// as zero-value in the override case so we can recognize whether
+	// or not they are set when we merge.
+	if !override {
+		v.Type = cty.DynamicPseudoType
+		v.ParsingMode = VariableParseLiteral
+	}
+
 	content, diags := block.Body.Content(variableBlockSchema)
 
 	if !hclsyntax.ValidIdentifier(v.Name) {
@@ -61,34 +73,145 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) {
 		v.DescriptionSet = true
 	}
 
+	if attr, exists := content.Attributes["type"]; exists {
+		ty, parseMode, tyDiags := decodeVariableType(attr.Expr)
+		diags = append(diags, tyDiags...)
+		v.Type = ty
+		v.ParsingMode = parseMode
+	}
+
 	if attr, exists := content.Attributes["default"]; exists {
 		val, valDiags := attr.Expr.Value(nil)
 		diags = append(diags, valDiags...)
+
+		// Convert the default to the expected type so we can catch invalid
+		// defaults early and allow later code to assume validity.
+		// Note that this depends on us having already processed any "type"
+		// attribute above.
+		// However, we can't do this if we're in an override file where
+		// the type might not be set; we'll catch that during merge.
+		if v.Type != cty.NilType {
+			var err error
+			val, err = convert.Convert(val, v.Type)
+			if err != nil {
+				diags = append(diags, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Invalid default value for variable",
+					Detail:   fmt.Sprintf("This default value is not compatible with the variable's type constraint: %s.", err),
+					Subject:  attr.Expr.Range().Ptr(),
+				})
+				val = cty.DynamicVal
+			}
+		}
+
 		v.Default = val
 	}
 
-	if attr, exists := content.Attributes["type"]; exists {
-		expr, shimDiags := shimTraversalInString(attr.Expr, true)
-		diags = append(diags, shimDiags...)
+	return v, diags
+}
 
-		switch hcl.ExprAsKeyword(expr) {
+func decodeVariableType(expr hcl.Expression) (cty.Type, VariableParsingMode, hcl.Diagnostics) {
+	if exprIsNativeQuotedString(expr) {
+		// Here we're accepting the pre-0.12 form of variable type argument where
+		// the string values "string", "list" and "map" are accepted has a hint
+		// about the type used primarily for deciding how to parse values
+		// given on the command line and in environment variables.
+		// Only the native syntax ends up in this codepath; we handle the
+		// JSON syntax (which is, of course, quoted even in the new format)
+		// in the normal codepath below.
+		val, diags := expr.Value(nil)
+		if diags.HasErrors() {
+			return cty.DynamicPseudoType, VariableParseHCL, diags
+		}
+		str := val.AsString()
+		switch str {
 		case "string":
-			v.TypeHint = TypeHintString
+			return cty.String, VariableParseLiteral, diags
 		case "list":
-			v.TypeHint = TypeHintList
+			return cty.List(cty.DynamicPseudoType), VariableParseHCL, diags
 		case "map":
-			v.TypeHint = TypeHintMap
+			return cty.Map(cty.DynamicPseudoType), VariableParseHCL, diags
 		default:
-			diags = append(diags, &hcl.Diagnostic{
+			return cty.DynamicPseudoType, VariableParseHCL, hcl.Diagnostics{{
 				Severity: hcl.DiagError,
-				Summary:  "Invalid variable type hint",
-				Detail:   "The type argument requires one of the following keywords: string, list, or map.",
+				Summary:  "Invalid legacy variable type hint",
+				Detail:   `The legacy variable type hint form, using a quoted string, allows only the values "string", "list", and "map". To provide a full type expression, remove the surrounding quotes and give the type expression directly.`,
 				Subject:  expr.Range().Ptr(),
-			})
+			}}
 		}
 	}
 
-	return v, diags
+	// First we'll deal with some shorthand forms that the HCL-level type
+	// expression parser doesn't include. These both emulate pre-0.12 behavior
+	// of allowing a list or map of any element type as long as all of the
+	// elements are consistent. This is the same as list(any) or map(any).
+	switch hcl.ExprAsKeyword(expr) {
+	case "list":
+		return cty.List(cty.DynamicPseudoType), VariableParseHCL, nil
+	case "map":
+		return cty.Map(cty.DynamicPseudoType), VariableParseHCL, nil
+	}
+
+	ty, diags := typeexpr.TypeConstraint(expr)
+	if diags.HasErrors() {
+		return cty.DynamicPseudoType, VariableParseHCL, diags
+	}
+
+	switch {
+	case ty.IsPrimitiveType():
+		// Primitive types use literal parsing.
+		return ty, VariableParseLiteral, diags
+	default:
+		// Everything else uses HCL parsing
+		return ty, VariableParseHCL, diags
+	}
+}
+
+// VariableParsingMode defines how values of a particular variable given by
+// text-only mechanisms (command line arguments and environment variables)
+// should be parsed to produce the final value.
+type VariableParsingMode rune
+
+// VariableParseLiteral is a variable parsing mode that just takes the given
+// string directly as a cty.String value.
+const VariableParseLiteral VariableParsingMode = 'L'
+
+// VariableParseHCL is a variable parsing mode that attempts to parse the given
+// string as an HCL expression and returns the result.
+const VariableParseHCL VariableParsingMode = 'H'
+
+// Parse uses the receiving parsing mode to process the given variable value
+// string, returning the result along with any diagnostics.
+//
+// A VariableParsingMode does not know the expected type of the corresponding
+// variable, so it's the caller's responsibility to attempt to convert the
+// result to the appropriate type and return to the user any diagnostics that
+// conversion may produce.
+//
+// The given name is used to create a synthetic filename in case any diagnostics
+// must be generated about the given string value. This should be the name
+// of the root module variable whose value will be populated from the given
+// string.
+//
+// If the returned diagnostics has errors, the returned value may not be
+// valid.
+func (m VariableParsingMode) Parse(name, value string) (cty.Value, hcl.Diagnostics) {
+	switch m {
+	case VariableParseLiteral:
+		return cty.StringVal(value), nil
+	case VariableParseHCL:
+		fakeFilename := fmt.Sprintf("<value for var.%s>", name)
+		expr, diags := hclsyntax.ParseExpression([]byte(value), fakeFilename, hcl.Pos{Line: 1, Column: 1})
+		if diags.HasErrors() {
+			return cty.DynamicVal, diags
+		}
+		val, valDiags := expr.Value(nil)
+		diags = append(diags, valDiags...)
+		return val, diags
+	default:
+		// Should never happen
+		panic(fmt.Errorf("Parse called on invalid VariableParsingMode %#v", m))
+	}
 }
 
 // Output represents an "output" block in a module or file.

configs/parser_config.go

@@ -86,7 +86,7 @@ func (p *Parser) loadConfigFile(path string, override bool) (*File, hcl.Diagnost
 			}
 
 		case "variable":
-			cfg, cfgDiags := decodeVariableBlock(block)
+			cfg, cfgDiags := decodeVariableBlock(block, override)
 			diags = append(diags, cfgDiags...)
 			if cfg != nil {
 				file.Variables = append(file.Variables, cfg)

configs/parser_config_test.go

@@ -97,7 +97,7 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) {
 		{
 			"invalid-files/variable-type-unknown.tf",
 			hcl.DiagError,
-			"Invalid variable type hint",
+			"Invalid type specification",
 		},
 		{
 			"invalid-files/unexpected-attr.tf",

configs/test-fixtures/invalid-files/variable-bad-default.tf

@@ -0,0 +1,4 @@
+variable "incorrectly_typed_default" {
+    type    = list(string)
+    default = "hello"
+}

configs/test-fixtures/invalid-modules/override-variable-causes-bad-default/base.tf

@@ -0,0 +1,4 @@
+variable "foo" {
+  type    = list(string)
+  default = ["this is valid"]
+}

configs/test-fixtures/invalid-modules/override-variable-causes-bad-default/override.tf

@@ -0,0 +1,5 @@
+variable "foo" {
+  type = string
+  # Since we didn't also override the default, this is now invalid because
+  # the existing default is not compatible with "string".
+}

configs/test-fixtures/valid-files/variables.tf

@@ -12,7 +12,7 @@ variable "baz" {
 
 variable "bar-baz" {
   default = []
-  type    = list
+  type    = list(string)
 }
 
 variable "cheeze_pizza" {