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.
BitYadou · Oct 11, 2023 · bad33d5 · bad33d5
1 parent e4589e3
commit bad33d5
Show file tree

Hide file tree

Showing 2 changed files with 172 additions and 51 deletions.
diff --git 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
@@ -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,
  },
  {