From 945568ef301a01a16564b885f6b25fc51f4974ac Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Sat, 30 Nov 2024 10:05:44 -0500
Subject: [PATCH] catch type errors in short-circuit ops

Don't let known type errors pass through boolean logic short-circuit
calls.
---
 hclsyntax/expression_ops.go  | 24 ++++++++++++++++++------
 hclsyntax/expression_test.go | 22 ++++++++++++++++++++++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/hclsyntax/expression_ops.go b/hclsyntax/expression_ops.go
index 82fb3242..2d02c5f6 100644
--- a/hclsyntax/expression_ops.go
+++ b/hclsyntax/expression_ops.go
@@ -180,10 +180,12 @@ 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)
 	lhsVal, err := convert.Convert(givenLHSVal, lhsParam.Type)
 	if err != nil {
-		lhsDiags = append(lhsDiags, &hcl.Diagnostic{
+		diags = append(diags, &hcl.Diagnostic{
 			Severity:    hcl.DiagError,
 			Summary:     "Invalid operand",
 			Detail:      fmt.Sprintf("Unsuitable value for left operand: %s.", err),
@@ -197,7 +199,7 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
 	givenRHSVal, rhsDiags := e.RHS.Value(ctx)
 	rhsVal, err := convert.Convert(givenRHSVal, rhsParam.Type)
 	if err != nil {
-		rhsDiags = append(rhsDiags, &hcl.Diagnostic{
+		diags = append(diags, &hcl.Diagnostic{
 			Severity:    hcl.DiagError,
 			Summary:     "Invalid operand",
 			Detail:      fmt.Sprintf("Unsuitable value for right operand: %s.", err),
@@ -208,24 +210,33 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
 		})
 	}
 
+	// diags so far only contains conversion errors, which should cover
+	// incorrect parameter types.
+	if diags.HasErrors() {
+		// Add the rest of the diagnostic in case that helps the user, but keep
+		// them separate as we continue for short-circuit handling.
+		diags = append(diags, lhsDiags...)
+		diags = append(diags, rhsDiags...)
+		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.
+	// executing the operation.
 	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
+			// would however not match the behavior of conditional expressions which
 			// do drop all diagnostics from the unevaluated expressions
-			return forceResult.WithMarks(lhsMarks, rhsMarks), nil
+			return forceResult.WithMarks(lhsMarks, rhsMarks), diags
 		}
 	}
 
-	var diags hcl.Diagnostics
 	diags = append(diags, lhsDiags...)
 	diags = append(diags, rhsDiags...)
 
@@ -234,6 +245,7 @@ func (e *BinaryOpExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
 		// 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 {
diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go
index 90ed741a..1a827f86 100644
--- a/hclsyntax/expression_test.go
+++ b/hclsyntax/expression_test.go
@@ -2510,6 +2510,28 @@ func TestExpressionErrorMessages(t *testing.T) {
 			"Unknown variable",
 			`There is no variable named "valeu". Did you mean "value"?`,
 		},
+		{
+			// Short-circuit must still catch type errors on the opposite side
+			"unknown && \"value\"",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"unknown": cty.UnknownVal(cty.Bool),
+				},
+			},
+			"Invalid operand",
+			`Unsuitable value for right operand: a bool is required.`,
+		},
+		{
+			// Short-circuiting must still catch type errors on the opposite side
+			"value && \"value\"",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"value": cty.False,
+				},
+			},
+			"Invalid operand",
+			`Unsuitable value for right operand: a bool is required.`,
+		},
 	}
 
 	for _, test := range tests {