internal/core/adt: reimplement close for v3

The main reason for this is that the current mechanism
is somewhat broken, as it creates an unitialized vertex
with a closeContext that is not in the correct state.
This may lead to an "already closed" panic if some of
the evaluation strategy is changed.

We now instead wrap using a Vertex that has the right
closedness bits set and uses structure sharing to
forward to the original node.

We could have instead make the close() builtin to
directly set the IsClosed flag in the CloseInfo,
rather than the indirect way of setting NeedClose.
But there are some paths, as through the Resolve
function, that do now use scheduleValue to do the
unwrapping.

Fixing this avoids some panics with other changes
down the line.

This is also a first step of getting rid of the
StructMarker and ListMarker types.

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I7de920e24143391308ec5a4b1136feb06beadd0c
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202905
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

cue/testdata/builtins/default.txtar

@@ -45,6 +45,30 @@ Retain: 0
 Unifications: 25
 Conjuncts:    52
 Disjuncts:    45
+-- out/evalalpha --
+(struct){
+  Len: (int){ 3 }
+  Close: (#struct){
+  }
+  And: (int){ 1 }
+  Or: (int){ 1 }
+  Div: (int){ 2 }
+  Mod: (int){ 1 }
+  Quo: (int){ 2 }
+  Rem: (int){ 1 }
+}
+-- diff/-out/evalalpha<==>+out/eval --
+diff old new
+--- old
++++ new
+@@ -1,6 +1,6 @@
+ (struct){
+   Len: (int){ 3 }
+-  Close: (struct){
++  Close: (#struct){
+   }
+   And: (int){ 1 }
+   Or: (int){ 1 }
 -- out/eval --
 (struct){
   Len: (int){ 3 }

cue/testdata/cycle/evaluate.txtar

@@ -127,7 +127,6 @@ Conjuncts:    299
 Disjuncts:    192
 -- out/evalalpha --
 Errors:
-closeCycle.c: structural cycle
 printCycle.a.X: structural cycle
 structCycle.c: structural cycle
 letCycleOK.t2.a.X: structural cycle:
@@ -139,6 +138,8 @@ disjunctionCycle.b: cannot use 1 (type int) as type list:
     ./in.cue:56:9
 b: structural cycle:
     ./in.cue:62:6
+closeCycle.c: structural cycle:
+    ./in.cue:73:5
 structural cycle:
     ./in.cue:85:12
 listAddCycle.c: structural cycle:
@@ -251,15 +252,17 @@ Result:
   }
   closeCycle: (_|_){
     // [structural cycle]
-    a: ~(closeCycle.b)
+    a: (_|_){
+      // [structural cycle] closeCycle.c: structural cycle:
+      //     ./in.cue:73:5
+    }
     b: (_|_){
-      // [structural cycle]
-      d: (_|_){
-        // [structural cycle] closeCycle.c: structural cycle
-      }
+      // [structural cycle] closeCycle.c: structural cycle:
+      //     ./in.cue:73:5
     }
     c: (_|_){
-      // [structural cycle] closeCycle.c: structural cycle
+      // [structural cycle] closeCycle.c: structural cycle:
+      //     ./in.cue:73:5
     }
   }
   structCycle: (_|_){
@@ -350,7 +353,7 @@ Result:
 diff old new
 --- old
 +++ new
-@@ -1,50 +1,49 @@
+@@ -1,50 +1,50 @@
  Errors:
 -closeCycle.a: structural cycle
 -closeCycle.b.d: structural cycle
@@ -365,7 +368,6 @@ diff old new
 -disjunctionCycle.a: cannot use 1 (type int) as type list:
 -    ./in.cue:56:5
 -    ./in.cue:56:9
-+closeCycle.c: structural cycle
 +printCycle.a.X: structural cycle
 +structCycle.c: structural cycle
 +letCycleOK.t2.a.X: structural cycle:
@@ -380,14 +382,15 @@ diff old new
 -    ./in.cue:56:9
  b: structural cycle:
      ./in.cue:62:6
--closeCycle.c: structural cycle:
+ closeCycle.c: structural cycle:
 -    ./in.cue:73:15
 -structCycle.c: structural cycle:
 -    ./in.cue:79:14
 -embedCycle: structural cycle:
 -    ./in.cue:85:11
 -printCycle.a.X.X: structural cycle:
 -    ./in.cue:113:6
++    ./in.cue:73:5
 +structural cycle:
 +    ./in.cue:85:12
 +listAddCycle.c: structural cycle:
@@ -428,7 +431,7 @@ diff old new
        }
      }
    }
