internal/core/adt: fix omission of closedness check

Analysis: the comprehension evaluates a with the state AllArcs,
which allows processing to stop and continue later.
The closeness check was only done, though, if a field was
evaluated completely in one go.

The solution is to also do the check in the alternative path,
where the evaluation is delayed due to the usage of AllArcs.

Fixes #852

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

cue/testdata/eval/closedness.txtar

@@ -1,3 +1,5 @@
+// Issue #852
+
 -- in.cue --
 #E: {
     c: int
@@ -16,6 +18,23 @@ a: #A & {
         e: 43
     }
 }
+
+// `a` is evaluated through the comprehension first. Ensure that
+// this does not bypass closedness checks.
+issue852: {
+  #A: {
+    [=~"^a-z$"]: string
+  }
+
+  a: #A
+
+  a: Foo: "foo"
+
+  for k, v in a {
+    b: "\(k)": v
+  }
+}
+
 -- out/eval --
 Errors:
 a.q: field e not allowed:
@@ -24,6 +43,10 @@ a.q: field e not allowed:
     ./in.cue:7:9
     ./in.cue:11:4
     ./in.cue:15:9
+issue852.a: field Foo not allowed:
+    ./in.cue:22:7
+    ./in.cue:26:6
+    ./in.cue:28:6
 
 Result:
 (_|_){
@@ -55,6 +78,23 @@ Result:
       }
     }
   }
+  issue852: (_|_){
+    // [eval]
+    #A: (#struct){
+    }
+    a: (_|_){
+      // [eval]
+      Foo: (_|_){
+        // [eval] issue852.a: field Foo not allowed:
+        //     ./in.cue:22:7
+        //     ./in.cue:26:6
+        //     ./in.cue:28:6
+      }
+    }
+    b: (struct){
+      Foo: (string){ "foo" }
+    }
+  }
 }
 -- out/compile --
 --- in.cue
@@ -76,4 +116,18 @@ Result:
       e: 43
     }
   })
+  issue852: {
+    #A: {
+      [=~"^a-z$"]: string
+    }
+    a: 〈0;#A〉
+    a: {
+      Foo: "foo"
+    }
+    for k, v in 〈0;a〉 {
+      b: {
+        "\(〈2;k〉)": 〈2;v〉
+      }
+    }
+  }
 }

internal/core/adt/eval.go

@@ -198,36 +198,8 @@ func (c *OpContext) Unify(v *Vertex, state VertexStatus) {
 			}
 		}
 
-		// TODO(perf): ideally we should always perform a closedness check if
-		// state is Finalized. This is currently not possible when computing a
-		// partial disjunction as the closedness information is not yet
-		// complete, possibly leading to a disjunct to be rejected prematurely.
-		// It is probably possible to fix this if we could add StructInfo
-		// structures demarked per conjunct.
-		//
-		// In practice this should not be a problem: when disjuncts originate
-		// from the same disjunct, they will have the same StructInfos, and thus
-		// Equal is able to equate them even in the precense of optional field.
-		// In general, combining any limited set of disjuncts will soon reach
-		// a fixed point where duplicate elements can be eliminated this way.
-		//
-		// Note that not checking closedness is irrelevant for disjunctions of
-		// scalars. This means it also doesn't hurt performance where structs
-		// have a discriminator field (e.g. Kubernetes). We should take care,
-		// though, that any potential performance issues are eliminated for
-		// Protobuf-like oneOf fields.
-		ignore := state != Finalized || n.skipNonMonotonicChecks()
-
-		if !v.Label.IsInt() && v.Parent != nil && !ignore {
-			// Visit arcs recursively to validate and compute error.
-			if _, err := verifyArc2(c, v.Label, v, v.Closed); err != nil {
-				// Record error in child node to allow recording multiple
-				// conflicts at the appropriate place, to allow valid fields to
-				// be represented normally and, most importantly, to avoid
-				// recursive processing of a disallowed field.
-				v.SetValue(c, Finalized, err)
-				return
-			}
+		if !n.checkClosed(state) {
+			return
 		}
 
 		defer c.PopArc(c.PushArc(v))
@@ -374,6 +346,10 @@ func (c *OpContext) Unify(v *Vertex, state VertexStatus) {
 		v.UpdateStatus(Finalized)
 
 	case AllArcs:
+		if !n.checkClosed(state) {
+			break
+		}
+
 		defer c.PopArc(c.PushArc(v))
 
 		n.completeArcs(state)
@@ -611,6 +587,43 @@ func (n *nodeContext) incompleteErrors() *Bottom {
 	return err
 }
 
+// TODO(perf): ideally we should always perform a closedness check if
+// state is Finalized. This is currently not possible when computing a
+// partial disjunction as the closedness information is not yet
+// complete, possibly leading to a disjunct to be rejected prematurely.
+// It is probably possible to fix this if we could add StructInfo
+// structures demarked per conjunct.
+//
+// In practice this should not be a problem: when disjuncts originate
+// from the same disjunct, they will have the same StructInfos, and thus
+// Equal is able to equate them even in the precense of optional field.
+// In general, combining any limited set of disjuncts will soon reach
+// a fixed point where duplicate elements can be eliminated this way.
+//
+// Note that not checking closedness is irrelevant for disjunctions of
+// scalars. This means it also doesn't hurt performance where structs
+// have a discriminator field (e.g. Kubernetes). We should take care,
+// though, that any potential performance issues are eliminated for
+// Protobuf-like oneOf fields.
+func (n *nodeContext) checkClosed(state VertexStatus) bool {
+	ignore := state != Finalized || n.skipNonMonotonicChecks()
+
+	v := n.node
+	if !v.Label.IsInt() && v.Parent != nil && !ignore {
+		ctx := n.ctx
+		// Visit arcs recursively to validate and compute error.
+		if _, err := verifyArc2(ctx, v.Label, v, v.Closed); err != nil {
+			// Record error in child node to allow recording multiple
+			// conflicts at the appropriate place, to allow valid fields to
+			// be represented normally and, most importantly, to avoid
+			// recursive processing of a disallowed field.
+			v.SetValue(ctx, Finalized, err)
+			return false
+		}
+	}
+	return true
+}
+
 func (n *nodeContext) completeArcs(state VertexStatus) {
 
 	if state <= AllArcs {