internal/core/adt: eagerly evaluate let upon lookup

As the TODO in unify.go suggestes (see where code
is edited), process needs to finalize, instead
of yield. However, this gives incorrect results
for some tests. It is most important, however,
to to this for let values, as let values do not
always have a guaranteed evaluation path
(for instance, they may be a cached version of
themselves).

We therefore only force a finalize when the
value is a let. To detect this, we add a
"withinLet" field that is similar to
nonRooted and anonymous, but just reflecting
let values.

Fixes #3763

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

Author: mpvl

cue/testdata/comprehensions/issue3763.txtar

@@ -104,7 +104,8 @@ Disjuncts:    76
         〈1;cps〉
       },
     ]
-    ports: (list){ list }
+    ports: (#list){
+    }
   }
   reduced: (struct){
     A: (struct){
@@ -123,7 +124,8 @@ Disjuncts:    76
         〈1;x〉
       },
     ]
-    B: (list){ list }
+    B: (#list){
+    }
   }
 }
 -- diff/-out/evalalpha<==>+out/eval --
@@ -154,17 +156,7 @@ diff old new
            }
          }
        }
-@@ -58,8 +43,7 @@
-         〈1;cps〉
-       },
-     ]
--    ports: (#list){
--    }
-+    ports: (list){ list }
-   }
-   reduced: (struct){
-     A: (struct){
-@@ -73,18 +57,11 @@
+@@ -73,17 +58,11 @@
          y?: (int){ int }
        }
      }
@@ -179,16 +171,14 @@ diff old new
 -        y?: (int){ int }
 -      }
 -    }
--    B: (#list){
--    }
 +    let Z#3multi = [
 +      for _, x in 〈2;A〉 {
 +        〈1;x〉
 +      },
 +    ]
-+    B: (list){ list }
+     B: (#list){
+     }
    }
- }
 -- diff/todo/p1 --
 full.ports|reduced.B: incomplete value (list)
 -- out/eval --

internal/core/adt/composite.go

@@ -219,13 +219,16 @@ type Vertex struct {
 	// rooted.
 	nonRooted bool // indicates that there is no path from the root of the tree.
 
-	// anonymous indicates that this Vertex is being computed within a
+	// anonymous indicates that this Vertex is being computed without an
 	// addressable context, or in other words, a context for which there is
-	// a path from the root of the file. Typically, the only addressable
+	// np path from the root of the file. Typically, the only addressable
 	// contexts are fields. Examples of fields that are not addressable are
 	// the for source of comprehensions and let fields or let clauses.
 	anonymous bool
 
+	// withinLet indicate that this vertex is a let value.
+	withinLet bool
+
 	// hasPendingArc is set if this Vertex has a void arc (e.g. for comprehensions)
 	hasPendingArc bool
 
@@ -1356,6 +1359,7 @@ func (v *Vertex) GetArc(c *OpContext, f Feature, t ArcType) (arc *Vertex, isNew
 		ArcType:   t,
 		nonRooted: v.IsDynamic || v.Label.IsLet() || v.nonRooted,
 		anonymous: v.anonymous || v.Label.IsLet(),
+		withinLet: v.withinLet || v.Label.IsLet(),
 	}
 	v.Arcs = append(v.Arcs, arc)
 	if t == ArcPending {

internal/core/adt/fields.go

@@ -169,6 +169,7 @@ func (n *nodeContext) getArc(f Feature, mode ArcType) (arc *Vertex, isNew bool)
 		ArcType:   mode,
 		nonRooted: v.IsDynamic || v.nonRooted,
 		anonymous: v.anonymous || v.Label.IsLet(),
+		withinLet: v.withinLet || v.Label.IsLet(),
 	}
 	if n.scheduler.frozen&fieldSetKnown != 0 {
 		b := n.ctx.NewErrf("adding field %v not allowed as field set was already referenced", f)

internal/core/adt/unify.go

@@ -555,6 +555,7 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode, checkTypos
 	// of range in case the Arcs grows during processing.
 	for arcPos := 0; arcPos < len(n.node.Arcs); arcPos++ {
 		a := n.node.Arcs[arcPos]
+		// TODO: Consider skipping lets.
 
 		if !a.unify(n.ctx, needs, mode, checkTypos) {
 			success = false
@@ -838,20 +839,29 @@ func (v *Vertex) lookup(c *OpContext, pos token.Pos, f Feature, flags combinedFl
 			if arc.ArcType == ArcPending {
 				return arcReturn
 			}
-			goto handleArcType
 
 		case yield:
 			arcState.process(needs, yield)
 			// continue processing, as successful processing may still result
 			// in an invalid field.
 
 		case finalize:
-			// TODO: should we try to use finalize? Using it results in errors and this works. It would be more principled, though.
-			arcState.process(needs, yield)
+			// TODO: we should try to always use finalize? Using it results in
+			// errors. For now we only use it for let values. Let values are
+			// not normally finalized (they may be cached) and as such might
+			// not trigger the usual unblocking. Force unblocking may cause
+			// some values to be remain unevaluated.
+			if arc.withinLet {
+				arcState.process(needs, finalize)
+				if arc.ArcType == ArcPending {
+					arc.ArcType = ArcNotPresent
+				}
+			} else {
+				arcState.process(needs, yield)
+			}
 		}
 	}
 
-handleArcType:
 	switch arc.ArcType {
 	case ArcMember, ArcRequired:
 		return arcReturn