From 02d5751faa367c4aa6140ce7d580f803f0db6184 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Tue, 22 Oct 2024 10:58:01 +0200 Subject: [PATCH] internal/core/adt: reimplement close for v3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main reason for this is that the current mechanism is somewhat broken, as it creates an unitialized vertex with a closeContext that is not in the correct state. This may lead to an "already closed" panic if some of the evaluation strategy is changed. We now instead wrap using a Vertex that has the right closedness bits set and uses structure sharing to forward to the original node. We could have instead make the close() builtin to directly set the IsClosed flag in the CloseInfo, rather than the indirect way of setting NeedClose. But there are some paths, as through the Resolve function, that do now use scheduleValue to do the unwrapping. Fixing this avoids some panics with other changes down the line. This is also a first step of getting rid of the StructMarker and ListMarker types. Signed-off-by: Marcel van Lohuizen Change-Id: I7de920e24143391308ec5a4b1136feb06beadd0c Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202905 Reviewed-by: Daniel Martí TryBot-Result: CUEcueckoo --- cue/testdata/builtins/default.txtar | 24 ++++++++++ cue/testdata/cycle/evaluate.txtar | 46 ++++++++++--------- cue/testdata/cycle/inline_non_recursive.txtar | 16 +++---- internal/core/adt/closed.go | 3 +- internal/core/adt/composite.go | 13 ++++++ internal/core/adt/conjunct.go | 24 +++++----- internal/core/adt/expr.go | 3 ++ internal/core/adt/sched.go | 12 ++++- internal/core/adt/share.go | 4 +- internal/core/adt/tasks.go | 23 +++++++--- internal/core/adt/unify.go | 2 + internal/core/compile/builtin.go | 22 ++++++--- internal/core/export/expr.go | 5 ++ 13 files changed, 140 insertions(+), 57 deletions(-) diff --git a/cue/testdata/builtins/default.txtar b/cue/testdata/builtins/default.txtar index 0d2c6ca6de6..13801a7fa2d 100644 --- a/cue/testdata/builtins/default.txtar +++ b/cue/testdata/builtins/default.txtar @@ -45,6 +45,30 @@ Retain: 0 Unifications: 25 Conjuncts: 52 Disjuncts: 45 +-- out/evalalpha -- +(struct){ + Len: (int){ 3 } + Close: (#struct){ + } + And: (int){ 1 } + Or: (int){ 1 } + Div: (int){ 2 } + Mod: (int){ 1 } + Quo: (int){ 2 } + Rem: (int){ 1 } +} +-- diff/-out/evalalpha<==>+out/eval -- +diff old new +--- old ++++ new +@@ -1,6 +1,6 @@ + (struct){ + Len: (int){ 3 } +- Close: (struct){ ++ Close: (#struct){ + } + And: (int){ 1 } + Or: (int){ 1 } -- out/eval -- (struct){ Len: (int){ 3 } diff --git a/cue/testdata/cycle/evaluate.txtar b/cue/testdata/cycle/evaluate.txtar index 36d568e2a26..e0abf12413d 100644 --- a/cue/testdata/cycle/evaluate.txtar +++ b/cue/testdata/cycle/evaluate.txtar @@ -127,7 +127,6 @@ Conjuncts: 299 Disjuncts: 192 -- out/evalalpha -- Errors: -closeCycle.c: structural cycle printCycle.a.X: structural cycle structCycle.c: structural cycle letCycleOK.t2.a.X: structural cycle: @@ -139,6 +138,8 @@ disjunctionCycle.b: cannot use 1 (type int) as type list: ./in.cue:56:9 b: structural cycle: ./in.cue:62:6 +closeCycle.c: structural cycle: + ./in.cue:73:5 structural cycle: ./in.cue:85:12 listAddCycle.c: structural cycle: @@ -251,15 +252,17 @@ Result: } closeCycle: (_|_){ // [structural cycle] - a: ~(closeCycle.b) + a: (_|_){ + // [structural cycle] closeCycle.c: structural cycle: + // ./in.cue:73:5 + } b: (_|_){ - // [structural cycle] - d: (_|_){ - // [structural cycle] closeCycle.c: structural cycle - } + // [structural cycle] closeCycle.c: structural cycle: + // ./in.cue:73:5 } c: (_|_){ - // [structural cycle] closeCycle.c: structural cycle + // [structural cycle] closeCycle.c: structural cycle: + // ./in.cue:73:5 } } structCycle: (_|_){ @@ -350,7 +353,7 @@ Result: diff old new --- old +++ new -@@ -1,50 +1,49 @@ +@@ -1,50 +1,50 @@ Errors: -closeCycle.a: structural cycle -closeCycle.b.d: structural cycle @@ -365,7 +368,6 @@ diff old new -disjunctionCycle.a: cannot use 1 (type int) as type list: - ./in.cue:56:5 - ./in.cue:56:9 -+closeCycle.c: structural cycle +printCycle.a.X: structural cycle +structCycle.c: structural cycle +letCycleOK.t2.a.X: structural cycle: @@ -380,7 +382,7 @@ diff old new - ./in.cue:56:9 b: structural cycle: ./in.cue:62:6 --closeCycle.c: structural cycle: + closeCycle.c: structural cycle: - ./in.cue:73:15 -structCycle.c: structural cycle: - ./in.cue:79:14 @@ -388,6 +390,7 @@ diff old new - ./in.cue:85:11 -printCycle.a.X.X: structural cycle: - ./in.cue:113:6 ++ ./in.cue:73:5 +structural cycle: + ./in.cue:85:12 +listAddCycle.c: structural cycle: @@ -428,7 +431,7 @@ diff old new } } } -@@ -59,20 +58,16 @@ +@@ -59,20 +59,16 @@ // [structural cycle] letCycleFail.t1.a.X: structural cycle } c: (_|_){ @@ -459,7 +462,7 @@ diff old new } x: (struct){ y: (string){ "" } -@@ -88,17 +83,17 @@ +@@ -88,17 +84,17 @@ disjunctionCycle: (_|_){ // [eval] a: (_|_){ @@ -488,7 +491,7 @@ diff old new // ./in.cue:56:5 // ./in.cue:56:9 } -@@ -124,80 +119,77 @@ +@@ -124,80 +120,79 @@ } b: (struct){ } @@ -500,7 +503,7 @@ diff old new } closeCycle: (_|_){ // [structural cycle] -- a: (_|_){ + a: (_|_){ - // [structural cycle] closeCycle.a: structural cycle - } - b: (_|_){ @@ -512,15 +515,16 @@ diff old new - c: (_|_){ - // [structural cycle] closeCycle.c: structural cycle: - // ./in.cue:73:15 -+ a: ~(closeCycle.b) ++ // [structural cycle] closeCycle.c: structural cycle: ++ // ./in.cue:73:5 ++ } + b: (_|_){ -+ // [structural cycle] -+ d: (_|_){ -+ // [structural cycle] closeCycle.c: structural cycle -+ } ++ // [structural cycle] closeCycle.c: structural cycle: ++ // ./in.cue:73:5 + } + c: (_|_){ -+ // [structural cycle] closeCycle.c: structural cycle ++ // [structural cycle] closeCycle.c: structural cycle: ++ // ./in.cue:73:5 } } structCycle: (_|_){ @@ -618,7 +622,7 @@ diff old new } } closeFail: (_|_){ -@@ -207,21 +199,22 @@ +@@ -207,21 +202,22 @@ } x: (_|_){ // [eval] diff --git a/cue/testdata/cycle/inline_non_recursive.txtar b/cue/testdata/cycle/inline_non_recursive.txtar index 0a7ca8ad733..1dd380ae90f 100644 --- a/cue/testdata/cycle/inline_non_recursive.txtar +++ b/cue/testdata/cycle/inline_non_recursive.txtar @@ -80,14 +80,14 @@ issue3182: { } } -- out/evalalpha/stats -- -Leaks: 250 +Leaks: 272 Freed: 0 Reused: 0 -Allocs: 250 +Allocs: 272 Retain: 0 -Unifications: 240 -Conjuncts: 1277 +Unifications: 251 +Conjuncts: 1299 Disjuncts: 0 -- out/evalalpha -- Errors: @@ -203,17 +203,17 @@ diff old new -Reused: 401 -Allocs: 310 -Retain: 1066 -+Leaks: 250 ++Leaks: 272 +Freed: 0 +Reused: 0 -+Allocs: 250 ++Allocs: 272 +Retain: 0 -Unifications: 711 -Conjuncts: 2759 -Disjuncts: 1446 -+Unifications: 240 -+Conjuncts: 1277 ++Unifications: 251 ++Conjuncts: 1299 +Disjuncts: 0 -- diff/-out/evalalpha<==>+out/eval -- diff old new diff --git a/internal/core/adt/closed.go b/internal/core/adt/closed.go index 6c639ef084e..b11fedbbb58 100644 --- a/internal/core/adt/closed.go +++ b/internal/core/adt/closed.go @@ -330,9 +330,10 @@ func isClosed(v *Vertex) bool { // We could have used IsRecursivelyClosed here, but (effectively) // implementing it again here allows us to only have to iterate over // Structs once. - if v.ClosedRecursive { + if v.ClosedRecursive || v.ClosedNonRecursive { return true } + // TODO(evalv3): this can be removed once we delete the evalv2 code. for _, s := range v.Structs { if s.IsClosed || s.IsInOneOf(DefinitionSpan) { return true diff --git a/internal/core/adt/composite.go b/internal/core/adt/composite.go index 8eae8a41c52..255151d6e78 100644 --- a/internal/core/adt/composite.go +++ b/internal/core/adt/composite.go @@ -187,6 +187,10 @@ type Vertex struct { // of the conjuncts, or ancestor conjuncts, is a definition. ClosedRecursive bool + // ClosedNonRecursive indicates that this Vertex has been closed for this + // level only. This supports the close builtin. + ClosedNonRecursive bool + // HasEllipsis indicates that this Vertex is open by means of an ellipsis. // TODO: combine this field with Closed once we removed the old evaluator. HasEllipsis bool @@ -583,6 +587,15 @@ const ( finalized ) +// Wrap creates a Vertex that takes w as a shared value. This allows users +// to set different flags for a wrapped Vertex. +func (c *OpContext) Wrap(v *Vertex, id CloseInfo) *Vertex { + w := c.newInlineVertex(nil, nil, v.Conjuncts...) + n := w.getState(c) + n.share(makeAnonymousConjunct(nil, v, nil), v, CloseInfo{}) + return w +} + // Status returns the status of the current node. When reading the status, one // should always use this method over directly reading status field. // diff --git a/internal/core/adt/conjunct.go b/internal/core/adt/conjunct.go index b5adca0d5eb..faee627f2dd 100644 --- a/internal/core/adt/conjunct.go +++ b/internal/core/adt/conjunct.go @@ -552,20 +552,20 @@ func (n *nodeContext) insertValueConjunct(env *Environment, v Value, id CloseInf switch x := v.(type) { case *Vertex: - if m, ok := x.BaseValue.(*StructMarker); ok { + if x.ClosedNonRecursive { + n.node.ClosedNonRecursive = true + var cc *closeContext + id, cc = id.spawnCloseContext(n.ctx, 0) + cc.isClosedOnce = true + + if v, ok := x.BaseValue.(*Vertex); ok { + n.insertValueConjunct(env, v, id) + return + } + } + if _, ok := x.BaseValue.(*StructMarker); ok { n.aStruct = x n.aStructID = id - if m.NeedClose { - // TODO: In the new evaluator this is used to mark a struct - // as closed in the debug output. Once the old evaluator is - // gone, we could simplify this. - id.IsClosed = true - if ctx.isDevVersion() { - var cc *closeContext - id, cc = id.spawnCloseContext(n.ctx, 0) - cc.isClosedOnce = true - } - } } if !x.IsData() { diff --git a/internal/core/adt/expr.go b/internal/core/adt/expr.go index 23be3e612e5..0a2a5a8f1ac 100644 --- a/internal/core/adt/expr.go +++ b/internal/core/adt/expr.go @@ -421,6 +421,9 @@ func (x *ListMarker) node() {} type StructMarker struct { // NeedClose is used to signal that the evaluator should close this struct. // It is only set by the close builtin. + // TODO(evalv3: remove this field. Once we removed this, and also introduced + // open by default lists, we can get rid of StructMarker and ListMarker + // in its entirety in favor of using type bit masks. NeedClose bool } diff --git a/internal/core/adt/sched.go b/internal/core/adt/sched.go index f4f3186c0d3..0030ac478d9 100644 --- a/internal/core/adt/sched.go +++ b/internal/core/adt/sched.go @@ -703,7 +703,10 @@ func runTask(t *task, mode runMode) { defer ctx.popTask() if t.env != nil { id := t.id - id.cc = nil // this is done to avoid struct args from passing fields up. + // This is done to avoid struct args from passing fields up. + // Use [task.updateCI] to get the current CloseInfo with this field + // restored. + id.cc = nil s := ctx.PushConjunct(MakeConjunct(t.env, t.x, id)) defer ctx.PopState(s) } @@ -736,6 +739,13 @@ func runTask(t *task, mode runMode) { } } +// updateCI stitches back the closeContext that more removed from the CloseInfo +// before in the given CloseInfo. +func (t *task) updateCI(ci CloseInfo) CloseInfo { + ci.cc = t.id.cc + return ci +} + // waitFor blocks task t until the needs for scheduler s are met. func (t *task) waitFor(s *scheduler, needs condition) { if s.meets(needs) { diff --git a/internal/core/adt/share.go b/internal/core/adt/share.go index 9cf1d77f40a..6caa3f5437a 100644 --- a/internal/core/adt/share.go +++ b/internal/core/adt/share.go @@ -113,7 +113,9 @@ func (n *nodeContext) share(c Conjunct, arc *Vertex, id CloseInfo) { // have been processed and it is clear that sharing is possible. Delaying // such a count should not hurt performance, as a shared node is completed // anyway. - id.cc.incDependent(n.ctx, SHARED, n.node.cc()) + if id.cc != nil { + id.cc.incDependent(n.ctx, SHARED, n.node.cc()) + } } func (n *nodeContext) shareIfPossible(c Conjunct, arc *Vertex, id CloseInfo) bool { diff --git a/internal/core/adt/tasks.go b/internal/core/adt/tasks.go index 8779a9e4cc5..9301ba26a7d 100644 --- a/internal/core/adt/tasks.go +++ b/internal/core/adt/tasks.go @@ -90,8 +90,8 @@ func processExpr(ctx *OpContext, t *task, mode runMode) { // This is an optional cycle that we will ignore. return } - t.id.CycleInfo = ci.CycleInfo - t.node.insertValueConjunct(t.env, v, t.id) + ci = t.updateCI(ci) + t.node.insertValueConjunct(t.env, v, ci) } func processResolver(ctx *OpContext, t *task, mode runMode) { @@ -113,9 +113,11 @@ func processResolver(ctx *OpContext, t *task, mode runMode) { // TODO: consider moving after markCycle or removing. d := arc.DerefDisjunct() + ci := t.updateCI(ctx.ci) + // A reference that points to itself indicates equality. In that case // we are done computing and we can return the arc as is. - ci, skip := t.node.detectCycleV3(d, t.env, r, t.id) + ci, skip := t.node.detectCycleV3(d, t.env, r, ci) if skip { return } @@ -162,9 +164,14 @@ func processDynamic(ctx *OpContext, t *task, mode runMode) { return } - c := MakeConjunct(t.env, field, t.id) + // Do not update the CloseInfo, as we are passing the field value + // unevaluated. + ci := t.id + + c := MakeConjunct(t.env, field, ci) + // TODO(evalv3): this does not seem to be necessary and even treacherous. c.CloseInfo.cc = nil - n.insertArc(f, field.ArcType, c, t.id, true) + n.insertArc(f, field.ArcType, c, ci, true) } func processPatternConstraint(ctx *OpContext, t *task, mode runMode) { @@ -179,7 +186,11 @@ func processPatternConstraint(ctx *OpContext, t *task, mode runMode) { return } - n.insertPattern(v, MakeConjunct(t.env, t.x, t.id)) + // Do not update the CloseInfo, as we are passing the constraint value + // unevaluated. + ci := t.id + + n.insertPattern(v, MakeConjunct(t.env, t.x, ci)) } func processComprehension(ctx *OpContext, t *task, mode runMode) { diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go index bea759be906..69ce7db8078 100644 --- a/internal/core/adt/unify.go +++ b/internal/core/adt/unify.go @@ -318,6 +318,8 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool { if w.ClosedRecursive { v.ClosedRecursive = true } + // NOTE: setting ClosedNonRecursive is not necessary, as it is + // handled by scheduleValue. if w.HasEllipsis { v.HasEllipsis = true } diff --git a/internal/core/compile/builtin.go b/internal/core/compile/builtin.go index 7242946fade..79e5bbca477 100644 --- a/internal/core/compile/builtin.go +++ b/internal/core/compile/builtin.go @@ -16,6 +16,7 @@ package compile import ( "cuelang.org/go/cue/errors" + "cuelang.org/go/internal" "cuelang.org/go/internal/core/adt" ) @@ -81,19 +82,26 @@ var closeBuiltin = &adt.Builtin{ Name: "close", Params: []adt.Param{structParam}, Result: adt.StructKind, - // NonConcrete: true, // TODO: should probably be noncrete Func: func(c *adt.OpContext, args []adt.Value) adt.Expr { s, ok := args[0].(*adt.Vertex) if !ok { return c.NewErrf("struct argument must be concrete") } - if m, ok := s.BaseValue.(*adt.StructMarker); ok && m.NeedClose { - return s + var v *adt.Vertex + if c.Version == internal.DevVersion { + // TODO(evalv3) this is a rather convoluted and inefficient way to + // accomplish signaling vertex should be closed. In most cases, it + // would suffice to set IsClosed in the CloseInfo. However, that + // does not cover all code paths. Consider simplifying this. + v = c.Wrap(s, c.CloseInfo()) + v.ClosedNonRecursive = true + } else { + if m, ok := s.BaseValue.(*adt.StructMarker); ok && m.NeedClose { + return s + } + v = s.Clone() + v.BaseValue = &adt.StructMarker{NeedClose: true} } - v := s.Clone() - // TODO(perf): do not copy the arc, but rather find a way to mark the - // calling nodeContext. - v.BaseValue = &adt.StructMarker{NeedClose: true} return v }, } diff --git a/internal/core/export/expr.go b/internal/core/export/expr.go index 3eeefa1831f..14c00c31768 100644 --- a/internal/core/export/expr.go +++ b/internal/core/export/expr.go @@ -301,7 +301,12 @@ func (x *exporter) mergeValues(label adt.Feature, src *adt.Vertex, a []conjunct, func (e *conjuncts) wrapCloseIfNecessary(s *ast.StructLit, v *adt.Vertex) ast.Expr { if !e.hasEllipsis && v != nil { + if v.ClosedNonRecursive { + // Eval V3 logic + return ast.NewCall(ast.NewIdent("close"), s) + } if st, ok := v.BaseValue.(*adt.StructMarker); ok && st.NeedClose { + // Eval V2 logic return ast.NewCall(ast.NewIdent("close"), s) } }