Skip to content

Commit 12437a2

Browse files
mpvlmpvl
committed
internal/core/adt: do not force externalDeps for disjunctions
We currently force close/evaluate externalDeps in completeAllArcs. We really should move to a call-by-need-like approach, were we rather close such nodes as we unwind the call stack. For now, though, we just ensure that these externalDeps are not closed across disjunction boundaries (see comment). Doing so might cause parts of disjunctions to be prematurely evaluated leading to an "already closed" panic. This also increases counters. We will address this later. Tests were added together with change as they would otherwise panic. Fixes #3635 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ibf689257179e8f1ac6411ee2e803326dac9e5a62 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207169 Unity-Result: CUE porcuepine <[email protected]> Reviewed-by: Matthew Sackman <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
1 parent 681ec9f commit 12437a2

File tree

3 files changed

+330
-11
lines changed

3 files changed

+330
-11
lines changed

‎cue/testdata/disjunctions/nested.txtar

Lines changed: 313 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,313 @@
1+
-- issue3635.cue --
2+
import "list"
3+
issue3635: case1: {
4+
Run: {
5+
env: *{[string]: string} | [...string]
6+
}
7+
8+
command: Run & {
9+
env: list.Concat([["foo"], ["bar"]])
10+
}
11+
}
12+
13+
issue3635: case2: {
14+
out: [...#Schema]
15+
out: [#Schema & {
16+
name: "x"
17+
}]
18+
19+
#Schema: {
20+
name?: string
21+
container?: [string]: string | #container
22+
23+
#container: string | {
24+
env?: #env
25+
}
26+
27+
#env: [string]: bool | string
28+
}
29+
}
30+
31+
issue3635: reduced: {
32+
out: {X} & X
33+
X: {
34+
b: int | c
35+
c: int | { e: d }
36+
}
37+
d: 1
38+
}
39+
40+
issue3635: reduced_noshare: {
41+
out: {X} & X
42+
X: {
43+
b: int | c
44+
c: int | { e: d & __no_sharing }
45+
}
46+
d: 1
47+
}
48+
-- out/eval/stats --
49+
Leaks: 0
50+
Freed: 162
51+
Reused: 148
52+
Allocs: 14
53+
Retain: 62
54+
55+
Unifications: 86
56+
Conjuncts: 315
57+
Disjuncts: 168
58+
-- out/evalalpha --
59+
(struct){
60+
issue3635: (struct){
61+
case1: (struct){
62+
Run: (struct){
63+
env: ((list|struct)){ |(*(struct){
64+
}, (list){
65+
}) }
66+
}
67+
command: (struct){
68+
env: (#list){
69+
0: (string){ string }
70+
1: (string){ string }
71+
}
72+
}
73+
}
74+
case2: (struct){
75+
out: (#list){
76+
0: (#struct){
77+
name: (string){ "x" }
78+
container?: (#struct){
79+
}
80+
#container: ((string|struct)){ |((string){ string }, (#struct){
81+
env?: (#struct){
82+
}
83+
}) }
84+
#env: (#struct){
85+
}
86+
}
87+
}
88+
#Schema: (#struct){
89+
name?: (string){ string }
90+
container?: (#struct){
91+
}
92+
#container: ((string|struct)){ |((string){ string }, (#struct){
93+
env?: (#struct){
94+
}
95+
}) }
96+
#env: (#struct){
97+
}
98+
}
99+
}
100+
reduced: (struct){
101+
out: (struct){
102+
b: ((int|struct)){ |((int){ int }, (struct){
103+
e: (int){ 1 }
104+
}) }
105+
c: ((int|struct)){ |((int){ int }, (struct){
106+
e: (int){ 1 }
107+
}) }
108+
}
109+
X: (struct){
110+
b: ((int|struct)){ |((int){ int }, (struct){
111+
e: (int){ 1 }
112+
}) }
113+
c: ((int|struct)){ |((int){ int }, (struct){
114+
e: (int){ 1 }
115+
}) }
116+
}
117+
d: (int){ 1 }
118+
}
119+
reduced_noshare: (struct){
120+
out: (struct){
121+
b: ((int|struct)){ |((int){ int }, (struct){
122+
e: (int){ 1 }
123+
}) }
124+
c: ((int|struct)){ |((int){ int }, (struct){
125+
e: (int){ 1 }
126+
}) }
127+
}
128+
X: (struct){
129+
b: ((int|struct)){ |((int){ int }, (struct){
130+
e: (int){ 1 }
131+
}) }
132+
c: ((int|struct)){ |((int){ int }, (struct){
133+
e: (int){ 1 }
134+
}) }
135+
}
136+
d: (int){ 1 }
137+
}
138+
}
139+
}
140+
-- diff/-out/evalalpha<==>+out/eval --
141+
diff old new
142+
--- old
143+
+++ new
144+
@@ -8,8 +8,8 @@
145+
}
146+
command: (struct){
147+
env: (#list){
148+
- 0: (string){ "foo" }
149+
- 1: (string){ "bar" }
150+
+ 0: (string){ string }
151+
+ 1: (string){ string }
152+
}
153+
}
154+
}
155+
-- out/eval --
156+
(struct){
157+
issue3635: (struct){
158+
case1: (struct){
159+
Run: (struct){
160+
env: ((list|struct)){ |(*(struct){
161+
}, (list){
162+
}) }
163+
}
164+
command: (struct){
165+
env: (#list){
166+
0: (string){ "foo" }
167+
1: (string){ "bar" }
168+
}
169+
}
170+
}
171+
case2: (struct){
172+
out: (#list){
173+
0: (#struct){
174+
name: (string){ "x" }
175+
container?: (#struct){
176+
}
177+
#container: ((string|struct)){ |((string){ string }, (#struct){
178+
env?: (#struct){
179+
}
180+
}) }
181+
#env: (#struct){
182+
}
183+
}
184+
}
185+
#Schema: (#struct){
186+
name?: (string){ string }
187+
container?: (#struct){
188+
}
189+
#container: ((string|struct)){ |((string){ string }, (#struct){
190+
env?: (#struct){
191+
}
192+
}) }
193+
#env: (#struct){
194+
}
195+
}
196+
}
197+
reduced: (struct){
198+
out: (struct){
199+
b: ((int|struct)){ |((int){ int }, (struct){
200+
e: (int){ 1 }
201+
}) }
202+
c: ((int|struct)){ |((int){ int }, (struct){
203+
e: (int){ 1 }
204+
}) }
205+
}
206+
X: (struct){
207+
b: ((int|struct)){ |((int){ int }, (struct){
208+
e: (int){ 1 }
209+
}) }
210+
c: ((int|struct)){ |((int){ int }, (struct){
211+
e: (int){ 1 }
212+
}) }
213+
}
214+
d: (int){ 1 }
215+
}
216+
reduced_noshare: (struct){
217+
out: (struct){
218+
b: ((int|struct)){ |((int){ int }, (struct){
219+
e: (int){ 1 }
220+
}) }
221+
c: ((int|struct)){ |((int){ int }, (struct){
222+
e: (int){ 1 }
223+
}) }
224+
}
225+
X: (struct){
226+
b: ((int|struct)){ |((int){ int }, (struct){
227+
e: (int){ 1 }
228+
}) }
229+
c: ((int|struct)){ |((int){ int }, (struct){
230+
e: (int){ 1 }
231+
}) }
232+
}
233+
d: (int){ 1 }
234+
}
235+
}
236+
}
237+
-- out/compile --
238+
--- issue3635.cue
239+
{
240+
issue3635: {
241+
case1: {
242+
Run: {
243+
env: (*{
244+
[string]: string
245+
}|[
246+
...string,
247+
])
248+
}
249+
command: (〈0;Run〉 & {
250+
env: 〈import;list〉.Concat([
251+
[
252+
"foo",
253+
],
254+
[
255+
"bar",
256+
],
257+
])
258+
})
259+
}
260+
}
261+
issue3635: {
262+
case2: {
263+
out: [
264+
...〈1;#Schema〉,
265+
]
266+
out: [
267+
(〈1;#Schema〉 & {
268+
name: "x"
269+
}),
270+
]
271+
#Schema: {
272+
name?: string
273+
container?: {
274+
[string]: (string|〈1;#container〉)
275+
}
276+
#container: (string|{
277+
env?: 〈1;#env〉
278+
})
279+
#env: {
280+
[string]: (bool|string)
281+
}
282+
}
283+
}
284+
}
285+
issue3635: {
286+
reduced: {
287+
out: ({
288+
〈1;X〉
289+
} & 〈0;X〉)
290+
X: {
291+
b: (int|〈0;c〉)
292+
c: (int|{
293+
e: 〈2;d〉
294+
})
295+
}
296+
d: 1
297+
}
298+
}
299+
issue3635: {
300+
reduced_noshare: {
301+
out: ({
302+
〈1;X〉
303+
} & 〈0;X〉)
304+
X: {
305+
b: (int|〈0;c〉)
306+
c: (int|{
307+
e: (〈2;d〉 & _|_(no sharing))
308+
})
309+
}
310+
d: 1
311+
}
312+
}
313+
}

