From defe105c1c8b2db57848cebdfc72d69fead243a3 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Tue, 24 Dec 2024 11:11:30 +0100 Subject: [PATCH] internal/core/adt: fix self reference issue Evaluating comprehension sources was done using "yield" mode. This caused sets to be evaluated out of band, causing structs to be closed before fields can be added. We now, instead, rely on the recursive call-by-need behavior in combination with the freezing mechanism. In the future we plan to move to a scheduler that relies fully on either call by need or scheduling, but for now this will have to do. comprehension.txtar: fixes a P0 bug freeze.txtar: fixes the target issue. It would also fix the comprehension.t1.ok tests if we use "attemptOnly" instead of "finalize". But this breaks issue 3616. It is either way unclear what the correct behavior should be and V2 also errors here, so it is not the worst. This change would also fix a possible issue with propagating up an incomplete error. Also here the behavior is the same as V2, so no problem for now. adt.txtar: the changes are benign and due to reordering. Fixes #3178 Signed-off-by: Marcel van Lohuizen Change-Id: I94e49767d59a5d0c13f9ebb2ce8882f8c7a68051 Dispatch-Trailer: {"type":"trybot","CL":1206318,"patchset":2,"ref":"refs/changes/18/1206318/2","targetBranch":"master"} --- cue/testdata/cycle/comprehension.txtar | 161 ++++---------- cue/testdata/cycle/freeze.txtar | 44 +--- internal/core/adt/expr.go | 21 +- internal/core/export/testdata/main/adt.txtar | 218 +++++++++++++++++++ 4 files changed, 286 insertions(+), 158 deletions(-) diff --git a/cue/testdata/cycle/comprehension.txtar b/cue/testdata/cycle/comprehension.txtar index 924e30668..c4cbb172a 100644 --- a/cue/testdata/cycle/comprehension.txtar +++ b/cue/testdata/cycle/comprehension.txtar @@ -310,23 +310,19 @@ issue2367: { } -- out/evalalpha/stats -- -Leaks: 778 +Leaks: 782 Freed: 63 Reused: 63 -Allocs: 778 +Allocs: 782 Retain: 0 -Unifications: 498 -Conjuncts: 2977 +Unifications: 502 +Conjuncts: 3031 Disjuncts: 196 -- out/evalalpha -- Errors: selfReferential.insertionError.A: adding field foo3 not allowed as field set was already referenced: ./in.cue:117:14 -siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: - ./in.cue:277:21 -siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: - ./in.cue:292:21 Result: (_|_){ @@ -651,8 +647,7 @@ Result: } } } - siblingInsertion: (_|_){ - // [eval] + siblingInsertion: (struct){ t1: (struct){ p1: (struct){ D: (struct){ @@ -685,28 +680,35 @@ Result: } } } - t2: (_|_){ - // [eval] - p1: (_|_){ - // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: - // ./in.cue:277:21 + t2: (struct){ + p1: (struct){ D: (struct){ logging: (_){ _ } } - deployment: (_|_){ - // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: - // ./in.cue:277:21 + deployment: (struct){ + logging: (struct){ + env2: (struct){ + ENV: (string){ "True" } + } + env: (struct){ + ENV: (string){ "True" } + } + } } } - p2: (_|_){ - // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: - // ./in.cue:292:21 + p2: (struct){ D: (struct){ logging: (_){ _ } } - deployment: (_|_){ - // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: - // ./in.cue:292:21 + deployment: (struct){ + logging: (struct){ + env2: (struct){ + ENV: (string){ "True" } + } + env: (struct){ + ENV: (string){ "True" } + } + } } } } @@ -719,19 +721,15 @@ Result: diff old new --- old +++ new -@@ -1,5 +1,10 @@ +@@ -1,5 +1,6 @@ Errors: -selfReferential.insertionError.A: field foo3 not allowed by earlier comprehension or reference cycle +selfReferential.insertionError.A: adding field foo3 not allowed as field set was already referenced: + ./in.cue:117:14 -+siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: -+ ./in.cue:277:21 -+siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: -+ ./in.cue:292:21 Result: (_|_){ -@@ -19,7 +24,9 @@ +@@ -19,7 +20,9 @@ B: (struct){ a: (struct){ parent: (string){ "" } @@ -742,7 +740,7 @@ diff old new } } } -@@ -52,17 +59,12 @@ +@@ -52,17 +55,12 @@ } } _e: (#struct){ @@ -766,7 +764,7 @@ diff old new } } e: (#struct){ -@@ -76,12 +78,7 @@ +@@ -76,12 +74,7 @@ } f2: (#struct){ a: (#struct){ @@ -780,7 +778,7 @@ diff old new } } } -@@ -127,9 +124,9 @@ +@@ -127,9 +120,9 @@ insertionError: (_|_){ // [eval] A: (_|_){ @@ -792,7 +790,7 @@ diff old new } } acrossOr: (struct){ -@@ -254,27 +251,31 @@ +@@ -254,27 +247,31 @@ issue1881: (struct){ p1: (struct){ o: (#struct){ @@ -845,7 +843,7 @@ diff old new }) } } p2: (struct){ -@@ -290,7 +291,7 @@ +@@ -290,7 +287,7 @@ } } o: (#struct){ @@ -854,90 +852,15 @@ diff old new output: (#struct){ reject: (string){ "ok" } } -@@ -320,15 +321,16 @@ +@@ -320,7 +317,7 @@ resource: (string){ string } }) } o: (#struct){ - retry: (#struct){ -- output: (#struct){ -- reject: (string){ "ok" } -- } -- } -- } -- } -- } -- siblingInsertion: (struct){ + retry: (struct){ -+ output: (#struct){ -+ reject: (string){ "ok" } -+ } -+ } -+ } -+ } -+ } -+ siblingInsertion: (_|_){ -+ // [eval] - t1: (struct){ - p1: (struct){ - D: (struct){ -@@ -361,35 +363,28 @@ - } - } - } -- t2: (struct){ -- p1: (struct){ -- D: (struct){ -- logging: (_){ _ } -- } -- deployment: (struct){ -- logging: (struct){ -- env2: (struct){ -- ENV: (string){ "True" } -- } -- env: (struct){ -- ENV: (string){ "True" } -- } -- } -- } -- } -- p2: (struct){ -- D: (struct){ -- logging: (_){ _ } -- } -- deployment: (struct){ -- logging: (struct){ -- env2: (struct){ -- ENV: (string){ "True" } -- } -- env: (struct){ -- ENV: (string){ "True" } -- } -- } -+ t2: (_|_){ -+ // [eval] -+ p1: (_|_){ -+ // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: -+ // ./in.cue:277:21 -+ D: (struct){ -+ logging: (_){ _ } -+ } -+ deployment: (_|_){ -+ // [eval] siblingInsertion.t2.p1.deployment: adding field logging not allowed as field set was already referenced: -+ // ./in.cue:277:21 -+ } -+ } -+ p2: (_|_){ -+ // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: -+ // ./in.cue:292:21 -+ D: (struct){ -+ logging: (_){ _ } -+ } -+ deployment: (_|_){ -+ // [eval] siblingInsertion.t2.p2.deployment: adding field logging not allowed as field set was already referenced: -+ // ./in.cue:292:21 - } - } - } + output: (#struct){ + reject: (string){ "ok" } + } -- diff/-out/evalalpha/stats<==>+out/eval/stats -- diff old new --- old @@ -948,17 +871,17 @@ diff old new -Reused: 1260 -Allocs: 60 -Retain: 145 -+Leaks: 778 ++Leaks: 782 +Freed: 63 +Reused: 63 -+Allocs: 778 ++Allocs: 782 +Retain: 0 -Unifications: 832 -Conjuncts: 2525 -Disjuncts: 1404 -+Unifications: 498 -+Conjuncts: 2977 ++Unifications: 502 ++Conjuncts: 3031 +Disjuncts: 196 -- out/eval/stats -- Leaks: 50 @@ -972,8 +895,6 @@ Conjuncts: 2525 Disjuncts: 1404 -- diff/explanation -- B.a.children: now correctly marked as incomplete --- diff/todo/p0 -- -siblingInsertion.t2: errors -- diff/todo/p2 -- issue1881.#AllOutputs.retry: additional entry. Might be error in practice and okay. diff --git a/cue/testdata/cycle/freeze.txtar b/cue/testdata/cycle/freeze.txtar index 3997ee7e7..3f34ede6f 100644 --- a/cue/testdata/cycle/freeze.txtar +++ b/cue/testdata/cycle/freeze.txtar @@ -158,8 +158,6 @@ comprehension.t3.err.a: adding field xq not allowed as field set was already ref ./comprehension.cue:82:13 comprehension.moreSpecific.err.a.x: adding field z not allowed as field set was already referenced: ./comprehension.cue:114:9 -issue3178.output: adding field Value not allowed as field set was already referenced: - ./issue3178.cue:9:23 Result: (_|_){ @@ -295,15 +293,12 @@ Result: } } } - issue3178: (_|_){ - // [eval] issue3178.output: adding field Value not allowed as field set was already referenced: - // ./issue3178.cue:9:23 + issue3178: (struct){ input: (#list){ 0: (string){ "Value" } } - output: (_|_){ - // [eval] issue3178.output: adding field Value not allowed as field set was already referenced: - // ./issue3178.cue:9:23 + output: (struct){ + Value: (bool){ true } } } } @@ -311,7 +306,7 @@ Result: diff old new --- old +++ new -@@ -1,11 +1,12 @@ +@@ -1,11 +1,10 @@ Errors: -comprehension.moreSpecific.err.a: field z not allowed by earlier comprehension or reference cycle -comprehension.t1.ok.p0.x: field z not allowed by earlier comprehension or reference cycle @@ -326,12 +321,10 @@ diff old new + ./comprehension.cue:82:13 +comprehension.moreSpecific.err.a.x: adding field z not allowed as field set was already referenced: + ./comprehension.cue:114:9 -+issue3178.output: adding field Value not allowed as field set was already referenced: -+ ./issue3178.cue:9:23 Result: (_|_){ -@@ -12,73 +13,65 @@ +@@ -12,73 +11,65 @@ // [eval] comprehension: (_|_){ // [eval] @@ -455,7 +448,7 @@ diff old new } } } -@@ -89,9 +82,9 @@ +@@ -89,9 +80,9 @@ err: (_|_){ // [eval] a: (_|_){ @@ -468,7 +461,7 @@ diff old new } } } -@@ -99,16 +92,14 @@ +@@ -99,16 +90,14 @@ // [eval] err: (_|_){ // [eval] @@ -491,7 +484,7 @@ diff old new } } } -@@ -132,10 +123,10 @@ +@@ -132,10 +121,10 @@ err: (_|_){ // [eval] a: (_|_){ @@ -504,27 +497,6 @@ diff old new } } } -@@ -151,12 +142,15 @@ - } - } - } -- issue3178: (struct){ -+ issue3178: (_|_){ -+ // [eval] issue3178.output: adding field Value not allowed as field set was already referenced: -+ // ./issue3178.cue:9:23 - input: (#list){ - 0: (string){ "Value" } - } -- output: (struct){ -- Value: (bool){ true } -+ output: (_|_){ -+ // [eval] issue3178.output: adding field Value not allowed as field set was already referenced: -+ // ./issue3178.cue:9:23 - } - } - } --- diff/todo/p3 -- -Error path location could improve. -- diff/explanation -- v0.7 fixes bugs in v0.6: - t1 should be incomplete error, as it is fixable. diff --git a/internal/core/adt/expr.go b/internal/core/adt/expr.go index 09d8ea90e..97a58a1ca 100644 --- a/internal/core/adt/expr.go +++ b/internal/core/adt/expr.go @@ -2026,7 +2026,13 @@ func (c *OpContext) forSource(x Expr) *Vertex { node, ok := v.(*Vertex) if ok && c.isDevVersion() { - node.unify(c, state.conditions(), yield) + // We do not request to "yield" here, but rather rely on the + // call-by-need behavior in combination with the freezing mechanism. + // TODO: this seems a bit fragile. At some point we need to make this + // more robust by moving to a pure call-by-need mechanism, for instance. + // TODO: using attemptOnly here will remove the cyclic reference error + // of comprehension.t1.ok (which also errors in V2), + node.unify(c, state.conditions(), finalize) } v, ok = c.getDefault(v) @@ -2064,7 +2070,7 @@ func (c *OpContext) forSource(x Expr) *Vertex { } default: - if kind := v.Kind(); kind&StructKind != 0 { + if kind := v.Kind(); kind&(StructKind|ListKind) != 0 { c.addErrf(IncompleteError, pos(x), "cannot range over %s (incomplete type %s)", x, kind) return emptyNode @@ -2076,6 +2082,17 @@ func (c *OpContext) forSource(x Expr) *Vertex { return emptyNode } } + if c.isDevVersion() { + kind := v.Kind() + // At this point it is possible that the Vertex represents an incomplete + // struct or list, which is the case if it may be struct or list, but + // is also at least some other type, such as is the case with top. + if kind&(StructKind|ListKind) != 0 && kind != StructKind && kind != ListKind { + c.addErrf(IncompleteError, pos(x), + "cannot range over %s (incomplete type %s)", x, kind) + return emptyNode + } + } return node } diff --git a/internal/core/export/testdata/main/adt.txtar b/internal/core/export/testdata/main/adt.txtar index 941e4c6d7..3832c035f 100644 --- a/internal/core/export/testdata/main/adt.txtar +++ b/internal/core/export/testdata/main/adt.txtar @@ -253,6 +253,107 @@ errorListDef: { } -- diff/doc/explanation -- Reordering. +-- out/doc-v3 -- +[] +[p1] +[d1] +[d1 foobar] +[bar] +[d2] +[d2 foobar] +[d2 foobar name] +[d2 foobar foo] +[a] +- Issue #1910 + +[comp] +[comp bar] +[bytes] +[c1] +[s1] +[l1] +[l1 0] +[l2] +[l3] +[l4] +[l4 0] +[l4 1] +[l5] +[l5 #foo] +[l5 0] +[l5 1] +[#foo] +[l6] +[l6 #foo] +[l6 0] +[l6 1] +[n1] +[n10] +[t] +- t is true + +[e1] +[e2] +[e3] +[e4] +[e5] +[e6] +[e7] +[e8] +[m1] +[m1 foo] +- foo is an optional field + +[m1 bar] +- bar is a field + +[m1 baz] +- baz is a required field. + +[x] +[y1] +[y1 src] +[y1 src 0] +[y1 src 1] +[y1 src 2] +[y1 x] +[y1 x 0] +[y1 x 1] +[y1 x 2] +[y1 foo0] +[y1 foo1] +[y1 foo2] +[y1 bar1] +[y1 bar2] +[preserveKeyFieldInComprehension] +[errorStructDef] +[errorStructDef a] +[errorStructDef b] +[errorStructDef #Def] +[errorList] +[errorList 0] +[errorList 1] +[errorListDef] +[errorListDef #Def] +[errorListDef 0] +[errorListDef 1] +-- diff/-out/doc-v3<==>+out/doc -- +diff old new +--- old ++++ new +@@ -65,10 +65,10 @@ + [y1 x 1] + [y1 x 2] + [y1 foo0] +-[y1 bar1] +-[y1 bar2] + [y1 foo1] + [y1 foo2] ++[y1 bar1] ++[y1 bar2] + [preserveKeyFieldInComprehension] + [errorStructDef] + [errorStructDef a] -- out/doc -- [] [p1] @@ -339,6 +440,123 @@ Reordering. [errorListDef 1] -- diff/value/todo/p3 -- Error message change. +-- out/value-v3 -- +== Simplified +_|_ // e3: index out of range [2] with length 2 +== Raw +_|_ // e3: index out of range [2] with length 2 +== Final +_|_ // e3: index out of range [2] with length 2 +== All +{ + @foo(bar) + p1: {} + d1: { + foobar: int + } + bar: "bar" + d2: { + foobar: { + name: "xx" + foo: "xx" + } + } + + // Issue #1910 + a: _ + comp: { + for k, v in [0] + let w = v { + "\(a)": w + "bar": w + } + } + bytes: '\xeb \x1a\xf5\xaa\xf0\xd6\x06)' + c1: true + s1: """ + multi + bar + line + """ + l1: [3, ...int] + l2: [...int] + l3: [] + l4: [1, 2] + l5: { + #foo: int + [1, 3] + } + #foo: int + l6: { + #foo: int + [1, 3] + } + n1: 1.0 + n10: 10 + + // t is true + t: true + e1: <1.0 + e2: >1.0 & <10 + e3: _|_ // e3: index out of range [2] with length 2 + e4: _|_ // e4: index 3 out of range + e5: _|_ // e3: index out of range [2] with length 2 + e6: false + e7: true + e8?: true + m1: { + // foo is an optional field + foo?: 3 + + // bar is a field + bar: 4 + + // baz is a required field. + baz!: 5 + } + y1: { + src: [1, 2, 3] + foo0: 1 + foo1: 2 + foo2: 3 + bar1: 2 + x: [1, 2, 3] + bar2: 3 + } + preserveKeyFieldInComprehension: 1 + errorStructDef: { + a: 1 + b: _|_ // errorStructDef.b: conflicting values 2 and 1 + #Def: 1 + } + errorList: [1, _|_] + x: int + errorListDef: { + #Def: 1 + [1, _|_] + } +} +== Eval +_|_ // e3: index out of range [2] with length 2 +-- diff/-out/value-v3<==>+out/value -- +diff old new +--- old ++++ new +@@ -74,11 +74,11 @@ + y1: { + src: [1, 2, 3] + foo0: 1 +- bar1: 2 +- bar2: 3 + foo1: 2 +- x: [1, 2, 3] + foo2: 3 ++ bar1: 2 ++ x: [1, 2, 3] ++ bar2: 3 + } + preserveKeyFieldInComprehension: 1 + errorStructDef: { -- out/value -- == Simplified _|_ // e3: index out of range [2] with length 2