Merge pull request #719 from hashicorp/jbardin/short-circuits

fix boolean short-circuit diagnostic handling

Author: jbardin

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
 		},
 	}
@@ -242,9 +255,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 +266,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.

hclsyntax/expression_test.go

@@ -2664,6 +2664,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 {