From abb6364b9a0277289e8edb610ad7c6f4974259f1 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Fri, 6 Sep 2024 10:34:19 +0200 Subject: [PATCH] internal/core/adt: use setBaseValue to set Vertex.BaseValue Having a single point of entry through which BaseValue is set helps debugging. evalv2 also needed to update the status while setting this value. This is no longer necessary for evalv3. To keep things simple, we only ensure that for evalv3 code uses this entry point. Note that the method is defined on nodeContext. This additionally enforces that a modification can only be made while a Vertex is actively being evaluated. Signed-off-by: Marcel van Lohuizen Change-Id: I3f10a1e47309a2808f873806c7625dc7c7012247 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1200791 Reviewed-by: Matthew Sackman TryBot-Result: CUEcueckoo --- internal/core/adt/composite.go | 12 ++++++++++++ internal/core/adt/context.go | 2 +- internal/core/adt/cycle.go | 4 ++-- internal/core/adt/disjunct.go | 2 +- internal/core/adt/disjunct2.go | 8 +++----- internal/core/adt/errors.go | 9 +++++---- internal/core/adt/eval.go | 2 +- internal/core/adt/tasks.go | 2 +- internal/core/adt/unify.go | 6 +++--- 9 files changed, 29 insertions(+), 18 deletions(-) diff --git a/internal/core/adt/composite.go b/internal/core/adt/composite.go index b7db4fd1ff8..8c8cd76eaef 100644 --- a/internal/core/adt/composite.go +++ b/internal/core/adt/composite.go @@ -851,6 +851,18 @@ func (v *Vertex) setValue(ctx *OpContext, state vertexStatus, value BaseValue) * return nil } +func (n *nodeContext) setBaseValue(value BaseValue) { + n.node.BaseValue = value +} + +// swapBaseValue swaps the BaseValue of a node with the given value and returns +// the previous value. +func (n *nodeContext) swapBaseValue(value BaseValue) (saved BaseValue) { + saved = n.node.BaseValue + n.setBaseValue(value) + return saved +} + // ToVertex wraps v in a new Vertex, if necessary. func ToVertex(v Value) *Vertex { switch x := v.(type) { diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go index 1e5b0b4f161..ceae3bab11d 100644 --- a/internal/core/adt/context.go +++ b/internal/core/adt/context.go @@ -753,7 +753,7 @@ func (c *OpContext) evalState(v Expr, state combinedFlags) (result Value) { } err := c.Newf("cycle with field %v", x) b := &Bottom{Code: CycleError, Err: err} - v.setValue(c, v.status, b) + s.setBaseValue(b) return b // TODO: use this instead, as is usual for incomplete errors, // and also move this block one scope up to also apply to diff --git a/internal/core/adt/cycle.go b/internal/core/adt/cycle.go index c22c535e2db..e6966b97bd8 100644 --- a/internal/core/adt/cycle.go +++ b/internal/core/adt/cycle.go @@ -611,14 +611,14 @@ func assertStructuralCycle(n *nodeContext) bool { } func (n *nodeContext) reportCycleError() { - n.node.BaseValue = CombineErrors(nil, + n.setBaseValue(CombineErrors(nil, n.node.Value(), &Bottom{ Code: StructuralCycleError, Err: n.ctx.Newf("structural cycle"), Value: n.node.Value(), // TODO: probably, this should have the referenced arc. - }) + })) n.node.Arcs = nil } diff --git a/internal/core/adt/disjunct.go b/internal/core/adt/disjunct.go index ee9cc0e5176..835f1d1002a 100644 --- a/internal/core/adt/disjunct.go +++ b/internal/core/adt/disjunct.go @@ -210,7 +210,7 @@ func (n *nodeContext) expandDisjuncts( n.disjuncts = append(n.disjuncts, n) } if n.node.BaseValue == nil { - n.node.BaseValue = n.getValidators(state) + n.setBaseValue(n.getValidators(state)) } n.usedDefault = append(n.usedDefault, defaultInfo{ diff --git a/internal/core/adt/disjunct2.go b/internal/core/adt/disjunct2.go index b8862592105..63e48989bb9 100644 --- a/internal/core/adt/disjunct2.go +++ b/internal/core/adt/disjunct2.go @@ -317,7 +317,7 @@ func (n *nodeContext) processDisjunctions() *Bottom { case 1: d := cross[0].node - n.node.BaseValue = d + n.setBaseValue(d) n.defaultMode = cross[0].defaultMode default: @@ -419,9 +419,7 @@ func (n *nodeContext) doDisjunct(c Conjunct, m defaultMode, mode runMode) (*node v := d.node - saved := n.node.BaseValue - n.node.BaseValue = v - defer func() { n.node.BaseValue = saved }() + defer n.setBaseValue(n.swapBaseValue(v)) // Clear relevant scheduler states. // TODO: do something more principled: just ensure that a node that has @@ -487,7 +485,7 @@ func (n *nodeContext) finalizeDisjunctions() { } v := n.node - v.BaseValue = d + n.setBaseValue(d) // The conjuncts will have too much information. Better have no // information than incorrect information. diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go index 44ef238041c..8a77e9b4855 100644 --- a/internal/core/adt/errors.go +++ b/internal/core/adt/errors.go @@ -131,7 +131,8 @@ func isIncomplete(v *Vertex) bool { // // If x is not already an error, the value is recorded in the error for // reference. -func (v *Vertex) AddChildError(recursive *Bottom) { +func (n *nodeContext) AddChildError(recursive *Bottom) { + v := n.node v.ChildErrors = CombineErrors(nil, v.ChildErrors, recursive) if recursive.IsIncomplete() { return @@ -139,13 +140,13 @@ func (v *Vertex) AddChildError(recursive *Bottom) { x := v.BaseValue err, _ := x.(*Bottom) if err == nil { - v.BaseValue = &Bottom{ + n.setBaseValue(&Bottom{ Code: recursive.Code, Value: v, HasRecursive: true, ChildError: true, Err: recursive.Err, - } + }) return } @@ -154,7 +155,7 @@ func (v *Vertex) AddChildError(recursive *Bottom) { err.Code = recursive.Code } - v.BaseValue = err + n.setBaseValue(err) } // CombineErrors combines two errors that originate at the same Vertex. diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go index 9a50a09e168..67e379fd98a 100644 --- a/internal/core/adt/eval.go +++ b/internal/core/adt/eval.go @@ -823,7 +823,7 @@ func (n *nodeContext) completeArcs(state vertexStatus) { } if err := a.Bottom(); err != nil { - n.node.AddChildError(err) + n.AddChildError(err) } } diff --git a/internal/core/adt/tasks.go b/internal/core/adt/tasks.go index 00412de8d94..49c2bcd06d7 100644 --- a/internal/core/adt/tasks.go +++ b/internal/core/adt/tasks.go @@ -332,7 +332,7 @@ func (n *nodeContext) updateListType(list Expr, id CloseInfo, isClosed bool, ell m = &ListMarker{ IsOpen: true, } - n.node.setValue(n.ctx, conjuncts, m) + n.setBaseValue(m) } m.IsOpen = m.IsOpen && !isClosed diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go index 65bdd21630e..056bcf9a46c 100644 --- a/internal/core/adt/unify.go +++ b/internal/core/adt/unify.go @@ -260,7 +260,7 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool { if b := n.node.Bottom(); b != nil { err = CombineErrors(nil, b, err) } - n.node.BaseValue = err + n.setBaseValue(err) } if mode == attemptOnly { @@ -434,7 +434,7 @@ func (n *nodeContext) updateScalar() { // Set BaseValue to scalar, but only if it was not set before. Most notably, // errors should not be discarded. if n.scalar != nil && (!n.node.IsErr() || isCyclePlaceholder(n.node.BaseValue)) { - n.node.BaseValue = n.scalar + n.setBaseValue(n.scalar) n.signal(scalarKnown) } } @@ -521,7 +521,7 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool { if !a.Label.IsLet() && a.ArcType <= ArcRequired { a := a.DerefValue() if err := a.Bottom(); err != nil { - n.node.AddChildError(err) + n.AddChildError(err) } success = true // other arcs are irrelevant }