Merge branch 'main' into main

Author: alisdair

.github/workflows/push.yml

@@ -28,24 +28,24 @@ jobs:
         run: |
           git config --global core.autocrlf false
       - name: "Fetch source code"
-        uses: actions/checkout@v2
+        uses: actions/checkout@755da8c3cf115ac066823e79a1e1788f8940201b # https://github.com/actions/checkout/releases/tag/v3.2.0
       - name: Install Go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@6edd4406fa81c3da01a34fa6f6343087c207a568 # https://github.com/actions/setup-go/releases/tag/v3.5.0
         with:
           go-version: 1.18
       - name: Go test
         run: |
-          go test ./...
+          go test ./... -race
 
   fmt_and_vet:
     name: "fmt and lint"
     runs-on: ubuntu-latest
 
     steps:
       - name: "Fetch source code"
-        uses: actions/checkout@v2
+        uses: actions/checkout@755da8c3cf115ac066823e79a1e1788f8940201b # https://github.com/actions/checkout/releases/tag/v3.2.0
       - name: Install Go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@6edd4406fa81c3da01a34fa6f6343087c207a568 # https://github.com/actions/setup-go/releases/tag/v3.5.0
         with:
           go-version: 1.18
       - name: "Check vet"

CHANGELOG.md

@@ -1,6 +1,26 @@
 # HCL Changelog
 
