Forward to 1.5.0 (#10)

* Changelog preparation for future 1.4.2 release

* convert: Fix panic: heterogeneous tuple with null

Tuples with elements of different types can be converted to homogeneous
collections (sets or lists), so long as their elements are unifiable.
For example:

  list("a", "b")     // all elements have the same type
  list("a", 5)       // "a" and 5 can be unified to string
  list("a", 5, null) // null is a valid value for string

However, tuples with elements which are not unifiable cannot be
converted to homogeneous collections:

  list(["a"], "b")   // no common type for list(string) and string

This commit fixes a panic for this failure case, when the tuple contains
both non-unifiable types and a null value:

  list(["a"], "b", null) // should not panic

The null value was causing the unification process to result in a list
or set of dynamic type, which causes the conversion functions to pass
through the original value. This meant that in the final conversion
step, we would attempt to construct a list or set of different values,
which panics.

* Update CHANGELOG.md

* cty: Value.HasWhollyKnownType

This tests whether a value contains any unknown values of unknown type.

This is different than just testing if any of the nested types
are DynamicPseudoType, because a null value of
DynamicPseudoType has a different meaning than an
unknown value of DynamicPseudoType: the null value's
type can't become any more "known".

* Update CHANGELOG.md

* v1.5.0

* Update CHANGELOG.md

---------

Co-authored-by: Martin Atkins <[email protected]>
Co-authored-by: Alisdair McDiarmid <[email protected]>
Co-authored-by: James Bardin <[email protected]>

Author: 4 people

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 1.5.0 (Unreleased)
+
+* `cty`: New `Value.HasWhollyKnownType` method, for testing whether a value's type could potentially change if any unknown values it was constructed from were to become known. ([#55](https://github.com/zclconf/go-cty/pull/55))
+* `convert`: Fix incorrect panic when converting a tuple with a dynamic-typed null member into a list or set, due to overly-liberal type unification. ([#56](https://github.com/zclconf/go-cty/pull/56))
+
 # 1.4.2 (Unreleased)
 
 * `function/stdlib`: The `jsonencode` function will now correctly accept a null as its argument, and produce the JSON representation `"null"` rather than returning an error. ([#54](https://github.com/zclconf/go-cty/pull/54))

cty/convert/conversion_collection.go

@@ -156,34 +156,45 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion {
 // given tuple type and return a set of the given element type.
 //
 // Will panic if the given tupleType isn't actually a tuple type.
-func conversionTupleToSet(tupleType cty.Type, listEty cty.Type, unsafe bool) conversion {
+func conversionTupleToSet(tupleType cty.Type, setEty cty.Type, unsafe bool) conversion {
 	tupleEtys := tupleType.TupleElementTypes()
 
 	if len(tupleEtys) == 0 {
 		// Empty tuple short-circuit
 		return func(val cty.Value, path cty.Path) (cty.Value, error) {
-			return cty.SetValEmpty(listEty), nil
+			return cty.SetValEmpty(setEty), nil
 		}
 	}
 
-	if listEty == cty.DynamicPseudoType {
+	if setEty == cty.DynamicPseudoType {
 		// This is a special case where the caller wants us to find
 		// a suitable single type that all elements can convert to, if
 		// possible.
-		listEty, _ = unify(tupleEtys, unsafe)
-		if listEty == cty.NilType {
+		setEty, _ = unify(tupleEtys, unsafe)
+		if setEty == cty.NilType {
 			return nil
 		}
+
+		// If the set element type after unification is still the dynamic
+		// type, the only way this can result in a valid set is if all values
+		// are of dynamic type
+		if setEty == cty.DynamicPseudoType {
+			for _, tupleEty := range tupleEtys {
+				if !tupleEty.Equals(cty.DynamicPseudoType) {
+					return nil
+				}
+			}
+		}
 	}
 
 	elemConvs := make([]conversion, len(tupleEtys))
 	for i, tupleEty := range tupleEtys {
-		if tupleEty.Equals(listEty) {
+		if tupleEty.Equals(setEty) {
 			// no conversion required
 			continue
 		}
 
-		elemConvs[i] = getConversion(tupleEty, listEty, unsafe)
+		elemConvs[i] = getConversion(tupleEty, setEty, unsafe)
 		if elemConvs[i] == nil {
 			// If any of our element conversions are impossible, then the our
 			// whole conversion is impossible.
@@ -244,6 +255,17 @@ func conversionTupleToList(tupleType cty.Type, listEty cty.Type, unsafe bool) co
 		if listEty == cty.NilType {
 			return nil
 		}
+
+		// If the list element type after unification is still the dynamic
+		// type, the only way this can result in a valid list is if all values
+		// are of dynamic type
+		if listEty == cty.DynamicPseudoType {
+			for _, tupleEty := range tupleEtys {
+				if !tupleEty.Equals(cty.DynamicPseudoType) {
+					return nil
+				}
+			}
+		}
 	}
 
 	elemConvs := make([]conversion, len(tupleEtys))

cty/convert/public_test.go

@@ -677,6 +677,95 @@ func TestConvert(t *testing.T) {
 			}),
 			WantError: false,
 		},
+		// https://github.com/hashicorp/terraform/issues/24377:
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.ListVal([]cty.Value{cty.StringVal("a")}),
+				cty.StringVal("b"),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type:      cty.Set(cty.DynamicPseudoType),
+			WantError: true,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.ListVal([]cty.Value{cty.StringVal("a")}),
+				cty.StringVal("b"),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type:      cty.List(cty.DynamicPseudoType),
+			WantError: true,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.ListVal([]cty.Value{cty.StringVal("a")}),
+				cty.StringVal("b"),
+			}),
+			Type:      cty.Set(cty.DynamicPseudoType),
+			WantError: true,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.ListVal([]cty.Value{cty.StringVal("a")}),
+				cty.StringVal("b"),
+			}),
+			Type:      cty.List(cty.DynamicPseudoType),
+			WantError: true,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.StringVal("a"),
+				cty.NumberIntVal(9),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type: cty.Set(cty.DynamicPseudoType),
+			Want: cty.SetVal([]cty.Value{
+				cty.StringVal("a"),
+				cty.StringVal("9"),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			WantError: false,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.StringVal("a"),
+				cty.NumberIntVal(9),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type: cty.List(cty.DynamicPseudoType),
+			Want: cty.ListVal([]cty.Value{
+				cty.StringVal("a"),
+				cty.StringVal("9"),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			WantError: false,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type: cty.Set(cty.DynamicPseudoType),
+			Want: cty.SetVal([]cty.Value{
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			WantError: false,
+		},
+		{
+			Value: cty.TupleVal([]cty.Value{
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			Type: cty.List(cty.DynamicPseudoType),
+			Want: cty.ListVal([]cty.Value{
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+				cty.NullVal(cty.DynamicPseudoType),
+			}),
+			WantError: false,
+		},
 	}
 
 	for _, test := range tests {

cty/type.go

@@ -87,7 +87,7 @@ func (t Type) HasDynamicTypes() bool {
 	case t.IsPrimitiveType():
 		return false
 	case t.IsCollectionType():
-		return false
+		return t.ElementType().HasDynamicTypes()
 	case t.IsObjectType():
 		attrTypes := t.AttributeTypes()
 		for _, at := range attrTypes {

cty/type_test.go

@@ -0,0 +1,56 @@
+package cty
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestHasDynamicTypes(t *testing.T) {
+	tests := []struct {
+		ty       Type
+		expected bool
+	}{
+		{
+			DynamicPseudoType,
+			true,
+		},
+		{
+			List(DynamicPseudoType),
+			true,
+		},
+		{
+			Tuple([]Type{String, DynamicPseudoType}),
+			true,
+		},
+		{
+			Object(map[string]Type{
+				"a":       String,
+				"unknown": DynamicPseudoType,
+			}),
+			true,
+		},
+		{
+			List(Object(map[string]Type{
+				"a":       String,
+				"unknown": DynamicPseudoType,
+			})),
+			true,
+		},
+		{
+			Tuple([]Type{Object(map[string]Type{
+				"a":       String,
+				"unknown": DynamicPseudoType,
+			})}),
+			true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%#v.HasDynamicTypes()", test.ty), func(t *testing.T) {
+			got := test.ty.HasDynamicTypes()
+			if got != test.expected {
+				t.Errorf("Equals returned %#v; want %#v", got, test.expected)
+			}
+		})
+	}
+}

cty/value.go

@@ -106,3 +106,37 @@ func (val Value) IsWhollyKnown() bool {
 		return true
 	}
 }
+
+// HasWhollyKnownType checks if the value is dynamic, or contains any nested
+// DynamicVal. This implies that both the value is not known, and the final
+// type may change.
+func (val Value) HasWhollyKnownType() bool {
+	// a null dynamic type is known
+	if val.IsNull() {
+		return true
+	}
+
+	// an unknown DynamicPseudoType is a DynamicVal, but we don't want to
+	// check that value for equality here, since this method is used within the
+	// equality check.
+	if !val.IsKnown() && val.ty == DynamicPseudoType {
+		return false
+	}
+
+	if val.CanIterateElements() {
+		// if the value is not known, then we can look directly at the internal
+		// types
+		if !val.IsKnown() {
+			return !val.ty.HasDynamicTypes()
+		}
+
+		for it := val.ElementIterator(); it.Next(); {
+			_, ev := it.Element()
+			if !ev.HasWhollyKnownType() {
+				return false
+			}
+		}
+	}
+
+	return true
+}

cty/value_ops.go

@@ -133,9 +133,9 @@ func (val Value) Equals(other Value) Value {
 	case val.IsKnown() && !other.IsKnown():
 		switch {
 		case val.IsNull(), other.ty.HasDynamicTypes():
-			// If known is Null, we need to wait for the unkown value since
+			// If known is Null, we need to wait for the unknown value since
 			// nulls of any type are equal.
-			// An unkown with a dynamic type compares as unknown, which we need
+			// An unknown with a dynamic type compares as unknown, which we need
 			// to check before the type comparison below.
 			return UnknownVal(Bool)
 		case !val.ty.Equals(other.ty):
@@ -148,9 +148,9 @@ func (val Value) Equals(other Value) Value {
 	case other.IsKnown() && !val.IsKnown():
 		switch {
 		case other.IsNull(), val.ty.HasDynamicTypes():
-			// If known is Null, we need to wait for the unkown value since
+			// If known is Null, we need to wait for the unknown value since
 			// nulls of any type are equal.
-			// An unkown with a dynamic type compares as unknown, which we need
+			// An unknown with a dynamic type compares as unknown, which we need
 			// to check before the type comparison below.
 			return UnknownVal(Bool)
 		case !other.ty.Equals(val.ty):
@@ -171,7 +171,15 @@ func (val Value) Equals(other Value) Value {
 		return BoolVal(false)
 	}
 
-	if val.ty.HasDynamicTypes() || other.ty.HasDynamicTypes() {
+	// Check if there are any nested dynamic values making this comparison
+	// unknown.
+	if !val.HasWhollyKnownType() || !other.HasWhollyKnownType() {
+		// Even if we have dynamic values, we can still determine inequality if
+		// there is no way the types could later conform.
+		if val.ty.TestConformance(other.ty) != nil && other.ty.TestConformance(val.ty) != nil {
+			return BoolVal(false)
+		}
+
 		return UnknownVal(Bool)
 	}