configs: allow full type constraints for variables

Previously we just ported over the simple "string", "list", and "map" type
hint keywords from the old loader, which exist primarily as hints to the
CLI for whether to treat -var=... arguments and environment variables as
literal strings or as HCL expressions.

However, we've been requested before to allow more specific constraints
here because it's generally better UX for a type error to be detected
within an expression in a calling "module" block rather than at some point
deep inside a third-party module.

To allow for more specific constraints, here we use the type constraint
expression syntax defined as an extension within HCL, which uses the
variable and function call syntaxes to represent types rather than values,
like this:
 - string
 - number
 - bool
 - list(string)
 - list(any)
 - list(map(string))
 - object({id=string,name=string})

In native HCL syntax this looks like:

    variable "foo" {
      type = map(string)
    }

In JSON, this looks like:

    {
      "variable": {
        "foo": {
          "type": "map(string)"
        }
      }
    }

The selection of literal processing or HCL parsing of CLI-set values is
now explicit in the model and separate from the type, though it's still
derived from the type constraint and thus not directly controllable in
configuration.

Since this syntax is more complex than the keywords that replaced it, for
now the simpler keywords are still supported and "list" and "map" are
interpreted as list(any) and map(any) respectively, mimicking how they
were interpreted by Terraform 0.11 and earlier. For the time being our
documentation should continue to recommend these shorthand versions until
we gain more experience with the more-specific type constraints; most
users should just make use of the additional primitive type constraints
this enables: bool and number.

As a result of these more-complete type constraints, we can now type-check
the default value at config load time, which has the nice side-effect of
allowing us to produce a tailored error message if an override file
produces an invalid situation; previously the result was rather confusing
because the error message referred to the original definition of the
variable and not the overridden parts.

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 {
+	if ov.Type != cty.NilType {
-		v.TypeHint = ov.TypeHint
+		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,36 +73,147 @@ 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...)
-
-		switch hcl.ExprAsKeyword(expr) {
-		case "string":
-			v.TypeHint = TypeHintString
-		case "list":
-			v.TypeHint = TypeHintList
-		case "map":
-			v.TypeHint = TypeHintMap
-		default:
-			diags = append(diags, &hcl.Diagnostic{
-				Severity: hcl.DiagError,
-				Summary:  "Invalid variable type hint",
-				Detail:   "The type argument requires one of the following keywords: string, list, or map.",
-				Subject:  expr.Range().Ptr(),
-			})
-		}
-	}
-
 	return v, diags
 }
 
+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":
+			return cty.String, VariableParseLiteral, diags
+		case "list":
+			return cty.List(cty.DynamicPseudoType), VariableParseHCL, diags
+		case "map":
+			return cty.Map(cty.DynamicPseudoType), VariableParseHCL, diags
+		default:
+			return cty.DynamicPseudoType, VariableParseHCL, hcl.Diagnostics{{
+				Severity: hcl.DiagError,
+				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(),
+			}}
+		}
+	}
+
+	// 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.
 type Output struct {
 	Name        string

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" {