pass all marks through conditional expressions

jbardin · jbardin · commit bbfec2d53251 · 2024-11-14T13:31:56.000-05:00
diff --git 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,15 +870,15 @@ 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
 			}
 		}
 
 		ret := cty.UnknownVal(resultType)
 		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
@@ -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
@@ -2175,7 +2175,7 @@ EOT
 					}).Mark("sensitive"),
 				},
 			},
-			cty.NumberIntVal(1),
+			cty.NumberIntVal(1).Mark("sensitive"),
 			0,
 		},
 		{ // auto-converts collection types

Original file line number	Diff line number	Diff line change
`@@ -788,21 +788,24 @@ func (e ConditionalExpr) Value(ctx hcl.EvalContext) (cty.Value, hcl.Diagnostic`
`788`	`788`	`})`
`789`	`789`	`return cty.UnknownVal(resultType), diags`
`790`	`790`	`}`
`791`		`- if !condResult.IsKnown() {`
`792`		`- // we use the unmarked values throughout the unknown branch`
`793`		`- _, condResultMarks := condResult.Unmark()`
`794`		`- trueResult, trueResultMarks := trueResult.Unmark()`
`795`		`- falseResult, falseResultMarks := falseResult.Unmark()`
`796`	`791`
`797`		`- // use a value to merge marks`
`798`		`- _, resMarks := cty.DynamicVal.WithMarks(condResultMarks, trueResultMarks, falseResultMarks).Unmark()`
	`792`	`+ // Now that we have all three values, collect all the marks for the result.`
	`793`	`+ // Since it's possible that a condition value could be unknown, and the`
	`794`	`+ // consumer needs to deal with any marks from either branch anyway, we must`
	`795`	`+ // always combine them for consistent results.`
	`796`	`+ condResult, condResultMarks := condResult.Unmark()`
	`797`	`+ trueResult, trueResultMarks := trueResult.Unmark()`
	`798`	`+ falseResult, falseResultMarks := falseResult.Unmark()`
	`799`	`+ var resMarks []cty.ValueMarks`
	`800`	`+ resMarks = append(resMarks, condResultMarks, trueResultMarks, falseResultMarks)`
`799`	`801`
	`802`	`+ if !condResult.IsKnown() {`
`800`	`803`	`trueRange := trueResult.Range()`
`801`	`804`	`falseRange := falseResult.Range()`
`802`	`805`
`803`	`806`	`// if both branches are known to be null, then the result must still be null`
`804`	`807`	`if trueResult.IsNull() && falseResult.IsNull() {`
`805`		`- return cty.NullVal(resultType).WithMarks(resMarks), diags`
	`808`	`+ return cty.NullVal(resultType).WithMarks(resMarks...), diags`
`806`	`809`	`}`
`807`	`810`
`808`	`811`	`// 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`
`841`	`844`	`ref = ref.NumberRangeUpperBound(hi, hiInc)`
`842`	`845`	`}`
`843`	`846`
`844`		`- return ref.NewValue().WithMarks(resMarks), diags`
	`847`	`+ return ref.NewValue().WithMarks(resMarks...), diags`
`845`	`848`	`}`
`846`	`849`
`847`	`850`	`if trueResult.Type().IsCollectionType() && falseResult.Type().IsCollectionType() {`
`@@ -867,15 +870,15 @@ func (e ConditionalExpr) Value(ctx hcl.EvalContext) (cty.Value, hcl.Diagnostic`
`867`	`870`	`}`
`868`	`871`
`869`	`872`	`ref = ref.CollectionLengthLowerBound(lo).CollectionLengthUpperBound(hi)`
`870`		`- return ref.NewValue().WithMarks(resMarks), diags`
	`873`	`+ return ref.NewValue().WithMarks(resMarks...), diags`
`871`	`874`	`}`
`872`	`875`	`}`
`873`	`876`
`874`	`877`	`ret := cty.UnknownVal(resultType)`
`875`	`878`	`if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() {`
`876`	`879`	`ret = ret.RefineNotNull()`
`877`	`880`	`}`
`878`		`- return ret.WithMarks(resMarks), diags`
	`881`	`+ return ret.WithMarks(resMarks...), diags`
`879`	`882`	`}`
`880`	`883`
`881`	`884`	`condResult, err := convert.Convert(condResult, cty.Bool)`
`@@ -892,8 +895,6 @@ func (e ConditionalExpr) Value(ctx hcl.EvalContext) (cty.Value, hcl.Diagnostic`
`892`	`895`	`return cty.UnknownVal(resultType), diags`
`893`	`896`	`}`
`894`	`897`
`895`		`- // Unmark result before testing for truthiness`
`896`		`- condResult, _ = condResult.UnmarkDeep()`
`897`	`898`	`if condResult.True() {`
`898`	`899`	`diags = append(diags, trueDiags...)`
`899`	`900`	`if convs[0] != nil {`
`@@ -916,7 +917,7 @@ func (e ConditionalExpr) Value(ctx hcl.EvalContext) (cty.Value, hcl.Diagnostic`
`916`	`917`	`trueResult = cty.UnknownVal(resultType)`
`917`	`918`	`}`
`918`	`919`	`}`
`919`		`- return trueResult, diags`
	`920`	`+ return trueResult.WithMarks(resMarks...), diags`
`920`	`921`	`} else {`
`921`	`922`	`diags = append(diags, falseDiags...)`
`922`	`923`	`if convs[1] != nil {`
`@@ -939,7 +940,7 @@ func (e ConditionalExpr) Value(ctx hcl.EvalContext) (cty.Value, hcl.Diagnostic`
`939`	`940`	`falseResult = cty.UnknownVal(resultType)`
`940`	`941`	`}`
`941`	`942`	`}`
`942`		`- return falseResult, diags`
	`943`	`+ return falseResult.WithMarks(resMarks...), diags`
`943`	`944`	`}`
`944`	`945`	`}`
`945`	`946`