Skip to content

Commit

Permalink
Allow can and try functions to handle more unknown
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jbardin committed Aug 14, 2023
1 parent 527ec31 commit 6af2870
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 41 deletions.
73 changes: 32 additions & 41 deletions ext/tryfunc/tryfunc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
30 changes: 30 additions & 0 deletions ext/tryfunc/tryfunc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 6af2870

Please sign in to comment.