internal/core/adt: remove direct access to BaseValue

This does not fix any tests, but it seems like a safe thing to do, as this was the source of various bugs. Also, ultimately, we probably want to move back to a real adt again, in which case all values should be accessible only through methods. Issue #2884 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: If5c0a9d11450b8d4b38c50f9be52edab282ccc95
cue-lang · May 8, 2024 · f125d3a · f125d3a
1 parent 9efad90
commit f125d3a
Show file tree

Hide file tree

Showing 13 changed files with 43 additions and 31 deletions.
diff --git a/cue/types.go b/cue/types.go
@@ -1182,7 +1182,7 @@ func (v Value) IsConcrete() bool {
 	if v.v == nil {
 		return false // any is neither concrete, not a list or struct.
 	}
-	if b, ok := v.v.BaseValue.(*adt.Bottom); ok {
+	if b := v.v.Bottom(); b != nil {
 		return !b.IsIncomplete()
 	}
 	if !adt.IsConcrete(v.v) {
@@ -1207,7 +1207,7 @@ func (v Value) Exists() bool {
 	if v.v == nil {
 		return false
 	}
-	if err, ok := v.v.BaseValue.(*adt.Bottom); ok {
+	if err := v.v.Bottom(); err != nil {
 		return !err.NotExists
 	}
 	return true
@@ -1412,8 +1412,8 @@ func (v Value) structValOpts(ctx *adt.OpContext, o options) (s structValue, err
 
 	obj := v.v
 
-	switch b, ok := v.v.BaseValue.(*adt.Bottom); {
-	case ok && b.IsIncomplete() && !o.concrete && !o.final:
+	switch b := v.v.Bottom(); {
+	case b != nil && b.IsIncomplete() && !o.concrete && !o.final:
 
 	// Allow scalar values if hidden or definition fields are requested.
 	case !o.omitHidden, !o.omitDefinitions:

diff --git a/cue/types_test.go b/cue/types_test.go
@@ -3115,7 +3115,7 @@ func TestWalk(t *testing.T) {
 				case ListKind:
 					buf = append(buf, '[')
 				default:
-					if b, _ := v.v.BaseValue.(*adt.Bottom); b != nil {
+					if b := v.v.Bottom(); b != nil {
 						s := debugStr(v.ctx(), b)
 						buf = append(buf, fmt.Sprint(s, ",")...)
 						return true

diff --git a/internal/core/adt/composite.go b/internal/core/adt/composite.go
@@ -557,7 +557,7 @@ func (v *Vertex) IsUnprocessed() bool {
 
 func (v *Vertex) updateStatus(s vertexStatus) {
 	if !isCyclePlaceholder(v.BaseValue) {
-		if _, ok := v.BaseValue.(*Bottom); !ok && v.state != nil {
+		if !v.IsErr() && v.state != nil {
 			Assertf(v.state.ctx, v.status <= s+1, "attempt to regress status from %d to %d", v.Status(), s)
 		}
 	}
@@ -780,17 +780,26 @@ func toDataAll(ctx *OpContext, v BaseValue) BaseValue {
 // 	return v.Value == cycle
 // }
 
+// IsErr is a convenience function to check whether a Vertex represents an
+// error currently. It does not finalize the value, so it is possible that
+// v may become erroneous after this call.
 func (v *Vertex) IsErr() bool {
 	// if v.Status() > Evaluating {
-	if _, ok := v.BaseValue.(*Bottom); ok {
-		return true
-	}
-	// }
-	return false
+	return v.Bottom() != nil
 }
 
+// Err finalizes v, if it isn't yet, and returns an error if v evaluates to an
+// error or nil otherwise.
 func (v *Vertex) Err(c *OpContext) *Bottom {
 	v.Finalize(c)
+	return v.Bottom()
+}
+
+// Bottom reports whether v is currently erroneous It does not finalize the
+// value, so it is possible that v may become erroneous after this call.
+func (v *Vertex) Bottom() *Bottom {
+	// TODO: should we consider errors recorded in the state?
+	v = v.DerefValue()
 	if b, ok := v.BaseValue.(*Bottom); ok {
 		return b
 	}

diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go
@@ -667,7 +667,7 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) {
 		c.errs = CombineErrors(c.src, c.errs, err)
 
 		if v, ok := result.(*Vertex); ok {
-			if b, _ := v.BaseValue.(*Bottom); b != nil {
+			if b := v.Bottom(); b != nil {
 				switch b.Code {
 				case IncompleteError:
 				case CycleError:
@@ -803,7 +803,7 @@ func (c *OpContext) unifyNode(v Expr, state combinedFlags) (result Value) {
 		c.errs = CombineErrors(c.src, c.errs, err)
 
 		if v, ok := result.(*Vertex); ok {
-			if b, _ := v.BaseValue.(*Bottom); b != nil && !b.IsIncomplete() {
+			if b := v.Bottom(); b != nil && !b.IsIncomplete() {
 				result = b
 			}
 		}

diff --git a/internal/core/adt/disjunct2.go b/internal/core/adt/disjunct2.go
@@ -496,7 +496,7 @@ func (n *nodeContext) finalizeDisjunctions() {
 }
 
 func (n *nodeContext) getError() *Bottom {
-	if b, ok := n.node.BaseValue.(*Bottom); ok && !isCyclePlaceholder(b) {
+	if b := n.node.Bottom(); b != nil && !isCyclePlaceholder(b) {
 		return b
 	}
 	if n.node.ChildErrors != nil {

diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go
@@ -133,7 +133,7 @@ func isIncomplete(v *Vertex) bool {
 	if v == nil {
 		return true
 	}
-	if b, ok := v.BaseValue.(*Bottom); ok {
+	if b := v.Bottom(); b != nil {
 		return b.IsIncomplete()
 	}
 	return false

diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go
@@ -353,7 +353,7 @@ func (c *OpContext) unify(v *Vertex, flags combinedFlags) {
 		// completing all disjunctions.
 		if !n.done() {
 			if err := n.incompleteErrors(true); err != nil {
-				b, _ := n.node.BaseValue.(*Bottom)
+				b := n.node.Bottom()
 				if b != err {
 					err = CombineErrors(n.ctx.src, b, err)
 				}
@@ -459,7 +459,7 @@ func (n *nodeContext) doNotify() {
 	for _, rec := range n.notify {
 		v := rec.v
 		if v.state == nil {
-			if b, ok := v.BaseValue.(*Bottom); ok {
+			if b := v.Bottom(); b != nil {
 				v.BaseValue = CombineErrors(nil, b, n.errs)
 			} else {
 				v.BaseValue = n.errs
@@ -822,7 +822,7 @@ func (n *nodeContext) completeArcs(state vertexStatus) {
 					state = conjuncts
 				}
 
-				if err, _ := a.BaseValue.(*Bottom); err != nil {
+				if err := a.Bottom(); err != nil {
 					n.node.AddChildError(err)
 				}
 			}
@@ -848,7 +848,7 @@ func (n *nodeContext) completeArcs(state vertexStatus) {
 				// faulty CUE using this mechanism, though. At most error
 				// messages are a bit unintuitive. This may change once we have
 				// functionality to reflect on types.
-				if _, ok := n.node.BaseValue.(*Bottom); !ok {
+				if !n.node.IsErr() {
 					n.node.BaseValue = &StructMarker{}
 					n.kind = StructKind
 				}
@@ -920,6 +920,10 @@ var cycle = &Bottom{
 }
 
 func isCyclePlaceholder(v BaseValue) bool {
+	// TODO: do not mark cycle in BaseValue.
+	if a, _ := v.(*Vertex); a != nil {
+		v = a.DerefValue().BaseValue
+	}
 	return v == cycle
 }
 

diff --git a/internal/core/adt/export_test.go b/internal/core/adt/export_test.go
@@ -48,12 +48,12 @@ func NewFieldTester(r Runtime) *FieldTester {
 }
 
 func (x *FieldTester) Error() string {
-	if b, ok := x.n.node.BaseValue.(*Bottom); ok && b.Err != nil {
+	if b := x.n.node.Bottom(); b != nil && b.Err != nil {
 		return b.Err.Error()
 	}
 	var errs []string
 	for _, a := range x.n.node.Arcs {
-		if b, ok := a.BaseValue.(*Bottom); ok && b.Err != nil {
+		if b := a.Bottom(); b != nil && b.Err != nil {
 			errs = append(errs, b.Err.Error())
 		}
 	}

diff --git a/internal/core/adt/expr.go b/internal/core/adt/expr.go
@@ -909,8 +909,8 @@ func (x *LetReference) resolve(ctx *OpContext, state combinedFlags) *Vertex {
 	// We can reevaluate this once we have redone some of the planned order of
 	// evaluation work.
 	arc.Finalize(ctx)
-	b, ok := arc.BaseValue.(*Bottom)
-	if !arc.MultiLet && !ok {
+	b := arc.Bottom()
+	if !arc.MultiLet && b == nil {
 		return arc
 	}
 

diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go
@@ -256,7 +256,7 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool {
 
 	if err := n.getErr(); err != nil {
 		n.errs = nil
-		if b, _ := n.node.BaseValue.(*Bottom); b != nil {
+		if b := n.node.Bottom(); b != nil {
 			err = CombineErrors(nil, b, err)
 		}
 		n.node.BaseValue = err
@@ -432,8 +432,7 @@ func (n *nodeContext) completeNodeTasks(mode runMode) {
 func (n *nodeContext) updateScalar() {
 	// Set BaseValue to scalar, but only if it was not set before. Most notably,
 	// errors should not be discarded.
-	_, isErr := n.node.BaseValue.(*Bottom)
-	if n.scalar != nil && (!isErr || isCyclePlaceholder(n.node.BaseValue)) {
+	if n.scalar != nil && (!n.node.IsErr() || isCyclePlaceholder(n.node.BaseValue)) {
 		n.node.BaseValue = n.scalar
 		n.signal(scalarKnown)
 	}
@@ -520,7 +519,7 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool {
 		// complete accordingly.
 		if !a.Label.IsLet() && a.ArcType <= ArcRequired {
 			a := a.DerefValue()
-			if err, _ := a.BaseValue.(*Bottom); err != nil {
+			if err := a.Bottom(); err != nil {
 				n.node.AddChildError(err)
 			}
 			success = true // other arcs are irrelevant

diff --git a/internal/core/export/expr.go b/internal/core/export/expr.go
@@ -418,7 +418,7 @@ func (e *conjuncts) addExpr(env *adt.Environment, src *adt.Vertex, x adt.Elem, i
 			e.addValueConjunct(src, env, x)
 
 		case *adt.Vertex:
-			if b, ok := v.BaseValue.(*adt.Bottom); ok {
+			if b := v.Bottom(); b != nil {
 				if !b.IsIncomplete() || e.cfg.Final {
 					e.addExpr(env, v, b, false)
 					return

diff --git a/internal/core/subsume/vertex.go b/internal/core/subsume/vertex.go
@@ -43,7 +43,7 @@ func (s *subsumer) vertices(x, y *adt.Vertex) bool {
 		y = y.Default()
 	}
 
-	if b, _ := y.BaseValue.(*adt.Bottom); b != nil {
+	if b := y.Bottom(); b != nil {
 		// If the value is incomplete, the error is not final. So either check
 		// structural equivalence or return an error.
 		return !b.IsIncomplete()
@@ -250,7 +250,7 @@ func (s *subsumer) verticesDev(x, y *adt.Vertex) bool {
 		y = y.Default()
 	}
 
-	if b, _ := y.BaseValue.(*adt.Bottom); b != nil {
+	if b := y.Bottom(); b != nil {
 		// If the value is incomplete, the error is not final. So either check
 		// structural equivalence or return an error.
 		return !b.IsIncomplete()

diff --git a/internal/core/validate/validate.go b/internal/core/validate/validate.go
@@ -74,7 +74,7 @@ func (v *validator) validate(x *adt.Vertex) {
 	// values. This prevents us from processing structure-shared nodes more than
 	// once and prevents potential cycles.
 	x = x.DerefNonRooted()
-	if b, _ := x.BaseValue.(*adt.Bottom); b != nil {
+	if b := x.Bottom(); b != nil {
 		switch b.Code {
 		case adt.CycleError:
 			if v.checkConcrete() || v.DisallowCycles {