internal/core/adt: split closeContext.arcs

Right now, closeContext.arcs is used to store both
arcs and notifications. Although they share a lot
of the same logic, it is confusing and prone to bugs.
It also makes changing either mechanism hard and
makes it harder to understand how everything works.

This CL does a "dumb" split of the functionality.
Further cleaning up can be done down the line.

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

Author: mpvl

Diff for: internal/core/adt/disjunct2.go

@@ -737,13 +737,7 @@ func equalPartialNode(ctx *OpContext, x, y *closeContext) bool {
 	// TODO(perf): use merge sort
 outer:
 	for _, a := range x.arcs {
-		if a.kind != ARC {
-			continue outer
-		}
 		for _, b := range y.arcs {
-			if b.kind != ARC {
-				continue
-			}
 			if a.key.src.Label != b.key.src.Label {
 				continue
 			}

Diff for: internal/core/adt/fields.go

@@ -268,6 +268,15 @@ type closeContext struct {
 	// tree as this closeContext. In both cases the are keyed by Vertex.
 	arcs []ccArc
 
+	// notify represents closeContexts which to notify of updates.
+	//
+	// TODO: Note that this slice is very similar to nodeContext.notify and the
+	// use of these can likely be merged. It may be better to let the notify
+	// originate from a more specific closeContext, allowing it to stopped
+	// sooner and possibly even remove the need for breaking dependency
+	// cycles.
+	notify []ccArc
+
 	// parentIndex is the position in the parent's arcs slice that corresponds
 	// to this closeContext. This is currently unused. The intention is to use
 	// this to allow groups with single elements (which will be the majority)
@@ -315,7 +324,6 @@ func (c *closeContext) updateArcType(ctx *OpContext, t ArcType) {
 }
 
 type ccArc struct {
-	kind        depKind
 	decremented bool
 	// matched indicates the arc is only added to track the destination of a
 	// matched pattern and that it is not explicitly defined as a field.
@@ -331,6 +339,7 @@ type ccArc struct {
 // closeContext.
 type ccArcRef struct {
 	src   *closeContext
+	kind  depKind
 	index int
 }
 
@@ -464,7 +473,7 @@ func (cc *closeContext) getKeyedCC(ctx *OpContext, key *closeContext, c CycleInf
 }
 
 func (cc *closeContext) linkNotify(ctx *OpContext, key *closeContext) bool {
-	for _, a := range cc.arcs {
+	for _, a := range cc.notify {
 		if a.key == key {
 			return false
 		}
@@ -556,33 +565,67 @@ func (c *closeContext) addDependency(ctx *OpContext, kind depKind, matched bool,
 	// NOTE: do not increment
 	// - either root closeContext or otherwise resulting from sub closeContext
 	//   all conjuncts will be added now, notified, or scheduled as task.
+	switch kind {
+	case ARC:
+		for _, a := range c.arcs {
+			if a.key == key {
+				panic("addArc: Label already exists")
+			}
+		}
+		child.incDependent(ctx, kind, c) // matched in decDependent REF(arcs)
+
+		c.arcs = append(c.arcs, ccArc{
+			matched: matched,
+			key:     key,
+			cc:      child,
+		})
+
+		// TODO: this tests seems sensible, but panics. Investigate what could
+		// trigger this.
+		// if child.src.Parent != c.src {
+		// 	panic("addArc: inconsistent parent")
+		// }
+		if child.src.cc() != root.src.cc() {
+			panic("addArc: inconsistent root")
+		}
 
-	child.incDependent(ctx, kind, c) // matched in decDependent REF(arcs)
-
-	for _, a := range c.arcs {
-		if a.key == key {
-			panic("addArc: Label already exists")
+		root.externalDeps = append(root.externalDeps, ccArcRef{
+			src:   c,
+			kind:  kind,
+			index: len(c.arcs) - 1,
+		})
+	case NOTIFY:
+		for _, a := range c.notify {
+			if a.key == key {
+				panic("addArc: Label already exists")
+			}
+		}
+		child.incDependent(ctx, kind, c) // matched in decDependent REF(arcs)
+
+		c.notify = append(c.notify, ccArc{
+			matched: matched,
+			key:     key,
+			cc:      child,
+		})
+
+		// TODO: this tests seems sensible, but panics. Investigate what could
+		// trigger this.
+		// if child.src.Parent != c.src {
+		// 	panic("addArc: inconsistent parent")
+		// }
+		if child.src.cc() != root.src.cc() {
+			panic("addArc: inconsistent root")
 		}
+
+		root.externalDeps = append(root.externalDeps, ccArcRef{
+			src:   c,
+			kind:  kind,
+			index: len(c.notify) - 1,
+		})
+	default:
+		panic(kind)
 	}
 
-	// TODO: this tests seems sensible, but panics. Investigate what could
-	// trigger this.
-	// if child.src.Parent != c.src {
-	// 	panic("addArc: inconsistent parent")
-	// }
-	if child.src.cc() != root.src.cc() {
-		panic("addArc: inconsistent root")
-	}
-	c.arcs = append(c.arcs, ccArc{
-		kind:    kind,
-		matched: matched,
-		key:     key,
-		cc:      child,
-	})
-	root.externalDeps = append(root.externalDeps, ccArcRef{
-		src:   c,
-		index: len(c.arcs) - 1,
-	})
 }
 
 // incDependent needs to be called for any conjunct or child closeContext
@@ -650,7 +693,16 @@ func (c *closeContext) decDependent(ctx *OpContext, kind depKind, dependant *clo
 			continue
 		}
 		c.arcs[i].decremented = true
-		cc.decDependent(ctx, a.kind, c) // REF(arcs)
+		cc.decDependent(ctx, ARC, c)
+	}
+
+	for i, a := range c.notify {
+		cc := a.cc
+		if a.decremented {
+			continue
+		}
+		c.notify[i].decremented = true
+		cc.decDependent(ctx, NOTIFY, c)
 	}
 
 	c.finalizePattern()
@@ -1015,9 +1067,6 @@ func isTotal(p Value) bool {
 // this is not the case.
 func injectClosed(ctx *OpContext, closed, dst *closeContext) {
 	for _, a := range dst.arcs {
-		if a.kind != ARC {
-			continue
-		}
 		ca := a.cc
 		switch f := ca.Label(); {
 		case ca.src.ArcType == ArcOptional,

Diff for: internal/core/adt/overlay.go

@@ -318,9 +318,7 @@ func (ctx *overlayContext) allocCC(cc *closeContext) *closeContext {
 	// We only explicitly tag dependencies of type ARC. Notifications that
 	// point within the disjunct overlay will be tagged elsewhere.
 	for _, a := range cc.arcs {
-		if a.kind == ARC {
-			ctx.allocCC(a.cc)
-		}
+		ctx.allocCC(a.cc)
 	}
 
 	return o
@@ -450,6 +448,19 @@ func (ctx *overlayContext) initCloneCC(x *closeContext) {
 		o.arcs = append(o.arcs, a)
 	}
 
+	for _, a := range x.notify {
+		// If an arc does not have an overlay, we should not decrement the
+		// dependency counter. We simply remove the dependency in that case.
+		if a.cc.overlay == nil {
+			continue
+		}
+		if a.key.overlay != nil {
+			a.key = a.key.overlay // TODO: is this necessary?
+		}
+		a.cc = a.cc.overlay
+		o.notify = append(o.notify, a)
+	}
+
 	// NOTE: copying externalDeps is hard and seems unnecessary, as it needs to
 	// be resolved in the base anyway.
 }

Diff for: internal/core/adt/unify.go

@@ -487,9 +487,12 @@ func (n *nodeContext) completeNodeTasks(mode runMode) {
 	// TODO: replace with something more principled that does not piggyback on
 	// debug information.
 	for _, r := range v.cc().externalDeps {
+		if r.kind != NOTIFY {
+			continue
+		}
 		src := r.src
-		a := &src.arcs[r.index]
-		if a.decremented || a.kind != NOTIFY {
+		a := &src.notify[r.index]
+		if a.decremented {
 			continue
 		}
 		if n := src.src.getState(n.ctx); n != nil {
@@ -556,14 +559,20 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool {
 		if src.src.IsDisjunct {
 			continue
 		}
-		a := &src.arcs[r.index]
+		var a *ccArc
+		switch r.kind {
+		case ARC:
+			a = &src.arcs[r.index]
+		case NOTIFY:
+			a = &src.notify[r.index]
+		}
 		if a.decremented {
 			continue
 		}
 		a.decremented = true
 
 		src.src.unify(n.ctx, needTasksDone, attemptOnly)
-		a.cc.decDependent(n.ctx, a.kind, src) // REF(arcs)
+		a.cc.decDependent(n.ctx, r.kind, src)
 	}
 
 	n.incDepth()