Forward to v1.6.0 (#15)

* Prepare for a possible future 1.5.2 release

* function/stdlib: Check for negative indices in ElementFunc

Previously a negative index would lead to a panic.

* Update CHANGELOG.md

* convert: Experimental optional object attributes

Previously we required that when converting to an object type the input
must have at least the attributes from the target type, and optionally
additional attributes that would then be discarded.

We'll now allow creating an object type with one or more optional
attributes. These are optional only for conversion purposes: converting
to an object type with optional arguments will substitute a null value
for any attribute that doesn't exist in the source object type.

This being handled only under conversion is consistent with some other
similar cases: an object type is not conforming to a type constraint if
it has attributes that are not in the type constraint, even though a
conversion to that type constraint would be accepted. Likewise, a number
doesn't conform to the string type even though a conversion is available.
This distinction is so that cty applications can potentially bring their
own separate conversion rules if they wish, ignoring the convert package
in this repository.

For now this new capability is explicitly experimental and so not subject
to our typical backward-compatibility promises. The behavior of any
function producing or working with object types that have optional
attributes is subject to change even in future minor versions of this
package.

* Update CHANGELOG.md

* dependencies: use major version 4 of the underlying msgpack library

This is just in the interests of keeping things up-to-date and doesn't
really confer any specific benefit.

The underlying library now defaults to always using the full-length
encoding for integers provided as int64, so this change also includes
an explicit opt-in to the old behavior of selecting the most compact
integer representation possible, because for cty the different numeric
encodings are just a size optimization and have no semantic meaning.

* cty: Fix various quirks of handling sets with unknown values

Early in the implementation of cty I made an oversight which has,
unfortunately, played out as a variety of incorrect behaviors throughout
the cty functions: when a set contains unknown values, those unknown
values can potentially be standing in for values that are equal to others
in the set, and thus the effective length of the set can't be predicted.

Previously the Length function would return, in effect, the maximum
possible size of the set, which would result if all of the values
represented by the unknowns are non-equal. The caller might then get a
different known result from a call with unknowns than from a subsequent
call with those unknowns replaced with known values, which violates the
guarantees that cty intends to make when handling unknown values.

This changeset therefore starts by addressing that root bug: Length will
now return an unknown number if called on a set containing unknown values,
correctly representing that the final length is unpredictable.

This in turn had some downstream consequences for other functionality.
In particular, it should not have been possible to convert a set whose
length is unknown to a list, because it's not possible for a list to have
an unknown length. Therefore this changeset also includes a fix to the
convert package so that an unknown-length set converts to an unknown
list, which also addresses the related problem that a conversion from a
set to list can't predict where the unknown values will appear in the
sort order and thus can't predict the list indices even for the known
elements.

The rest of the changes here are similar adjustments to account for the
possibility of an unknown set length, in most cases similarly returning
an unknown result.

This changeset causes a difference in observable behavior from the
previous version, but it's not considered to be a breaking change because
the previous behavior was defective. With that said, callers that were
inadvertently depending on the defective behavior will find that their
calling application now behaves slightly differently, and so may wish to
make adjustments if the defective prior behavior was actually desirable
for some unusual situation.

As a pragmatic accommodation for existing callers, the Value.LengthInt
function is now defined as returning the maximum possible length in
situations where the length is unknown. This aligns with the number of
iterations a caller would encounter using an ElementIterator on such a
value, and also reflects a suitable capacity to use when allocating a
Go slice to append iterated elements into.

* Update CHANGELOG.md

* v1.6.0

* Update CHANGELOG.md

---------

Co-authored-by: Martin Atkins <[email protected]>
Co-authored-by: Kristin Laemmert <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -1,3 +1,20 @@
+# 1.6.0 (Unreleased)
+
+* Fixed various defects in the handling of sets containing unknown values. This will cause unknown values to now be returned in more situations, whereas before `cty` would often return incorrect results when working with sets containing unknown values. The list of defects fixed in this release includes:
+    - `cty`: The length of a set containing unknown values, as defined by `Value.Length`, is itself unknown, reflecting the fact that unknown values may be placeholders for values that are equal to other values in the set, which would thus coalesce into a single value.
+    - `cty:` Converting a set with unknown values to a list produces an unknown value, because type conversion can't predict which indices each element of the set should take (the unknown elements could appear anywhere in the sort order) or the length of the resulting list.
+    - `function/stdlib`: the `LengthFunc` and `ToList` functions wrap the behaviors described in the previous two items and are therefore also fixed in the same way.
+    - `function/stclib`: `FormatListFunc` depends on knowing the length of all of its sequence arguments (which includes support for sets), so it will return an unknown result if given a set with an unknown length.
+    - `function/stdlib`: The various set operation functions were previously producing incorrect results if one of their given sets contained unknown values, because they didn't consider that unknown values on one set may be placeholders for values that are equal to elements of the other set. For example, `SetSubtractFunc` now produces a wholly-unknown result if either of its arguments contains an unknown element, because it can't predict whether that unknown element represents a value equal to an element in the other set.
+    - `cty`: The `Value.Equal` function would previously incorrectly return a known `cty.False` if one of the given sets contained an unknown value. It will now return `cty.UnknownVal(cty.Bool)` in that case, reflecting that the result could be either `cty.True` or `cty.False` were the unknown values to be replaced with known values.
+    - `cty`: The `Value.LengthInt` function was also returning incorrect results for sets containing unknown elements. However, given that it is commonly used in conjunction with `ElementIterator` to determine the capacity for a slice to append elements to, it is not fixed and is instead redefined to return the _maximum possible length_, which would result if all of the unknown values represent values that are not equal to any other set element. Applications that use `Value.LengthInt` to determine lengths to return to users who are working in the space of `cty` values should switch to using `Value.Length` instead and handle the possibility of the length being unknown, to avoid returning incorrect results for sets with unknown values.
+
+    These are not classified as breaking changes because the previous behavior was defective per the design goals for unknown values. However, callers may notice their application behavior changes along with these fixes when upgrading. The new behaviors should all be more correct than the old; if you observe a change in behavior where there is now an _incorrect_ result for sets containing unknown values (that is, where `cty` claims it knows an answer that it should not actually know), please report that in a GitHub issue.
+
+    We advise callers which work with sets that may potentially contain unknown values to review their own set-handling functions to check if they too might be handling sets with unknown values incorrectly, particularly if they work with sets using [integration methods rather than operation methods](./docs/types.md#common-operations-and-integration-methods) (for example, using `Value.ValueList` or `Value.ValueSet` to extract elements directly). It seems that incorrect handling of sets with unknown values has been a common hazard, particularly in codepaths that aim to treat lists and sets as being interchangable.
+* `function/stdlib`: The `element` function will no longer panic if given a negative index. Instead, it will return a proper error. ([#62](https://github.com/zclconf/go-cty/pull/62))
+* `convert`: **Experimental** support for annotating one or more attributes of an object type as "optional", which the `convert` package can then use to suppress the error that would normally be returned if the source type has no corresponding attribute, and can substitute a correctly-typed null value instead. This new behavior is subject to change even in minor release of `cty`, until it has been tested in experimental releases of downstream applications and potentially modified in response.
+
 # 1.5.1 (Unreleased)
 
 * `function/stdlib`: The `merge` function will no longer panic if all given maps are empty. ([#58](https://github.com/zclconf/go-cty/pull/58))

cty/convert/conversion_collection.go

@@ -13,6 +13,14 @@ import (
 // if we're converting from a set into a list of the same element type.)
 func conversionCollectionToList(ety cty.Type, conv conversion) conversion {
 	return func(val cty.Value, path cty.Path) (cty.Value, error) {
+		if !val.Length().IsKnown() {
+			// If the input collection has an unknown length (which is true
+			// for a set containing unknown values) then our result must be
+			// an unknown list, because we can't predict how many elements
+			// the resulting list should have.
+			return cty.UnknownVal(cty.List(val.Type().ElementType())), nil
+		}
+
 		elems := make([]cty.Value, 0, val.LengthInt())
 		i := int64(0)
 		elemPath := append(path.Copy(), nil)
@@ -458,6 +466,16 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv
 			elems[name.AsString()] = val
 		}
 
+		for name, aty := range objectAtys {
+			if _, exists := elems[name]; !exists {
+				if optional := objType.AttributeOptional(name); optional {
+					elems[name] = cty.NullVal(aty)
+				} else {
+					return cty.NilVal, path.NewErrorf("map has no element for required attribute %q", name)
+				}
+			}
+		}
+
 		return cty.ObjectVal(elems), nil
 	}
 }

cty/convert/conversion_object.go

@@ -11,17 +11,29 @@ import (
 // type, meaning that each attribute of the output type has a corresponding
 // attribute in the input type where a recursive conversion is available.
 //
+// If the "out" type has any optional attributes, those attributes may be
+// absent in the "in" type, in which case null values will be used in their
+// place in the result.
+//
 // Shallow object conversions work the same for both safe and unsafe modes,
 // but the safety flag is passed on to recursive conversions and may thus
 // limit the above definition of "subset".
 func conversionObjectToObject(in, out cty.Type, unsafe bool) conversion {
 	inAtys := in.AttributeTypes()
 	outAtys := out.AttributeTypes()
+	outOptionals := out.OptionalAttributes()
 	attrConvs := make(map[string]conversion)
 
 	for name, outAty := range outAtys {
 		inAty, exists := inAtys[name]
 		if !exists {
+			if _, optional := outOptionals[name]; optional {
+				// If it's optional then we'll skip inserting an
+				// attribute conversion and then deal with inserting
+				// the default value in our overall conversion logic
+				// later.
+				continue
+			}
 			// No conversion is available, then.
 			return nil
 		}
@@ -71,6 +83,13 @@ func conversionObjectToObject(in, out cty.Type, unsafe bool) conversion {
 			attrVals[name] = val
 		}
 
+		for name := range outOptionals {
+			if _, exists := attrVals[name]; !exists {
+				wantTy := outAtys[name]
+				attrVals[name] = cty.NullVal(wantTy)
+			}
+		}
+
 		return cty.ObjectVal(attrVals), nil
 	}
 }

cty/convert/mismatch_msg.go

@@ -78,7 +78,9 @@ func mismatchMessageObjects(got, want cty.Type) string {
 	for name, wantAty := range wantAtys {
 		gotAty, exists := gotAtys[name]
 		if !exists {
-			missingAttrs = append(missingAttrs, name)
+			if !want.AttributeOptional(name) {
+				missingAttrs = append(missingAttrs, name)
+			}
 			continue
 		}
 

cty/convert/public_test.go

@@ -143,6 +143,14 @@ func TestConvert(t *testing.T) {
 			Type:      cty.List(cty.DynamicPseudoType),
 			WantError: true, // there is no type that both tuple elements can unify to for conversion to list
 		},
+		{
+			Value: cty.SetVal([]cty.Value{
+				cty.StringVal("5"),
+				cty.UnknownVal(cty.String),
+			}),
+			Type: cty.Set(cty.Number),
+			Want: cty.SetVal([]cty.Value{cty.NumberIntVal(5), cty.UnknownVal(cty.Number)}),
+		},
 		{
 			Value: cty.SetVal([]cty.Value{
 				cty.StringVal("5"),
@@ -182,6 +190,27 @@ func TestConvert(t *testing.T) {
 				cty.StringVal("10"),
 			}),
 		},
+		{
+			Value: cty.SetVal([]cty.Value{
+				cty.StringVal("5"),
+				cty.UnknownVal(cty.String),
+			}),
+			Type: cty.List(cty.String),
+			Want: cty.UnknownVal(cty.List(cty.String)),
+		},
+		{
+			Value: cty.SetVal([]cty.Value{
+				cty.UnknownVal(cty.String),
+			}),
+			Type: cty.List(cty.String),
+			// We get a known list value this time because even though we
+			// don't know the single value that's in the list, we _do_ know
+			// that there are no other values in the set for it to coalesce
+			// with.
+			Want: cty.ListVal([]cty.Value{
+				cty.UnknownVal(cty.String),
+			}),
+		},
 		{
 			Value: cty.ListVal([]cty.Value{
 				cty.NumberIntVal(5),
@@ -381,6 +410,32 @@ func TestConvert(t *testing.T) {
 				"name": cty.StringVal("John"),
 			}),
 		},
+		{
+			Value: cty.MapVal(map[string]cty.Value{
+				"name": cty.StringVal("John"),
+			}),
+			Type: cty.Object(map[string]cty.Type{
+				"name":     cty.String,
+				"greeting": cty.String,
+			}),
+			WantError: true, // map has no element for required attribute "greeting"
+		},
+		{
+			Value: cty.MapVal(map[string]cty.Value{
+				"name": cty.StringVal("John"),
+			}),
+			Type: cty.ObjectWithOptionalAttrs(
+				map[string]cty.Type{
+					"name":     cty.String,
+					"greeting": cty.String,
+				},
+				[]string{"greeting"},
+			),
+			Want: cty.ObjectVal(map[string]cty.Value{
+				"greeting": cty.NullVal(cty.String),
+				"name":     cty.StringVal("John"),
+			}),
+		},
 		{
 			Value: cty.MapVal(map[string]cty.Value{
 				"a": cty.NumberIntVal(2),
@@ -476,6 +531,50 @@ func TestConvert(t *testing.T) {
 			}),
 			WantError: true, // given value must have superset object type
 		},
+		{
+			Value: cty.ObjectVal(map[string]cty.Value{
+				"bar": cty.StringVal("bar value"),
+			}),
+			Type: cty.ObjectWithOptionalAttrs(
+				map[string]cty.Type{
+					"foo": cty.String,
+					"bar": cty.String,
+				},
+				[]string{"foo"},
+			),
+			Want: cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.NullVal(cty.String),
+				"bar": cty.StringVal("bar value"),
+			}),
+		},
+		{
+			Value: cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.StringVal("foo value"),
+				"bar": cty.StringVal("bar value"),
+			}),
+			Type: cty.ObjectWithOptionalAttrs(
+				map[string]cty.Type{
+					"foo": cty.String,
+					"bar": cty.String,
+				},
+				[]string{"foo"},
+			),
+			Want: cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.StringVal("foo value"),
+				"bar": cty.StringVal("bar value"),
+			}),
+		},
+		{
+			Value: cty.EmptyObjectVal,
+			Type: cty.ObjectWithOptionalAttrs(
+				map[string]cty.Type{
+					"foo": cty.String,
+					"bar": cty.String,
+				},
+				[]string{"foo"},
+			),
+			WantError: true, // Attribute "bar" is required
+		},
 		{
 			Value: cty.ObjectVal(map[string]cty.Value{
 				"foo": cty.True,

cty/function/stdlib/collection.go

@@ -138,6 +138,13 @@ var ElementFunc = function.New(&function.Spec{
 	},
 	Type: func(args []cty.Value) (cty.Type, error) {
 		list := args[0]
+		index := args[1]
+		if index.IsKnown() {
+			if index.LessThan(cty.NumberIntVal(0)).True() {
+				return cty.DynamicPseudoType, fmt.Errorf("cannot use element function with a negative index")
+			}
+		}
+
 		listTy := list.Type()
 		switch {
 		case listTy.IsListType():
@@ -173,6 +180,10 @@ var ElementFunc = function.New(&function.Spec{
 			return cty.DynamicVal, fmt.Errorf("invalid index: %s", err)
 		}
 
+		if args[1].LessThan(cty.NumberIntVal(0)).True() {
+			return cty.DynamicVal, fmt.Errorf("cannot use element function with a negative index")
+		}
+
 		if !args[0].IsKnown() {
 			return cty.UnknownVal(retType), nil
 		}
@@ -492,6 +503,11 @@ var FlattenFunc = function.New(&function.Spec{
 // We can flatten lists with unknown values, as long as they are not
 // lists themselves.
 func flattener(flattenList cty.Value) ([]cty.Value, bool) {
+	if !flattenList.Length().IsKnown() {
+		// If we don't know the length of what we're flattening then we can't
+		// predict the length of our result yet either.
+		return nil, false
+	}
 	out := make([]cty.Value, 0)
 	for it := flattenList.ElementIterator(); it.Next(); {
 		_, val := it.Element()
@@ -872,6 +888,10 @@ var SetProductFunc = function.New(&function.Spec{
 
 		total := 1
 		for _, arg := range args {
+			if !arg.Length().IsKnown() {
+				return cty.UnknownVal(retType), nil
+			}
+
 			// Because of our type checking function, we are guaranteed that
 			// all of the arguments are known, non-null values of types that
 			// support LengthInt.
@@ -1019,7 +1039,8 @@ func sliceIndexes(args []cty.Value) (int, int, bool, error) {
 	var startIndex, endIndex, length int
 	var startKnown, endKnown, lengthKnown bool
 
-	if args[0].Type().IsTupleType() || args[0].IsKnown() { // if it's a tuple then we always know the length by the type, but lists must be known
+	// If it's a tuple then we always know the length by the type, but collections might be unknown or have unknown length
+	if args[0].Type().IsTupleType() || args[0].Length().IsKnown() {
 		length = args[0].LengthInt()
 		lengthKnown = true
 	}