internal/core/adt: only set scalar if not already defined

Not doing so could result in a bug where an error
was erased. This would only be exposed by CLs that
will be submitted later.

Note that "cyclic" denotes the inprogress marker and
means "not set".

Issue #2169

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

Author: mpvl

cue/testdata/cycle/inline.txtar

@@ -153,15 +153,15 @@ inline: acrossFields: ok1: {
   }
 }
 -- out/eval/stats --
-Leaks:  268
-Freed:  168
-Reused: 162
-Allocs: 274
-Retain: 821
+Leaks:  247
+Freed:  141
+Reused: 136
+Allocs: 252
+Retain: 734
 
-Unifications: 436
-Conjuncts:    1499
-Disjuncts:    795
+Unifications: 388
+Conjuncts:    1297
+Disjuncts:    688
 -- out/eval --
 Errors:
 structural cycle:

internal/core/adt/eval.go

@@ -235,6 +235,7 @@ func (c *OpContext) Unify(v *Vertex, state VertexStatus) {
 		fallthrough
 
 	case Partial, Conjuncts:
+		// TODO: remove this optimization or make it correct.
 		// No need to do further processing when we have errors and all values
 		// have been considered.
 		// TODO: is checkClosed really still necessary here?
@@ -1348,10 +1349,9 @@ func (n *nodeContext) getValidators(state VertexStatus) BaseValue {
 
 // TODO: this function can probably go as this is now handled in the nodeContext.
 func (n *nodeContext) maybeSetCache() {
-	if n.node.Status() > Partial { // n.node.BaseValue != nil
-		return
-	}
-	if n.scalar != nil {
+	// Set BaseValue to scalar, but only if it was not set before. Most notably,
+	// errors should not be discarded.
+	if n.scalar != nil && isCyclePlaceholder(n.node.BaseValue) {
 		n.node.BaseValue = n.scalar
 	}
 	// NOTE: this is now handled by associating the nodeContext