diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index a5f6b7af..75c1f981 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -30,9 +30,9 @@ jobs: run: | git config --global core.autocrlf false - name: "Fetch source code" - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Install Go - uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 + uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 with: go-version-file: go.mod - name: Go test @@ -44,9 +44,9 @@ jobs: runs-on: ubuntu-latest steps: - name: "Fetch source code" - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Install Go - uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 + uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 with: go-version-file: go.mod - name: "copyright headers check" @@ -58,9 +58,9 @@ jobs: runs-on: ubuntu-latest steps: - name: "Fetch source code" - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Install Go - uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 + uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 with: go-version-file: go.mod - name: "go vet" @@ -72,9 +72,9 @@ jobs: runs-on: ubuntu-latest steps: - name: "Fetch source code" - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - name: Install Go - uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 + uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 with: go-version-file: go.mod - name: "gofmt" diff --git a/ext/dynblock/README.md b/ext/dynblock/README.md index f59ce92e..1cee2b93 100644 --- a/ext/dynblock/README.md +++ b/ext/dynblock/README.md @@ -134,7 +134,7 @@ func walkVariables(node dynblock.WalkVariablesNode, schema *hcl.BodySchema) []hc panic(fmt.Errorf("can't find schema for unknown block type %q", child.BlockTypeName)) } - vars = append(vars, testWalkAndAccumVars(child.Node, childSchema)...) + vars = append(vars, walkVariables(child.Node, childSchema)...) } } ``` diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 577a50fa..f4c3a6d7 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -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 @@ -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() { @@ -867,7 +870,7 @@ 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 } } @@ -875,7 +878,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic 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) @@ -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 { @@ -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 { @@ -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 } } @@ -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 @@ -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{ @@ -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 @@ -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 { @@ -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 @@ -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 @@ -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...) @@ -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 { @@ -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 @@ -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 @@ -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 diff --git a/hclsyntax/expression_template_test.go b/hclsyntax/expression_template_test.go index 31198458..92c11afe 100644 --- a/hclsyntax/expression_template_test.go +++ b/hclsyntax/expression_template_test.go @@ -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 diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index b50dc137..df11d28c 100644 --- a/hclsyntax/expression_test.go +++ b/hclsyntax/expression_test.go @@ -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, @@ -2175,7 +2218,7 @@ EOT }).Mark("sensitive"), }, }, - cty.NumberIntVal(1), + cty.NumberIntVal(1).Mark("sensitive"), 0, }, { // auto-converts collection types diff --git a/ops.go b/ops.go index bdf23614..3cd7b205 100644 --- a/ops.go +++ b/ops.go @@ -49,7 +49,7 @@ func Index(collection, key cty.Value, srcRange *Range) (cty.Value, Diagnostics) ty := collection.Type() kty := key.Type() if kty == cty.DynamicPseudoType || ty == cty.DynamicPseudoType { - return cty.DynamicVal, nil + return cty.DynamicVal.WithSameMarks(collection), nil } switch { @@ -87,9 +87,9 @@ func Index(collection, key cty.Value, srcRange *Range) (cty.Value, Diagnostics) has, _ := collection.HasIndex(key).Unmark() if !has.IsKnown() { if ty.IsTupleType() { - return cty.DynamicVal, nil + return cty.DynamicVal.WithSameMarks(collection), nil } else { - return cty.UnknownVal(ty.ElementType()), nil + return cty.UnknownVal(ty.ElementType()).WithSameMarks(collection), nil } } if has.False() { @@ -196,10 +196,10 @@ func Index(collection, key cty.Value, srcRange *Range) (cty.Value, Diagnostics) } } if !collection.IsKnown() { - return cty.DynamicVal, nil + return cty.DynamicVal.WithSameMarks(collection), nil } if !key.IsKnown() { - return cty.DynamicVal, nil + return cty.DynamicVal.WithSameMarks(collection), nil } key, _ = key.Unmark() @@ -291,13 +291,13 @@ func GetAttr(obj cty.Value, attrName string, srcRange *Range) (cty.Value, Diagno } if !obj.IsKnown() { - return cty.UnknownVal(ty.AttributeType(attrName)), nil + return cty.UnknownVal(ty.AttributeType(attrName)).WithSameMarks(obj), nil } return obj.GetAttr(attrName), nil case ty.IsMapType(): if !obj.IsKnown() { - return cty.UnknownVal(ty.ElementType()), nil + return cty.UnknownVal(ty.ElementType()).WithSameMarks(obj), nil } idx := cty.StringVal(attrName) @@ -319,7 +319,7 @@ func GetAttr(obj cty.Value, attrName string, srcRange *Range) (cty.Value, Diagno return obj.Index(idx), nil case ty == cty.DynamicPseudoType: - return cty.DynamicVal, nil + return cty.DynamicVal.WithSameMarks(obj), nil case ty.IsListType() && ty.ElementType().IsObjectType(): // It seems a common mistake to try to access attributes on a whole // list of objects rather than on a specific individual element, so diff --git a/ops_test.go b/ops_test.go index 7aabd7a2..b7b35ca7 100644 --- a/ops_test.go +++ b/ops_test.go @@ -249,6 +249,113 @@ func TestApplyPath(t *testing.T) { cty.NilVal, `Attempt to get attribute from null value: This value is null, so it does not have any attributes.`, }, + + // Marks should be retained during index and getattr ops, even when + // types and values are unknown. This reflects the same behavior when + // using cty to directly call GetAttr and Index methods. + { + cty.DynamicVal.Mark("marked"), + (cty.Path)(nil).GetAttr("foo"), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("should be marked"), + }).Mark("marked"), + (cty.Path)(nil).GetAttr("foo"), + cty.StringVal("should be marked").Mark("marked"), + ``, + }, + { + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "foo": cty.DynamicPseudoType, + })).Mark("marked"), + (cty.Path)(nil).GetAttr("foo"), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.DynamicVal.Mark("marked"), + (cty.Path)(nil).Index(cty.StringVal("foo")), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("should be marked"), + }).Mark("marked"), + (cty.Path)(nil).Index(cty.StringVal("foo")), + cty.StringVal("should be marked").Mark("marked"), + ``, + }, + { + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "foo": cty.DynamicPseudoType, + })).Mark("marked"), + (cty.Path)(nil).Index(cty.StringVal("foo")), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.DynamicVal.Mark("marked"), + (cty.Path)(nil).Index(cty.NumberIntVal(0)), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.ListVal([]cty.Value{cty.StringVal("should be marked")}).Mark("marked"), + (cty.Path)(nil).Index(cty.NumberIntVal(0)), + cty.StringVal("should be marked").Mark("marked"), + ``, + }, + { + cty.UnknownVal(cty.List(cty.String)).Mark("marked"), + (cty.Path)(nil).Index(cty.NumberIntVal(0)), + cty.UnknownVal(cty.String).Mark("marked"), + ``, + }, + + { + cty.DynamicVal.Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.String)), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.StringVal("should be marked"), + }).Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.String)), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "foo": cty.DynamicPseudoType, + })).Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.String)), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.DynamicVal.Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.Number)), + cty.DynamicVal.Mark("marked"), + ``, + }, + { + cty.ListVal([]cty.Value{cty.StringVal("should be marked")}).Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.Number)), + cty.UnknownVal(cty.String).Mark("marked"), + ``, + }, + { + cty.UnknownVal(cty.List(cty.String)).Mark("marked"), + (cty.Path)(nil).Index(cty.UnknownVal(cty.Number)), + cty.UnknownVal(cty.String).Mark("marked"), + ``, + }, } for _, test := range tests { @@ -257,7 +364,7 @@ func TestApplyPath(t *testing.T) { t.Logf("testing ApplyPath\nstart: %#v\npath: %#v", test.Start, test.Path) for _, diag := range diags { - t.Logf(diag.Error()) + t.Log(diag.Error()) } if test.WantErr != "" { diff --git a/spec.md b/spec.md index 97ef6131..d52ed70b 100644 --- a/spec.md +++ b/spec.md @@ -96,7 +96,7 @@ of the implementation language. ### _Dynamic Attributes_ Processing The _schema-driven_ processing model is useful when the expected structure -of a body is known a priori by the calling application. Some blocks are +of a body is known by the calling application. Some blocks are instead more free-form, such as a user-provided set of arbitrary key/value pairs.