Skip to content

Commit

Permalink
convert: Don't panic when collection element type unification fails
Browse files Browse the repository at this point in the history
Despite our best efforts to correctly unify element types during conversion,
sometimes some inconsistent types slip through anyway and make the
conversion panic.

Although in the long run it would be better to fix these problems upstream,
it's the responsibility of the caller of cty to not try to perform any
invalid operations, and so here we make the convert package pre-verify
that it's found a sensible result before trying to construct it, and
thus it can return a proper error if not rather than panicking.
  • Loading branch information
mildwonkey authored and apparentlymart committed Mar 16, 2021
1 parent 080b168 commit 139e794
Show file tree
Hide file tree
Showing 3 changed files with 389 additions and 26 deletions.
51 changes: 25 additions & 26 deletions cty/convert/conversion_collection.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package convert

import (
"fmt"

"github.com/zclconf/go-cty/cty"
)

Expand Down Expand Up @@ -51,6 +53,10 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion {
return cty.ListValEmpty(ety), nil
}

if !cty.CanListVal(elems) {
return cty.NilVal, fmt.Errorf("element types must all match for conversion to list")
}

return cty.ListVal(elems), nil
}
}
Expand Down Expand Up @@ -96,6 +102,10 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion {
return cty.SetValEmpty(ety), nil
}

if !cty.CanSetVal(elems) {
return cty.NilVal, fmt.Errorf("element types must all match for conversion to set")
}

return cty.SetVal(elems), nil
}
}
Expand Down Expand Up @@ -152,8 +162,8 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion {
}
}

if err := conversionCheckMapElementTypes(elems, path); err != nil {
return cty.NilVal, err
if !cty.CanMapVal(elems) {
return cty.NilVal, fmt.Errorf("element types must all match for conversion to map")
}

return cty.MapVal(elems), nil
Expand Down Expand Up @@ -237,6 +247,10 @@ func conversionTupleToSet(tupleType cty.Type, setEty cty.Type, unsafe bool) conv
i++
}

if !cty.CanSetVal(elems) {
return cty.NilVal, fmt.Errorf("element types must all match for conversion to set")
}

return cty.SetVal(elems), nil
}
}
Expand Down Expand Up @@ -324,6 +338,11 @@ func conversionTupleToList(tupleType cty.Type, listEty cty.Type, unsafe bool) co
if err != nil {
return cty.NilVal, err
}

if !cty.CanListVal(elems) {
return cty.NilVal, fmt.Errorf("element types must all match for conversion to list")
}

return cty.ListVal(elems), nil
}
}
Expand Down Expand Up @@ -402,8 +421,8 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co
}
}

if err := conversionCheckMapElementTypes(elems, path); err != nil {
return cty.NilVal, err
if !cty.CanMapVal(elems) {
return cty.NilVal, fmt.Errorf("attribute types must all match for conversion to map")
}

return cty.MapVal(elems), nil
Expand Down Expand Up @@ -487,7 +506,7 @@ func conversionUnifyCollectionElements(elems map[string]cty.Value, path cty.Path
}
unifiedType, _ := unify(elemTypes, unsafe)
if unifiedType == cty.NilType {
return nil, path.NewErrorf("collection elements cannot be unified")
return nil, path.NewErrorf("cannot find a common base type for all elements")
}

unifiedElems := make(map[string]cty.Value)
Expand All @@ -514,34 +533,14 @@ func conversionUnifyCollectionElements(elems map[string]cty.Value, path cty.Path
return unifiedElems, nil
}

func conversionCheckMapElementTypes(elems map[string]cty.Value, path cty.Path) error {
elementType := cty.NilType
elemPath := append(path.Copy(), nil)

for name, elem := range elems {
if elementType == cty.NilType {
elementType = elem.Type()
continue
}
if !elementType.Equals(elem.Type()) {
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.StringVal(name),
}
return elemPath.NewErrorf("%s is required", elementType.FriendlyName())
}
}

return nil
}

func conversionUnifyListElements(elems []cty.Value, path cty.Path, unsafe bool) ([]cty.Value, error) {
elemTypes := make([]cty.Type, len(elems))
for i, elem := range elems {
elemTypes[i] = elem.Type()
}
unifiedType, _ := unify(elemTypes, unsafe)
if unifiedType == cty.NilType {
return nil, path.NewErrorf("collection elements cannot be unified")
return nil, path.NewErrorf("cannot find a common base type for all elements")
}

ret := make([]cty.Value, len(elems))
Expand Down
48 changes: 48 additions & 0 deletions cty/value_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,20 @@ func ListValEmpty(element Type) Value {
}
}

// CanListVal returns false if the given Values can not be coalesced
// into a single List due to inconsistent element types.
func CanListVal(vals []Value) bool {
elementType := DynamicPseudoType
for _, val := range vals {
if elementType == DynamicPseudoType {
elementType = val.ty
} else if val.ty != DynamicPseudoType && !elementType.Equals(val.ty) {
return false
}
}
return true
}

// MapVal returns a Value of a map type whose element type is defined by
// the types of the given values, which must be homogenous.
//
Expand Down Expand Up @@ -227,6 +241,20 @@ func MapValEmpty(element Type) Value {
}
}

// CanMapVal returns false if the given Values can not be coalesced into a
// single Map due to inconsistent element types.
func CanMapVal(vals map[string]Value) bool {
elementType := DynamicPseudoType
for _, val := range vals {
if elementType == DynamicPseudoType {
elementType = val.ty
} else if val.ty != DynamicPseudoType && !elementType.Equals(val.ty) {
return false
}
}
return true
}

// SetVal returns a Value of set type whose element type is defined by
// the types of the given values, which must be homogenous.
//
Expand Down Expand Up @@ -267,6 +295,26 @@ func SetVal(vals []Value) Value {
}.WithMarks(markSets...)
}

// CanSetVal returns false if the given Values can not be coalesced
// into a single Set due to inconsistent element types.
func CanSetVal(vals []Value) bool {
elementType := DynamicPseudoType
var markSets []ValueMarks

for _, val := range vals {
if unmarkedVal, marks := val.UnmarkDeep(); len(marks) > 0 {
val = unmarkedVal
markSets = append(markSets, marks)
}
if elementType == DynamicPseudoType {
elementType = val.ty
} else if val.ty != DynamicPseudoType && !elementType.Equals(val.ty) {
return false
}
}
return true
}

// SetValFromValueSet returns a Value of set type based on an already-constructed
// ValueSet.
//
Expand Down

0 comments on commit 139e794

Please sign in to comment.