Skip to content

Commit

Permalink
short-circuit for && and || operators
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbardin
jbardin committed Nov 20, 2024
1 parent bee2dc2 commit 8dbb282
Show file tree
Hide file tree
Showing 2 changed files with 177 additions and 12 deletions.
78 changes: 66 additions & 12 deletions hclsyntax/expression_ops.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,54 @@ 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, rhs cty.Value) cty.Value
}

var (
OpLogicalOr = &Operation{
Impl: stdlib.OrFunc,
Type: cty.Bool,

ShortCircuit: func(lhs, rhs cty.Value) cty.Value {
if lhs.IsKnown() && lhs.True() {
return cty.True
}

if !lhs.IsKnown() {
if rhs.IsKnown() && rhs.True() {
return cty.True
}
return cty.UnknownVal(cty.Bool)
}

return cty.NilVal
},
}
OpLogicalAnd = &Operation{
Impl: stdlib.AndFunc,
Type: cty.Bool,

ShortCircuit: func(lhs, rhs cty.Value) cty.Value {
if lhs.IsKnown() && lhs.False() {
return cty.False
}

if !lhs.IsKnown() {
if rhs.IsKnown() && rhs.False() {
return cty.False
}
return cty.UnknownVal(cty.Bool)
}
return cty.NilVal
},
}
OpLogicalNot = &Operation{
Impl: stdlib.NotFunc,
Expand Down Expand Up @@ -142,16 +180,10 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
lhsParam := params[0]
rhsParam := params[1]

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{
lhsDiags = append(lhsDiags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid operand",
Detail: fmt.Sprintf("Unsuitable value for left operand: %s.", err),
Expand All @@ -161,9 +193,11 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
EvalContext: ctx,
})
}

givenRHSVal, rhsDiags := e.RHS.Value(ctx)
rhsVal, err := convert.Convert(givenRHSVal, rhsParam.Type)
if err != nil {
diags = append(diags, &hcl.Diagnostic{
rhsDiags = append(rhsDiags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid operand",
Detail: fmt.Sprintf("Unsuitable value for right operand: %s.", err),
Expand All @@ -174,12 +208,32 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
})
}

if diags.HasErrors() {
// Don't actually try the call if we have errors already, since the
// this will probably just produce a confusing duplicative diagnostic.
return cty.UnknownVal(e.Op.Type), diags
_, lhsMarks := lhsVal.Unmark()
_, rhsMarks := rhsVal.Unmark()

// 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 e.Op.ShortCircuit != nil {
forceResult := e.Op.ShortCircuit(lhsVal, rhsVal)
if forceResult != cty.NilVal {
// It would be technically more correct to insert rhs diagnostics if
// forceResult is not known since we didn't really short-circuit. That
// would however not match the behavior fo conditional expressions which
// do drop all diagnostics from the unevaluated expressions
return forceResult.WithMarks(lhsMarks, rhsMarks), nil
}
}

var diags hcl.Diagnostics
diags = append(diags, lhsDiags...)
diags = append(diags, rhsDiags...)

if diags.HasErrors() {
// Don't actually try the call if we have errors, since the this will
// probably just produce confusing duplicate diagnostics.
return cty.UnknownVal(e.Op.Type).WithMarks(lhsMarks, rhsMarks), diags
}
args := []cty.Value{lhsVal, rhsVal}
result, err := impl.Call(args)
if err != nil {
Expand Down
111 changes: 111 additions & 0 deletions hclsyntax/expression_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1913,6 +1913,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{
Expand Down Expand Up @@ -2399,6 +2457,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 {
Expand Down

0 comments on commit 8dbb282

Please sign in to comment.