From e9ad14f8c2c48f8022e87ce486cf51277550b1d6 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 16 Mar 2023 09:15:10 -0700 Subject: [PATCH] function: Don't call function Impl if we didn't call Type By default cty function calls "short circuit" -- skip calling the Type function and just immediately return cty.DynamicPseudoType -- if any of the arguments are cty.DynamicVal. However, in that case we were previously only skipping the call to Type but yet still expecting Impl to be able to run. That's incorrect because Impl functions should be able to treat Type as a "guard" and be guaranteed that Impl will never run if Type failed. To fix that hole we'll now track whether we skipped calling Type, and if so we'll also skip calling Impl and just immediately return an unknown value. Individual functions can still opt out of this behavior by declaring on or more of their parameters as AllowDynamicType: true, in which case their own Type function will get to decide how to handle that situation. --- CHANGELOG.md | 8 +++++ cty/function/function.go | 49 ++++++++++++++++---------- cty/function/stdlib/collection_test.go | 8 +++++ 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 115071b5..e7602b63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +# 1.13.1 (Unreleased) + +* `function`: If a function parameter that doesn't declare `AllowDynamicType: true` recieves a `cty.DynamicVal`, the function system would previously just skip calling the function's `Type` callback and treat the result type as unknown. However, the `Call` method was then still calling a function's `Impl` callback anyway, which violated the usual contract that `Type` acts as a guard for `Impl` so `Impl` doesn't have to repeat type-checking already done in `Type`: it's only valid to call `Impl` if `Type` was previosly called _and_ it succeeded. + + The function system will now skip calling `Impl` if it skips calling `Type`, immediately returning `cty.DynamicVal` in that case. Individual functions can opt out of this behavior by marking one or more of their parameters as `AllowDynamicType: true` and then handling that situation manually inside the `Type` and `Impl` callbacks. + + As a result of this problem, some of the `function/stdlib` functions were not correctly handling `cty.DynamicVal` arguments after being extended to support refinements in the v1.13.0 release, causing unexpected errors or panics when calling them. Those functions are fixed indirectly by this change, since their callbacks will no longer run at all in those cases, as was true before they were extended to support refinements. + # 1.13.0 (February 23, 2023) ## Upgrade Notes diff --git a/cty/function/function.go b/cty/function/function.go index 1cfbf2b7..6fc96828 100644 --- a/cty/function/function.go +++ b/cty/function/function.go @@ -122,20 +122,13 @@ func (f Function) ReturnType(argTypes []cty.Type) (cty.Type, error) { return f.ReturnTypeForValues(vals) } -// ReturnTypeForValues is similar to ReturnType but can be used if the caller -// already knows the values of some or all of the arguments, in which case -// the function may be able to determine a more definite result if its -// return type depends on the argument *values*. -// -// For any arguments whose values are not known, pass an Unknown value of -// the appropriate type. -func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) { +func (f Function) returnTypeForValues(args []cty.Value) (ty cty.Type, dynTypedArgs bool, err error) { var posArgs []cty.Value var varArgs []cty.Value if f.spec.VarParam == nil { if len(args) != len(f.spec.Params) { - return cty.Type{}, fmt.Errorf( + return cty.Type{}, false, fmt.Errorf( "wrong number of arguments (%d required; %d given)", len(f.spec.Params), len(args), ) @@ -145,7 +138,7 @@ func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) varArgs = nil } else { if len(args) < len(f.spec.Params) { - return cty.Type{}, fmt.Errorf( + return cty.Type{}, false, fmt.Errorf( "wrong number of arguments (at least %d required; %d given)", len(f.spec.Params), len(args), ) @@ -174,7 +167,7 @@ func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) } if val.IsNull() && !spec.AllowNull { - return cty.Type{}, NewArgErrorf(i, "argument must not be null") + return cty.Type{}, false, NewArgErrorf(i, "argument must not be null") } // AllowUnknown is ignored for type-checking, since we expect to be @@ -184,13 +177,13 @@ func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) if val.Type() == cty.DynamicPseudoType { if !spec.AllowDynamicType { - return cty.DynamicPseudoType, nil + return cty.DynamicPseudoType, true, nil } } else if errs := val.Type().TestConformance(spec.Type); errs != nil { // For now we'll just return the first error in the set, since // we don't have a good way to return the whole list here. // Would be good to do something better at some point... - return cty.Type{}, NewArgError(i, errs[0]) + return cty.Type{}, false, NewArgError(i, errs[0]) } } @@ -209,18 +202,18 @@ func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) } if val.IsNull() && !spec.AllowNull { - return cty.Type{}, NewArgErrorf(realI, "argument must not be null") + return cty.Type{}, false, NewArgErrorf(realI, "argument must not be null") } if val.Type() == cty.DynamicPseudoType { if !spec.AllowDynamicType { - return cty.DynamicPseudoType, nil + return cty.DynamicPseudoType, true, nil } } else if errs := val.Type().TestConformance(spec.Type); errs != nil { // For now we'll just return the first error in the set, since // we don't have a good way to return the whole list here. // Would be good to do something better at some point... - return cty.Type{}, NewArgError(i, errs[0]) + return cty.Type{}, false, NewArgError(i, errs[0]) } } } @@ -234,17 +227,37 @@ func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) } }() - return f.spec.Type(args) + ty, err = f.spec.Type(args) + return ty, false, err +} + +// ReturnTypeForValues is similar to ReturnType but can be used if the caller +// already knows the values of some or all of the arguments, in which case +// the function may be able to determine a more definite result if its +// return type depends on the argument *values*. +// +// For any arguments whose values are not known, pass an Unknown value of +// the appropriate type. +func (f Function) ReturnTypeForValues(args []cty.Value) (ty cty.Type, err error) { + ty, _, err = f.returnTypeForValues(args) + return ty, err } // Call actually calls the function with the given arguments, which must // conform to the function's parameter specification or an error will be // returned. func (f Function) Call(args []cty.Value) (val cty.Value, err error) { - expectedType, err := f.ReturnTypeForValues(args) + expectedType, dynTypeArgs, err := f.returnTypeForValues(args) if err != nil { return cty.NilVal, err } + if dynTypeArgs { + // returnTypeForValues sets this if any argument was inexactly typed + // and the corresponding parameter did not indicate it could deal with + // that. In that case we also avoid calling the implementation function + // because it will also typically not be ready to deal with that case. + return cty.UnknownVal(expectedType), nil + } if refineResult := f.spec.RefineResult; refineResult != nil { // If this function has a refinement callback then we'll refine diff --git a/cty/function/stdlib/collection_test.go b/cty/function/stdlib/collection_test.go index 9221bfef..b6b9b6b8 100644 --- a/cty/function/stdlib/collection_test.go +++ b/cty/function/stdlib/collection_test.go @@ -2498,6 +2498,14 @@ func TestSetproduct(t *testing.T) { cty.UnknownVal(cty.Set(cty.Tuple([]cty.Type{cty.String, cty.Bool}))).RefineNotNull().WithMarks(cty.NewValueMarks("a", "b")), ``, }, + { + []cty.Value{ + cty.SetVal([]cty.Value{cty.True}), + cty.DynamicVal, + }, + cty.DynamicVal, + ``, + }, // If the inputs have unknown lengths but have length refinements then // we can potentially refine our unknown result too.