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>

cmd/cue/cmd/fix.go

@@ -22,9 +22,33 @@ import (
 	"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

cmd/cue/cmd/fix_test.go

@@ -68,6 +68,48 @@ a: [ for x in y { x } ]
 		`,
 		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",

cue/builtin_test.go

@@ -404,7 +404,7 @@ func TestBuiltins(t *testing.T) {
 		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])`),

cue/export.go

@@ -233,10 +233,15 @@ func (p *exporter) mkTemplate(v value, n *ast.Ident) ast.Label {
 
 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 @@ func hasTemplate(s *ast.StructLit) bool {
 						return true
 					}
 				}
-				return false
 			}
 		}
 	}
@@ -589,7 +593,7 @@ func (p *exporter) expr(v value) ast.Expr {
 			if x.optionals == nil {
 				break
 			}
-			p.optionals(st, x.optionals)
+			p.optionals(len(x.arcs) > 0, st, x.optionals)
 		}
 		return expr
 
@@ -763,15 +767,16 @@ func (p *exporter) optionalsExpr(x *optionals, isClosed bool) ast.Expr {
 	// 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)
 	}
 	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 @@ func (p *exporter) optionals(st *ast.StructLit, x *optionals) (skippedEllipsis b
 				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 @@ func (p *exporter) structure(x *structLit, addTempl bool) (ret *ast.StructLit, e
 		// 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 @@ func (p *exporter) structure(x *structLit, addTempl bool) (ret *ast.StructLit, e
 	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 @@ func (p *exporter) embedding(s *ast.StructLit, n value) (closed bool) {
 			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:

cue/export_test.go

@@ -351,6 +351,7 @@ func TestExport(t *testing.T) {
 
 				b: int
 				[_]: <100
+				{[_]: <300}
 			}
 		}`,
 		out: unindent(`
@@ -367,9 +368,10 @@ func TestExport(t *testing.T) {
 				emb
 			}
 			e :: {
-				[string]: <100
-				b:        int
+				{[string]: <100}
+				b: int
 				f
+				{[string]: <300}
 			}
 		}`),
 	}, {
@@ -710,6 +712,18 @@ func TestExportFile(t *testing.T) {
 			[D]: int
 		}
 		D :: string`),
+	}, {
+		in: `
+		a: {
+			foo: 3
+			[=~"foo"]: int
+		}
+		`,
+		out: unindent(`
+		a: {
+			{[=~"foo"]: int}
+			foo: 3
+		}`),
 	}, {
 		in: `
 		import "time"
@@ -787,12 +801,12 @@ func TestExportFile(t *testing.T) {
 			[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 @@ func TestExportFile(t *testing.T) {
 		// 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`),
 	}, {

cue/parser/parser.go

@@ -24,6 +24,7 @@ import (
 	"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 @@ func (p *parser) parseFieldList() (list []ast.Decl) {
 		// 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}

cue/parser/parser_test.go

@@ -538,6 +538,11 @@ func TestStrict(t *testing.T) {
 			`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) {

cue/resolve_test.go

@@ -1180,7 +1180,7 @@ a: {
 
 			Bar :: {
 				field: int
-				[A=_]:   int
+				{[A=_]:   int}
 			}
 			bar: Bar
 			bar: { feild: 2 }
@@ -1664,17 +1664,17 @@ a: {
 		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: _
 			}