cue: disallow bulk optional fields with other fields
This change is the first step to change the semantics
of bulk optional fields to apply only to additional fields.
The first step is to encourage bulk optional fields to be
in a struct with any other field.
`cue fmt` now rewrites violating bulk optional fields by
wrapping them in a embedded struct, which is
equivalent.
TestDiff was changed to ensure that diff will identify
the two forms as equivalent.
Issue #339
Change-Id: I52753ff9854598394726585b85b3c27caad85458
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/5781
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/cmd/cue/cmd/fix.go b/cmd/cue/cmd/fix.go
index d8b05fe..99d87cd 100644
--- a/cmd/cue/cmd/fix.go
+++ b/cmd/cue/cmd/fix.go
@@ -22,9 +22,33 @@
"cuelang.org/go/cue/ast/astutil"
"cuelang.org/go/cue/parser"
"cuelang.org/go/cue/token"
+ "cuelang.org/go/internal"
)
func fix(f *ast.File) *ast.File {
+ // Isolate bulk optional fields into a single struct.
+ ast.Walk(f, func(n ast.Node) bool {
+ var decls []ast.Decl
+ switch x := n.(type) {
+ case *ast.StructLit:
+ decls = x.Elts
+ case *ast.File:
+ decls = x.Decls
+ }
+
+ if len(decls) <= 1 {
+ return true
+ }
+
+ for i, d := range decls {
+ if internal.IsBulkField(d) {
+ decls[i] = internal.EmbedStruct(ast.NewStruct(d))
+ }
+ }
+
+ return true
+ }, nil)
+
// Rewrite an old-style alias to a let clause.
ast.Walk(f, func(n ast.Node) bool {
var decls []ast.Decl
diff --git a/cmd/cue/cmd/fix_test.go b/cmd/cue/cmd/fix_test.go
index 259362d..f4d5eb9 100644
--- a/cmd/cue/cmd/fix_test.go
+++ b/cmd/cue/cmd/fix_test.go
@@ -69,6 +69,48 @@
out: `
let y = foo
`,
+ }, {
+ name: "wrap bulk fields",
+ in: `
+ a: {
+ [allGood]: int
+ }
+ b: {
+ a: int
+
+ b: [string]: string
+ [string]: wrap
+
+ // Comment
+ [string]: wrap
+ ...
+ }
+ c: {
+ a: int
+
+ {[string]: alreadyGreat}
+ }
+
+ `,
+ out: `a: {
+ [allGood]: int
+}
+b: {
+ a: int
+
+ b: [string]: string
+ {[string]: wrap}
+
+ // Comment
+ {[string]: wrap}
+ ...
+}
+c: {
+ a: int
+
+ {[string]: alreadyGreat}
+}
+`,
// }, {
// name: "slice",
// in: `package foo
diff --git a/cue/builtin_test.go b/cue/builtin_test.go
index 8d3428a..f65246b 100644
--- a/cue/builtin_test.go
+++ b/cue/builtin_test.go
@@ -404,7 +404,7 @@
testExpr(`len({})`),
`0`,
}, {
- testExpr(`len({a: 1, b: 2, [foo=_]: int, _c: 3})`),
+ testExpr(`len({a: 1, b: 2, {[foo=_]: int}, _c: 3})`),
`2`,
}, {
testExpr(`len([1, 2, 3])`),
diff --git a/cue/builtins.go b/cue/builtins.go
index ffb39e6..969bc46 100644
--- a/cue/builtins.go
+++ b/cue/builtins.go
@@ -3742,16 +3742,16 @@
Name :: !="" & !~"^[$]"
Value :: bool | number | *string | null
Setenv: {
- [Name]: Value
- $id: "tool/os.Setenv"
+ $id: "tool/os.Setenv"
+ {[Name]: Value}
}
Getenv: {
- [Name]: Value
- $id: "tool/os.Getenv"
+ $id: "tool/os.Getenv"
+ {[Name]: Value}
}
Environ: {
- [Name]: Value
- $id: "tool/os.Environ"
+ $id: "tool/os.Environ"
+ {[Name]: Value}
}
Clearenv: {
$id: "tool/os.Clearenv"
diff --git a/cue/export.go b/cue/export.go
index d6d6072..6fe9f2b 100644
--- a/cue/export.go
+++ b/cue/export.go
@@ -233,10 +233,15 @@
func hasTemplate(s *ast.StructLit) bool {
for _, e := range s.Elts {
- if _, ok := e.(*ast.Ellipsis); ok {
+ switch f := e.(type) {
+ case *ast.Ellipsis:
return true
- }
- if f, ok := e.(*ast.Field); ok {
+
+ case *ast.EmbedDecl:
+ if st, ok := f.Expr.(*ast.StructLit); ok && hasTemplate(st) {
+ return true
+ }
+ case *ast.Field:
label := f.Label
if _, ok := label.(*ast.TemplateLabel); ok {
return true
@@ -260,7 +265,6 @@
return true
}
}
- return false
}
}
}
@@ -589,7 +593,7 @@
if x.optionals == nil {
break
}
- p.optionals(st, x.optionals)
+ p.optionals(len(x.arcs) > 0, st, x.optionals)
}
return expr
@@ -763,7 +767,7 @@
// indicate no other fields may be added. Non-closed empty structs should
// have been optimized away. In case they are not, it is just a no-op.
if x != nil {
- p.optionals(st, x)
+ p.optionals(false, st, x)
}
if isClosed {
return ast.NewCall(ast.NewIdent("close"), st)
@@ -771,7 +775,8 @@
return st
}
-func (p *exporter) optionals(st *ast.StructLit, x *optionals) (skippedEllipsis bool) {
+func (p *exporter) optionals(wrap bool, st *ast.StructLit, x *optionals) (skippedEllipsis bool) {
+ wrap = wrap || len(x.fields) > 1
switch x.op {
default:
for _, t := range x.fields {
@@ -792,7 +797,11 @@
skippedEllipsis = true
continue
}
- st.Elts = append(st.Elts, f)
+ if !wrap {
+ st.Elts = append(st.Elts, f)
+ continue
+ }
+ st.Elts = append(st.Elts, internal.EmbedStruct(ast.NewStruct(f)))
}
case opUnify:
@@ -843,7 +852,7 @@
// Optional field constraints may be omitted if they were already
// applied and no more new fields may be added.
!(doEval(p.mode) && x.optionals.isEmpty() && p.isClosed(x)) {
- hasEllipsis = p.optionals(obj, x.optionals)
+ hasEllipsis = p.optionals(len(x.arcs) > 0, obj, x.optionals)
}
for i, a := range x.arcs {
f := &ast.Field{
@@ -942,6 +951,15 @@
return obj, nil
}
+func hasBulk(a []ast.Decl) bool {
+ for _, d := range a {
+ if internal.IsBulkField(d) {
+ return true
+ }
+ }
+ return false
+}
+
func (p *exporter) embedding(s *ast.StructLit, n value) (closed bool) {
switch x := n.(type) {
case *structLit:
@@ -950,7 +968,11 @@
n = err
break
}
- s.Elts = append(s.Elts, st.Elts...)
+ if hasBulk(st.Elts) {
+ s.Elts = append(s.Elts, internal.EmbedStruct(st))
+ } else {
+ s.Elts = append(s.Elts, st.Elts...)
+ }
return p.isClosed(x)
case *binaryExpr:
diff --git a/cue/export_test.go b/cue/export_test.go
index dcad849..3743a4a 100644
--- a/cue/export_test.go
+++ b/cue/export_test.go
@@ -351,6 +351,7 @@
b: int
[_]: <100
+ {[_]: <300}
}
}`,
out: unindent(`
@@ -367,9 +368,10 @@
emb
}
e :: {
- [string]: <100
- b: int
+ {[string]: <100}
+ b: int
f
+ {[string]: <300}
}
}`),
}, {
@@ -712,6 +714,18 @@
D :: string`),
}, {
in: `
+ a: {
+ foo: 3
+ [=~"foo"]: int
+ }
+ `,
+ out: unindent(`
+ a: {
+ {[=~"foo"]: int}
+ foo: 3
+ }`),
+ }, {
+ in: `
import "time"
a: { b: time.Duration } | { c: time.Duration }
@@ -787,12 +801,12 @@
[string]: int64
}
x: {
- [string]: int64
- x: int64
+ {[string]: int64}
+ x: int64
}
X :: {
- [string]: int64
- x: int64
+ {[string]: int64}
+ x: int64
}`),
}, {
eval: true,
@@ -937,25 +951,25 @@
// It is okay to allow bulk-optional fields along-side definitions.
in: `
"#A": {
- [string]: int
+ {[string]: int}
"#B": 4
}
// Definition
#A: {
- [string]: int
+ {[string]: int}
#B: 4
}
a: #A.#B
`,
out: unindent(`
"#A": {
- [string]: int
- "#B": 4
+ {[string]: int}
+ "#B": 4
}
// Definition
#A: {
- [string]: int
- #B: 4
+ {[string]: int}
+ #B: 4
}
a: 4`),
}, {
diff --git a/cue/parser/parser.go b/cue/parser/parser.go
index 4a598c0..436a42c 100644
--- a/cue/parser/parser.go
+++ b/cue/parser/parser.go
@@ -24,6 +24,7 @@
"cuelang.org/go/cue/literal"
"cuelang.org/go/cue/scanner"
"cuelang.org/go/cue/token"
+ "cuelang.org/go/internal"
)
// The parser structure holds the parser's internal state.
@@ -722,6 +723,15 @@
// and we have eliminated the need comment positions.
}
+ if len(list) > 1 {
+ for _, d := range list {
+ if internal.IsBulkField(d) {
+ p.assertV0(p.pos, 1, 3, `only one bulk optional field allowed per struct`)
+ break
+ }
+ }
+ }
+
if p.tok == token.ELLIPSIS {
c := p.openComments()
ellipsis := &ast.Ellipsis{Ellipsis: p.pos}
diff --git a/cue/parser/parser_test.go b/cue/parser/parser_test.go
index f033572..e717836 100644
--- a/cue/parser/parser_test.go
+++ b/cue/parser/parser_test.go
@@ -538,6 +538,11 @@
`a b c: 2`},
{"reserved identifiers",
`__foo: 3`},
+ {"bulk optional fields",
+ `a: {
+ foo: "bar"
+ [string]: string
+ }`},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
diff --git a/cue/resolve_test.go b/cue/resolve_test.go
index 7e7dbcb..c0c5b60 100644
--- a/cue/resolve_test.go
+++ b/cue/resolve_test.go
@@ -1180,7 +1180,7 @@
Bar :: {
field: int
- [A=_]: int
+ {[A=_]: int}
}
bar: Bar
bar: { feild: 2 }
@@ -1664,17 +1664,17 @@
desc: "field templates",
in: `
a: {
- [name=_]: int
+ {[name=_]: int}
k: 1
}
b: {
- [X=_]: { x: 0, y: *1 | int }
+ {[X=_]: { x: 0, y: *1 | int }}
v: {}
w: { x: 0 }
}
b: { [y=_]: {} }
c: {
- [Name=_]: { name: Name, y: 1 }
+ {[Name=_]: { name: Name, y: 1 }}
foo: {}
bar: _
}
diff --git a/encoding/jsonschema/constraints.go b/encoding/jsonschema/constraints.go
index 8d4aa4e..b41b798 100644
--- a/encoding/jsonschema/constraints.go
+++ b/encoding/jsonschema/constraints.go
@@ -499,12 +499,12 @@
// [!~(properties) & pattern]: schema
s.patterns = append(s.patterns,
&ast.UnaryExpr{Op: token.NMAT, X: ast.NewString(key)})
- f := &ast.Field{
+ f := internal.EmbedStruct(ast.NewStruct(&ast.Field{
Label: ast.NewList(ast.NewBinExpr(token.AND,
&ast.UnaryExpr{Op: token.MAT, X: ast.NewString(key)},
existing)),
Value: s.schema(n),
- }
+ }))
ast.SetRelPos(f, token.NewSection)
s.obj.Elts = append(s.obj.Elts, f)
})
@@ -530,10 +530,10 @@
}
// [!~(properties|patternProperties)]: schema
existing := append(s.patterns, excludeFields(s.obj.Elts))
- f := &ast.Field{
+ f := internal.EmbedStruct(ast.NewStruct(&ast.Field{
Label: ast.NewList(ast.NewBinExpr(token.AND, existing...)),
Value: s.schema(n),
- }
+ }))
ast.SetRelPos(f, token.NewSection)
s.obj.Elts = append(s.obj.Elts, f)
diff --git a/encoding/jsonschema/jsonschema.go b/encoding/jsonschema/jsonschema.go
index 5f5791b..4a59ab1 100644
--- a/encoding/jsonschema/jsonschema.go
+++ b/encoding/jsonschema/jsonschema.go
@@ -17,7 +17,7 @@
// Mapping and Linking
//
// JSON Schema are often defined in a single file. CUE, on the other hand
-// idomatically defines schema as a definition.
+// idiomatically defines schema as a definition.
//
// CUE:
// $schema: which schema is used for validation.
diff --git a/encoding/jsonschema/testdata/object.txtar b/encoding/jsonschema/testdata/object.txtar
index 167a88c..f25256e 100644
--- a/encoding/jsonschema/testdata/object.txtar
+++ b/encoding/jsonschema/testdata/object.txtar
@@ -70,32 +70,32 @@
foo?: number
bar?: number
- [!~"^(foo|bar)$"]: string
+ {[!~"^(foo|bar)$"]: string}
}
map?: [string]: string
patterns?: {
foo?: number
bar?: number
- [=~"^\\P{Lu}" & !~"^(foo|bar)$"]: string
+ {[=~"^\\P{Lu}" & !~"^(foo|bar)$"]: string}
- [=~"^\\P{Lo}" & !~"^(foo|bar)$"]: int
+ {[=~"^\\P{Lo}" & !~"^(foo|bar)$"]: int}
...
}
patternsNoProps?: {
- [=~"^\\P{Lu}" & !~"^()$"]: string
+ {[=~"^\\P{Lu}" & !~"^()$"]: string}
- [=~"^\\P{Lo}" & !~"^()$"]: int
+ {[=~"^\\P{Lo}" & !~"^()$"]: int}
...
}
complex?: {
foo?: number
bar?: number
- [=~"^\\P{Lu}" & !~"^(foo|bar)$"]: string
+ {[=~"^\\P{Lu}" & !~"^(foo|bar)$"]: string}
- [=~"^\\P{Lo}" & !~"^(foo|bar)$"]: int
+ {[=~"^\\P{Lo}" & !~"^(foo|bar)$"]: int}
- [!~"^\\P{Lu}" & !~"^\\P{Lo}" & !~"^(foo|bar)$"]: string
+ {[!~"^\\P{Lu}" & !~"^\\P{Lo}" & !~"^(foo|bar)$"]: string}
}
}
diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go
index c52a2a1..775c7c3 100644
--- a/internal/diff/diff_test.go
+++ b/internal/diff/diff_test.go
@@ -289,7 +289,8 @@
}
`,
}, {
- x: `[_]: x: "hello"
+ name: "bulk optional",
+ x: `{[_]: x: "hello"}
a: x: "hello"
`,
diff --git a/internal/internal.go b/internal/internal.go
index 1cf2584..a2cd8c7 100644
--- a/internal/internal.go
+++ b/internal/internal.go
@@ -271,6 +271,32 @@
}
}
+func IsBulkField(d ast.Decl) bool {
+ if f, ok := d.(*ast.Field); ok {
+ if _, ok := f.Label.(*ast.ListLit); ok {
+ return true
+ }
+ }
+ return false
+}
+
+func EmbedStruct(s *ast.StructLit) *ast.EmbedDecl {
+ e := &ast.EmbedDecl{Expr: s}
+ if len(s.Elts) == 1 {
+ d := s.Elts[0]
+ astutil.CopyPosition(e, d)
+ ast.SetRelPos(d, token.NoSpace)
+ astutil.CopyComments(e, d)
+ ast.SetComments(d, nil)
+ if f, ok := d.(*ast.Field); ok {
+ ast.SetRelPos(f.Label, token.NoSpace)
+ }
+ }
+ s.Lbrace = token.Newline.Pos()
+ s.Rbrace = token.NoSpace.Pos()
+ return e
+}
+
// IsEllipsis reports whether the declaration can be represented as an ellipsis.
func IsEllipsis(x ast.Decl) bool {
// ...
diff --git a/pkg/tool/os/os.cue b/pkg/tool/os/os.cue
index d7e4322..6881bd9 100644
--- a/pkg/tool/os/os.cue
+++ b/pkg/tool/os/os.cue
@@ -29,14 +29,14 @@
Setenv: {
$id: "tool/os.Setenv"
- [Name]: Value
+ {[Name]: Value}
}
// Getenv gets and parses the specific command line variables.
Getenv: {
$id: "tool/os.Getenv"
- [Name]: Value
+ {[Name]: Value}
}
// Environ populates a struct with all environment variables.
@@ -47,7 +47,7 @@
// Individual entries may be specified ahead of time to enable
// validation and parsing. Values that are marked as required
// will fail the task if they are not found.
- [Name]: Value
+ {[Name]: Value}
}
// Clearenv clears all environment variables.
