From b868a8dcbbef5655b5b74c0ffbd6cd7e8c4346ae Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 5 Oct 2023 12:03:02 -0700 Subject: [PATCH] cty: Silently ignore refinements of cty.DynamicVal For historical reasons we cannot permit any refinements on cty.DynamicVal, because existing callers expect that (marks notwithstanding) cty.DynamicVal is the only possible unknown value of an unknown type, and so adding refinements would invalidate that assumption. In the initial implementation of refinements it was treated as a caller programming error to try to refine cty.DynamicVal. However, that violates the convention that cty.DynamicVal is usable as a broad placeholder value that supports any operation that would be valid on at least one possible value. Instead, we'll now compromise by just silently ignoring any attempt to refine cty.DynamicVal. The refinement operation will run to completion without panicking but it will also completely ignore all of the refinement builder calls and eventually just return another plain cty.DynamicVal as the "refined" result. This is consistent with our typical expectation that refinements are propagated on a best-effort basis but can be silently discarded (or lose some detail) when passing through operations that don't or can't support them. This means that it should now be safe to call Refine on any value, but refining can still panic if the specific refinements selected don't make sense for whatever value is being refined. This also includes some direct tests of the Value.Refine API, since it was previously only tested indirectly through other operations that consume or manipulate refinements. --- CHANGELOG.md | 1 + cty/unknown_refinement.go | 61 +++- cty/unknown_refinement_test.go | 511 +++++++++++++++++++++++++++++++++ docs/refinements.md | 10 +- 4 files changed, 568 insertions(+), 15 deletions(-) create mode 100644 cty/unknown_refinement_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index dbd33331..c13b72e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # 1.14.1 (Unreleased) +* `cty`: It's now valid to use the `Refine` method on `cty.DynamicVal`, although all refinements will be silently discarded. This replaces the original behavior of panicking when trying to refine `cty.DynamicVal`. # 1.14.0 (August 30, 2023) diff --git a/cty/unknown_refinement.go b/cty/unknown_refinement.go index d90bcbc3..85fb28d6 100644 --- a/cty/unknown_refinement.go +++ b/cty/unknown_refinement.go @@ -44,7 +44,17 @@ func (v Value) Refine() *RefinementBuilder { var wip unknownValRefinement switch { case ty == DynamicPseudoType && !v.IsKnown(): - panic("cannot refine an unknown value of an unknown type") + // This case specifically matches DynamicVal, which is constrained + // by backward compatibility to be a singleton and so we cannot allow + // any refinements to it. + // To preserve the typical assumption that DynamicVal is a safe + // placeholder to use when no value is known at all, we silently + // ignore all attempts to refine this particular value and just + // always echo back a totally-unrefined DynamicVal. + return &RefinementBuilder{ + orig: DynamicVal, + marks: marks, + } case ty == String: wip = &refinementString{} case ty == Number: @@ -136,10 +146,27 @@ type RefinementBuilder struct { wip unknownValRefinement } -func (b *RefinementBuilder) assertRefineable() { +// refineable is an internal detail to help with two special situations +// related to refinements: +// - If the refinement is to a value of a type that doesn't support any +// refinements at all, this function will immediately panic with a +// message reporting that, because it's a caller bug to try to refine +// a value in a way that's inappropriate for its known type. +// - If the refinement is to an unknown value of an unknown type +// (i.e. cty.DynamicVal) then it returns false, indicating that the +// caller should just silently ignore whatever refinement was requested. +// - In all other cases this function returns true, which means the direct +// caller should attempt to apply the requested refinement, and then +// panic itself if the requested refinement doesn't make sense for the +// specific value being refined. +func (b *RefinementBuilder) refineable() bool { + if b.orig == DynamicVal { + return false + } if b.wip == nil { panic(fmt.Sprintf("cannot refine a %#v value", b.orig.Type())) } + return true } // NotNull constrains the value as definitely not being null. @@ -157,7 +184,9 @@ func (b *RefinementBuilder) assertRefineable() { // -- a value whose type is `cty.DynamicPseudoType` -- as being non-null. // An unknown value of an unknown type is always completely unconstrained. func (b *RefinementBuilder) NotNull() *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } if b.orig.IsKnown() && b.orig.IsNull() { panic("refining null value as non-null") @@ -181,7 +210,9 @@ func (b *RefinementBuilder) NotNull() *RefinementBuilder { // value for each type constraint -- but this is here for symmetry with the // fact that a [ValueRange] can also represent that a value is definitely null. func (b *RefinementBuilder) Null() *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } if b.orig.IsKnown() && !b.orig.IsNull() { panic("refining non-null value as null") @@ -209,7 +240,9 @@ func (b *RefinementBuilder) NumberRangeInclusive(min, max Value) *RefinementBuil // NumberRangeLowerBound constraints the lower bound of a number value, or // panics if this builder is not refining a number value. func (b *RefinementBuilder) NumberRangeLowerBound(min Value, inclusive bool) *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } wip, ok := b.wip.(*refinementNumber) if !ok { @@ -258,7 +291,9 @@ func (b *RefinementBuilder) NumberRangeLowerBound(min Value, inclusive bool) *Re // NumberRangeUpperBound constraints the upper bound of a number value, or // panics if this builder is not refining a number value. func (b *RefinementBuilder) NumberRangeUpperBound(max Value, inclusive bool) *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } wip, ok := b.wip.(*refinementNumber) if !ok { @@ -308,7 +343,9 @@ func (b *RefinementBuilder) NumberRangeUpperBound(max Value, inclusive bool) *Re // collection value, or panics if this builder is not refining a collection // value. func (b *RefinementBuilder) CollectionLengthLowerBound(min int) *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } wip, ok := b.wip.(*refinementCollection) if !ok { @@ -340,7 +377,9 @@ func (b *RefinementBuilder) CollectionLengthLowerBound(min int) *RefinementBuild // The upper bound must be a known, non-null number or this function will // panic. func (b *RefinementBuilder) CollectionLengthUpperBound(max int) *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } wip, ok := b.wip.(*refinementCollection) if !ok { @@ -419,7 +458,9 @@ func (b *RefinementBuilder) StringPrefix(prefix string) *RefinementBuilder { // Use [RefinementBuilder.StringPrefix] instead if an application cannot fully // control the final result to avoid violating this rule. func (b *RefinementBuilder) StringPrefixFull(prefix string) *RefinementBuilder { - b.assertRefineable() + if !b.refineable() { + return b + } wip, ok := b.wip.(*refinementString) if !ok { @@ -487,7 +528,7 @@ func (b *RefinementBuilder) NewValue() (ret Value) { ret = ret.WithMarks(b.marks) }() - if b.orig.IsKnown() { + if b.orig.IsKnown() || b.orig == DynamicVal { return b.orig } diff --git a/cty/unknown_refinement_test.go b/cty/unknown_refinement_test.go new file mode 100644 index 00000000..a9f1cc68 --- /dev/null +++ b/cty/unknown_refinement_test.go @@ -0,0 +1,511 @@ +package cty + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestValueRefine(t *testing.T) { + tests := map[string]struct { + Build func() Value + Want Value + WantPanic any + }{ + "DynamicVal silently ignores all refinements": { + Build: func() Value { + // This particular value, unlike any other value, will just + // accept whatever refinements that are thrown at it and + // completely ignore all of them and just continue being + // itself. + // This is a compromise for backward-compatiblity because + // existing codebases expect cty.DynamicVal itself to be + // the only value that is an unknown value of an unknown + // type, aside from the possibility of marks. + return DynamicVal.Refine(). + NotNull(). + StringPrefix("beep"). + NumberRangeInclusive(Zero, NumberIntVal(10)). + CollectionLength(5). + NewValue() + }, + Want: DynamicVal, + }, + "untyped null can be refined as being null": { + Build: func() Value { + return NullVal(DynamicPseudoType).Refine(). + Null(). + NewValue() + }, + Want: NullVal(DynamicPseudoType), + }, + "untyped null cannot be refined as being non-null": { + Build: func() Value { + return NullVal(DynamicPseudoType).RefineNotNull() + }, + WantPanic: `refining null value as non-null`, + }, + "unknown object can be refined non-null": { + Build: func() Value { + return UnknownVal(EmptyObject).RefineNotNull() + }, + Want: UnknownVal(EmptyObject).RefineNotNull(), + }, + "unknown tuple can be refined non-null": { + Build: func() Value { + return UnknownVal(EmptyTuple).RefineNotNull() + }, + Want: UnknownVal(EmptyTuple).RefineNotNull(), + }, + "unknown list can be refined non-null": { + Build: func() Value { + return UnknownVal(List(String)).RefineNotNull() + }, + Want: UnknownVal(List(String)).RefineNotNull(), + }, + "unknown map can be refined non-null": { + Build: func() Value { + return UnknownVal(Map(String)).RefineNotNull() + }, + Want: UnknownVal(Map(String)).RefineNotNull(), + }, + "unknown set can be refined non-null": { + Build: func() Value { + return UnknownVal(Set(String)).RefineNotNull() + }, + Want: UnknownVal(Set(String)).RefineNotNull(), + }, + "unknown string can be refined non-null": { + Build: func() Value { + return UnknownVal(String).RefineNotNull() + }, + Want: UnknownVal(String).RefineNotNull(), + }, + "unknown number can be refined non-null": { + Build: func() Value { + return UnknownVal(Number).RefineNotNull() + }, + Want: UnknownVal(Number).RefineNotNull(), + }, + "unknown bool can be refined non-null": { + Build: func() Value { + return UnknownVal(Bool).RefineNotNull() + }, + Want: UnknownVal(Bool).RefineNotNull(), + }, + "known null value can have its nullness confirmed": { + Build: func() Value { + return NullVal(Bool).Refine(). + Null(). + NewValue() + }, + Want: NullVal(Bool), + }, + "known null value cannot be refined as not null": { + Build: func() Value { + return NullVal(Bool).RefineNotNull() + }, + WantPanic: `refining null value as non-null`, + }, + + // String refinements + "unknown string can be refined with a prefix": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefix("foo-"). + NewValue() + }, + Want: UnknownVal(String).Refine(). + StringPrefixFull("foo-"). + NewValue(), + }, + "string prefix gets truncated if it might combine (latin diacritics)": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefix("foo"). + NewValue() + }, + Want: UnknownVal(String).Refine(). + StringPrefixFull("fo"). + NewValue(), + }, + "string prefix gets truncated if it might combine (emoji sequences)": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefix("a😶"). // Can combine with "clouds" to produce "face in clouds" + NewValue() + }, + Want: UnknownVal(String).Refine(). + StringPrefixFull("a"). + NewValue(), + }, + "string prefix forced despite possibility of combining": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefixFull("foo"). + NewValue() + }, + Want: UnknownVal(String).Refine(). + StringPrefixFull("foo"). + NewValue(), + }, + "a string prefix can be extended": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefixFull("foo-"). + StringPrefixFull("foo-bar-"). + NewValue() + }, + Want: UnknownVal(String).Refine(). + StringPrefixFull("foo-bar-"). + NewValue(), + }, + "cannot provide a string prefix that conflicts with existing refinement": { + Build: func() Value { + return UnknownVal(String).Refine(). + StringPrefixFull("foo-"). + StringPrefixFull("bar-"). + NewValue() + }, + WantPanic: `refined prefix is inconsistent with previous refined prefix`, + }, + "a known string can have its prefix confirmed": { + Build: func() Value { + return StringVal("foo-baz").Refine(). + StringPrefixFull("foo-"). + NewValue() + }, + Want: StringVal("foo-baz"), + }, + "a known string does not accept a conflicting prefix": { + Build: func() Value { + return StringVal("foo-baz").Refine(). + StringPrefixFull("bar-"). + NewValue() + }, + WantPanic: `refined prefix is inconsistent with known value`, + }, + "non-string values cannot be refined with string prefix": { + Build: func() Value { + return UnknownVal(Number).Refine(). + StringPrefixFull("foo"). + NewValue() + }, + WantPanic: `cannot refine string prefix for a cty.Number value`, + }, + + // Number refinements + "unknown number can have refined lower bound": { + Build: func() Value { + return UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(1), true). + NewValue() + }, + Want: UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(1), true). + NewValue(), + }, + "unknown number can have refined upper bound": { + Build: func() Value { + return UnknownVal(Number).Refine(). + NumberRangeUpperBound(NumberIntVal(1), true). + NewValue() + }, + Want: UnknownVal(Number).Refine(). + NumberRangeUpperBound(NumberIntVal(1), true). + NewValue(), + }, + "unknown number can have refined both bounds": { + Build: func() Value { + return UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(1), true). + NumberRangeUpperBound(NumberIntVal(2), false). + NewValue() + }, + Want: UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(1), true). + NumberRangeUpperBound(NumberIntVal(2), false). + NewValue(), + }, + "refining unknown non-null number with equal upper and lower bound produces known number": { + Build: func() Value { + return UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(1), true). + NumberRangeUpperBound(NumberIntVal(1), true). + NotNull(). + NewValue() + }, + Want: NumberIntVal(1), + }, + "unknown number cannot have conflicting bounds": { + Build: func() Value { + return UnknownVal(Number).Refine(). + NumberRangeLowerBound(NumberIntVal(2), true). + NumberRangeUpperBound(NumberIntVal(1), false). + NewValue() + }, + WantPanic: `number lower bound cty.NumberIntVal(2) is greater than upper bound cty.NumberIntVal(1)`, + }, + "known number can have its bounds confirmed": { + Build: func() Value { + return NumberIntVal(1).Refine(). + NumberRangeLowerBound(NumberIntVal(0), true). + NumberRangeUpperBound(NumberIntVal(2), true). + NotNull(). + NewValue() + }, + Want: NumberIntVal(1), + }, + "can't refine a known number with non-matching bounds": { + Build: func() Value { + return NumberIntVal(10).Refine(). + NumberRangeLowerBound(NumberIntVal(0), true). + NumberRangeUpperBound(NumberIntVal(2), true). + NotNull(). + NewValue() + }, + WantPanic: `refining cty.NumberIntVal(10) to be <= cty.NumberIntVal(2)`, + }, + + // List length refinements + "unknown list can be refined with length lower bound": { + Build: func() Value { + return UnknownVal(List(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue() + }, + Want: UnknownVal(List(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue(), + }, + "unknown list can be refined with length upper bound": { + Build: func() Value { + return UnknownVal(List(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue() + }, + Want: UnknownVal(List(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue(), + }, + "unknown list can be refined with length bounds": { + Build: func() Value { + return UnknownVal(List(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue() + }, + Want: UnknownVal(List(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue(), + }, + "unknown non-null list with known length becomes known list of unknowns": { + Build: func() Value { + return UnknownVal(List(String)).Refine(). + NotNull(). + CollectionLength(2). + NewValue() + }, + Want: ListVal([]Value{UnknownVal(String), UnknownVal(String)}), + }, + "unknown non-null list with known zero length becomes known list": { + Build: func() Value { + return UnknownVal(List(String)).Refine(). + NotNull(). + CollectionLength(0). + NewValue() + }, + Want: ListValEmpty(String), + }, + "known list can have its length confirmed with a refinement": { + Build: func() Value { + return ListValEmpty(String).Refine(). + CollectionLength(0). + NewValue() + }, + Want: ListValEmpty(String), + }, + "cannot refine known list with conflicting length bounds": { + Build: func() Value { + return ListValEmpty(String).Refine(). + CollectionLength(1). + NewValue() + }, + WantPanic: `refining collection of length cty.NumberIntVal(0) with lower bound 1`, + }, + + // Map length refinements + "unknown map can be refined with length lower bound": { + Build: func() Value { + return UnknownVal(Map(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue() + }, + Want: UnknownVal(Map(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue(), + }, + "unknown map can be refined with length upper bound": { + Build: func() Value { + return UnknownVal(Map(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue() + }, + Want: UnknownVal(Map(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue(), + }, + "unknown map can be refined with length bounds": { + Build: func() Value { + return UnknownVal(Map(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue() + }, + Want: UnknownVal(Map(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue(), + }, + "unknown map can be refined with known length": { + Build: func() Value { + return UnknownVal(Map(String)).Refine(). + NotNull(). + CollectionLength(2). + NewValue() + }, + Want: UnknownVal(Map(String)).Refine(). + NotNull(). + CollectionLength(2). + NewValue(), + }, + "unknown non-null map with known zero length becomes known map": { + Build: func() Value { + return UnknownVal(Map(String)).Refine(). + NotNull(). + CollectionLength(0). + NewValue() + }, + Want: MapValEmpty(String), + }, + "known map can have its length confirmed with a refinement": { + Build: func() Value { + return MapValEmpty(String).Refine(). + CollectionLength(0). + NewValue() + }, + Want: MapValEmpty(String), + }, + "cannot refine known map with conflicting length bounds": { + Build: func() Value { + return MapValEmpty(String).Refine(). + CollectionLength(1). + NewValue() + }, + WantPanic: `refining collection of length cty.NumberIntVal(0) with lower bound 1`, + }, + + // Set length refinements + "unknown set can be refined with length lower bound": { + Build: func() Value { + return UnknownVal(Set(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue() + }, + Want: UnknownVal(Set(String)).Refine(). + CollectionLengthLowerBound(1). + NewValue(), + }, + "unknown set can be refined with length upper bound": { + Build: func() Value { + return UnknownVal(Set(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue() + }, + Want: UnknownVal(Set(String)).Refine(). + CollectionLengthUpperBound(1). + NewValue(), + }, + "unknown set can be refined with length bounds": { + Build: func() Value { + return UnknownVal(Set(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue() + }, + Want: UnknownVal(Set(String)).Refine(). + CollectionLengthLowerBound(1). + CollectionLengthUpperBound(3). + NewValue(), + }, + "unknown set can be refined with known length": { + Build: func() Value { + return UnknownVal(Set(String)).Refine(). + NotNull(). + CollectionLength(2). + NewValue() + }, + Want: UnknownVal(Set(String)).Refine(). + NotNull(). + CollectionLength(2). + NewValue(), + }, + "unknown non-null set with known zero length becomes known empty set": { + Build: func() Value { + return UnknownVal(Set(String)).Refine(). + NotNull(). + CollectionLength(0). + NewValue() + }, + Want: SetValEmpty(String), + }, + "known set can have its length confirmed with a refinement": { + Build: func() Value { + return SetValEmpty(String).Refine(). + CollectionLength(0). + NewValue() + }, + Want: SetValEmpty(String), + }, + "cannot refine known set with conflicting length bounds": { + Build: func() Value { + return SetValEmpty(String).Refine(). + CollectionLength(1). + NewValue() + }, + WantPanic: `refining collection of length cty.NumberIntVal(0) with lower bound 1`, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + try := func(f func() Value) (ret Value, panicVal any) { + defer func() { + panicVal = recover() + }() + ret = f() + return + } + got, panicVal := try(test.Build) + + if test.WantPanic == nil { + if panicVal != nil { + t.Fatalf("unexpected panic: %s", panicVal) + } + if !test.Want.RawEquals(got) { + t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.Want) + } + } else { + if panicVal == nil { + t.Fatalf("unexpected success\nresult: %#v\nwant panic: %#v", got, test.WantPanic) + } + + if diff := cmp.Diff(test.WantPanic, panicVal); diff != "" { + t.Errorf("wrong panic value\n%s", diff) + } + } + + }) + } +} diff --git a/docs/refinements.md b/docs/refinements.md index ac474510..51d7193f 100644 --- a/docs/refinements.md +++ b/docs/refinements.md @@ -100,11 +100,11 @@ The most notable restriction on refinements is that the available refinements vary depending on the type constraint of the value being refined. The least flexible case is `cty.DynamicVal` -- an unknown value of an unknown -type -- which is the one value that cannot be refined at all and will cause -a panic if you try. This is a pragmatic compromise for backward compatibility: -existing callers use patterns like `val == cty.DynamicVal` to test for this -specific special value, and any refinements of that value would make it no -longer equal. +type -- which is the one value that cannot be refined at all and will silently +ignore any attempts to do so, leaving the result still totally unrefined. This +is a pragmatic compromise for backward compatibility: existing callers use +patterns like `val == cty.DynamicVal` to test for this specific special value, +and any refinements of that value would make it no longer equal. Unknown values of built-in exact types, and also unknown values whose type _kind_ is constrained even if the element/attribute types are not, can at