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.

Author: apparentlymart

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

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

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.