diff --git a/eval_context.go b/eval_context.go index 915910ad..87f9bb58 100644 --- a/eval_context.go +++ b/eval_context.go @@ -23,3 +23,37 @@ func (ctx *EvalContext) NewChild() *EvalContext { func (ctx *EvalContext) Parent() *EvalContext { return ctx.parent } + +// NewChildAllVariablesUnknown is an extension of NewChild which, in addition +// to creating a child context, also pre-populates its Variables table +// with variable definitions masking every variable define in the reciever +// and its ancestors with an unknown value of the same type as the original. +// +// The child does not initially have any of its own functions defined, and so +// it can still inherit any defined functions from the reciever. +// +// Because is function effectively takes a snapshot of the variables as they +// are defined at the time of the call, it is incorrect to subsequently +// modify the variables in any of the ancestor contexts in a way that would +// change which variables are defined or what value types they each have. +// +// This is a specialized helper function intended to support type-checking +// use-cases, where the goal is only to check whether values are being used +// in a way that makes sense for their types while not reacting to their +// actual values. +func (ctx *EvalContext) NewChildAllVariablesUnknown() *EvalContext { + ret := ctx.NewChild() + ret.Variables = make(map[string]cty.Value) + + currentAncestor := ctx + for currentAncestor != nil { + for name, val := range currentAncestor.Variables { + if _, ok := ret.Variables[name]; !ok { + ret.Variables[name] = cty.UnknownVal(val.Type()) + } + } + currentAncestor = currentAncestor.parent + } + + return ret +} diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index a351096d..f6674949 100644 --- a/hclsyntax/expression_ops.go +++ b/hclsyntax/expression_ops.go @@ -13,16 +13,39 @@ import ( type Operation struct { Impl function.Function Type cty.Type + + // ShortCircuit is an optional callback for binary operations which, if + // set, will be called with the result of evaluating the LHS expression. + // + // ShortCircuit may return cty.NilVal to allow evaluation to proceed + // as normal, or it may return a non-nil value to force the operation + // to return that value and perform only type checking on the RHS + // expression, as opposed to full evaluation. + ShortCircuit func(lhs cty.Value) cty.Value } var ( OpLogicalOr = &Operation{ Impl: stdlib.OrFunc, Type: cty.Bool, + + ShortCircuit: func(lhs cty.Value) cty.Value { + if lhs.RawEquals(cty.True) { + return cty.True + } + return cty.NilVal + }, } OpLogicalAnd = &Operation{ Impl: stdlib.AndFunc, Type: cty.Bool, + + ShortCircuit: func(lhs cty.Value) cty.Value { + if lhs.RawEquals(cty.False) { + return cty.False + } + return cty.NilVal + }, } OpLogicalNot = &Operation{ Impl: stdlib.NotFunc, @@ -146,10 +169,7 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) var diags hcl.Diagnostics givenLHSVal, lhsDiags := e.LHS.Value(ctx) - givenRHSVal, rhsDiags := e.RHS.Value(ctx) diags = append(diags, lhsDiags...) - diags = append(diags, rhsDiags...) - lhsVal, err := convert.Convert(givenLHSVal, lhsParam.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ @@ -162,6 +182,35 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) EvalContext: ctx, }) } + + // If this is a short-circuiting operator and the LHS produces a + // short-circuiting result then we'll evaluate the RHS only for type + // checking purposes, ignoring any specific values, as a compromise + // between the convenience of a total short-circuit behavior and the + // benefit of not masking type errors on the RHS that we could still + // give earlier feedback about. + var forceResult cty.Value + rhsCtx := ctx + if e.Op.ShortCircuit != nil { + if !givenLHSVal.IsKnown() { + // If this is a short-circuit operator and our LHS value is + // unknown then we can't predict whether we would short-circuit + // yet, and so we must proceed under the assumption that we _will_ + // short-circuit to avoid raising any errors on the RHS that would + // eventually be hidden by the short-circuit behavior once LHS + // becomes known. + forceResult = cty.UnknownVal(e.Op.Type) + rhsCtx = ctx.NewChildAllVariablesUnknown() + } else if forceResult = e.Op.ShortCircuit(givenLHSVal); forceResult != cty.NilVal { + // This ensures that we'll only be type-checking against any + // variables used on the RHS, while not raising any errors about + // their values. + rhsCtx = ctx.NewChildAllVariablesUnknown() + } + } + + givenRHSVal, rhsDiags := e.RHS.Value(rhsCtx) + diags = append(diags, rhsDiags...) rhsVal, err := convert.Convert(givenRHSVal, rhsParam.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ @@ -181,6 +230,13 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) return cty.UnknownVal(e.Op.Type), diags } + // If we short-circuited above and still passed the type-check of RHS then + // we'll halt here and return the short-circuit result rather than actually + // executing the opertion. + if forceResult != cty.NilVal { + return forceResult, diags + } + args := []cty.Value{lhsVal, rhsVal} result, err := impl.Call(args) if err != nil { diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 77d959fb..0c2840a9 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1740,6 +1740,64 @@ EOT cty.False, 0, }, + { + // Logical AND operator short-circuit behavior + `nullobj != null && nullobj.is_thingy`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "nullobj": cty.NullVal(cty.Object(map[string]cty.Type{ + "is_thingy": cty.Bool, + })), + }, + }, + cty.False, + 0, // nullobj != null prevents evaluating nullobj.is_thingy + }, + { + // Logical AND short-circuit handling of unknown values + // If the first operand is an unknown bool then we can't know if + // we will short-circuit or not, and so we must assume we will + // and wait until the value becomes known before fully evaluating RHS. + `unknown < 4 && list[zero]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Number), + "zero": cty.Zero, + "list": cty.ListValEmpty(cty.Bool), + }, + }, + cty.UnknownVal(cty.Bool), + 0, + }, + { + // Logical OR operator short-circuit behavior + `nullobj == null || nullobj.is_thingy`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "nullobj": cty.NullVal(cty.Object(map[string]cty.Type{ + "is_thingy": cty.Bool, + })), + }, + }, + cty.True, + 0, // nullobj == null prevents evaluating nullobj.is_thingy + }, + { + // Logical OR short-circuit handling of unknown values + // If the first operand is an unknown bool then we can't know if + // we will short-circuit or not, and so we must assume we will + // and wait until the value becomes known before fully evaluating RHS. + `unknown > 4 || list[zero]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Number), + "zero": cty.Zero, + "list": cty.ListValEmpty(cty.Bool), + }, + }, + cty.UnknownVal(cty.Bool), + 0, + }, { `true ? var : null`, &hcl.EvalContext{ @@ -1983,6 +2041,59 @@ func TestExpressionErrorMessages(t *testing.T) { // describe coherently. "The true and false result expressions must have consistent types. At least one deeply-nested attribute or element is not compatible across both the 'true' and the 'false' value.", }, + + // Error messages describing situations where the logical operator + // short-circuit behavior still found a type error on the RHS that + // we therefore still report, because the LHS only guards against + // value-related problems in the RHS. + { + // It's not valid to access an attribute on a non-object-typed + // value even if we've proven it isn't null. + "notobj != null && notobj.foo", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notobj": cty.True, + }, + }, + "Unsupported attribute", + "Can't access attributes on a primitive-typed value (bool).", + }, + { + // It's not valid to access an attribute on a non-object-typed + // value even if we've proven it isn't null. + "notobj == null || notobj.foo", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notobj": cty.True, + }, + }, + "Unsupported attribute", + "Can't access attributes on a primitive-typed value (bool).", + }, + { + // It's not valid to access an index on an unindexable type + // even if we've proven it isn't null. + "notlist != null && notlist[0]", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "notlist": cty.True, + }, + }, + "Invalid index", + "This value does not have any indices.", + }, + { + // Short-circuit can't avoid an error accessing a variable that + // doesn't exist at all, so we can still report typos. + "value != null && valeu", + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "value": cty.True, + }, + }, + "Unknown variable", + `There is no variable named "valeu". Did you mean "value"?`, + }, } for _, test := range tests {