‎internal/core/adt/eval_test.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,16 +84,17 @@ var skipDebugDepErrors = map[string]int{
8484
"cycle/comprehension": 2,
8585
"cycle/disjunction": 4,
8686
"cycle/evaluate": 1,
87-
"cycle/structural": 14,
87+
"cycle/structural": 18,
8888
"disjunctions/edge": 1,
89-
"disjunctions/errors": 2,
90-
"disjunctions/elimination": 11,
91-
"disjunctions/embed": 6,
89+
"disjunctions/errors": 4,
90+
"disjunctions/elimination": 15,
91+
"disjunctions/embed": 12,
92+
"disjunctions/nested": 1,
9293
"eval/conjuncts": 3,
9394
"eval/disjunctions": 3,
94-
"eval/notify": 17,
95-
"fulleval/054_issue312": 1,
96-
"scalars/embed": 1,
95+
"eval/notify": 18,
96+
"fulleval/054_issue312": 2,
97+
"scalars/embed": 2,
9798
}
9899

99100
func TestEvalAlpha(t *testing.T) {

‎internal/core/adt/unify.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -534,18 +534,23 @@ func (n *nodeContext) completeAllArcs(needs condition, mode runMode) bool {
534534
// Investigate how to work around this.
535535
n.completeNodeTasks(finalize)
536536

537+
// TODO: remove this block in favor of finalizing notification nodes,
538+
// or what have you. We have patched this to skip evaluating when using
539+
// disjunctions, but this is overall a brittle approach.
537540
for _, r := range n.node.cc().externalDeps {
538541
src := r.src
542+
// We should be careful to not evaluate parent nodes if we are inside a
543+
// disjunction, or at least ensure that there are no disjunction values
544+
// leaked into non-disjunction nodes through evaluating externalDeps.
545+
if src.src.IsDisjunct {
546+
continue
547+
}
539548
a := &src.arcs[r.index]
540549
if a.decremented {
541550
continue
542551
}
543552
a.decremented = true
544553

545-
// FIXME: we should be careful to not evaluate parent nodes if we
546-
// are inside a disjunction, or at least ensure that there are no
547-
// disjunction values leaked into non-disjunction nodes through
548-
// evaluating externalDeps.
549554
src.src.unify(n.ctx, needTasksDone, attemptOnly)
550555
a.cc.decDependent(n.ctx, a.kind, src) // REF(arcs)
551556
}

0 commit comments

Comments
 (0)