Skip to content

Commit

Permalink
Merge pull request #710 from hashicorp/jbardin/marked-conditions
Browse files Browse the repository at this point in the history 
Better control of marks through conditional and for expressions
  • Loading branch information
@jbardin
jbardin authored Nov 15, 2024
2 parents d20d07f + b48ba6e commit 56a9aee
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 30 deletions.
74 changes: 47 additions & 27 deletions hclsyntax/expression.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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...)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions hclsyntax/expression_template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
45 changes: 44 additions & 1 deletion hclsyntax/expression_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2175,7 +2218,7 @@ EOT
}).Mark("sensitive"),
},
},
cty.NumberIntVal(1),
cty.NumberIntVal(1).Mark("sensitive"),
0,
},
{ // auto-converts collection types
Expand Down

0 comments on commit 56a9aee

Please sign in to comment.