From bbfec2d532518e6536b9924d1f039abd07c6f2ef Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 14 Nov 2024 13:25:10 -0500
Subject: [PATCH] pass all marks through conditional expressions

---
 hclsyntax/expression.go               | 31 ++++++++++++++-------------
 hclsyntax/expression_template_test.go |  4 ++--
 hclsyntax/expression_test.go          |  2 +-
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go
index 577a50fa..94977460 100644
--- a/hclsyntax/expression.go
+++ b/hclsyntax/expression.go
@@ -788,21 +788,24 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 		})
 		return cty.UnknownVal(resultType), diags
 	}
-	if !condResult.IsKnown() {
-		// we use the unmarked values throughout the unknown branch
-		_, condResultMarks := condResult.Unmark()
-		trueResult, trueResultMarks := trueResult.Unmark()
-		falseResult, falseResultMarks := falseResult.Unmark()
 
-		// use a value to merge marks
-		_, resMarks := cty.DynamicVal.WithMarks(condResultMarks, trueResultMarks, falseResultMarks).Unmark()
+	// Now that we have all three values, collect all the marks for the result.
+	// Since it's possible that a condition value could be unknown, and the
+	// consumer needs to deal with any marks from either branch anyway, we must
+	// always combine them for consistent results.
+	condResult, condResultMarks := condResult.Unmark()
+	trueResult, trueResultMarks := trueResult.Unmark()
+	falseResult, falseResultMarks := falseResult.Unmark()
+	var resMarks []cty.ValueMarks
+	resMarks = append(resMarks, condResultMarks, trueResultMarks, falseResultMarks)
 
+	if !condResult.IsKnown() {
 		trueRange := trueResult.Range()
 		falseRange := falseResult.Range()
 
 		// if both branches are known to be null, then the result must still be null
 		if trueResult.IsNull() && falseResult.IsNull() {
-			return cty.NullVal(resultType).WithMarks(resMarks), diags
+			return cty.NullVal(resultType).WithMarks(resMarks...), diags
 		}
 
 		// We might be able to offer a refined range for the result based on
@@ -841,7 +844,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 				ref = ref.NumberRangeUpperBound(hi, hiInc)
 			}
 
-			return ref.NewValue().WithMarks(resMarks), diags
+			return ref.NewValue().WithMarks(resMarks...), diags
 		}
 
 		if trueResult.Type().IsCollectionType() && falseResult.Type().IsCollectionType() {
@@ -867,7 +870,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 				}
 
 				ref = ref.CollectionLengthLowerBound(lo).CollectionLengthUpperBound(hi)
-				return ref.NewValue().WithMarks(resMarks), diags
+				return ref.NewValue().WithMarks(resMarks...), diags
 			}
 		}
 
@@ -875,7 +878,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 		if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() {
 			ret = ret.RefineNotNull()
 		}
-		return ret.WithMarks(resMarks), diags
+		return ret.WithMarks(resMarks...), diags
 	}
 
 	condResult, err := convert.Convert(condResult, cty.Bool)
@@ -892,8 +895,6 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 		return cty.UnknownVal(resultType), diags
 	}
 
-	// Unmark result before testing for truthiness
-	condResult, _ = condResult.UnmarkDeep()
 	if condResult.True() {
 		diags = append(diags, trueDiags...)
 		if convs[0] != nil {
@@ -916,7 +917,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 				trueResult = cty.UnknownVal(resultType)
 			}
 		}
-		return trueResult, diags
+		return trueResult.WithMarks(resMarks...), diags
 	} else {
 		diags = append(diags, falseDiags...)
 		if convs[1] != nil {
@@ -939,7 +940,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 				falseResult = cty.UnknownVal(resultType)
 			}
 		}
-		return falseResult, diags
+		return falseResult.WithMarks(resMarks...), diags
 	}
 }
 
diff --git a/hclsyntax/expression_template_test.go b/hclsyntax/expression_template_test.go
index 31198458..92c11afe 100644
--- a/hclsyntax/expression_template_test.go
+++ b/hclsyntax/expression_template_test.go
@@ -318,14 +318,14 @@ trim`,
 			cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull(strings.Repeat("_", 128)).NewValue(),
 			0,
 		},
-		{ // marks from uninterpolated values are ignored
+		{ // all marks are passed through to ensure result is always consistent
 			`hello%{ if false } ${target}%{ endif }`,
 			&hcl.EvalContext{
 				Variables: map[string]cty.Value{
 					"target": cty.StringVal("world").Mark("sensitive"),
 				},
 			},
-			cty.StringVal("hello"),
+			cty.StringVal("hello").Mark("sensitive"),
 			0,
 		},
 		{ // marks from interpolated values are passed through
diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go
index b50dc137..cb1132e3 100644
--- a/hclsyntax/expression_test.go
+++ b/hclsyntax/expression_test.go
@@ -2175,7 +2175,7 @@ EOT
 					}).Mark("sensitive"),
 				},
 			},
-			cty.NumberIntVal(1),
+			cty.NumberIntVal(1).Mark("sensitive"),
 			0,
 		},
 		{ // auto-converts collection types