Merge pull request #710 from hashicorp/jbardin/marked-conditions

jbardin · web-flow · commit 56a9aee5207d · 2024-11-15T11:25:01.000-05:00
Better control of marks through conditional and for expressions
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
 	}
 }
 
@@ -1429,9 +1430,9 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 		})
 		return cty.DynamicVal, diags
 	}
-	if !collVal.IsKnown() {
-		return cty.DynamicVal, diags
-	}
+
+	// Grab the CondExpr marks when we're returning early with an unknown
+	var condMarks cty.ValueMarks
 
 	// Before we start we'll do an early check to see if any CondExpr we've
 	// been given is of the wrong type. This isn't 100% reliable (it may
@@ -1459,6 +1460,9 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 			})
 			return cty.DynamicVal, diags
 		}
+
+		_, condMarks = result.Unmark()
+
 		_, err := convert.Convert(result, cty.Bool)
 		if err != nil {
 			diags = append(diags, &hcl.Diagnostic{
@@ -1477,6 +1481,10 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 		}
 	}
 
+	if !collVal.IsKnown() {
+		return cty.DynamicVal.WithMarks(append(marks, condMarks)...), diags
+	}
+
 	if e.KeyExpr != nil {
 		// Producing an object
 		var vals map[string]cty.Value
@@ -1517,6 +1525,12 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 					known = false
 					continue
 				}
+
+				// Extract and merge marks from the include expression into the
+				// main set of marks
+				_, includeMarks := includeRaw.Unmark()
+				marks = append(marks, includeMarks)
+
 				include, err := convert.Convert(includeRaw, cty.Bool)
 				if err != nil {
 					if known {
@@ -1540,7 +1554,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 
 				// Extract and merge marks from the include expression into the
 				// main set of marks
-				includeUnmarked, includeMarks := include.Unmark()
+				includeUnmarked, _ := include.Unmark()
 				marks = append(marks, includeMarks)
 				if includeUnmarked.False() {
 					// Skip this element
@@ -1565,6 +1579,10 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 				known = false
 				continue
 			}
+
+			_, keyMarks := keyRaw.Unmark()
+			marks = append(marks, keyMarks)
+
 			if !keyRaw.IsKnown() {
 				known = false
 				continue
@@ -1587,8 +1605,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 				continue
 			}
 
-			key, keyMarks := key.Unmark()
-			marks = append(marks, keyMarks)
+			key, _ = key.Unmark()
 
 			val, valDiags := e.ValExpr.Value(childCtx)
 			diags = append(diags, valDiags...)
@@ -1618,7 +1635,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 		}
 
 		if !known {
-			return cty.DynamicVal, diags
+			return cty.DynamicVal.WithMarks(marks...), diags
 		}
 
 		if e.Group {
@@ -1664,6 +1681,12 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 					known = false
 					continue
 				}
+
+				// Extract and merge marks from the include expression into the
+				// main set of marks
+				_, includeMarks := includeRaw.Unmark()
+				marks = append(marks, includeMarks)
+
 				if !includeRaw.IsKnown() {
 					// We will eventually return DynamicVal, but we'll continue
 					// iterating in case there are other diagnostics to gather
@@ -1689,10 +1712,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 					continue
 				}
 
-				// Extract and merge marks from the include expression into the
-				// main set of marks
-				includeUnmarked, includeMarks := include.Unmark()
-				marks = append(marks, includeMarks)
+				includeUnmarked, _ := include.Unmark()
 				if includeUnmarked.False() {
 					// Skip this element
 					continue
@@ -1705,7 +1725,7 @@ func (e *ForExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
 		}
 
 		if !known {
-			return cty.DynamicVal, diags
+			return cty.DynamicVal.WithMarks(marks...), diags
 		}
 
 		return cty.TupleVal(vals).WithMarks(marks...), 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
@@ -1143,6 +1143,49 @@ upper(
 			}).Mark("sensitive"),
 			0,
 		},
+		{
+			`{ for k, v in things: k => v if k != unknown_secret }`,
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"things": cty.MapVal(map[string]cty.Value{
+						"a": cty.True,
+						"b": cty.False,
+						"c": cty.False,
+					}),
+					"unknown_secret": cty.UnknownVal(cty.String).Mark("sensitive"),
+				},
+			},
+			cty.DynamicVal.Mark("sensitive"),
+			0,
+		},
+		{
+			`[ for v in things: v if v != unknown_secret ]`,
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"things": cty.TupleVal([]cty.Value{
+						cty.StringVal("a"),
+						cty.StringVal("b"),
+					}),
+					"unknown_secret": cty.UnknownVal(cty.String).Mark("sensitive"),
+				},
+			},
+			cty.DynamicVal.Mark("sensitive"),
+			0,
+		},
+		{
+			`[ for v in things: v if v != secret ]`,
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"things": cty.TupleVal([]cty.Value{
+						cty.UnknownVal(cty.String).Mark("mark"),
+						cty.StringVal("b"),
+					}),
+					"secret": cty.StringVal("b").Mark("sensitive"),
+				},
+			},
+			cty.DynamicVal.WithMarks(cty.NewValueMarks("mark", "sensitive")),
+			0,
+		},
 		{
 			`[{name: "Steve"}, {name: "Ermintrude"}].*.name`,
 			nil,
@@ -2175,7 +2218,7 @@ EOT
 					}).Mark("sensitive"),
 				},
 			},
-			cty.NumberIntVal(1),
+			cty.NumberIntVal(1).Mark("sensitive"),
 			0,
 		},
 		{ // auto-converts collection types
