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

Jump to bottom

hclsyntax: conditional refinements of collections must use Range() #633

Merged
merged 3 commits into from
Oct 12, 2023
Merged

hclsyntax: conditional refinements of collections must use Range() #633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a30333
refinements of collections must use Range()
jbardin Oct 11, 2023
e4589e3
Range() calls must always be unmarked
jbardin Oct 11, 2023
bad33d5
further refine refinement handling
jbardin Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
124 changes: 78 additions & 46 deletions hclsyntax/expression.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,64 +696,95 @@ 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting trick! Took me some squinting at it to understand what was going on here but I guess it's reasonable. Part of me wants to suggest a more elaborate comment explaining what's going on here but I have trouble imagining what would be useful to say. 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, I thought that would get noticed ;) As soon as I started looking at conditional handling I wanted set operations on ValueMarks; maybe later.


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
}

// We might be able to offer a refined range for the result based on
// the two possible outcomes.
if trueResult.Type() == cty.Number && falseResult.Type() == cty.Number {
// This case deals with the common case of (predicate ? 1 : 0) and
// significantly decreases the range of the result in that case.
if !(trueResult.IsNull() || falseResult.IsNull()) {
if gt := trueResult.GreaterThan(falseResult); gt.IsKnown() {
b := cty.UnknownVal(cty.Number).Refine()
if gt.True() {
b = b.
NumberRangeLowerBound(falseResult, true).
NumberRangeUpperBound(trueResult, true)
} else {
b = b.
NumberRangeLowerBound(trueResult, true).
NumberRangeUpperBound(falseResult, true)
}
b = b.NotNull() // If neither of the results is null then the result can't be either
return b.NewValue().WithSameMarks(condResult).WithSameMarks(trueResult).WithSameMarks(falseResult), diags
ref := cty.UnknownVal(cty.Number).Refine()
if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() {
ref = ref.NotNull()
}

falseLo, falseLoInc := falseRange.NumberLowerBound()
falseHi, falseHiInc := falseRange.NumberUpperBound()
trueLo, trueLoInc := trueRange.NumberLowerBound()
trueHi, trueHiInc := trueRange.NumberUpperBound()

if falseLo.IsKnown() && trueLo.IsKnown() {
lo, loInc := falseLo, falseLoInc
switch {
case trueLo.LessThan(falseLo).True():
lo, loInc = trueLo, trueLoInc
case trueLo.Equals(falseLo).True():
loInc = trueLoInc || falseLoInc
}

ref = ref.NumberRangeLowerBound(lo, loInc)
}

if falseHi.IsKnown() && trueHi.IsKnown() {
hi, hiInc := falseHi, falseHiInc
switch {
case trueHi.GreaterThan(falseHi).True():
hi, hiInc = trueHi, trueHiInc
case trueHi.Equals(falseHi).True():
hiInc = trueHiInc || falseHiInc
}
ref = ref.NumberRangeUpperBound(hi, hiInc)
}

return ref.NewValue().WithMarks(resMarks), diags
}

if trueResult.Type().IsCollectionType() && falseResult.Type().IsCollectionType() {
if trueResult.Type().Equals(falseResult.Type()) {
if !(trueResult.IsNull() || falseResult.IsNull()) {
trueLen := trueResult.Length()
falseLen := falseResult.Length()
if gt := trueLen.GreaterThan(falseLen); gt.IsKnown() {
b := cty.UnknownVal(resultType).Refine()
trueLen, _ := trueLen.AsBigFloat().Int64()
falseLen, _ := falseLen.AsBigFloat().Int64()
if gt.True() {
b = b.
CollectionLengthLowerBound(int(falseLen)).
CollectionLengthUpperBound(int(trueLen))
} else {
b = b.
CollectionLengthLowerBound(int(trueLen)).
CollectionLengthUpperBound(int(falseLen))
}
b = b.NotNull() // If neither of the results is null then the result can't be either
return b.NewValue().WithSameMarks(condResult).WithSameMarks(trueResult).WithSameMarks(falseResult), diags
}
ref := cty.UnknownVal(resultType).Refine()
if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() {
ref = ref.NotNull()
}

falseLo := falseRange.LengthLowerBound()
falseHi := falseRange.LengthUpperBound()
trueLo := trueRange.LengthLowerBound()
trueHi := trueRange.LengthUpperBound()

lo := falseLo
if trueLo < falseLo {
lo = trueLo
}

hi := falseHi
if trueHi > falseHi {
hi = trueHi
}

ref = ref.CollectionLengthLowerBound(lo).CollectionLengthUpperBound(hi)
return ref.NewValue().WithMarks(resMarks), diags
}
}
_, condResultMarks := condResult.Unmark()
trueResult, trueResultMarks := trueResult.Unmark()
falseResult, falseResultMarks := falseResult.Unmark()

trueRng := trueResult.Range()
falseRng := falseResult.Range()
ret := cty.UnknownVal(resultType)
if trueRng.DefinitelyNotNull() && falseRng.DefinitelyNotNull() {
if trueRange.DefinitelyNotNull() && falseRange.DefinitelyNotNull() {
ret = ret.RefineNotNull()
}
return ret.WithMarks(condResultMarks, trueResultMarks, falseResultMarks), diags
return ret.WithMarks(resMarks), diags
}

condResult, err := convert.Convert(condResult, cty.Bool)
if err != nil {
diags = append(diags, &hcl.Diagnostic{
Expand Down Expand Up @@ -1244,9 +1275,9 @@ func (e *ObjectConsKeyExpr) UnwrapExpression() Expression {

// ForExpr represents iteration constructs:
//
// tuple = [for i, v in list: upper(v) if i > 2]
// object = {for k, v in map: k => upper(v)}
// object_of_tuples = {for v in list: v.key: v...}
// tuple = [for i, v in list: upper(v) if i > 2]
// object = {for k, v in map: k => upper(v)}
// object_of_tuples = {for v in list: v.key: v...}
type ForExpr struct {
KeyVar string // empty if ignoring the key
ValVar string
Expand Down Expand Up @@ -1742,7 +1773,8 @@ func (e *SplatExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
if ty.IsListType() && sourceVal.Type().IsCollectionType() {
// We can refine the length of an unknown list result based on
// the source collection's own length.
sourceRng := sourceVal.Range()
sv, _ := sourceVal.Unmark()
sourceRng := sv.Range()
ret = ret.Refine().
CollectionLengthLowerBound(sourceRng.LengthLowerBound()).
CollectionLengthUpperBound(sourceRng.LengthUpperBound()).
Expand Down
128 changes: 125 additions & 3 deletions hclsyntax/expression_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1933,6 +1933,74 @@ EOT
NewValue(),
0,
},
{
`unknown ? i : j`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"i": cty.NullVal(cty.Number),
"j": cty.NullVal(cty.Number),
},
},
cty.NullVal(cty.Number),
0,
},
{
`unknown ? im : jm`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"im": cty.NullVal(cty.Number).Mark("a"),
"jm": cty.NullVal(cty.Number).Mark("b"),
},
},
cty.NullVal(cty.Number).Mark("a").Mark("b"),
0,
},
{
`unknown ? im : jm`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool).Mark("a"),
"im": cty.UnknownVal(cty.Number),
"jm": cty.UnknownVal(cty.Number).Mark("b"),
},
},
// the empty refinement may eventually be removed, but does nothing here
cty.UnknownVal(cty.Number).Refine().NewValue().Mark("a").Mark("b"),
0,
},
{
`unknown ? ix : jx`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"ix": cty.UnknownVal(cty.Number),
"jx": cty.UnknownVal(cty.Number),
},
},
// the empty refinement may eventually be removed, but does nothing here
cty.UnknownVal(cty.Number).Refine().NewValue(),
0,
},
{
`unknown ? ir : jr`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"ir": cty.UnknownVal(cty.Number).Refine().
NumberRangeLowerBound(cty.NumberIntVal(1), false).
NumberRangeUpperBound(cty.NumberIntVal(3), false).NewValue(),
"jr": cty.UnknownVal(cty.Number).Refine().
NumberRangeLowerBound(cty.NumberIntVal(2), true).
NumberRangeUpperBound(cty.NumberIntVal(4), true).NewValue(),
},
},
cty.UnknownVal(cty.Number).Refine().
NumberRangeLowerBound(cty.NumberIntVal(1), false).
NumberRangeUpperBound(cty.NumberIntVal(4), true).NewValue(),
0,
},
{
`unknown ? a : b`,
&hcl.EvalContext{
Expand All @@ -1946,17 +2014,71 @@ EOT
0,
},
{
`unknown ? a : b`,
`unknown ? al : bl`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"a": cty.ListValEmpty(cty.String),
"b": cty.ListValEmpty(cty.String),
"al": cty.ListValEmpty(cty.String),
"bl": cty.ListValEmpty(cty.String),
},
},
cty.ListValEmpty(cty.String), // deduced through refinements
0,
},
{
`unknown ? am : bm`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"am": cty.MapValEmpty(cty.String),
"bm": cty.MapValEmpty(cty.String).Mark("test"),
},
},
cty.MapValEmpty(cty.String).Mark("test"), // deduced through refinements
0,
},
{
`unknown ? ar : br`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"ar": cty.UnknownVal(cty.Set(cty.String)).Refine().
CollectionLengthLowerBound(1).CollectionLengthUpperBound(3).NewValue(),
"br": cty.UnknownVal(cty.Set(cty.String)).Refine().
CollectionLengthLowerBound(2).CollectionLengthUpperBound(4).NewValue(),
},
},
cty.UnknownVal(cty.Set(cty.String)).Refine().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue(), // deduced through refinements
0,
},
{
`unknown ? arn : brn`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"arn": cty.UnknownVal(cty.Set(cty.String)).Refine().NotNull().
CollectionLengthLowerBound(1).CollectionLengthUpperBound(2).NewValue(),
"brn": cty.UnknownVal(cty.Set(cty.String)).Refine().NotNull().
CollectionLengthLowerBound(3).CollectionLengthUpperBound(4).NewValue(),
},
},
cty.UnknownVal(cty.Set(cty.String)).Refine().NotNull().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue(), // deduced through refinements
0,
},
{
`unknown ? amr : bmr`,
&hcl.EvalContext{
Variables: map[string]cty.Value{
"unknown": cty.UnknownVal(cty.Bool),
"amr": cty.UnknownVal(cty.Set(cty.String)).Mark("test").Refine().
CollectionLengthLowerBound(1).CollectionLengthUpperBound(2).NewValue(),
"bmr": cty.UnknownVal(cty.Set(cty.String)).Mark("test").Refine().
CollectionLengthLowerBound(3).CollectionLengthUpperBound(4).NewValue(),
},
},
cty.UnknownVal(cty.Set(cty.String)).Refine().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue().Mark("test"), // deduced through refinements
0,
},
{
`unknown ? a : b`,
&hcl.EvalContext{
Expand Down