From 6e3021c9befc2a005f66baf3b7b468e9380c57a1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 30 May 2020 09:38:53 -0400 Subject: [PATCH 1/6] a null dynamic value is a known value When checking for nested dynamic types within complex types for equality comparison, we need to also verify that those dynamic types are not already known to be null. Add the Value.HasDynamicValues method, to check for nested DynamicVal values. HasDynamicValues indicates not only that this value is unknown, but the type may change as well. This is used in the Value.Equals method before checking the type for equality. --- cty/value.go | 27 ++++++++++ cty/value_ops.go | 12 +++-- cty/value_ops_test.go | 116 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 5 deletions(-) diff --git a/cty/value.go b/cty/value.go index 1025ba82..b6a1bbda 100644 --- a/cty/value.go +++ b/cty/value.go @@ -106,3 +106,30 @@ func (val Value) IsWhollyKnown() bool { return true } } + +// HasDynamicValues 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) HasDynamicValues() bool { + // a null dynamic type is known + if val.IsNull() { + return false + } + + // 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.ty == DynamicPseudoType { + return true + } + + if val.CanIterateElements() { + for it := val.ElementIterator(); it.Next(); { + _, ev := it.Element() + if ev.HasDynamicValues() { + return true + } + } + } + return false +} diff --git a/cty/value_ops.go b/cty/value_ops.go index 35a644be..957dd4a3 100644 --- a/cty/value_ops.go +++ b/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,9 @@ 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.HasDynamicValues() || other.HasDynamicValues() { return UnknownVal(Bool) } diff --git a/cty/value_ops_test.go b/cty/value_ops_test.go index 8e313fac..de94cf5f 100644 --- a/cty/value_ops_test.go +++ b/cty/value_ops_test.go @@ -633,6 +633,47 @@ func TestValueEquals(t *testing.T) { NumberIntVal(1), False, // because no string value -- even null -- can be equal to a non-null number }, + { + ObjectVal(map[string]Value{ + "a": StringVal("a"), + }), + // A null value is always known + ObjectVal(map[string]Value{ + "a": NullVal(DynamicPseudoType), + }), + BoolVal(false), + }, + { + ObjectVal(map[string]Value{ + "a": NullVal(DynamicPseudoType), + }), + ObjectVal(map[string]Value{ + "a": NullVal(DynamicPseudoType), + }), + BoolVal(true), + }, + { + ObjectVal(map[string]Value{ + "a": StringVal("a"), + "b": UnknownVal(Number), + }), + // While we have a dynamic type, the different object types should + // still compare false + ObjectVal(map[string]Value{ + "a": NullVal(DynamicPseudoType), + "c": UnknownVal(Number), + }), + BoolVal(false), + }, + { + ObjectVal(map[string]Value{ + "a": NullVal(DynamicPseudoType), + }), + ObjectVal(map[string]Value{ + "a": DynamicVal, + }), + UnknownVal(Bool), + }, // Marks { @@ -2435,3 +2476,78 @@ func TestValueGoString(t *testing.T) { }) } } + +func TestHasDynamicValues(t *testing.T) { + tests := []struct { + Value Value + Want bool + }{ + { + Value: DynamicVal, + Want: true, + }, + { + Value: ObjectVal(map[string]Value{ + "dyn": DynamicVal, + }), + Want: true, + }, + { + Value: NullVal(Object(map[string]Type{ + "dyn": DynamicPseudoType, + })), + Want: false, + }, + { + Value: TupleVal([]Value{ + StringVal("a"), + NullVal(DynamicPseudoType), + }), + Want: false, + }, + { + Value: ListVal([]Value{ + ObjectVal(map[string]Value{ + "null": NullVal(DynamicPseudoType), + }), + }), + Want: false, + }, + { + Value: ListVal([]Value{ + NullVal(Object(map[string]Type{ + "dyn": DynamicPseudoType, + })), + }), + Want: false, + }, + { + Value: ObjectVal(map[string]Value{ + "tuple": TupleVal([]Value{ + StringVal("a"), + NullVal(DynamicPseudoType), + }), + }), + Want: false, + }, + { + Value: ObjectVal(map[string]Value{ + "tuple": TupleVal([]Value{ + ObjectVal(map[string]Value{ + "dyn": DynamicVal, + }), + }), + }), + Want: true, + }, + } + for _, test := range tests { + t.Run(test.Value.GoString(), func(t *testing.T) { + got := test.Value.HasDynamicValues() + want := test.Want + if got != want { + t.Errorf("wrong result\ngot: %v\nwant: %v", got, want) + } + }) + } +} From 5449661d98786e604d1c69596d0a78d8d3b67a68 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 30 May 2020 11:28:37 -0400 Subject: [PATCH 2/6] check type conformance in Value.Equals Even when a value contains dynamic values, we may still be able to determine inequality when there is no way the types could conform. --- cty/value_ops.go | 6 ++++++ cty/value_ops_test.go | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/cty/value_ops.go b/cty/value_ops.go index 957dd4a3..4f842e80 100644 --- a/cty/value_ops.go +++ b/cty/value_ops.go @@ -174,6 +174,12 @@ func (val Value) Equals(other Value) Value { // Check if there are any nested dynamic values making this comparison // unknown. if val.HasDynamicValues() || other.HasDynamicValues() { + // 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) } diff --git a/cty/value_ops_test.go b/cty/value_ops_test.go index de94cf5f..36257195 100644 --- a/cty/value_ops_test.go +++ b/cty/value_ops_test.go @@ -665,6 +665,19 @@ func TestValueEquals(t *testing.T) { }), BoolVal(false), }, + { + ObjectVal(map[string]Value{ + "a": StringVal("a"), + "b": UnknownVal(Number), + }), + // While we have a dynamic type, the different object types should + // still compare false + ObjectVal(map[string]Value{ + "a": DynamicVal, + "c": UnknownVal(Number), + }), + BoolVal(false), + }, { ObjectVal(map[string]Value{ "a": NullVal(DynamicPseudoType), From 4e2092e5609842a178a0ef3861c2031e7da4db63 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 5 Jun 2020 12:17:50 -0400 Subject: [PATCH 3/6] correctly handle the case of unknown element types We weren't checking for unknown types within an unknown container. --- cty/value.go | 6 ++++++ cty/value_ops_test.go | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/cty/value.go b/cty/value.go index b6a1bbda..c25326fd 100644 --- a/cty/value.go +++ b/cty/value.go @@ -124,6 +124,12 @@ func (val Value) HasDynamicValues() bool { } 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.HasDynamicValues() { diff --git a/cty/value_ops_test.go b/cty/value_ops_test.go index 36257195..78bd5aff 100644 --- a/cty/value_ops_test.go +++ b/cty/value_ops_test.go @@ -687,6 +687,26 @@ func TestValueEquals(t *testing.T) { }), UnknownVal(Bool), }, + { + ObjectVal(map[string]Value{ + "a": NullVal(List(String)), + }), + // While the unknown val does contain dynamic types, the overall + // container types can't conform. + ObjectVal(map[string]Value{ + "a": UnknownVal(List(List(DynamicPseudoType))), + }), + BoolVal(false), + }, + { + ObjectVal(map[string]Value{ + "a": NullVal(List(List(String))), + }), + ObjectVal(map[string]Value{ + "a": UnknownVal(List(List(DynamicPseudoType))), + }), + UnknownVal(Bool), + }, // Marks { From a255ebc28dd5377672d486bc4c8bc730299c1d25 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 5 Jun 2020 12:34:10 -0400 Subject: [PATCH 4/6] HasDynamicTypes() must check element types HasDynamicTypes was not checking element collection types --- cty/type.go | 2 +- cty/type_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 cty/type_test.go diff --git a/cty/type.go b/cty/type.go index 730cb986..5f7813e8 100644 --- a/cty/type.go +++ b/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 { diff --git a/cty/type_test.go b/cty/type_test.go new file mode 100644 index 00000000..7861542e --- /dev/null +++ b/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) + } + }) + } +} From 62b2955e82f7d96cb5865b2a6ec8eea5d9b742a0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 5 Jun 2020 12:37:41 -0400 Subject: [PATCH 5/6] change HasDynamicValues to HasWhollyKnownType This better describes the intent, and is a little more constant with the other Value method names. --- cty/value.go | 6 +++--- cty/value_ops.go | 2 +- cty/value_ops_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cty/value.go b/cty/value.go index c25326fd..5c6d8f22 100644 --- a/cty/value.go +++ b/cty/value.go @@ -107,10 +107,10 @@ func (val Value) IsWhollyKnown() bool { } } -// HasDynamicValues checks if the value is dynamic, or contains any nested +// 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) HasDynamicValues() bool { +func (val Value) HasWhollyKnownType() bool { // a null dynamic type is known if val.IsNull() { return false @@ -132,7 +132,7 @@ func (val Value) HasDynamicValues() bool { for it := val.ElementIterator(); it.Next(); { _, ev := it.Element() - if ev.HasDynamicValues() { + if ev.HasWhollyKnownType() { return true } } diff --git a/cty/value_ops.go b/cty/value_ops.go index 4f842e80..e5ea8ac6 100644 --- a/cty/value_ops.go +++ b/cty/value_ops.go @@ -173,7 +173,7 @@ func (val Value) Equals(other Value) Value { // Check if there are any nested dynamic values making this comparison // unknown. - if val.HasDynamicValues() || other.HasDynamicValues() { + 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 { diff --git a/cty/value_ops_test.go b/cty/value_ops_test.go index 78bd5aff..96b0cd22 100644 --- a/cty/value_ops_test.go +++ b/cty/value_ops_test.go @@ -2510,7 +2510,7 @@ func TestValueGoString(t *testing.T) { } } -func TestHasDynamicValues(t *testing.T) { +func TestHasWhollyKnownType(t *testing.T) { tests := []struct { Value Value Want bool @@ -2576,7 +2576,7 @@ func TestHasDynamicValues(t *testing.T) { } for _, test := range tests { t.Run(test.Value.GoString(), func(t *testing.T) { - got := test.Value.HasDynamicValues() + got := test.Value.HasWhollyKnownType() want := test.Want if got != want { t.Errorf("wrong result\ngot: %v\nwant: %v", got, want) From 9bbaaeeb1799c47b76e7d0e18214ebca9a4c9f8c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 5 Jun 2020 18:50:22 -0400 Subject: [PATCH 6/6] invert the HasWhollyKnownType logic The name was updated, but the actual logic was not inverted to match the new name. --- cty/value.go | 15 ++++++++------- cty/value_ops.go | 2 +- cty/value_ops_test.go | 16 ++++++++-------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/cty/value.go b/cty/value.go index 5c6d8f22..f6a25dde 100644 --- a/cty/value.go +++ b/cty/value.go @@ -113,29 +113,30 @@ func (val Value) IsWhollyKnown() bool { func (val Value) HasWhollyKnownType() bool { // a null dynamic type is known if val.IsNull() { - return false + 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.ty == DynamicPseudoType { - return true + 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() + return !val.ty.HasDynamicTypes() } for it := val.ElementIterator(); it.Next(); { _, ev := it.Element() - if ev.HasWhollyKnownType() { - return true + if !ev.HasWhollyKnownType() { + return false } } } - return false + + return true } diff --git a/cty/value_ops.go b/cty/value_ops.go index e5ea8ac6..966b999c 100644 --- a/cty/value_ops.go +++ b/cty/value_ops.go @@ -173,7 +173,7 @@ func (val Value) Equals(other Value) Value { // Check if there are any nested dynamic values making this comparison // unknown. - if val.HasWhollyKnownType() || other.HasWhollyKnownType() { + 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 { diff --git a/cty/value_ops_test.go b/cty/value_ops_test.go index 96b0cd22..7392047a 100644 --- a/cty/value_ops_test.go +++ b/cty/value_ops_test.go @@ -2517,26 +2517,26 @@ func TestHasWhollyKnownType(t *testing.T) { }{ { Value: DynamicVal, - Want: true, + Want: false, }, { Value: ObjectVal(map[string]Value{ "dyn": DynamicVal, }), - Want: true, + Want: false, }, { Value: NullVal(Object(map[string]Type{ "dyn": DynamicPseudoType, })), - Want: false, + Want: true, }, { Value: TupleVal([]Value{ StringVal("a"), NullVal(DynamicPseudoType), }), - Want: false, + Want: true, }, { Value: ListVal([]Value{ @@ -2544,7 +2544,7 @@ func TestHasWhollyKnownType(t *testing.T) { "null": NullVal(DynamicPseudoType), }), }), - Want: false, + Want: true, }, { Value: ListVal([]Value{ @@ -2552,7 +2552,7 @@ func TestHasWhollyKnownType(t *testing.T) { "dyn": DynamicPseudoType, })), }), - Want: false, + Want: true, }, { Value: ObjectVal(map[string]Value{ @@ -2561,7 +2561,7 @@ func TestHasWhollyKnownType(t *testing.T) { NullVal(DynamicPseudoType), }), }), - Want: false, + Want: true, }, { Value: ObjectVal(map[string]Value{ @@ -2571,7 +2571,7 @@ func TestHasWhollyKnownType(t *testing.T) { }), }), }), - Want: true, + Want: false, }, } for _, test := range tests {