Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better control of marks through conditional and for expressions #710

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Better control of marks through conditional and for expressions #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbfec2d
pass all marks through conditional expressions
jbardin Nov 14, 2024
b48ba6e
pass marks through unknown ForExpr values
jbardin Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 47 additions & 27 deletions hclsyntax/expression.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading