From 624ec4e0d3249bfa8e1328365a5c6cf0d18ea2c9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 2 Feb 2023 16:30:00 -0800 Subject: [PATCH 1/6] go.mod: Use cty v1.13.0, with support for refined unknown values This new concept allows constraining the range of an unknown value beyond what can be captured in a type constraint. We'll make more use of this in subsequent commits. --- go.mod | 6 +++--- go.sum | 14 ++++++-------- hclsyntax/expression_test.go | 12 ++++++------ 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index 065cac36..0f44c307 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 github.com/sergi/go-diff v1.0.0 github.com/spf13/pflag v1.0.2 - github.com/zclconf/go-cty v1.12.1 + github.com/zclconf/go-cty v1.13.0 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167 ) @@ -23,7 +23,7 @@ require ( github.com/kr/text v0.1.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/stretchr/testify v1.2.2 // indirect - golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 // indirect + golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 // indirect - golang.org/x/text v0.3.7 // indirect + golang.org/x/text v0.3.8 // indirect ) diff --git a/go.sum b/go.sum index a064ebde..b9544a42 100644 --- a/go.sum +++ b/go.sum @@ -31,10 +31,8 @@ github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/vmihailenco/msgpack v3.3.3+incompatible/go.mod h1:fy3FlTQTDXWkZ7Bh6AcGMlsjHatGryHQYUTf1ShIgkk= github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= -github.com/zclconf/go-cty v1.12.0 h1:F5E/vbilcrCtat9sYcEjlwwg1mDqbRTjyXR57nnx5sc= -github.com/zclconf/go-cty v1.12.0/go.mod h1:s9IfD1LK5ccNMSWCVFCE2rJfHiZgi7JijgeWIMfhLvA= -github.com/zclconf/go-cty v1.12.1 h1:PcupnljUm9EIvbgSHQnHhUr3fO6oFmkOrvs2BAFNXXY= -github.com/zclconf/go-cty v1.12.1/go.mod h1:s9IfD1LK5ccNMSWCVFCE2rJfHiZgi7JijgeWIMfhLvA= +github.com/zclconf/go-cty v1.13.0 h1:It5dfKTTZHe9aeppbNOda3mN7Ag7sg6QkBNm6TkyFa0= +github.com/zclconf/go-cty v1.13.0/go.mod h1:YKQzy/7pZ7iq2jNFzy5go57xdxdWoLLpaEp4u238AE0= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI= github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8= golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167 h1:O8uGbHCqlTp2P6QJSLmCojM4mN6UemYv8K+dCnmHmu0= @@ -42,12 +40,12 @@ golang.org/x/crypto v0.0.0-20220517005047-85d78b3ac167/go.mod h1:IxCIyHEi3zRg3s0 golang.org/x/net v0.0.0-20180811021610-c39426892332/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 h1:SrN+KX8Art/Sf4HNj6Zcz06G7VEz+7w9tdXTPOZ7+l4= -golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f h1:v4INt8xihDGvnrfjMDVXGxw9wrfxYyCjk0KbXjhR55s= +golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 h1:v+OssWQX+hTHEmOBgwxdZxK4zHq3yOs8F9J7mk0PY8E= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= -golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk= -golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= +golang.org/x/text v0.3.8 h1:nAL+RVCQ9uMn3vJZbV+MRnydTJFPf8qqY42YiA6MrqY= +golang.org/x/text v0.3.8/go.mod h1:E6s5w1FMmriuDzIBO73fBruAKo1PCIq6d2Q6DHfQ8WQ= google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index df1e1939..80f65132 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -60,7 +60,7 @@ func TestExpressionParseAndValue(t *testing.T) { "unk": cty.UnknownVal(cty.Number), }, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), 0, }, { @@ -70,7 +70,7 @@ func TestExpressionParseAndValue(t *testing.T) { "unk": cty.DynamicVal, }, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), 0, }, { @@ -80,7 +80,7 @@ func TestExpressionParseAndValue(t *testing.T) { "unk": cty.DynamicVal, }, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).RefineNotNull(), 0, }, { @@ -2098,7 +2098,7 @@ func TestFunctionCallExprValue(t *testing.T) { &hcl.EvalContext{ Functions: funcs, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).Refine().NotNull().NumberRangeLowerBound(cty.NumberIntVal(0), true).NewValue(), 0, }, "valid call with unknown arg needing conversion": { @@ -2113,7 +2113,7 @@ func TestFunctionCallExprValue(t *testing.T) { &hcl.EvalContext{ Functions: funcs, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).Refine().NotNull().NumberRangeLowerBound(cty.NumberIntVal(0), true).NewValue(), 0, }, "valid call with dynamic arg": { @@ -2128,7 +2128,7 @@ func TestFunctionCallExprValue(t *testing.T) { &hcl.EvalContext{ Functions: funcs, }, - cty.UnknownVal(cty.Number), + cty.UnknownVal(cty.Number).Refine().NotNull().NumberRangeLowerBound(cty.NumberIntVal(0), true).NewValue(), 0, }, "invalid arg type": { From 936a4706387387b3112666f16e8ef869417d88d2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 2 Feb 2023 16:51:44 -0800 Subject: [PATCH 2/6] hclsyntax: TemplateExpr can refine its unknown results If we encounter an interpolated unknown value during template rendering, we can report the partial buffer we've completed so far as the refined prefix of the resulting unknown value, which can then potentially allow downstream comparisons to produce a known false result instead of unknown if the prefix is sufficient to satisfy them. --- hclsyntax/expression_template.go | 23 +++++++++++----- hclsyntax/expression_template_test.go | 38 +++++++++++++++++++++++---- json/structure_test.go | 6 ++--- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/hclsyntax/expression_template.go b/hclsyntax/expression_template.go index 0b5ac195..f175fc5f 100644 --- a/hclsyntax/expression_template.go +++ b/hclsyntax/expression_template.go @@ -38,11 +38,9 @@ func (e *TemplateExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) if partVal.IsNull() { diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid template interpolation value", - Detail: fmt.Sprintf( - "The expression result is null. Cannot include a null value in a string template.", - ), + Severity: hcl.DiagError, + Summary: "Invalid template interpolation value", + Detail: "The expression result is null. Cannot include a null value in a string template.", Subject: part.Range().Ptr(), Context: &e.SrcRange, Expression: part, @@ -83,16 +81,29 @@ func (e *TemplateExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) continue } - buf.WriteString(strVal.AsString()) + // If we're just continuing to validate after we found an unknown value + // then we'll skip appending so that "buf" will contain only the + // known prefix of the result. + if isKnown && !diags.HasErrors() { + buf.WriteString(strVal.AsString()) + } } var ret cty.Value if !isKnown { ret = cty.UnknownVal(cty.String) + if !diags.HasErrors() { // Invalid input means our partial result buffer is suspect + if knownPrefix := buf.String(); knownPrefix != "" { + ret = ret.Refine().StringPrefix(knownPrefix).NewValue() + } + } } else { ret = cty.StringVal(buf.String()) } + // A template rendering result is never null. + ret = ret.RefineNotNull() + // Apply the full set of marks to the returned value return ret.WithMarks(marks), diags } diff --git a/hclsyntax/expression_template_test.go b/hclsyntax/expression_template_test.go index 624a2cd0..6d1b2d51 100644 --- a/hclsyntax/expression_template_test.go +++ b/hclsyntax/expression_template_test.go @@ -177,7 +177,7 @@ trim`, { `%{ of true ~} hello %{~ endif}`, nil, - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), 2, // "of" is not a valid control keyword, and "endif" is therefore also unexpected }, { @@ -277,15 +277,36 @@ trim`, { `%{ endif }`, nil, - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), 1, // Unexpected endif directive }, { `%{ endfor }`, nil, - cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String).RefineNotNull(), 1, // Unexpected endfor directive }, + { // can preserve a static prefix as a refinement of an unknown result + `test_${unknown}`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.String), + }, + }, + cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull("test_").NewValue(), + 0, + }, + { // can preserve a dynamic known prefix as a refinement of an unknown result + `test_${known}_${unknown}`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "known": cty.StringVal("known"), + "unknown": cty.UnknownVal(cty.String), + }, + }, + cty.UnknownVal(cty.String).Refine().NotNull().StringPrefixFull("test_known_").NewValue(), + 0, + }, { // marks from uninterpolated values are ignored `hello%{ if false } ${target}%{ endif }`, &hcl.EvalContext{ @@ -368,7 +389,7 @@ trim`, "target": cty.UnknownVal(cty.String).Mark("sensitive"), }, }, - cty.UnknownVal(cty.String).Mark("sensitive"), + cty.UnknownVal(cty.String).Mark("sensitive").Refine().NotNull().StringPrefixFull("test_").NewValue(), 0, }, } @@ -377,7 +398,14 @@ trim`, t.Run(test.input, func(t *testing.T) { expr, parseDiags := ParseTemplate([]byte(test.input), "", hcl.Pos{Line: 1, Column: 1, Byte: 0}) - got, valDiags := expr.Value(test.ctx) + // We'll skip evaluating if there were parse errors because it + // isn't reasonable to evaluate a syntactically-invalid template; + // it'll produce strange results that we don't care about. + got := test.want + var valDiags hcl.Diagnostics + if !parseDiags.HasErrors() { + got, valDiags = expr.Value(test.ctx) + } diagCount := len(parseDiags) + len(valDiags) diff --git a/json/structure_test.go b/json/structure_test.go index d05a1d68..bf6be2f2 100644 --- a/json/structure_test.go +++ b/json/structure_test.go @@ -1433,7 +1433,7 @@ func TestExpressionValue_Diags(t *testing.T) { { name: "string: unhappy", src: `{"v": "happy ${UNKNOWN}"}`, - expected: cty.UnknownVal(cty.String), + expected: cty.UnknownVal(cty.String).RefineNotNull(), error: "Unknown variable", }, { @@ -1447,7 +1447,7 @@ func TestExpressionValue_Diags(t *testing.T) { name: "object_val: unhappy", src: `{"v": {"key": "happy ${UNKNOWN}"}}`, expected: cty.ObjectVal(map[string]cty.Value{ - "key": cty.UnknownVal(cty.String), + "key": cty.UnknownVal(cty.String).RefineNotNull(), }), error: "Unknown variable", }, @@ -1472,7 +1472,7 @@ func TestExpressionValue_Diags(t *testing.T) { { name: "array: unhappy", src: `{"v": ["happy ${UNKNOWN}"]}`, - expected: cty.TupleVal([]cty.Value{cty.UnknownVal(cty.String)}), + expected: cty.TupleVal([]cty.Value{cty.UnknownVal(cty.String).RefineNotNull()}), error: "Unknown variable", }, } From 03a98479294637f124227414b77ae9eb29478b13 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 2 Feb 2023 18:16:54 -0800 Subject: [PATCH 3/6] hclsyntax: ConditionalExpr can refine its unknown results When ConditionalExpr has an unknown predicate it can still often infer some refinement to the range of its result by noticing characteristics that the two results have in common. In all cases we can test if either result could be null and return a definitely-not-null unknown value if not. For two known numbers we can constrain the range to be between those two numbers. This is primarily aimed at the common case where the two possible results are zero and one, which significantly constrains the range. For two known collections of the same kind we can constrain the length to be between the two collection lengths. In these last two cases we can also sometimes collapse the unknown into a known value if the range gets reduced enough. For example, if choosing between two collections of the same length we might return a known collection of that length containing unknown elements, rather than an unknown collection. --- hclsyntax/expression.go | 54 +++++++++++++++++++++++- hclsyntax/expression_test.go | 79 ++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 55fecd4e..0ee8de46 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -696,7 +696,59 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic return cty.UnknownVal(resultType), diags } if !condResult.IsKnown() { - return cty.UnknownVal(resultType), 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 + } + } + } + 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 + } + } + } + } + trueRng := trueResult.Range() + falseRng := falseResult.Range() + ret := cty.UnknownVal(resultType) + if trueRng.DefinitelyNotNull() && falseRng.DefinitelyNotNull() { + ret = ret.RefineNotNull() + } + return ret.WithSameMarks(condResult).WithSameMarks(trueResult).WithSameMarks(falseResult), diags } condResult, err := convert.Convert(condResult, cty.Bool) if err != nil { diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 80f65132..57fcc54e 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1840,6 +1840,85 @@ EOT cty.DynamicVal, 0, }, + { + `unknown ? 1 : 0`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + }, + }, + cty.UnknownVal(cty.Number).Refine(). + NotNull(). + NumberRangeLowerBound(cty.Zero, true). + NumberRangeUpperBound(cty.NumberIntVal(1), true). + NewValue(), + 0, + }, + { + `unknown ? 0 : 1`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + }, + }, + cty.UnknownVal(cty.Number).Refine(). + NotNull(). + NumberRangeLowerBound(cty.Zero, true). + NumberRangeUpperBound(cty.NumberIntVal(1), true). + NewValue(), + 0, + }, + { + `unknown ? a : b`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + "a": cty.UnknownVal(cty.Bool).RefineNotNull(), + "b": cty.UnknownVal(cty.Bool).RefineNotNull(), + }, + }, + cty.UnknownVal(cty.Bool).RefineNotNull(), + 0, + }, + { + `unknown ? a : b`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + "a": cty.ListValEmpty(cty.String), + "b": cty.ListValEmpty(cty.String), + }, + }, + cty.ListValEmpty(cty.String), // deduced through refinements + 0, + }, + { + `unknown ? a : b`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + "a": cty.ListValEmpty(cty.String), + "b": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}), + }, + }, + cty.UnknownVal(cty.List(cty.String)).Refine(). + NotNull(). + CollectionLengthUpperBound(1). + NewValue(), + 0, + }, + { + `unknown ? a : b`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unknown": cty.UnknownVal(cty.Bool), + "a": cty.ListVal([]cty.Value{cty.StringVal("hello")}), + "b": cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}), + }, + }, + cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}), // deduced through refinements + 0, + }, { // marked conditional `var.foo ? 1 : 0`, &hcl.EvalContext{ From 0921c992b4d41863d6fd70f67d2da5729062a07b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 3 Feb 2023 10:39:13 -0800 Subject: [PATCH 4/6] ext/typeexpr: Refinements when applying defaults with unknown values If either the given value is refined non-null or if the default value is refined non-null then the final attribute value after defaults processing is also guaranteed non-null even if we don't yet know exactly what the value will be. This rule is pretty marginal on its own, but refining some types of value as non-null creates opportunities to deduce further information when the value is used under other operations later, such as collapsing an unknown but definitely not null list of a known length into a known list of that length containing unknown values. --- ext/typeexpr/defaults.go | 3 ++ ext/typeexpr/defaults_test.go | 68 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/ext/typeexpr/defaults.go b/ext/typeexpr/defaults.go index 4aeb1fb9..43a2539d 100644 --- a/ext/typeexpr/defaults.go +++ b/ext/typeexpr/defaults.go @@ -95,6 +95,9 @@ func (d *Defaults) apply(v cty.Value) cty.Value { } values[key] = defaultValue } + if defaultRng := defaultValue.Range(); defaultRng.DefinitelyNotNull() { + values[key] = values[key].RefineNotNull() + } } if v.Type().IsMapType() { diff --git a/ext/typeexpr/defaults_test.go b/ext/typeexpr/defaults_test.go index 8b90e752..5f0588f5 100644 --- a/ext/typeexpr/defaults_test.go +++ b/ext/typeexpr/defaults_test.go @@ -830,6 +830,74 @@ func TestDefaults_Apply(t *testing.T) { }), }), }, + "optional attribute with a default can never be null": { + defaults: &Defaults{ + Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "foo": cty.String, + }, []string{"foo"}), + DefaultValues: map[string]cty.Value{ + "foo": cty.StringVal("bar"), // Important: default is non-null + }, + }, + value: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), // could potentially be null once known + }), + want: cty.ObjectVal(map[string]cty.Value{ + // Because the default isn't null we can guarantee that the + // result cannot be null even if the given value turns out to be. + "foo": cty.UnknownVal(cty.String).RefineNotNull(), + }), + }, + "optional attribute with a null default could be null": { + defaults: &Defaults{ + Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "foo": cty.String, + }, []string{"foo"}), + DefaultValues: map[string]cty.Value{ + "foo": cty.NullVal(cty.String), // Important: default is null + }, + }, + value: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), // could potentially be null once known + }), + want: cty.ObjectVal(map[string]cty.Value{ + // The default value is itself null, so this result is nullable. + "foo": cty.UnknownVal(cty.String), + }), + }, + "optional attribute with no default could be null": { + defaults: &Defaults{ + Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "foo": cty.String, + }, []string{"foo"}), + }, + value: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String), // could potentially be null once known + }), + want: cty.ObjectVal(map[string]cty.Value{ + // The default value is itself null, so this result is nullable. + "foo": cty.UnknownVal(cty.String), + }), + }, + "optional attribute with non-null unknown value cannot be null": { + defaults: &Defaults{ + Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{ + "foo": cty.String, + }, []string{"foo"}), + DefaultValues: map[string]cty.Value{ + "foo": cty.NullVal(cty.String), // Important: default is null + }, + }, + value: cty.ObjectVal(map[string]cty.Value{ + "foo": cty.UnknownVal(cty.String).RefineNotNull(), + }), + want: cty.ObjectVal(map[string]cty.Value{ + // If the input is guaranteed not null then the default + // value can't possibly be selected, and so the result can + // also not be null. + "foo": cty.UnknownVal(cty.String).RefineNotNull(), + }), + }, } for name, tc := range testCases { From e0af0415f7e0c38f1e63ebe1f0448e8e2604b8d1 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 3 Feb 2023 12:17:11 -0800 Subject: [PATCH 5/6] hclsyntax: Refinements to unknown splat expression results We know that a splat expression can never produce a null result, and also in many cases we can use length refinements from the source collection to also refine the destination collection because we know that a splat expression produces exactly one result for each input element. This also allows us to be a little more precise in the case where the splat operator is projecting a non-list/set value into a zero or one element list and we know the source value isn't null. This refinement is a bit more marginal since it would be weird to apply the splat operator to a value already known to be non-null anyway, but the refinement might come from far away from the splat expression and so could still have useful downstream effects in some cases. --- hclsyntax/expression.go | 23 ++++++++++++++--- hclsyntax/expression_test.go | 49 ++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 0ee8de46..5df423fe 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -1684,11 +1684,15 @@ func (e *SplatExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { // example, it is valid to use a splat on a single object to retrieve a // list of a single attribute, but we still need to check if that // attribute actually exists. - upgradedUnknown = !sourceVal.IsKnown() + if !sourceVal.IsKnown() { + sourceRng := sourceVal.Range() + if sourceRng.CouldBeNull() { + upgradedUnknown = true + } + } sourceVal = cty.TupleVal([]cty.Value{sourceVal}) sourceTy = sourceVal.Type() - } // We'll compute our result type lazily if we need it. In the normal case @@ -1727,7 +1731,20 @@ func (e *SplatExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { // checking to proceed. ty, tyDiags := resultTy() diags = append(diags, tyDiags...) - return cty.UnknownVal(ty), diags + ret := cty.UnknownVal(ty) + if ty != cty.DynamicPseudoType { + ret = ret.RefineNotNull() + } + 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() + ret = ret.Refine(). + CollectionLengthLowerBound(sourceRng.LengthLowerBound()). + CollectionLengthUpperBound(sourceRng.LengthUpperBound()). + NewValue() + } + return ret.WithSameMarks(sourceVal), diags } // Unmark the collection, and save the marks to apply to the returned diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index 57fcc54e..5a6fedbc 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -1150,6 +1150,20 @@ upper( cty.DynamicVal, 0, }, + { + `unkstr[*]`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unkstr": cty.UnknownVal(cty.String).RefineNotNull(), + }, + }, + // If the unknown string is definitely not null then we already + // know that the result will be a single-element tuple. + cty.TupleVal([]cty.Value{ + cty.UnknownVal(cty.String).RefineNotNull(), + }), + 0, + }, { `unkstr.*.name`, &hcl.EvalContext{ @@ -1182,6 +1196,20 @@ upper( cty.DynamicVal, 0, }, + { + `unkobj.*.name`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unkobj": cty.UnknownVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })).RefineNotNull(), + }, + }, + cty.TupleVal([]cty.Value{ + cty.UnknownVal(cty.String), + }), + 0, + }, { `unkobj.*.names`, &hcl.EvalContext{ @@ -1203,7 +1231,24 @@ upper( }))), }, }, - cty.UnknownVal(cty.List(cty.String)), + cty.UnknownVal(cty.List(cty.String)).RefineNotNull(), + 0, + }, + { + `unklistobj.*.name`, + &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unklistobj": cty.UnknownVal(cty.List(cty.Object(map[string]cty.Type{ + "name": cty.String, + }))).Refine(). + CollectionLengthUpperBound(5). + NewValue(), + }, + }, + cty.UnknownVal(cty.List(cty.String)).Refine(). + NotNull(). + CollectionLengthUpperBound(5). + NewValue(), 0, }, { @@ -1222,7 +1267,7 @@ upper( ), }, }, - cty.UnknownVal(cty.Tuple([]cty.Type{cty.String, cty.Bool})), + cty.UnknownVal(cty.Tuple([]cty.Type{cty.String, cty.Bool})).RefineNotNull(), 0, }, { From 75e3f8a8efb930678ba784aff0a09c21bacaf48d Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 7 Feb 2023 18:00:32 -0800 Subject: [PATCH 6/6] hcldec: RefineValueSpec This new spec type allows adding value refinements to the results of some other spec, as long as the wrapped spec does indeed enforce the constraints described by the refinements. --- hcldec/spec.go | 47 +++++++++++++++++++++++++++++- hcldec/spec_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/hcldec/spec.go b/hcldec/spec.go index b31ec175..7fc1ffbf 100644 --- a/hcldec/spec.go +++ b/hcldec/spec.go @@ -1606,7 +1606,52 @@ func (s *TransformFuncSpec) sourceRange(content *hcl.BodyContent, blockLabels [] return s.Wrapped.sourceRange(content, blockLabels) } -// ValidateFuncSpec is a spec that allows for extended +// RefineValueSpec is a spec that wraps another and applies a fixed set of [cty] +// value refinements to whatever value it produces. +// +// Refinements serve to constrain the range of any unknown values, and act as +// assertions for known values by panicking if the final value does not meet +// the refinement. Therefore applications using this spec must guarantee that +// any value passing through the RefineValueSpec will always be consistent with +// the refinements; if not then that is a bug in the application. +// +// The wrapped spec should typically be a [ValidateSpec], a [TransformFuncSpec], +// or some other adapter that guarantees that the inner result cannot possibly +// violate the refinements. +type RefineValueSpec struct { + Wrapped Spec + + // Refine is a function which accepts a builder for a refinement in + // progress and uses the builder pattern to add extra refinements to it, + // finally returning the same builder with those modifications applied. + Refine func(*cty.RefinementBuilder) *cty.RefinementBuilder +} + +func (s *RefineValueSpec) visitSameBodyChildren(cb visitFunc) { + cb(s.Wrapped) +} + +func (s *RefineValueSpec) decode(content *hcl.BodyContent, blockLabels []blockLabel, ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { + wrappedVal, diags := s.Wrapped.decode(content, blockLabels, ctx) + if diags.HasErrors() { + // We won't try to run our function in this case, because it'll probably + // generate confusing additional errors that will distract from the + // root cause. + return cty.UnknownVal(s.impliedType()), diags + } + + return wrappedVal.RefineWith(s.Refine), diags +} + +func (s *RefineValueSpec) impliedType() cty.Type { + return s.Wrapped.impliedType() +} + +func (s *RefineValueSpec) sourceRange(content *hcl.BodyContent, blockLabels []blockLabel) hcl.Range { + return s.Wrapped.sourceRange(content, blockLabels) +} + +// ValidateSpec is a spec that allows for extended // developer-defined validation. The validation function receives the // result of the wrapped spec. // diff --git a/hcldec/spec_test.go b/hcldec/spec_test.go index d59b6050..093b032b 100644 --- a/hcldec/spec_test.go +++ b/hcldec/spec_test.go @@ -9,6 +9,8 @@ import ( "testing" "github.com/apparentlymart/go-dump/dump" + "github.com/google/go-cmp/cmp" + "github.com/zclconf/go-cty-debug/ctydebug" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/hcl/v2" @@ -210,3 +212,72 @@ foo = "invalid" }) } } + +func TestRefineValueSpec(t *testing.T) { + config := ` +foo = "hello" +bar = unk +` + + f, diags := hclsyntax.ParseConfig([]byte(config), "", hcl.InitialPos) + if diags.HasErrors() { + t.Fatal(diags.Error()) + } + + attrSpec := func(name string) Spec { + return &RefineValueSpec{ + // RefineValueSpec should typically have a ValidateSpec wrapped + // inside it to catch any values that are outside of the required + // range and return a helpful error message about it. In this + // case our refinement is .NotNull so the validation function + // must reject null values. + Wrapped: &ValidateSpec{ + Wrapped: &AttrSpec{ + Name: name, + Required: true, + Type: cty.String, + }, + Func: func(value cty.Value) hcl.Diagnostics { + var diags hcl.Diagnostics + if value.IsNull() { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot be null", + Detail: "Argument is required.", + }) + } + return diags + }, + }, + Refine: func(rb *cty.RefinementBuilder) *cty.RefinementBuilder { + return rb.NotNull() + }, + } + } + spec := &ObjectSpec{ + "foo": attrSpec("foo"), + "bar": attrSpec("bar"), + } + + got, diags := Decode(f.Body, spec, &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "unk": cty.UnknownVal(cty.String), + }, + }) + if diags.HasErrors() { + t.Fatal(diags.Error()) + } + + want := cty.ObjectVal(map[string]cty.Value{ + // This argument had a known value, so it's unchanged but the + // RefineValueSpec still checks that it isn't null to catch + // bugs in the application's validation function. + "foo": cty.StringVal("hello"), + + // The final value of bar is unknown but refined as non-null. + "bar": cty.UnknownVal(cty.String).RefineNotNull(), + }) + if diff := cmp.Diff(want, got, ctydebug.CmpOptions); diff != "" { + t.Errorf("wrong result\n%s", diff) + } +}