doc/ref/spec.md: refinement of closedness

This makes some of the wording regarding closed
structs and embedding more specific.

It also allows embeddings in any struct (not yet
fully implemented) to see the overall effect of the
changes in the ultimate context.

This also adds some examples in the testdata to
pinpoint the semantics (and coincidentally fixes
a bug that was discovered while doing so).

Change-Id: I218f9de28f8226f6aa1ffe4c2a7163ca3cb4c725
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/7181
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>

cue/testdata/definitions/embed.txtar

@@ -12,8 +12,52 @@ deployment: foo: spec: replicas: 1
 #Spec: replicas: int
 
 #TypeMeta: {}
+
+// Reclosing
+reclose1: {
+  #D: {
+    x: int
+    y: int
+  }
+  #a: {
+    #D
+    b: {
+      c: int
+    }
+  }
+
+  z: #a.b
+  z: d: 3 // don't allow this
+}
+
+reclose2: {
+  #D: {
+    x: int
+    y: int
+  }
+  a: {
+    #D
+    b: { // keep open
+      c: int
+    }
+  }
+
+  z: a.b
+  z: d: 3 // allow this
+}
+
+
+
 -- out/eval --
-(struct){
+Errors:
+reclose1.z: field `d` not allowed:
+    ./in.cue:23:8
+    ./in.cue:28:6
+    ./in.cue:29:6
+
+Result:
+(_|_){
+  // [eval]
   deployment: (struct){
     foo: (#struct){
       spec: (#struct){
@@ -31,6 +75,47 @@ deployment: foo: spec: replicas: 1
   }
   #TypeMeta: (#struct){
   }
+  reclose1: (_|_){
+    // [eval]
+    #D: (#struct){
+      x: (int){ int }
+      y: (int){ int }
+    }
+    #a: (#struct){
+      x: (int){ int }
+      y: (int){ int }
+      b: (#struct){
+        c: (int){ int }
+      }
+    }
+    z: (_|_){
+      // [eval]
+      c: (int){ int }
+      d: (_|_){
+        // [eval] reclose1.z: field `d` not allowed:
+        //     ./in.cue:23:8
+        //     ./in.cue:28:6
+        //     ./in.cue:29:6
+      }
+    }
+  }
+  reclose2: (struct){
+    #D: (#struct){
+      x: (int){ int }
+      y: (int){ int }
+    }
+    a: (#struct){
+      x: (int){ int }
+      y: (int){ int }
+      b: (#struct){
+        c: (int){ int }
+      }
+    }
+    z: (struct){
+      c: (int){ int }
+      d: (int){ 3 }
+    }
+  }
 }
 -- out/compile --
 --- in.cue
@@ -53,4 +138,36 @@ deployment: foo: spec: replicas: 1
     replicas: int
   }
   #TypeMeta: {}
+  reclose1: {
+    #D: {
+      x: int
+      y: int
+    }
+    #a: {
+      〈1;#D〉
+      b: {
+        c: int
+      }
+    }
+    z: 〈0;#a〉.b
+    z: {
+      d: 3
+    }
+  }
+  reclose2: {
+    #D: {
+      x: int
+      y: int
+    }
+    a: {
+      〈1;#D〉
+      b: {
+        c: int
+      }
+    }
+    z: 〈0;a〉.b
+    z: {
+      d: 3
+    }
+  }
 }

doc/ref/spec.md

@@ -1240,8 +1240,7 @@ or the pattern of a pattern constraint defined in `c`.
 Hidden fields are excluded from this limitation.
 A struct that is the result of unifying any struct with a [`...`](#Structs)
 declaration is defined for all fields.
-Recursively closing a struct is equivalent to adding `..._|_` to its its root
-and any of its substructures that are not defined for all fields.
+Closing a struct is equivalent to adding `..._|_` to it.
 
 Syntactically, structs are closed explicitly with the `close` builtin or
 implicitly and recursively by [definitions](#definitions-and-hidden-fields).
@@ -1288,17 +1287,17 @@ D: close({
 
 #### Embedding
 
-A struct may contain an _embedded value_, an operand used
-as a declaration, which must evaluate to a struct.
+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.
-A struct resulting from such a unification is closed if either of the involved
-structs were closed.
+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
+normal rules for closed structs were observed.
 
-At the top level, an embedded value may be any type.
-In this case, a CUE program will evaluate to the embedded value
-and the CUE program may not have top-level regular or optional
-fields (definitions and aliases are allowed).
+If an embedded value is not of type struct, the struct may only have
+definitions or hidden fields. Regular fields are not allowed in such case.
+
+The result of `{ A }` is `A` for any `A` (including definitions).
 
 Syntactically, embeddings may be any expression.
 
@@ -1339,10 +1338,11 @@ A field is _hidden_ if its starts with a `_`.
 Definitions and hidden fields are not emitted when converting a CUE program
 to data and are never required to be concrete.
 
-Referencing a definition will implicitely [close](#ClosedStructs) it.
-A struct that embeds a referenced definition will itself be closed
-after first allowing any other fields or embedded structs to unify.
-The result of `{ #A }` is `#A` for any `#A`.
+Referencing a definition will recursively [close](#ClosedStructs) it.
+That is, a referenced definition will not unify with a struct
+that would add a field anywhere within the definition that it does not
+already define or explicitly allow with a pattern constraint or `...`.
+[Embeddings](#Embedding) allow bypassing this check.
 
 If referencing a definition would always result in an error, implementations
 may report this inconsistency at the point of its declaration.
@@ -1375,6 +1375,30 @@ D2: #D & { a: 12, b: 33 }  // _|_ // cannot define both `a` and `b`
 ```
 
 
+```
+#A: {a: int}
+
+B: {
+    #A
+    b: c: int
+}
+
+x: B
+x: d: 3  // not allowed, as closed by embedded #A
+
+y: B.b
+y: d: 3  // allowed as nothing closes b
+
+#B: {
+    #A
+    b: c: int
+}
+
+z: #B.b
+z: d: 3  // not allowed, as referencing #B closes b
+```
+
+
 <!---
 JSON fields are usual camelCase. Clashes can be avoided by adopting the
 convention that definitions be TitleCase. Unexported definitions are still

internal/core/eval/eval.go

@@ -1048,7 +1048,7 @@ outer:
 			defer func() { arc.SelfCount-- }()
 		}
 
-		if arc.Label.IsDef() { // should test whether closed, not isDef?
+		if isDef(v.Expr()) { // should test whether closed, not isDef?
 			c := closedInfo(n.node)
 			closeID = c.InsertDefinition(v.CloseID, x)
 			n.needClose = true // TODO: is this still necessary?
@@ -1096,6 +1096,28 @@ outer:
 	}
 }
 
+// isDef reports whether an expressions is a reference that references a
+// definition anywhere in its selection path.
+//
+// TODO(performance): this should be merged with resolve(). But for now keeping
+// this code isolated makes it easier to see what it is for.
+func isDef(x adt.Expr) bool {
+	switch r := x.(type) {
+	case *adt.FieldReference:
+		return r.Label.IsDef()
+
+	case *adt.SelectorExpr:
+		if r.Sel.IsDef() {
+			return true
+		}
+		return isDef(r.X)
+
+	case *adt.IndexExpr:
+		return isDef(r.X)
+	}
+	return false
+}
+
 // updateCyclicStatus looks for proof of non-cyclic conjuncts to override
 // a structural cycle.
 func (n *nodeContext) updateCyclicStatus(env *adt.Environment) {