From 6af2870c111bc3fe3364decc2902a803a5550eed Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 14 Aug 2023 13:07:03 -0400 Subject: [PATCH] Allow can and try functions to handle more unknown The `can` and `try` functions can return more precise results in some cases. Rather than try to inspect the expressions for any unknown values, rely on the evaluation result to be correct or error and base the decision on the evaluated values and errors. A fundamental requirement for the `try` and `can` functions is that the value and types remain consistent as argument values are refined. This can be done provided we hold these conditions regarding unknowns to be true: - An evaluation error can never be fixed by an unknown value becoming known. - An entirely known value from an expression cannot later become unknown as values are refined. - A expression result must always have a consistent type and value. only allowing the refinement of unknown values and types. - An expression result cannot be conditionally based on the "known-ness" of a value (which is really the premise for all previous statements). As long as those conditions remain true, the result of the `try` argument's evaluation can be trusted, and we don't need to bail out early at any sign of an unknown in the argument expressions. While the evaluation result of each argument can be trusted in isolation however, the fact that different types and values can be returned by `try` means we need to convert the return to the most generic value possible to prevent inconsistent results ourself (adhering to the 3rd condition above). That means anything which is not entirely known must be converted to a dynamic value. Evan more refinement might still be possible in the future if all arguments are evaluated and compared for compatibility, but care needs to be taken to prevent changing known values within collections from different arguments even when types are identical. --- ext/tryfunc/tryfunc.go | 73 ++++++++++++++++--------------------- ext/tryfunc/tryfunc_test.go | 30 +++++++++++++++ 2 files changed, 62 insertions(+), 41 deletions(-) diff --git a/ext/tryfunc/tryfunc.go b/ext/tryfunc/tryfunc.go index 47789858..fa601da4 100644 --- a/ext/tryfunc/tryfunc.go +++ b/ext/tryfunc/tryfunc.go @@ -72,18 +72,35 @@ func try(args []cty.Value) (cty.Value, error) { var diags hcl.Diagnostics for _, arg := range args { closure := customdecode.ExpressionClosureFromVal(arg) - if dependsOnUnknowns(closure.Expression, closure.EvalContext) { - // We can't safely decide if this expression will succeed yet, - // and so our entire result must be unknown until we have - // more information. - return cty.DynamicVal, nil - } v, moreDiags := closure.Value() diags = append(diags, moreDiags...) + if moreDiags.HasErrors() { - continue // try the next one, if there is one to try + // If there's an error we know it will always fail and can + // continue. A more refined value will not remove an error from + // the expression. + continue } + + if !v.IsWhollyKnown() { + // If there are any unknowns in the value at all, we cannot be + // certain that the final value will be consistent or have the same + // type, so wee need to be conservative and return a dynamic value. + + // There are two different classes of failure that can happen when + // an expression transitions from unknown to known; an operation on + // a dynamic value becomes invalid for the type once the type is + // known, or an index expression on a collection fails once the + // collection value is known. These changes from a + // valid-partially-unknown expression to an invalid-known + // expression can produce inconsistent results by changing which + // "try" argument is returned, which may be a collection with + // different previously known values, or a different type entirely + // ("try" does not require consistent argument types) + return cty.DynamicVal, nil + } + return v, nil // ignore any accumulated diagnostics if one succeeds } @@ -111,43 +128,17 @@ func try(args []cty.Value) (cty.Value, error) { func can(arg cty.Value) (cty.Value, error) { closure := customdecode.ExpressionClosureFromVal(arg) - if dependsOnUnknowns(closure.Expression, closure.EvalContext) { - // Can't decide yet, then. - return cty.UnknownVal(cty.Bool), nil - } - - _, diags := closure.Value() + v, diags := closure.Value() if diags.HasErrors() { return cty.False, nil } - return cty.True, nil -} -// dependsOnUnknowns returns true if any of the variables that the given -// expression might access are unknown values or contain unknown values. -// -// This is a conservative result that prefers to return true if there's any -// chance that the expression might derive from an unknown value during its -// evaluation; it is likely to produce false-positives for more complex -// expressions involving deep data structures. -func dependsOnUnknowns(expr hcl.Expression, ctx *hcl.EvalContext) bool { - for _, traversal := range expr.Variables() { - val, diags := traversal.TraverseAbs(ctx) - if diags.HasErrors() { - // If the traversal returned a definitive error then it must - // not traverse through any unknowns. - continue - } - if !val.IsWhollyKnown() { - // The value will be unknown if either it refers directly to - // an unknown value or if the traversal moves through an unknown - // collection. We're using IsWhollyKnown, so this also catches - // situations where the traversal refers to a compound data - // structure that contains any unknown values. That's important, - // because during evaluation the expression might evaluate more - // deeply into this structure and encounter the unknowns. - return true - } + if !v.IsWhollyKnown() { + // If the value is not wholly known, we still cannot be certain that + // the expression was valid. There may be yet index expressions which + // will fail once values are completely known. + return cty.UnknownVal(cty.Bool), nil } - return false + + return cty.True, nil } diff --git a/ext/tryfunc/tryfunc_test.go b/ext/tryfunc/tryfunc_test.go index 1e75a163..7bd7af6d 100644 --- a/ext/tryfunc/tryfunc_test.go +++ b/ext/tryfunc/tryfunc_test.go @@ -107,6 +107,36 @@ func TestTryFunc(t *testing.T) { cty.StringVal("list").Mark("secret"), ``, }, + "nested known expression from unknown": { + // this expression contains an unknown, but will always return in + // "bar" + `try({u: false ? unknown : "bar"}, other)`, + map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.String), + "other": cty.MapVal(map[string]cty.Value{ + "v": cty.StringVal("oops"), + }), + }, + cty.ObjectVal(map[string]cty.Value{ + "u": cty.StringVal("bar"), + }), + ``, + }, + "nested index op on unknown": { + // unknown and other have identical types, but we must return a + // dynamic value since v could change within the final result value + // after the first argument becomes known. + `try({u: unknown["foo"], v: "orig"}, other)`, + map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Map(cty.String)), + "other": cty.MapVal(map[string]cty.Value{ + "u": cty.StringVal("oops"), + "v": cty.StringVal("oops"), + }), + }, + cty.DynamicVal, + ``, + }, "three arguments, all fail": { `try(this, that, this_thing_in_particular)`, nil,