From 2caf96eee335ea6737ad8750b70dee1a3f969cd5 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 Dispatch-Trailer: {"type":"trybot","CL":1200791,"patchset":2,"ref":"refs/changes/91/1200791/2","targetBranch":"master"} --- 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 b7db4fd1f..8c8cd76ea 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 1e5b0b4f1..ceae3bab1 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 c22c535e2..e6966b97b 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 ee9cc0e51..835f1d100 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 b88625921..63e48989b 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 44ef23804..8a77e9b48 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 9a50a09e1..67e379fd9 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 00412de8d..49c2bcd06 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 65bdd2163..056bcf9a4 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 }