internal/core/eval: fix result of closed embedding

This brings the implementation in line with the
spec.

Includes a small change to the formulation
in the spec to clarify things. But it doesn't
aim to change the meaning.

Change-Id: I35dadb4af53ea3f008e39dea92ccfef0fb33f375
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8061
Reviewed-by: CUE cueckoo <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>

cue/testdata/definitions/embed.txtar

@@ -13,6 +13,11 @@ deployment: foo: spec: replicas: 1
 
 #TypeMeta: {}
 
+recloseSimple: {
+  #foo: {}
+  a: {#foo} & {b: int}
+}
+
 // Reclosing
 reclose1: {
   #D: {
@@ -47,7 +52,8 @@ reclose2: {
 }
 
 reclose3: {
-  #Step: (#A | #B) & {
+  #Step: {
+    (#A | #B)
     #Common
   }
   #Common: {
@@ -71,9 +77,14 @@ reclose3: {
 -- out/eval --
 Errors:
 reclose1.z: field `d` not allowed:
-    ./in.cue:23:8
-    ./in.cue:28:6
-    ./in.cue:29:6
+    ./in.cue:28:8
+    ./in.cue:33:6
+    ./in.cue:34:6
+recloseSimple.a: field `b` not allowed:
+    ./in.cue:16:9
+    ./in.cue:17:6
+    ./in.cue:17:7
+    ./in.cue:17:16
 
 Result:
 (_|_){
@@ -95,6 +106,21 @@ Result:
   }
   #TypeMeta: (#struct){
   }
+  recloseSimple: (_|_){
+    // [eval]
+    #foo: (#struct){
+    }
+    a: (_|_){
+      // [eval]
+      b: (_|_){
+        // [eval] recloseSimple.a: field `b` not allowed:
+        //     ./in.cue:16:9
+        //     ./in.cue:17:6
+        //     ./in.cue:17:7
+        //     ./in.cue:17:16
+      }
+    }
+  }
   reclose1: (_|_){
     // [eval]
     #D: (#struct){
@@ -113,9 +139,9 @@ Result:
       c: (int){ int }
       d: (_|_){
         // [eval] reclose1.z: field `d` not allowed:
-        //     ./in.cue:23:8
-        //     ./in.cue:28:6
-        //     ./in.cue:29:6
+        //     ./in.cue:28:8
+        //     ./in.cue:33:6
+        //     ./in.cue:34:6
       }
     }
   }
@@ -182,6 +208,14 @@ Result:
     replicas: int
   }
   #TypeMeta: {}
+  recloseSimple: {
+    #foo: {}
+    a: ({
+      〈1;#foo〉
+    } & {
+      b: int
+    })
+  }
   reclose1: {
     #D: {
       x: int
@@ -215,9 +249,10 @@ Result:
     }
   }
   reclose3: {
-    #Step: ((〈0;#A〉|〈0;#B〉) & {
+    #Step: {
+      (〈1;#A〉|〈1;#B〉)
       〈1;#Common〉
-    })
+    }
     #Common: {
       Name: string
     }

doc/ref/spec.md

