From fe177269ab5aaa417d3a89455e1a3078f77784fd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 16 Dec 2024 17:35:45 -0500 Subject: [PATCH 1/2] dropped diagnostics There was no test looking for dropped diagnostics when boolean opts don't short-circuit. The missing diagnostics were what made some of the tests from the original PR pass through, so a correction to that will follow. --- hclsyntax/expression_ops.go | 5 ++--- hclsyntax/expression_test.go | 10 ++++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index 6d9e1af5..76b9d336 100644 --- a/hclsyntax/expression_ops.go +++ b/hclsyntax/expression_ops.go @@ -242,9 +242,6 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) lhsVal, lhsMarks := lhsVal.Unmark() rhsVal, 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 operation. if e.Op.ShortCircuit != nil { forceResult, diags := e.Op.ShortCircuit(lhsVal, rhsVal, lhsDiags, rhsDiags) if forceResult != cty.NilVal { @@ -256,6 +253,8 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, 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. diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 843847df..636a541d 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -2616,6 +2616,16 @@ func TestExpressionErrorMessages(t *testing.T) { "Function calls not allowed", `Functions may not be called here.`, }, + { + `map != null || map["key"] == "value"`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "map": cty.NullVal(cty.Map(cty.String)), + }, + }, + "Attempt to index null value", + `This value is null, so it does not have any indices.`, + }, } for _, test := range tests { From d69d8de449e3d652ae3a0b0f8dafb5ff644c7814 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 16 Dec 2024 19:08:42 -0500 Subject: [PATCH 2/2] re-capture the short-circuit with unknowns Drop RHS diagnostics when bot boolean op expression values are unknown. This allows skipping an evaluation error on the RHS in case we can short-circuit when the LHS is known. --- hclsyntax/expression_ops.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go index 76b9d336..da146ef8 100644 --- a/hclsyntax/expression_ops.go +++ b/hclsyntax/expression_ops.go @@ -38,6 +38,13 @@ var ( switch { // if both are unknown, we don't short circuit anything case !lhs.IsKnown() && !rhs.IsKnown(): + // short-circuit left-to-right when encountering a good unknown + // value and both are unknown. + if !lhsDiags.HasErrors() { + return cty.UnknownVal(cty.Bool).RefineNotNull(), lhsDiags + } + // If the LHS has an error, the RHS might too. Don't + // short-circuit so both diags get collected. return cty.NilVal, nil // for ||, a single true is the controlling condition @@ -46,7 +53,7 @@ var ( case rhs.IsKnown() && rhs.True(): return cty.True, rhsDiags - // if the opposing side is false we can't sort-circuit based on + // if the opposing side is false we can't short-circuit based on // boolean logic, so an unknown becomes the controlling condition case !lhs.IsKnown() && rhs.False(): return cty.UnknownVal(cty.Bool).RefineNotNull(), lhsDiags @@ -62,9 +69,16 @@ var ( Type: cty.Bool, ShortCircuit: func(lhs, rhs cty.Value, lhsDiags, rhsDiags hcl.Diagnostics) (cty.Value, hcl.Diagnostics) { + switch { - // if both are unknown, we don't short circuit anything case !lhs.IsKnown() && !rhs.IsKnown(): + // short-circuit left-to-right when encountering a good unknown + // value and both are unknown. + if !lhsDiags.HasErrors() { + return cty.UnknownVal(cty.Bool).RefineNotNull(), lhsDiags + } + // If the LHS has an error, the RHS might too. Don't + // short-circuit so both diags get collected. return cty.NilVal, nil // For &&, a single false is the controlling condition @@ -73,14 +87,13 @@ var ( case rhs.IsKnown() && rhs.False(): return cty.False, rhsDiags - // if the opposing side is true we can't sort-circuit based on + // if the opposing side is true we can't short-circuit based on // boolean logic, so an unknown becomes the controlling condition case !lhs.IsKnown() && rhs.True(): return cty.UnknownVal(cty.Bool).RefineNotNull(), lhsDiags case !rhs.IsKnown() && lhs.True(): return cty.UnknownVal(cty.Bool).RefineNotNull(), rhsDiags } - return cty.NilVal, nil }, }