From 3a3033373598fe15e8cf82f6151168880a8dcbc9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 11 Oct 2023 10:13:14 -0400 Subject: [PATCH 1/3] refinements of collections must use Range() When attempting to determine the final length range for a conditional expression with collections, the length values may still be unknown. Always use `Range()` to get the lower and upper bounds. --- hclsyntax/expression.go | 16 +++++++--------- hclsyntax/expression_test.go | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 5ed35273..71eb46ee 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -725,16 +725,14 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic 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)) + CollectionLengthLowerBound(falseResult.Range().LengthLowerBound()). + CollectionLengthUpperBound(trueResult.Range().LengthUpperBound()) } else { b = b. - CollectionLengthLowerBound(int(trueLen)). - CollectionLengthUpperBound(int(falseLen)) + CollectionLengthLowerBound(trueResult.Range().LengthLowerBound()). + CollectionLengthUpperBound(falseResult.Range().LengthUpperBound()) } 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 @@ -1244,9 +1242,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 diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 1af93dad..e64047a0 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1957,6 +1957,20 @@ EOT cty.ListValEmpty(cty.String), // 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(2).NewValue(), + "br": cty.UnknownVal(cty.Set(cty.String)).Refine(). + CollectionLengthLowerBound(3).CollectionLengthUpperBound(4).NewValue(), + }, + }, + cty.UnknownVal(cty.Set(cty.String)).Refine().NotNull().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue(), // deduced through refinements + 0, + }, { `unknown ? a : b`, &hcl.EvalContext{ From e4589e3cfa1d12e457d5f06cc37812b8264f7d0c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 11 Oct 2023 14:19:52 -0400 Subject: [PATCH 2/3] Range() calls must always be unmarked --- hclsyntax/expression.go | 23 +++++++++++++++-------- hclsyntax/expression_test.go | 14 ++++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 71eb46ee..8192d43a 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -721,18 +721,24 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic 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() { + // the bounds are not part of the final result value, so + // the marks are not needed + tr, _ := trueResult.Unmark() + fr, _ := falseResult.Unmark() + trueRange := tr.Range() + falseRange := fr.Range() + + if gt := trueResult.Length().GreaterThan(falseResult.Length()); gt.IsKnown() { + gt, _ := gt.Unmark() b := cty.UnknownVal(resultType).Refine() if gt.True() { b = b. - CollectionLengthLowerBound(falseResult.Range().LengthLowerBound()). - CollectionLengthUpperBound(trueResult.Range().LengthUpperBound()) + CollectionLengthLowerBound(falseRange.LengthLowerBound()). + CollectionLengthUpperBound(trueRange.LengthUpperBound()) } else { b = b. - CollectionLengthLowerBound(trueResult.Range().LengthLowerBound()). - CollectionLengthUpperBound(falseResult.Range().LengthUpperBound()) + CollectionLengthLowerBound(trueRange.LengthLowerBound()). + CollectionLengthUpperBound(falseRange.LengthUpperBound()) } 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 @@ -1740,7 +1746,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()). diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index e64047a0..3bfef63c 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1971,6 +1971,20 @@ EOT 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().NotNull().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue().Mark("test"), // deduced through refinements + 0, + }, { `unknown ? a : b`, &hcl.EvalContext{ From bad33d51fc6add9a3f777405e6efb0a9a0f87b29 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 11 Oct 2023 17:31:35 -0400 Subject: [PATCH 3/3] further refine refinement handling Correct mark handling for some conditional values. Find correct refinement for overlapping ranges which could not have been compared with `GreaterThan`. Also map inclusive flags for numeric ranges. Correct handling of DefinitelyNotNull collections. Return a known null early when both conditional branches are null. --- hclsyntax/expression.go | 119 +++++++++++++++++++++-------------- hclsyntax/expression_test.go | 104 ++++++++++++++++++++++++++++-- 2 files changed, 172 insertions(+), 51 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 8192d43a..e0de1c3d 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -696,68 +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() + + 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()) { - // the bounds are not part of the final result value, so - // the marks are not needed - tr, _ := trueResult.Unmark() - fr, _ := falseResult.Unmark() - trueRange := tr.Range() - falseRange := fr.Range() - - if gt := trueResult.Length().GreaterThan(falseResult.Length()); gt.IsKnown() { - gt, _ := gt.Unmark() - b := cty.UnknownVal(resultType).Refine() - if gt.True() { - b = b. - CollectionLengthLowerBound(falseRange.LengthLowerBound()). - CollectionLengthUpperBound(trueRange.LengthUpperBound()) - } else { - b = b. - CollectionLengthLowerBound(trueRange.LengthLowerBound()). - CollectionLengthUpperBound(falseRange.LengthUpperBound()) - } - 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{ diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 3bfef63c..e71dd310 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -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{ @@ -1946,25 +2014,51 @@ 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(2).NewValue(), + 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(), }, }, @@ -1982,7 +2076,7 @@ EOT CollectionLengthLowerBound(3).CollectionLengthUpperBound(4).NewValue(), }, }, - cty.UnknownVal(cty.Set(cty.String)).Refine().NotNull().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue().Mark("test"), // deduced through refinements + cty.UnknownVal(cty.Set(cty.String)).Refine().CollectionLengthLowerBound(1).CollectionLengthUpperBound(4).NewValue().Mark("test"), // deduced through refinements 0, }, {