From ae730eb72b426ee38e7535f87e0d84b5ac952b46 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Mon, 20 Jan 2025 19:45:31 +0100 Subject: [PATCH] internal/core/adt: break notification dependency on structural cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This case is specifically is not handled if structure sharing is off, or when a let is involved. Consider, for instance, this case: a: next: X let X = a Here, `a` is substituted for `X`, after which a structural cycle is detected. At this point, any notification resulting from X should be cancelled. Basically, cycle detection followed a different path, where this was not done. Signed-off-by: Marcel van Lohuizen Change-Id: Iba7a4399fb4d52d87c4ccfb3adba3bf79f14ca4b Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207520 TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine Reviewed-by: Daniel Martí --- internal/core/adt/conjunct.go | 4 ++++ internal/core/adt/eval_test.go | 5 +---- internal/core/adt/unify.go | 11 +++++++---- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/core/adt/conjunct.go b/internal/core/adt/conjunct.go index 856978f0a2c..5150f2b8e32 100644 --- a/internal/core/adt/conjunct.go +++ b/internal/core/adt/conjunct.go @@ -554,6 +554,10 @@ func (n *nodeContext) addNotify2(v *Vertex, c CloseInfo) { cc := c.cc + // TODO: it should not be necessary to register for notifications for + // let expressions, so we could also filter for !n.node.Label.IsLet(). + // However, somehow this appears to result in slightly better error + // messages. if root.addNotifyDependency(n.ctx, cc) { // TODO: this is mostly identical to the slice in the root closeContext. // Use only one once V2 is removed. diff --git a/internal/core/adt/eval_test.go b/internal/core/adt/eval_test.go index e47dd734f4b..8d46c92f24f 100644 --- a/internal/core/adt/eval_test.go +++ b/internal/core/adt/eval_test.go @@ -76,14 +76,11 @@ var needFix = map[string]string{ // counter errors. // TODO: These counters should all go to zero. var skipDebugDepErrors = map[string]int{ - "compile/scope": 1, - "comprehensions/pushdown": 2, "cycle/builtins": 2, "cycle/comprehension": 1, "cycle/disjunction": 4, - "cycle/evaluate": 1, "cycle/issue990": 1, - "cycle/structural": 9, + "cycle/structural": 7, "disjunctions/errors": 3, "disjunctions/elimination": 19, "disjunctions/nested": 1, diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go index aab9ee95cb4..df5be62c606 100644 --- a/internal/core/adt/unify.go +++ b/internal/core/adt/unify.go @@ -285,6 +285,7 @@ func (v *Vertex) unify(c *OpContext, needs condition, mode runMode) bool { case needs&subFieldsProcessed != 0: switch { case assertStructuralCycleV3(n): + n.breakIncomingDeps() // TODO: consider bailing on error if n.errs != nil. case n.completeAllArcs(needs, mode): } @@ -462,13 +463,15 @@ func (n *nodeContext) completeNodeTasks(mode runMode) { }() } - if p := v.Parent; p != nil && p.state != nil { - if !v.IsDynamic && n.completed&allAncestorsProcessed == 0 { - p.state.completeNodeTasks(mode) + if !v.Label.IsLet() { + if p := v.Parent; p != nil && p.state != nil { + if !v.IsDynamic && n.completed&allAncestorsProcessed == 0 { + p.state.completeNodeTasks(mode) + } } } - if v.IsDynamic || v.Parent.allChildConjunctsKnown() { + if v.IsDynamic || v.Label.IsLet() || v.Parent.allChildConjunctsKnown() { n.signal(allAncestorsProcessed) }