internal/core/adt: handle vertices properly for comprehensions spanning disjunctions

Previously, if a comprehension spanned across
a disjunction, the arc type might not be properly
resolved, possibly leading to dropped fields in the
worst case. This change fixes that by keeping track
of the overlay context.

Fixes #3894

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

Author: mpvl

cue/testdata/comprehensions/pushdown.txtar

@@ -1742,7 +1742,12 @@ Result:
             #sub2?: (bool){ bool }
           }
         }
-        root: ((null|struct)){ |((null){ null }, (#struct){
+        root: ((null|struct)){ |((null){
+            null
+            #sub1?: (#struct){
+              #sub2: (bool){ true }
+            }
+          }, (#struct){
             #sub1?: (#struct){
               #sub2: (bool){ true }
             }
@@ -1787,7 +1792,7 @@ Result:
   arcTypeAcrossTwo: (struct){
     a: (struct){
       b: (struct){
-        c?: (struct){
+        c: (struct){
           d: (bool){ true }
         }
       }
@@ -1796,6 +1801,9 @@ Result:
   issue3894: (struct){
     reduced: (struct){
       p1: (struct){ |((struct){
+          b: (struct){
+            value: (string){ "true" }
+          }
           a: (string){ string }
         }, (struct){
           b: (struct){
@@ -1821,13 +1829,11 @@ Result:
             value: (string){ string }
           }
         }) }
-      out: (#struct){ |((#struct){
-          a?: (string){ string }
-        }, (#struct){
-          b: (#struct){
-            value: (string){ "true" }
-          }
-        }) }
+      out: (#struct){
+        b: (#struct){
+          value: (string){ "true" }
+        }
+      }
     }
   }
   unifyDynamicReflectSuccess: (struct){
@@ -2162,21 +2168,7 @@ diff old new
          //     ./issue3729.cue:11:9
          #sub1?: (#struct){
            #sub2: (bool){ true }
-@@ -850,12 +789,7 @@
-             #sub2?: (bool){ bool }
-           }
-         }
--        root: ((null|struct)){ |((null){
--            null
--            #sub1?: (#struct){
--              #sub2: (bool){ true }
--            }
--          }, (#struct){
-+        root: ((null|struct)){ |((null){ null }, (#struct){
-             #sub1?: (#struct){
-               #sub2: (bool){ true }
-             }
-@@ -864,14 +798,7 @@
+@@ -864,14 +803,7 @@
      }
      full: (struct){
        #Application: (#struct){
@@ -2192,44 +2184,6 @@ diff old new
        }
        #ApplicationSpec: (#struct){
          syncPolicy?: ((null|struct)){ |((null){ null }, (#struct){
-@@ -907,7 +834,7 @@
-   arcTypeAcrossTwo: (struct){
-     a: (struct){
-       b: (struct){
--        c: (struct){
-+        c?: (struct){
-           d: (bool){ true }
-         }
-       }
-@@ -916,9 +843,6 @@
-   issue3894: (struct){
-     reduced: (struct){
-       p1: (struct){ |((struct){
--          b: (struct){
--            value: (string){ "true" }
--          }
-           a: (string){ string }
-         }, (struct){
-           b: (struct){
-@@ -944,11 +868,13 @@
-             value: (string){ string }
-           }
-         }) }
--      out: (#struct){
--        b: (#struct){
--          value: (string){ "true" }
--        }
--      }
-+      out: (#struct){ |((#struct){
-+          a?: (string){ string }
-+        }, (#struct){
-+          b: (#struct){
-+            value: (string){ "true" }
-+          }
-+        }) }
-     }
-   }
-   unifyDynamicReflectSuccess: (struct){
 -- out/evalalpha/stats --
 Leaks:  690
 Freed:  0
@@ -2238,10 +2192,10 @@ Allocs: 690
 Retain: 0
 
 Unifications: 602
-Conjuncts:    1027
+Conjuncts:    1029
 Disjuncts:    44
 
-CloseIDElems: 218
+CloseIDElems: 222
 NumCloseIDs: 181
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -2264,10 +2218,10 @@ diff old new
 +Retain: 0
 +
 +Unifications: 602
-+Conjuncts:    1027
++Conjuncts:    1029
 +Disjuncts:    44
 +
-+CloseIDElems: 218
++CloseIDElems: 222
 +NumCloseIDs: 181
 -- out/eval/stats --
 Leaks:  73

internal/core/adt/comprehension.go

@@ -459,6 +459,7 @@ func (n *nodeContext) processComprehension(d *envYield, state vertexStatus) *Bot
 
 	v := n.node
 	for c := d.leaf; c.parent != nil; c = c.parent {
+		v = n.ctx.deref(v)
 		v.updateArcType(c.arcType)
 		if v.ArcType == ArcNotPresent {
 			parent := v.Parent

internal/core/adt/context.go

@@ -178,6 +178,8 @@ type OpContext struct {
 	// TODO: strictly separate validators and functions.
 	IsValidator bool
 
+	overlays []overlayFrame
+
 	// ==== Debugging ====
 	logID int // sequence number for log messages
 

internal/core/adt/disjunct2.go

@@ -493,6 +493,10 @@ func (n *nodeContext) doDisjunct(c Conjunct, m defaultMode, mode runMode, orig *
 	n.scheduler.blocking = n.scheduler.blocking[:0]
 
 	d := oc.cloneRoot(n)
+
+	n.ctx.pushOverlay(n.node, oc.vertexMap)
+	defer n.ctx.popOverlay()
+
 	d.runMode = mode
 	c.Env = oc.derefDisjunctsEnv(c.Env)
 

internal/core/adt/overlay.go

@@ -68,6 +68,45 @@ type overlayContext struct {
 
 type vertexMap map[*Vertex]*Vertex
 
+// overlayFrom is used to store overlay information in the OpContext. This
+// is used for dynamic resolution of vertices, which prevents data structures
+// from having to be copied in the overlay.
+//
+// TODO(perf): right now this is only used for resolving vertices in
+// comprehensions. We could also use this for resolving environments, though.
+// Furthermore, we could used the "cleared" vertexMaps on this stack to avoid
+// allocating memory.
+//
+// NOTE: using a stack globally in OpContext is not very principled, as we
+// may be evaluating nested evaluations of different disjunctions. However,
+// in practice this just results in more work: as the vertices should not
+// overlap, there will be no cycles.
+type overlayFrame struct {
+	vertexMap vertexMap
+	root      *Vertex
+}
+
+func (c *OpContext) pushOverlay(v *Vertex, m vertexMap) {
+	c.overlays = append(c.overlays, overlayFrame{m, v})
+}
+
+func (c *OpContext) popOverlay() {
+	c.overlays = c.overlays[:len(c.overlays)-1]
+}
+
+func (c *OpContext) deref(v *Vertex) *Vertex {
+	for i := len(c.overlays) - 1; i >= 0; i-- {
+		f := c.overlays[i]
+		if f.root == v {
+			continue
+		}
+		if x, ok := f.vertexMap[v]; ok {
+			return x
+		}
+	}
+	return v
+}
+
 // deref reports a replacement of v or v itself if such a replacement does not
 // exists. It computes the transitive closure of the replacement graph.
 // TODO(perf): it is probably sufficient to only replace one level. But we need
@@ -346,7 +385,7 @@ func (ctx *overlayContext) cloneTask(t *task, dst, src *scheduler) *task {
 
 		node: dst.node,
 
-		// TODO: need to copy closeContexts?
+		// TODO(evalv3): rewrite comp and leaf to reflect disjunctions.
 		comp: t.comp,
 		leaf: t.leaf,
 	}