Allow variables to be explicitly typed (and enforced)

This allows variables to have explicit types, similar to Terraform
variables.  It uses HCL's `typeexpr` extension for the specification.
For conversion of overrides to complex types (when explicit typing is
provided), HCL's native JSON-based unmarshalling is used.

Typing is independent of any default, but if a default is provided, it
will be validated.  Similarly, if an override is provided, it will be
converted to that type.

When typing is not provided, previous behavior is used, namely
passing through as a string when no default, converting to primitives if
 the default was primitive, and failing otherwise (complex types).

For complex types, the happy path is lists of primitives, but in theory
any complex/composite type can be used provided they are expressed
correctly in JSON.  In the interest of simplicity and correctness, there
 are no shortcuts for lists.  There *is* a shortcut for strings as users
  don't provide them for untyped variables and would be unintuitive.

Signed-off-by: Roberto Villarreal <rrjjvv@yahoo.com>

bake/hcl_test.go

@@ -1,6 +1,7 @@
 package bake
 
 import (
+	"fmt"
 	"reflect"
 	"regexp"
 	"testing"
@@ -1645,6 +1646,328 @@ func TestHCLIndexOfFunc(t *testing.T) {
 	require.Empty(t, c.Targets[1].Tags[1])
 }
 
+func TestVarTypingSpec(t *testing.T) {
+	templ := `
+        variable "FOO" {
+          type = %s
+        }
+        target "default" {
+        }`
+
+	// not exhaustive, but the common ones
+	for _, s := range []string{
+		"bool", "number", "string", "any",
+		"list(string)", "set(string)", "tuple([string, number])",
+	} {
+		dt := fmt.Sprintf(templ, s)
+		_, err := ParseFile([]byte(dt), "docker-bake.hcl")
+		require.NoError(t, err)
+	}
+
+	for _, s := range []string{
+		"boolean",       // no synonyms/aliases
+		"BOOL",          // case matters
+		`lower("bool")`, // must be literals
+	} {
+		dt := fmt.Sprintf(templ, s)
+		_, err := ParseFile([]byte(dt), "docker-bake.hcl")
+		require.ErrorContains(t, err, "not a valid type")
+	}
+}
+
+func TestDefaultVarTypeEnforcement(t *testing.T) {
+	// To help prove a given default doesn't just pass the type check, but *is* that type,
+	// we use argValue to provide an expression that would work only on that type.
+	tests := []struct {
+		name       string
+		varType    string
+		varDefault any
+		argValue   string
+		wantValue  string
+		wantError  bool
+	}{
+		{
+			name:       "number (happy)",
+			varType:    "number",
+			varDefault: 99,
+			argValue:   "FOO + 1",
+			wantValue:  "100",
+		},
+		{
+			name:       "numeric string compatible with number",
+			varType:    "number",
+			varDefault: `"99"`,
+			argValue:   "FOO + 1",
+			wantValue:  "100",
+		},
+		{
+			name:       "boolean (happy)",
+			varType:    "bool",
+			varDefault: true,
+			argValue:   "and(FOO, true)",
+			wantValue:  "true",
+		},
+		{
+			name:       "numeric boolean compatible with boolean",
+			varType:    "bool",
+			varDefault: `"true"`,
+			argValue:   "and(FOO, true)",
+			wantValue:  "true",
+		},
+		// should be representative of flagrant primitive type mismatches; not worth listing all possibilities?
+		{
+			name:       "non-numeric string default incompatible with number",
+			varType:    "number",
+			varDefault: `"oops"`,
+			wantError:  true,
+		},
+		{
+			name:       "list of numbers (happy)",
+			varType:    "list(number)",
+			varDefault: "[2,3]",
+			argValue:   `join("", [for v in FOO: v + 1])`,
+			wantValue:  "34",
+		},
+		{
+			name:       "list of numbers with numeric strings okay",
+			varType:    "list(number)",
+			varDefault: `["2","3"]`,
+			argValue:   `join("", [for v in FOO: v + 1])`,
+			wantValue:  "34",
+		},
+		// represent flagrant mismatches for list types
+		{
+			name:       "non-numeric strings in numeric list rejected",
+			varType:    "list(number)",
+			varDefault: `["oops"]`,
+			wantError:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			argValue := tt.argValue
+			if argValue == "" {
+				argValue = "FOO"
+			}
+			dt := fmt.Sprintf(`
+                variable "FOO" {
+                    type = %s
+                    default = %v
+                }
+
+                target "default" {
+                    args = {
+                        foo = %s
+                    }
+                }`, tt.varType, tt.varDefault, argValue)
+			c, err := ParseFile([]byte(dt), "docker-bake.hcl")
+			if tt.wantError {
+				require.ErrorContains(t, err, "invalid type")
+			} else {
+				require.NoError(t, err)
+				if tt.wantValue != "" {
+					require.Equal(t, 1, len(c.Targets))
+					require.Equal(t, ptrstr(tt.wantValue), c.Targets[0].Args["foo"])
+				}
+			}
+		})
+	}
+}
+
+func TestDefaultVarTypeWithAttrValuesEnforcement(t *testing.T) {
+	tests := []struct {
+		name      string
+		attrValue any
+		varType   string
+		wantError bool
+	}{
+		{
+			name:      "attribute literal which matches var type",
+			attrValue: `"hello"`,
+			varType:   "string",
+		},
+		{
+			name:      "attribute literal which coerces to var type",
+			attrValue: `"99"`,
+			varType:   "number",
+		},
+		{
+			name:      "mismatch",
+			attrValue: 99,
+			varType:   "bool",
+			wantError: true,
+		},
+		{
+			name:      "attribute correctly typed via function",
+			attrValue: `split(",", "1,2,3")`,
+			varType:   "list(number)",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dt := fmt.Sprintf(`
+                BAR = %v
+                variable "FOO" {
+                    type = %s
+                    default = BAR
+                }
+
+                target "default" {
+                }`, tt.attrValue, tt.varType)
+			_, err := ParseFile([]byte(dt), "docker-bake.hcl")
+			if tt.wantError {
+				require.ErrorContains(t, err, "invalid type")
+				require.ErrorContains(t, err, "FOO default value")
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestTypedVarOverrides(t *testing.T) {
+	const convertFailure = "failed to convert FOO"
+	tests := []struct {
+		name         string
+		varType      string
+		override     string
+		argValue     string
+		wantValue    string
+		wantErrorMsg string
+	}{
+		{
+			name:      "boolean",
+			varType:   "bool",
+			override:  "true",
+			wantValue: "true",
+		},
+		{
+			name:      "number",
+			varType:   "number",
+			override:  "99",
+			wantValue: "99",
+		},
+		// this breaks the rule about needing proper JSON as it would violate
+		// the principle of least surprise and hinder usability
+		{
+			name:      "enquoted string accepted",
+			varType:   "string",
+			override:  "hello",
+			wantValue: "hello",
+		},
+		// similar to above, an environment variable with a quoted string would
+		// most likely be intended to be a string whose first and last characters
+		// are quotes
+		{
+			name:      "quoted string keeps quotes in value",
+			varType:   "string",
+			override:  `"hello"`,
+			wantValue: `"hello"`,
+		},
+		{
+			name:      "any",
+			varType:   "any",
+			override:  "[1,2]",
+			wantValue: "[1,2]",
+		},
+		{
+			name:         "any never convert to complex types",
+			varType:      "any",
+			override:     "[1,2]",
+			argValue:     "length(FOO)",
+			wantErrorMsg: "collection must be a list",
+		},
+		{
+			name:      "proper JSON list of strings",
+			varType:   "list(string)",
+			override:  `["hi","there"]`,
+			argValue:  `join("-", FOO)`,
+			wantValue: "hi-there",
+		},
+		// not that this *should* be an error, but pseudo-documentation that this is
+		// a scenario that might be expected to work, but doesn't (yet) for simplicity
+		{
+			name:         "JSON list of unquoted strings not okay",
+			varType:      "list(string)",
+			override:     `[hi,there]`,
+			wantErrorMsg: convertFailure,
+		},
+		// ditto above
+		{
+			name:         "CSV of quoted strings not okay",
+			varType:      "list(string)",
+			override:     `"hi","there"`,
+			wantErrorMsg: convertFailure,
+		},
+		// ditto above
+		{
+			name:         "CSV of unquoted strings not okay",
+			varType:      "list(string)",
+			override:     `hi,there`,
+			wantErrorMsg: convertFailure,
+		},
+		{
+			name:      "JSON list of numbers",
+			varType:   "list(number)",
+			override:  "[3, 1, 4]",
+			argValue:  `join("-", [for v in FOO: v + 1])`,
+			wantValue: "4-2-5",
+		},
+		{
+			name:      "JSON map of numbers",
+			varType:   "map(number)",
+			override:  `{"foo": 1, "bar": 2}`,
+			argValue:  `join("-", sort(values(FOO)))`,
+			wantValue: "1-2",
+		},
+		{
+			name:         "invalid JSON map of numbers",
+			varType:      "map(number)",
+			override:     `{"foo": "oops", "bar": 2}`,
+			wantErrorMsg: convertFailure,
+		},
+		{
+			name:      "JSON object",
+			varType:   `object({messages: list(string)})`,
+			override:  `{"messages": ["hi", "there"]}`,
+			argValue:  `join("-", FOO["messages"])`,
+			wantValue: "hi-there",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			argValue := tt.argValue
+			if argValue == "" {
+				argValue = "FOO"
+			}
+			dt := fmt.Sprintf(`
+                variable "FOO" {
+                    type = %s
+                }
+
+                target "default" {
+                    args = {
+                        foo = %s
+                    }
+                }`, tt.varType, argValue)
+			t.Setenv("FOO", tt.override)
+			c, err := ParseFile([]byte(dt), "docker-bake.hcl")
+			if tt.wantErrorMsg != "" {
+				require.ErrorContains(t, err, tt.wantErrorMsg)
+			} else {
+				require.NoError(t, err)
+				if tt.wantValue != "" {
+					require.Equal(t, 1, len(c.Targets))
+					require.Equal(t, ptrstr(tt.wantValue), c.Targets[0].Args["foo"])
+				}
+			}
+		})
+	}
+}
+
 func ptrstr(s any) *string {
 	var n *string
 	if reflect.ValueOf(s).Kind() == reflect.String {

bake/hclparser/hclparser.go

@@ -14,9 +14,11 @@ import (
 	"github.com/docker/buildx/bake/hclparser/gohcl"
 	"github.com/docker/buildx/util/userfunc"
 	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/hcl/v2/ext/typeexpr"
 	"github.com/pkg/errors"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/convert"
+	ctyjson "github.com/zclconf/go-cty/cty/json"
 )
 
 type Opt struct {
@@ -27,6 +29,7 @@ type Opt struct {
 
 type variable struct {
 	Name        string                `json:"-" hcl:"name,label"`
+	Type        hcl.Expression        `json:"type,omitempty" hcl:"type,optional"`
 	Default     *hcl.Attribute        `json:"default,omitempty" hcl:"default,optional"`
 	Description string                `json:"description,omitempty" hcl:"description,optional"`
 	Validations []*variableValidation `json:"validation,omitempty" hcl:"validation,block"`
@@ -267,38 +270,68 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
 		}
 	}()
 
+	// built-in vars aren't intended to be overridden and are statically typed as strings;
+	// no sense sending them through type checks or waiting to return them
+	if val, ok := p.opt.Vars[name]; ok {
+		vv := cty.StringVal(val)
+		v = &vv
+		return
+	}
+
+	var diags hcl.Diagnostics
+	varType := cty.DynamicPseudoType
 	def, ok := p.attrs[name]
-	if _, builtin := p.opt.Vars[name]; !ok && !builtin {
+	if !ok {
 		vr, ok := p.vars[name]
 		if !ok {
 			return errors.Wrapf(errUndefined{}, "variable %q does not exist", name)
 		}
 		def = vr.Default
 		ectx = p.ectx
+		varType, diags = typeConstraint(vr.Type)
+		if diags.HasErrors() {
+			return diags
+		}
 	}
 
 	if def == nil {
-		val, ok := p.opt.Vars[name]
-		if !ok {
-			val, _ = p.opt.LookupVar(name)
+		// lack of specified value is considered to have an empty string value,
+		// but any overrides get type checked
+		if _, ok := p.opt.LookupVar(name); !ok {
+			vv := cty.StringVal("")
+			v = &vv
+			return
 		}
-		vv := cty.StringVal(val)
-		v = &vv
-		return
 	}
 
-	if diags := p.loadDeps(ectx, def.Expr, nil, true); diags.HasErrors() {
-		return diags
-	}
-	vv, diags := def.Expr.Value(ectx)
-	if diags.HasErrors() {
-		return diags
+	var vv cty.Value
+	if def != nil {
+		if diags := p.loadDeps(ectx, def.Expr, nil, true); diags.HasErrors() {
+			return diags
+		}
+		vv, diags = def.Expr.Value(ectx)
+		if diags.HasErrors() {
+			return diags
+		}
+		vv, err = convert.Convert(vv, varType)
+		if err != nil {
+			return errors.Wrapf(err, "invalid type %s for variable %s default value", varType.FriendlyName(), name)
+		}
 	}
 
 	_, isVar := p.vars[name]
 
 	if envv, ok := p.opt.LookupVar(name); ok && isVar {
 		switch {
+		case varType.Equals(cty.String): // don't parse as JSON; users don't expect to have to quote strings
+			vv = cty.StringVal(envv)
+		case !varType.Equals(cty.DynamicPseudoType): // typing was explicitly specified
+			vv, err = ctyjson.Unmarshal([]byte(envv), varType)
+			if err != nil {
+				return errors.Wrapf(err, "failed to convert %s as required %s", name, varType.FriendlyName())
+			}
+		case def == nil: // no default from which to infer typing
+			vv = cty.StringVal(envv)
 		case vv.Type().Equals(cty.Bool):
 			b, err := strconv.ParseBool(envv)
 			if err != nil {
@@ -317,7 +350,6 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) {
 			}
 			vv = cty.NumberVal(big.NewFloat(n))
 		default:
-			// TODO: support lists with csv values
 			return errors.Errorf("unsupported type %s for variable %s", vv.Type().FriendlyName(), name)
 		}
 	}
@@ -907,6 +939,23 @@ func Parse(b hcl.Body, opt Opt, val any) (*ParseMeta, hcl.Diagnostics) {
 	}, nil
 }
 
+// typeConstraint wraps typeexpr.TypeConstraint to differentiate between errors in the
+// specification and errors due to being cty.NullVal (not provided).
+func typeConstraint(expr hcl.Expression) (cty.Type, hcl.Diagnostics) {
+	t, diag := typeexpr.TypeConstraint(expr)
+	if !diag.HasErrors() {
+		return t, diag
+	}
+	// if had errors, it could be because the expression is 'nil', i.e., unspecified
+	if v, err := expr.Value(nil); err == nil {
+		if v.IsNull() {
+			return cty.DynamicPseudoType, nil
+		}
+	}
+	// even if the evaluation resulted in error, the original (error) diagnostics are likely more useful
+	return t, diag
+}
+
 // wrapErrorDiagnostic wraps an error into a hcl.Diagnostics object.
 // If the error is already an hcl.Diagnostics object, it is returned as is.
 func wrapErrorDiagnostic(message string, err error, subject *hcl.Range, context *hcl.Range) hcl.Diagnostics {