@@ -1306,8 +1306,8 @@ D: close({
 A struct may contain an _embedded value_, an operand used as a declaration.
 An embedded value of type struct is unified with the struct in which it is
 embedded, but disregarding the restrictions imposed by closed structs.
-So if an embedding contains a closed struct, the corresponding resulting struct
-will also be closed, but may have fields that are not allowed if
+So if an embedding resolves to a closed struct, the corresponding enclosing
+struct will also be closed, but may have fields that are not allowed if
 normal rules for closed structs were observed.
 
 If an embedded value is not of type struct, the struct may only have

internal/core/adt/closed.go

@@ -118,6 +118,24 @@ func (c CloseInfo) SpawnEmbed(x Expr) CloseInfo {
 	return c
 }
 
+// SpawnGroup is used for structs that contain embeddings that may end up
+// closing the struct. This is to force that `b` is not allowed in
+//
+//      a: {#foo} & {b: int}
+//
+func (c CloseInfo) SpawnGroup(x Expr) CloseInfo {
+	embedded := false
+	if c.closeInfo != nil {
+		embedded = c.embedded
+	}
+	c.closeInfo = &closeInfo{
+		parent:   c.closeInfo,
+		location: x,
+		embedded: embedded,
+	}
+	return c
+}
+
 func (c CloseInfo) SpawnRef(arc *Vertex, isDef bool, x Expr) CloseInfo {
 	embedded := false
 	if c.closeInfo != nil {

internal/core/adt/closed_test.go

@@ -300,6 +300,46 @@ func TestClosedness(t *testing.T) {
 			{"f", false},
 		},
 		required: true,
+	}, {
+		desc: "local closing of def",
+		// #test: {
+		//     #def
+		//     a: 1
+		//     b: 1
+		// }
+		// #test: {
+		//     c: 1
+		//     d: 1
+		// }
+		// #def: {
+		//     c: 1
+		//     e: 1
+		// }
+		n: func() *adt.Vertex {
+			var (
+				root adt.CloseInfo
+				test = root.SpawnRef(nil, true, mkRef("#test"))
+				// isolate local struct.
+				spawned = test.SpawnRef(nil, false, mkRef("dummy"))
+				embed   = spawned.SpawnEmbed(mkRef("dummy"))
+				def     = embed.SpawnRef(nil, true, mkRef("#def"))
+			)
+			return &adt.Vertex{
+				Structs: []*adt.StructInfo{
+					mkStruct(spawned, "{a: 1, b: 1}"),
+					mkStruct(test, "{c: 1, d: 1}"),
+					mkStruct(def, "{c: 1, e: 1}"),
+				},
+			}
+		},
+		tests: []test{
+			{"a", true},
+			{"d", false},
+			{"c", true},
+			{"e", true},
+			{"f", false},
+		},
+		required: true,
 	}, {
 		desc: "branching",
 		// test: #foo
@@ -372,11 +412,11 @@ func TestClosedness(t *testing.T) {
 	// -----
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
+			n := tc.n()
 			for _, sub := range tc.tests {
 				t.Run(sub.f, func(t *testing.T) {
 					f := adt.MakeIdentLabel(r, sub.f, "")
 
-					n := tc.n()
 					ok, required := adt.Accept(ctx, n, f)
 					if ok != sub.found || required != tc.required {
 						t.Errorf("got (%v, %v); want (%v, %v)",

internal/core/adt/eval.go

@@ -1569,14 +1569,16 @@ func (n *nodeContext) addStruct(
 		childEnv.Deref = env.Deref
 	}
 
+	s.Init()
+
+	if s.HasEmbed {
+		closeInfo = closeInfo.SpawnGroup(nil)
+	}
+
 	parent := n.node.AddStruct(s, childEnv, closeInfo)
 	closeInfo.IsClosed = false
 	parent.Disable = true // disable until processing is done.
 
-	hasEmbed := false
-
-	s.Init()
-
 	for _, d := range s.Decls {
 		switch x := d.(type) {
 		case *Field:
@@ -1602,8 +1604,6 @@ func (n *nodeContext) addStruct(
 			n.ifClauses = append(n.ifClauses, envYield{childEnv, x, closeInfo, nil})
 
 		case Expr:
-			hasEmbed = true
-
 			// add embedding to optional
 
 			// TODO(perf): only do this if addExprConjunct below will result in
@@ -1626,7 +1626,7 @@ func (n *nodeContext) addStruct(
 		}
 	}
 
-	if !hasEmbed {
+	if !s.HasEmbed {
 		n.aStruct = s
 		n.aStructID = closeInfo
 	}

internal/core/adt/expr.go

@@ -46,6 +46,7 @@ type StructLit struct {
 	Bulk []*BulkOptionalField
 
 	Additional  []Expr
+	HasEmbed    bool
 	IsOpen      bool // has a ...
 	initialized bool
 
@@ -95,8 +96,10 @@ func (o *StructLit) Init() {
 		case *DynamicField:
 			o.Dynamic = append(o.Dynamic, x)
 
-		case *ForClause, Yielder, Expr:
-			// Set hasEmbed, or maybe add embeddings.
+		case Expr:
+			o.HasEmbed = true
+
+		case *ForClause, Yielder:
 
 		case *BulkOptionalField:
 			o.Bulk = append(o.Bulk, x)