-@@ -59,20 +58,16 @@
+@@ -59,20 +59,16 @@
            // [structural cycle] letCycleFail.t1.a.X: structural cycle
          }
          c: (_|_){
@@ -459,7 +462,7 @@ diff old new
          }
          x: (struct){
            y: (string){ "" }
-@@ -88,17 +83,17 @@
+@@ -88,17 +84,17 @@
    disjunctionCycle: (_|_){
      // [eval]
      a: (_|_){
@@ -488,7 +491,7 @@ diff old new
        //     ./in.cue:56:5
        //     ./in.cue:56:9
      }
-@@ -124,80 +119,77 @@
+@@ -124,80 +120,79 @@
      }
      b: (struct){
      }
@@ -500,7 +503,7 @@ diff old new
    }
    closeCycle: (_|_){
      // [structural cycle]
--    a: (_|_){
+     a: (_|_){
 -      // [structural cycle] closeCycle.a: structural cycle
 -    }
 -    b: (_|_){
@@ -512,15 +515,16 @@ diff old new
 -    c: (_|_){
 -      // [structural cycle] closeCycle.c: structural cycle:
 -      //     ./in.cue:73:15
-+    a: ~(closeCycle.b)
++      // [structural cycle] closeCycle.c: structural cycle:
++      //     ./in.cue:73:5
++    }
 +    b: (_|_){
-+      // [structural cycle]
-+      d: (_|_){
-+        // [structural cycle] closeCycle.c: structural cycle
-+      }
++      // [structural cycle] closeCycle.c: structural cycle:
++      //     ./in.cue:73:5
 +    }
 +    c: (_|_){
-+      // [structural cycle] closeCycle.c: structural cycle
++      // [structural cycle] closeCycle.c: structural cycle:
++      //     ./in.cue:73:5
      }
    }
    structCycle: (_|_){
@@ -618,7 +622,7 @@ diff old new
      }
    }
    closeFail: (_|_){
-@@ -207,21 +199,22 @@
+@@ -207,21 +202,22 @@
      }
      x: (_|_){
        // [eval]

cue/testdata/cycle/inline_non_recursive.txtar

@@ -80,14 +80,14 @@ issue3182: {
 	}
 }
 -- out/evalalpha/stats --
-Leaks:  250
+Leaks:  272
 Freed:  0
 Reused: 0
-Allocs: 250
+Allocs: 272
 Retain: 0
 
-Unifications: 240
-Conjuncts:    1277
+Unifications: 251
+Conjuncts:    1299
 Disjuncts:    0
 -- out/evalalpha --
 Errors:
@@ -203,17 +203,17 @@ diff old new
 -Reused: 401
 -Allocs: 310
 -Retain: 1066
-+Leaks:  250
++Leaks:  272
 +Freed:  0
 +Reused: 0
-+Allocs: 250
++Allocs: 272
 +Retain: 0
 
 -Unifications: 711
 -Conjuncts:    2759
 -Disjuncts:    1446
-+Unifications: 240
-+Conjuncts:    1277
++Unifications: 251
++Conjuncts:    1299
 +Disjuncts:    0
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new

internal/core/adt/closed.go

@@ -330,9 +330,10 @@ func isClosed(v *Vertex) bool {
 	// We could have used IsRecursivelyClosed here, but (effectively)
 	// implementing it again here allows us to only have to iterate over
 	// Structs once.
-	if v.ClosedRecursive {
+	if v.ClosedRecursive || v.ClosedNonRecursive {
 		return true
 	}
+	// TODO(evalv3): this can be removed once we delete the evalv2 code.
 	for _, s := range v.Structs {
 		if s.IsClosed || s.IsInOneOf(DefinitionSpan) {
 			return true

internal/core/adt/composite.go

@@ -187,6 +187,10 @@ type Vertex struct {
 	// of the conjuncts, or ancestor conjuncts, is a definition.
 	ClosedRecursive bool
 
+	// ClosedNonRecursive indicates that this Vertex has been closed for this
+	// level only. This supports the close builtin.
+	ClosedNonRecursive bool
+
 	// HasEllipsis indicates that this Vertex is open by means of an ellipsis.
 	// TODO: combine this field with Closed once we removed the old evaluator.
 	HasEllipsis bool
@@ -583,6 +587,15 @@ const (
 	finalized
 )
 
+// Wrap creates a Vertex that takes w as a shared value. This allows users
+// to set different flags for a wrapped Vertex.
+func (c *OpContext) Wrap(v *Vertex, id CloseInfo) *Vertex {
+	w := c.newInlineVertex(nil, nil, v.Conjuncts...)
+	n := w.getState(c)
+	n.share(makeAnonymousConjunct(nil, v, nil), v, CloseInfo{})
+	return w
+}
+
 // Status returns the status of the current node. When reading the status, one
 // should always use this method over directly reading status field.
 //

internal/core/adt/conjunct.go

@@ -552,20 +552,20 @@ func (n *nodeContext) insertValueConjunct(env *Environment, v Value, id CloseInf
 
 	switch x := v.(type) {
 	case *Vertex:
-		if m, ok := x.BaseValue.(*StructMarker); ok {
+		if x.ClosedNonRecursive {
+			n.node.ClosedNonRecursive = true
+			var cc *closeContext
+			id, cc = id.spawnCloseContext(n.ctx, 0)
+			cc.isClosedOnce = true
+
+			if v, ok := x.BaseValue.(*Vertex); ok {
+				n.insertValueConjunct(env, v, id)
+				return
+			}
+		}
+		if _, ok := x.BaseValue.(*StructMarker); ok {
 			n.aStruct = x
 			n.aStructID = id
-			if m.NeedClose {
-				// TODO: In the new evaluator this is used to mark a struct
-				// as closed in the debug output. Once the old evaluator is
-				// gone, we could simplify this.
-				id.IsClosed = true
-				if ctx.isDevVersion() {
-					var cc *closeContext
-					id, cc = id.spawnCloseContext(n.ctx, 0)
-					cc.isClosedOnce = true
-				}
-			}
 		}
 
 		if !x.IsData() {

internal/core/adt/expr.go

@@ -421,6 +421,9 @@ func (x *ListMarker) node()            {}
 type StructMarker struct {
 	// NeedClose is used to signal that the evaluator should close this struct.
 	// It is only set by the close builtin.
+	// TODO(evalv3: remove this field. Once we removed this, and also introduced
+	// open by default lists, we can get rid of StructMarker and ListMarker
+	// in its entirety in favor of using type bit masks.
 	NeedClose bool
 }
 

internal/core/adt/sched.go

@@ -703,7 +703,10 @@ func runTask(t *task, mode runMode) {
 	defer ctx.popTask()
 	if t.env != nil {
 		id := t.id
-		id.cc = nil // this is done to avoid struct args from passing fields up.
+		// This is done to avoid struct args from passing fields up.
+		// Use [task.updateCI] to get the current CloseInfo with this field
+		// restored.
+		id.cc = nil
 		s := ctx.PushConjunct(MakeConjunct(t.env, t.x, id))
 		defer ctx.PopState(s)
 	}
@@ -736,6 +739,13 @@ func runTask(t *task, mode runMode) {
 	}
 }
 
+// updateCI stitches back the closeContext that more removed from the CloseInfo
+// before in the given CloseInfo.
+func (t *task) updateCI(ci CloseInfo) CloseInfo {
+	ci.cc = t.id.cc
+	return ci
+}
+
 // waitFor blocks task t until the needs for scheduler s are met.
 func (t *task) waitFor(s *scheduler, needs condition) {
 	if s.meets(needs) {

internal/core/adt/share.go

@@ -113,7 +113,9 @@ func (n *nodeContext) share(c Conjunct, arc *Vertex, id CloseInfo) {
 	// have been processed and it is clear that sharing is possible. Delaying
 	// such a count should not hurt performance, as a shared node is completed
 	// anyway.
-	id.cc.incDependent(n.ctx, SHARED, n.node.cc())
+	if id.cc != nil {
+		id.cc.incDependent(n.ctx, SHARED, n.node.cc())
+	}
 }
 
 func (n *nodeContext) shareIfPossible(c Conjunct, arc *Vertex, id CloseInfo) bool {

internal/core/adt/tasks.go

@@ -90,8 +90,8 @@ func processExpr(ctx *OpContext, t *task, mode runMode) {
 		// This is an optional cycle that we will ignore.
 		return
 	}
-	t.id.CycleInfo = ci.CycleInfo
-	t.node.insertValueConjunct(t.env, v, t.id)
+	ci = t.updateCI(ci)
+	t.node.insertValueConjunct(t.env, v, ci)
 }
 
 func processResolver(ctx *OpContext, t *task, mode runMode) {
@@ -113,9 +113,11 @@ func processResolver(ctx *OpContext, t *task, mode runMode) {
 	// TODO: consider moving after markCycle or removing.
 	d := arc.DerefDisjunct()
 
+	ci := t.updateCI(ctx.ci)
+
 	// A reference that points to itself indicates equality. In that case
 	// we are done computing and we can return the arc as is.
-	ci, skip := t.node.detectCycleV3(d, t.env, r, t.id)
+	ci, skip := t.node.detectCycleV3(d, t.env, r, ci)
 	if skip {
 		return
 	}
@@ -162,9 +164,14 @@ func processDynamic(ctx *OpContext, t *task, mode runMode) {
 		return
 	}
 
-	c := MakeConjunct(t.env, field, t.id)
+	// Do not update the CloseInfo, as we are passing the field value
+	// unevaluated.
+	ci := t.id
+
+	c := MakeConjunct(t.env, field, ci)
+	// TODO(evalv3): this does not seem to be necessary and even treacherous.
 	c.CloseInfo.cc = nil
-	n.insertArc(f, field.ArcType, c, t.id, true)
+	n.insertArc(f, field.ArcType, c, ci, true)
 }
 
 func processPatternConstraint(ctx *OpContext, t *task, mode runMode) {
@@ -179,7 +186,11 @@ func processPatternConstraint(ctx *OpContext, t *task, mode runMode) {
 		return
 	}
 
-	n.insertPattern(v, MakeConjunct(t.env, t.x, t.id))
+	// Do not update the CloseInfo, as we are passing the constraint value
+	// unevaluated.
+	ci := t.id
+
+	n.insertPattern(v, MakeConjunct(t.env, t.x, ci))
 }
 
 func processComprehension(ctx *OpContext, t *task, mode runMode) {

internal/core/adt/unify.go

@@ -318,6 +318,8 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool {
 		if w.ClosedRecursive {
 			v.ClosedRecursive = true
 		}
+		// NOTE: setting ClosedNonRecursive is not necessary, as it is
+		// handled by scheduleValue.
 		if w.HasEllipsis {
 			v.HasEllipsis = true
 		}