Skip to content

Commit bdc2929

Browse files
mpvlmpvl
committed
internal/core/adt: prevent early decrements in comprehensions
Pending arcs in comprehensions follow a slightly different code path and are evaluated a bit earlier to determine whether the fields will exist. We need to ensure that if a "complete all" path is agitated, we do not break incoming dependencies if we are not finalizing. Doing so might cause the closeContexts to be closed, causing an "already closed" panic when a matching pattern constraint is later evaluated. We enforce this by requiring `breakIncomingDeps` to take the current mode and then only commence when mode is finalize. Note that there are two call sites. We added two test cases, the reported one and a variant, each of which triggered a panic for the respective code paths. Fixes #3691 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ib4e9dda97a002f2ac5f96f4b31985e6c7ddb4f07 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207545 Reviewed-by: Daniel Martí <[email protected]> Unity-Result: CUE porcuepine <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
1 parent 6b292d4 commit bdc2929

File tree

3 files changed

+349
-11
lines changed

3 files changed

+349
-11
lines changed

‎cue/testdata/eval/comprehensions.txtar

+338-8
Original file line numberDiff line numberDiff line change
@@ -96,19 +96,268 @@ matchOrder: {
9696
out: [string]: Val: 1
9797
}
9898
}
99-
99+
-- issue3691.cue --
100+
// Ensure that parent nodes are properly processed, even when a pending arc
101+
// is evaluated early.
102+
issue3691: original: {
103+
X: [string]: string
104+
a: [string]: X
105+
a: {
106+
if true {
107+
b: c: 1
108+
}
109+
}
110+
}
111+
// Structural cycles follow a different code path.
112+
issue3691: structuralCycle: {
113+
X: [string]: string
114+
a: [string]: X
115+
a: {
116+
if true {
117+
b: c: a
118+
}
119+
}
120+
}
100121
-- out/eval/stats --
101122
Leaks: 4
102-
Freed: 64
103-
Reused: 57
104-
Allocs: 11
123+
Freed: 77
124+
Reused: 69
125+
Allocs: 12
105126
Retain: 19
106127