-## v2.15.0 (Unreleased)
+## v2.16.0 (Unreleased)
+
+### Bugs Fixed
+
+* ext/typeexpr: Modify the `Defaults` functionality to implement additional flexibility. HCL will now upcast lists and sets into tuples, and maps into objects, when applying default values if the applied defaults cause the elements within a target collection to have differing types. Previously, this would have resulted in a panic, now HCL will return a modified overall type. ([#574](https://github.com/hashicorp/hcl/pull/574))
+
+### Enhancements
+
+* ext/typeexpr: Users should return to the advice provided by v2.14.0, and apply the go-cty convert functionality *after* setting defaults on a given `cty.Value`, rather than before.  ([#574](https://github.com/hashicorp/hcl/pull/574))
+
+## v2.15.0 (November 10, 2022)
+
+### Bugs Fixed
+
+* ext/typeexpr: Skip null objects when applying defaults. This prevents crashes when null objects are creating inside collections, and stops incomplete objects being created with only optional attributes set. ([#567](https://github.com/hashicorp/hcl/pull/567))
+* ext/typeexpr: Ensure default values do not have optional metadata attached. This prevents crashes when default values are inserted into concrete go-cty values that have also been stripped of their optional metadata. ([#568](https://github.com/hashicorp/hcl/pull/568))
+
+### Enhancements
+
+* ext/typeexpr: With the [go-cty](https://github.com/zclconf/go-cty) upstream depenendency updated to v1.12.0, the `Defaults` struct and associated functions can apply additional and more flexible 'unsafe' conversions (examples include tuples into collections such as lists and sets, and additional safety around null and dynamic values). ([#564](https://github.com/hashicorp/hcl/pull/564))
+* ext/typeexpr: With the [go-cty](https://github.com/zclconf/go-cty) upstream depenendency updated to v1.12.0, users should now apply the go-cty convert functionality *before* setting defaults on a given `cty.Value`, rather than after, if they require a specific `cty.Type`.  ([#564](https://github.com/hashicorp/hcl/pull/564))
 
 ## v2.14.1 (September 23, 2022)
 

LICENSE

@@ -1,3 +1,5 @@
+Copyright (c) 2014 HashiCorp, Inc.
+
 Mozilla Public License, version 2.0
 
 1. Definitions

ext/typeexpr/README.md

@@ -66,6 +66,27 @@ types with weird attributes generally show up only from arbitrary object
 constructors in configuration files, which are usually treated either as maps
 or as the dynamic pseudo-type.
 
+### Optional Object Attributes
+
+As part of object expressions attributes can be marked as optional. Missing 
+object attributes would typically result in an error when type constraints are
+validated or used. Optional missing attributes, however, would not result in an 
+error. The `cty` ["convert" function](#the-convert-cty-function) will populate 
+missing optional attributes with null values.
+
+For example:
+
+* `object({name=string,age=optional(number)})`
+
+Optional attributes can also be specified with default values. The 
+`TypeConstraintWithDefaults` function will return a `Defaults` object that can
+be used to populate missing optional attributes with defaults in a given 
+`cty.Value`.
+
+For example:
+
+* `object({name=string,age=optional(number, 0)})`
+
 ## Type Constraints as Values
 
 Along with defining a convention for writing down types using HCL expression

ext/typeexpr/defaults.go

@@ -1,12 +1,16 @@
 package typeexpr
 
 import (
+	"sort"
+	"strconv"
+
 	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/convert"
 )
 
 // Defaults represents a type tree which may contain default values for
 // optional object attributes at any level. This is used to apply nested
-// defaults to an input value before converting it to the concrete type.
+// defaults to a given cty.Value before converting it to a concrete type.
 type Defaults struct {
 	// Type of the node for which these defaults apply. This is necessary in
 	// order to determine how to inspect the Defaults and Children collections.
@@ -35,123 +39,168 @@ type Defaults struct {
 // caller will have better context to report useful type conversion failure
 // diagnostics.
 func (d *Defaults) Apply(val cty.Value) cty.Value {
-	val, err := cty.TransformWithTransformer(val, &defaultsTransformer{defaults: d})
-
-	// The transformer should never return an error.
-	if err != nil {
-		panic(err)
-	}
-
-	return val
+	return d.apply(val)
 }
 
-// defaultsTransformer implements cty.Transformer, as a pre-order traversal,
-// applying defaults as it goes. The pre-order traversal allows us to specify
-// defaults more loosely for structural types, as the defaults for the types
-// will be applied to the default value later in the walk.
-type defaultsTransformer struct {
-	defaults *Defaults
-}
-
-var _ cty.Transformer = (*defaultsTransformer)(nil)
-
-func (t *defaultsTransformer) Enter(p cty.Path, v cty.Value) (cty.Value, error) {
-	// Cannot apply defaults to an unknown value
-	if !v.IsKnown() {
-		return v, nil
+func (d *Defaults) apply(v cty.Value) cty.Value {
+	// We don't apply defaults to null values or unknown values. To be clear,
+	// we will overwrite children values with defaults if they are null but not
+	// if the actual value is null.
+	if !v.IsKnown() || v.IsNull() {
+		return v
 	}
 
-	// Look up the defaults for this path.
-	defaults := t.defaults.traverse(p)
-
-	// If we have no defaults, nothing to do.
-	if len(defaults) == 0 {
-		return v, nil
+	// Also, do nothing if we have no defaults to apply.
+	if len(d.DefaultValues) == 0 && len(d.Children) == 0 {
+		return v
 	}
 
-	// Ensure we are working with an object or map.
-	vt := v.Type()
-	if !vt.IsObjectType() && !vt.IsMapType() {
-		// Cannot apply defaults because the value type is incompatible.
-		// We'll ignore this and let the later conversion stage display a
-		// more useful diagnostic.
-		return v, nil
+	v, marks := v.Unmark()
+
+	switch {
+	case v.Type().IsSetType(), v.Type().IsListType(), v.Type().IsTupleType():
+		values := d.applyAsSlice(v)
+
+		if v.Type().IsSetType() {
+			if len(values) == 0 {
+				v = cty.SetValEmpty(v.Type().ElementType())
+				break
+			}
+			if converts := d.unifyAsSlice(values); len(converts) > 0 {
+				v = cty.SetVal(converts).WithMarks(marks)
+				break
+			}
+		} else if v.Type().IsListType() {
+			if len(values) == 0 {
+				v = cty.ListValEmpty(v.Type().ElementType())
+				break
+			}
+			if converts := d.unifyAsSlice(values); len(converts) > 0 {
+				v = cty.ListVal(converts)
+				break
+			}
+		}
+		v = cty.TupleVal(values)
+	case v.Type().IsObjectType(), v.Type().IsMapType():
+		values := d.applyAsMap(v)
+
+		for key, defaultValue := range d.DefaultValues {
+			if value, ok := values[key]; !ok || value.IsNull() {
+				if defaults, ok := d.Children[key]; ok {
+					values[key] = defaults.apply(defaultValue)
+					continue
+				}
+				values[key] = defaultValue
+			}
+		}
+
+		if v.Type().IsMapType() {
+			if len(values) == 0 {
+				v = cty.MapValEmpty(v.Type().ElementType())
+				break
+			}
+			if converts := d.unifyAsMap(values); len(converts) > 0 {
+				v = cty.MapVal(converts)
+				break
+			}
+		}
+		v = cty.ObjectVal(values)
 	}
 
-	// Unmark the value and reapply the marks later.
-	v, valMarks := v.Unmark()
+	return v.WithMarks(marks)
+}
 
-	// Convert the given value into an attribute map (if it's non-null and
-	// non-empty).
-	attrs := make(map[string]cty.Value)
-	if !v.IsNull() && v.LengthInt() > 0 {
-		attrs = v.AsValueMap()
+func (d *Defaults) applyAsSlice(value cty.Value) []cty.Value {
+	var elements []cty.Value
+	for ix, element := range value.AsValueSlice() {
+		if childDefaults := d.getChild(ix); childDefaults != nil {
+			element = childDefaults.apply(element)
+			elements = append(elements, element)
+			continue
+		}
+		elements = append(elements, element)
 	}
+	return elements
+}
 
-	// Apply defaults where attributes are missing, constructing a new
-	// value with the same marks.
-	for attr, defaultValue := range defaults {
-		if attrValue, ok := attrs[attr]; !ok || attrValue.IsNull() {
-			attrs[attr] = defaultValue
+func (d *Defaults) applyAsMap(value cty.Value) map[string]cty.Value {
+	elements := make(map[string]cty.Value)
+	for key, element := range value.AsValueMap() {
+		if childDefaults := d.getChild(key); childDefaults != nil {
+			elements[key] = childDefaults.apply(element)
+			continue
 		}
+		elements[key] = element
 	}
-
-	// We construct an object even if the input value was a map, as the
-	// type of an attribute's default value may be incompatible with the
-	// map element type.
-	return cty.ObjectVal(attrs).WithMarks(valMarks), nil
+	return elements
 }
 
-func (t *defaultsTransformer) Exit(p cty.Path, v cty.Value) (cty.Value, error) {
-	return v, nil
+func (d *Defaults) getChild(key interface{}) *Defaults {
+	switch {
+	case d.Type.IsMapType(), d.Type.IsSetType(), d.Type.IsListType():
+		return d.Children[""]
+	case d.Type.IsTupleType():
+		return d.Children[strconv.Itoa(key.(int))]
+	case d.Type.IsObjectType():
+		return d.Children[key.(string)]
+	default:
+		return nil
+	}
 }
 
-// traverse walks the abstract defaults structure for a given path, returning
-// a set of default values (if any are present) or nil (if not). This operation
-// differs from applying a path to a value because we need to customize the
-// traversal steps for collection types, where a single set of defaults can be
-// applied to an arbitrary number of elements.
-func (d *Defaults) traverse(path cty.Path) map[string]cty.Value {
-	if len(path) == 0 {
-		return d.DefaultValues
+func (d *Defaults) unifyAsSlice(values []cty.Value) []cty.Value {
+	var types []cty.Type
+	for _, value := range values {
+		types = append(types, value.Type())
+	}
+	unify, conversions := convert.UnifyUnsafe(types)
+	if unify == cty.NilType {
+		return nil
 	}
 
-	switch s := path[0].(type) {
-	case cty.GetAttrStep:
-		if d.Type.IsObjectType() {
-			// Attribute path steps are normally applied to objects, where each
-			// attribute may have different defaults.
-			return d.traverseChild(s.Name, path)
-		} else if d.Type.IsMapType() {
-			// Literal values for maps can result in attribute path steps, in which
-			// case we need to disregard the attribute name, as maps can have only
-			// one child.
-			return d.traverseChild("", path)
+	var converts []cty.Value
+	for ix := 0; ix < len(conversions); ix++ {
+		if conversions[ix] == nil {
+			converts = append(converts, values[ix])
+			continue
 		}
 
-		return nil
-	case cty.IndexStep:
-		if d.Type.IsTupleType() {
-			// Tuples can have different types for each element, so we look
-			// up the defaults based on the index key.
-			return d.traverseChild(s.Key.AsBigFloat().String(), path)
-		} else if d.Type.IsCollectionType() {
-			// Defaults for collection element types are stored with a blank
-			// key, so we disregard the index key.
-			return d.traverseChild("", path)
+		converted, err := conversions[ix](values[ix])
+		if err != nil {
+			return nil
 		}
-		return nil
-	default:
-		// At time of writing there are no other path step types.
-		return nil
+		converts = append(converts, converted)
 	}
+	return converts
 }
 
-// traverseChild continues the traversal for a given child key, and mutually
-// recurses with traverse.
-func (d *Defaults) traverseChild(name string, path cty.Path) map[string]cty.Value {
-	if child, ok := d.Children[name]; ok {
-		return child.traverse(path[1:])
+func (d *Defaults) unifyAsMap(values map[string]cty.Value) map[string]cty.Value {
+	var keys []string
+	for key := range values {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+
+	var types []cty.Type
+	for _, key := range keys {
+		types = append(types, values[key].Type())
+	}
+	unify, conversions := convert.UnifyUnsafe(types)
+	if unify == cty.NilType {
+		return nil
+	}
+
+	converts := make(map[string]cty.Value)
+	for ix, key := range keys {
+		if conversions[ix] == nil {
+			converts[key] = values[key]
+			continue
+		}
+
+		var err error
+		if converts[key], err = conversions[ix](values[key]); err != nil {
+			return nil
+		}
 	}
-	return nil
+	return converts
 }