internal/core/adt: clean up indirects and mark disjuncts

Vertex.BaseValue may point to another Vertex. There are now
three kinds of vertices that this value may point to:
- computed, external results, like from builtins.
- disjuncts resulting from resolved disjunctions
- structure shared nodes
The old evaluator only had the first.

When accessing the actual value of a Vertex, one should
dereference the pointers. However, there is lots of interesting
meta information that one may want to access, i.e. the original
path and the original full disjunction expression. The new evaluator
should therefore be more careful as to when something is
dereferenced.

In this CL, we introduce a means to identify whether a Vertex
was the result of a disjunction. We adjust various Indirect calls
by removing them or replacing them with more fine-grained
versions.

This should be a no-op change. Various points where indirect
have been tightened to dereference only the kinds of vertices
that should be dereferenced. The idea is to do a bunch of such
changes in separate CLs to allow bisecting to specific changes
if there are any issues.

We rename the new "Indirect*" functions to "Deref*", which is
a more accurate term. This also allows us to visually inspect
if we have considered all call sites. We will rename and
inspect Indirect in a separate CL.

Issue #2854
Issue #2851
Issue #2884

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

Author: mpvl

cue/types.go

@@ -235,6 +235,10 @@ func (i *Iterator) Next() bool {
 	p := linkParent(i.val.parent_, i.val.v, arc)
 	i.f = arc.Label
 	i.arcType = arc.ArcType
+	// TODO: this should not indirect disjunctions, as this will cause losing
+	// information, which is now available in the new evaluator. Find a safe
+	// way to preserve disjunction information while keeping backward
+	// compatibility in the API.
 	arc = arc.Indirect()
 	i.cur = makeValue(i.val.idx, arc, p)
 	i.p++

internal/core/adt/composite.go

@@ -206,6 +206,10 @@ type Vertex struct {
 	// hasPendingArc is set if this Vertex has a void arc (e.g. for comprehensions)
 	hasPendingArc bool
 
+	// IsDisjunct indicates this Vertex is a disjunct resulting from a
+	// disjunction evaluation.
+	IsDisjunct bool
+
 	// ArcType indicates the level of optionality of this arc.
 	ArcType ArcType
 

internal/core/adt/context.go

@@ -496,7 +496,9 @@ func (c *OpContext) resolveState(x Conjunct, r Resolver, state combinedFlags) (*
 		return nil, arc.ChildErrors
 	}
 
-	arc = arc.Indirect()
+	if !c.isDevVersion() {
+		arc = arc.Indirect()
+	}
 
 	return arc, err
 }
@@ -707,8 +709,9 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 			return nil
 		}
 		// TODO: consider moving this after markCycle, depending on how we
-		// implement markCycle.
-		arc = arc.Indirect()
+		// implement markCycle, or whether we need it at all.
+		// TODO: is this indirect necessary?
+		// arc = arc.Indirect()
 
 		// Save the old CloseInfo and restore after evaluate to avoid detecting
 		// spurious cycles.
@@ -834,8 +837,9 @@ func (c *OpContext) unifyNode(v Expr, state combinedFlags) (result Value) {
 			return nil
 		}
 		// TODO: consider moving this after markCycle, depending on how we
-		// implement markCycle.
-		v = v.Indirect()
+		// implement markCycle, or whether we need it at all.
+		// TODO: is this indirect necessary?
+		// v = v.Indirect()
 
 		if c.isDevVersion() {
 			if n := v.getState(c); n != nil {

internal/core/adt/overlay.go

@@ -79,6 +79,7 @@ type overlayContext struct {
 func (ctx *overlayContext) cloneRoot(root *nodeContext) *nodeContext {
 	// Clone all vertices that need to be cloned to support the overlay.
 	v := ctx.cloneVertex(root.node)
+	v.IsDisjunct = true
 
 	// TODO: patch notifications to any node that is within the disjunct to
 	// point to the new vertex instead.

internal/core/adt/share.go

@@ -104,10 +104,32 @@ func (n *nodeContext) shareIfPossible(c Conjunct, arc *Vertex, id CloseInfo) boo
 	return true
 }
 
-// IndirectNonShared finds the indirection of an arc that is not the result of
+// DerefDisjunct indirects a node that points to a disjunction.
+func (v *Vertex) DerefDisjunct() *Vertex {
+	for {
+		arc, ok := v.BaseValue.(*Vertex)
+		if !ok || !arc.IsDisjunct {
+			return v
+		}
+		v = arc
+	}
+}
+
+// DerefNonDisjunct indirects a node that points to a disjunction.
+func (v *Vertex) DerefNonDisjunct() *Vertex {
+	for {
+		arc, ok := v.BaseValue.(*Vertex)
+		if !ok || arc.IsDisjunct {
+			return v
+		}
+		v = arc
+	}
+}
+
+// DerefNonShared finds the indirection of an arc that is not the result of
 // structure sharing. This is especially relevant when indirecting disjunction
 // values.
-func (v *Vertex) IndirectNonShared() *Vertex {
+func (v *Vertex) DerefNonShared() *Vertex {
 	if v.state != nil && v.state.isShared {
 		return v
 	}

internal/core/adt/tasks.go

@@ -103,11 +103,11 @@ func processResolver(ctx *OpContext, t *task, mode runMode) {
 	}
 	ctx.Logf(t.node.node, "RESOLVED %v to %v %v", r, arc.Label, fmt.Sprintf("%p", arc))
 	// TODO: consider moving after markCycle or removing.
-	arc = arc.Indirect()
+	d := arc.DerefDisjunct()
 
 	// 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.markCycle(arc, t.env, r, t.id)
+	ci, skip := t.node.markCycle(d, t.env, r, t.id)
 	if skip {
 		return
 	}

internal/core/adt/unify.go

@@ -160,7 +160,7 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool {
 
 	defer c.PopArc(c.PushArc(v))
 
-	w := v.IndirectNonShared() // Dereference the disjunction result.
+	w := v.DerefNonShared() // Dereference the disjunction result.
 	if w != v {
 		if w.Closed {
 			// Should resolve with dereference.