107-
Unifications: 68
108-
Conjuncts: 96
109-
Disjuncts: 81
128+
Unifications: 81
129+
Conjuncts: 121
130+
Disjuncts: 94
131+
-- out/evalalpha --
132+
Errors:
133+
issue3691.original.a.b: conflicting values 1 and string (mismatched types int and string):
134+
./issue3691.cue:4:15
135+
./issue3691.cue:8:10
136+
issue3691.structuralCycle.a.b.c: structural cycle
137+
138+
Result:
139+
(_|_){
140+
// [eval]
141+
a: (struct){
142+
x: (int){ 10 }
143+
y: (int){ 100 }
144+
z: (int){ 50 }
145+
}
146+
b: (struct){
147+
x: (int){ 10 }
148+
k: (int){ 20 }
149+
l: (int){ 40 }
150+
z: (int){ 50 }
151+
}
152+
c: (struct){
153+
y: (int){ 110 }
154+
z: (int){ 60 }
155+
}
156+
A: (struct){
157+
X: (struct){
158+
run: (string){ "dfoo" }
159+
files: (string){ "dfoo" }
160+
}
161+
}
162+
matchOrder: (struct){
163+
a1: (struct){
164+
out: (struct){
165+
b: (struct){
166+
Val: (int){ 1 }
167+
}
168+
}
169+
in: (struct){
170+
a: (struct){
171+
b: (struct){
172+
}
173+
}
174+
}
175+
}
176+
a2: (struct){
177+
out: (struct){
178+
b: (struct){
179+
Val: (int){ 1 }
180+
}
181+
}
182+
in: (struct){
183+
a: (struct){
184+
b: (struct){
185+
}
186+
}
187+
}
188+
}
189+
a3: (struct){
190+
in: (struct){
191+
a: (struct){
192+
b: (struct){
193+
}
194+
}
195+
}
196+
out: (struct){
197+
b: (struct){
198+
Val: (int){ 1 }
199+
}
200+
}
201+
}
202+
a4: (struct){
203+
out: (struct){
204+
b: (struct){
205+
Val: (int){ 1 }
206+
}
207+
}
208+
in: (struct){
209+
a: (struct){
210+
b: (struct){
211+
}
212+
}
213+
}
214+
}
215+
a5: (struct){
216+
out: (struct){
217+
b: (struct){
218+
Val: (int){ 1 }
219+
}
220+
}
221+
in: (struct){
222+
a: (struct){
223+
b: (struct){
224+
}
225+
}
226+
}
227+
}
228+
a6: (struct){
229+
in: (struct){
230+
a: (struct){
231+
b: (struct){
232+
}
233+
}
234+
}
235+
out: (struct){
236+
b: (struct){
237+
Val: (int){ 1 }
238+
}
239+
}
240+
}
241+
}
242+
issue3691: (_|_){
243+
// [eval]
244+
original: (_|_){
245+
// [eval]
246+
X: (struct){
247+
}
248+
a: (_|_){
249+
// [eval]
250+
b: (_|_){
251+
// [eval]
252+
c: (_|_){
253+
// [eval] issue3691.original.a.b: conflicting values 1 and string (mismatched types int and string):
254+
// ./issue3691.cue:4:15
255+
// ./issue3691.cue:8:10
256+
}
257+
}
258+
}
259+
}
260+
structuralCycle: (_|_){
261+
// [structural cycle]
262+
X: (struct){
263+
}
264+
a: (_|_){
265+
// [structural cycle]
266+
b: (_|_){
267+
// [structural cycle]
268+
c: (_|_){
269+
// [structural cycle] issue3691.structuralCycle.a.b.c: structural cycle
270+
}
271+
}
272+
}
273+
}
274+
}
275+
}
276+
-- diff/-out/evalalpha<==>+out/eval --
277+
diff old new
278+
--- old
279+
+++ new
280+
@@ -1,14 +1,8 @@
281+
Errors:
282+
-issue3691.original.a.b.c: conflicting values string and 1 (mismatched types string and int):
283+
+issue3691.original.a.b: conflicting values 1 and string (mismatched types int and string):
284+
./issue3691.cue:4:15
285+
- ./issue3691.cue:7:3
286+
./issue3691.cue:8:10
287+
-issue3691.structuralCycle.a.b.c: conflicting values string and {[string]:X} (mismatched types string and struct):
288+
- ./issue3691.cue:14:15
289+
- ./issue3691.cue:15:5
290+
- ./issue3691.cue:17:3
291+
- ./issue3691.cue:18:10
292+
-issue3691.structuralCycle.a.b.c.b.c: structural cycle
293+
+issue3691.structuralCycle.a.b.c: structural cycle
294+
295+
Result:
296+
(_|_){
297+
@@ -125,9 +119,8 @@
298+
b: (_|_){
299+
// [eval]
300+
c: (_|_){
301+
- // [eval] issue3691.original.a.b.c: conflicting values string and 1 (mismatched types string and int):
302+
+ // [eval] issue3691.original.a.b: conflicting values 1 and string (mismatched types int and string):
303+
// ./issue3691.cue:4:15
304+
- // ./issue3691.cue:7:3
305+
// ./issue3691.cue:8:10
306+
}
307+
}
308+
@@ -134,25 +127,15 @@
309+
}
310+
}
311+
structuralCycle: (_|_){
312+
- // [eval]
313+
- X: (struct){
314+
- }
315+
- a: (_|_){
316+
- // [eval]
317+
- b: (_|_){
318+
- // [eval]
319+
- c: (_|_){
320+
- // [eval] issue3691.structuralCycle.a.b.c: conflicting values string and {[string]:X} (mismatched types string and struct):
321+
- // ./issue3691.cue:14:15
322+
- // ./issue3691.cue:15:5
323+
- // ./issue3691.cue:17:3
324+
- // ./issue3691.cue:18:10
325+
- b: (_|_){
326+
- // [structural cycle]
327+
- c: (_|_){
328+
- // [structural cycle] issue3691.structuralCycle.a.b.c.b.c: structural cycle
329+
- }
330+
- }
331+
+ // [structural cycle]
332+
+ X: (struct){
333+
+ }
334+
+ a: (_|_){
335+
+ // [structural cycle]
336+
+ b: (_|_){
337+
+ // [structural cycle]
338+
+ c: (_|_){
339+
+ // [structural cycle] issue3691.structuralCycle.a.b.c: structural cycle
340+
}
341+
}
342+
}
343+
-- diff/todo/p2 --
344+
Missing error positions.
110345
-- out/eval --
111-
(struct){
346+
Errors:
347+
issue3691.original.a.b.c: conflicting values string and 1 (mismatched types string and int):
348+
./issue3691.cue:4:15
349+
./issue3691.cue:7:3
350+
./issue3691.cue:8:10
351+
issue3691.structuralCycle.a.b.c: conflicting values string and {[string]:X} (mismatched types string and struct):
352+
./issue3691.cue:14:15
353+
./issue3691.cue:15:5
354+
./issue3691.cue:17:3
355+
./issue3691.cue:18:10
356+
issue3691.structuralCycle.a.b.c.b.c: structural cycle
357+
358+
Result:
359+
(_|_){
360+
// [eval]
112361
a: (struct){
113362
x: (int){ 10 }
114363
y: (int){ 100 }
@@ -210,6 +459,50 @@ Disjuncts: 81
210459
}
211460
}
212461
}
462+
issue3691: (_|_){
463+
// [eval]
464+
original: (_|_){
465+
// [eval]
466+
X: (struct){
467+
}
468+
a: (_|_){
469+
// [eval]
470+
b: (_|_){
471+
// [eval]
472+
c: (_|_){
473+
// [eval] issue3691.original.a.b.c: conflicting values string and 1 (mismatched types string and int):
474+
// ./issue3691.cue:4:15
475+
// ./issue3691.cue:7:3
476+
// ./issue3691.cue:8:10
477+
}
478+
}
479+
}
480+
}
481+
structuralCycle: (_|_){
482+
// [eval]
483+
X: (struct){
484+
}
485+
a: (_|_){
486+
// [eval]
487+
b: (_|_){
488+
// [eval]
489+
c: (_|_){
490+
// [eval] issue3691.structuralCycle.a.b.c: conflicting values string and {[string]:X} (mismatched types string and struct):
491+
// ./issue3691.cue:14:15
492+
// ./issue3691.cue:15:5
493+
// ./issue3691.cue:17:3
494+
// ./issue3691.cue:18:10
495+
b: (_|_){
496+
// [structural cycle]
497+
c: (_|_){
498+
// [structural cycle] issue3691.structuralCycle.a.b.c.b.c: structural cycle
499+
}
500+
}
501+
}
502+
}
503+
}
504+
}
505+
}
213506
}
214507
-- out/compile --
215508
--- in.cue
@@ -345,3 +638,40 @@ Disjuncts: 81
345638
}
346639
}
347640
}
641+
--- issue3691.cue
642+
{
643+
issue3691: {
644+
original: {
645+
X: {
646+
[string]: string
647+
}
648+
a: {
649+
[string]: 〈1;X〉
650+
}
651+
a: {
652+
if true {
653+
b: {
654+
c: 1
655+
}
656+
}
657+
}
658+
}
659+
}
660+
issue3691: {
661+
structuralCycle: {
662+
X: {
663+
[string]: string
664+
}
665+
a: {
666+
[string]: 〈1;X〉
667+
}
668+
a: {
669+
if true {
670+
b: {
671+
c: 〈3;a〉
672+
}
673+
}
674+
}
675+
}
676+
}
677+
}

‎internal/core/adt/dep.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,15 @@ func (n *nodeContext) breakIncomingArcs(mode runMode) {
337337

338338
// breakIncomingDeps breaks all incoming dependencies, which includes arcs and
339339
// pending notifications and attempts all remaining work.
340-
func (n *nodeContext) breakIncomingDeps() {
340+
//
341+
// We should only break incoming dependencies if we are finalizing nodes, as
342+
// breaking them earlier can cause a "already closed" panic. To make sure of
343+
// this, we force the caller to pass mode.
344+
func (n *nodeContext) breakIncomingDeps(mode runMode) {
345+
if mode != finalize {
346+
return
347+
}
348+
341349
// TODO: remove this block in favor of finalizing notification nodes,
342350
// or what have you. We have patched this to skip evaluating when using
343351
// disjunctions, but this is overall a brittle approach.

0 commit comments

Comments
 (0)