Skip to content

Commit d950d55

Browse files
saghulsaghul
authored
Fix Iterator Helper close when the source iterator has no return (#1558)
1 parent 3c8f3d6 commit d950d55

2 files changed

Lines changed: 101 additions & 90 deletions

File tree

‎quickjs.c‎

Lines changed: 46 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -45492,33 +45492,22 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4549245492
switch (it->kind) {
4549345493
case JS_ITERATOR_HELPER_KIND_DROP:
4549445494
{
45495-
JSValue item, method;
45496-
if (magic == GEN_MAGIC_NEXT) {
45497-
method = js_dup(it->next);
45498-
} else {
45499-
method = JS_GetProperty(ctx, it->obj, JS_ATOM_return);
45500-
if (JS_IsException(method))
45501-
goto fail;
45502-
}
45495+
JSValue item;
45496+
if (magic == GEN_MAGIC_RETURN)
45497+
goto close;
4550345498
while (it->count > 0) {
4550445499
it->count--;
45505-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45506-
if (JS_IsException(item)) {
45507-
JS_FreeValue(ctx, method);
45500+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
45501+
if (JS_IsException(item))
4550845502
goto fail;
45509-
}
4551045503
JS_FreeValue(ctx, item);
45511-
if (magic == GEN_MAGIC_RETURN)
45512-
*pdone = true;
4551345504
if (*pdone) {
45514-
JS_FreeValue(ctx, method);
4551545505
ret = JS_UNDEFINED;
4551645506
goto done;
4551745507
}
4551845508
}
4551945509

45520-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45521-
JS_FreeValue(ctx, method);
45510+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
4552245511
if (JS_IsException(item))
4552345512
goto fail;
4552445513
ret = item;
@@ -45527,23 +45516,15 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4552745516
break;
4552845517
case JS_ITERATOR_HELPER_KIND_FILTER:
4552945518
{
45530-
JSValue item, method, selected, index_val;
45519+
JSValue item, selected, index_val;
4553145520
JSValueConst args[2];
45532-
if (magic == GEN_MAGIC_NEXT) {
45533-
method = js_dup(it->next);
45534-
} else {
45535-
method = JS_GetProperty(ctx, it->obj, JS_ATOM_return);
45536-
if (JS_IsException(method))
45537-
goto fail;
45538-
}
45521+
if (magic == GEN_MAGIC_RETURN)
45522+
goto close;
4553945523
filter_again:
45540-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45541-
if (JS_IsException(item)) {
45542-
JS_FreeValue(ctx, method);
45524+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
45525+
if (JS_IsException(item))
4554345526
goto fail;
45544-
}
45545-
if (*pdone || magic == GEN_MAGIC_RETURN) {
45546-
JS_FreeValue(ctx, method);
45527+
if (*pdone) {
4554745528
ret = item;
4554845529
goto done;
4554945530
}
@@ -45554,11 +45535,9 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4555445535
JS_FreeValue(ctx, index_val);
4555545536
if (JS_IsException(selected)) {
4555645537
JS_FreeValue(ctx, item);
45557-
JS_FreeValue(ctx, method);
4555845538
goto fail;
4555945539
}
4556045540
if (JS_ToBoolFree(ctx, selected)) {
45561-
JS_FreeValue(ctx, method);
4556245541
ret = item;
4556345542
goto done;
4556445543
}
@@ -45570,20 +45549,14 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4557045549
{
4557145550
JSValue item, method, index_val, iter;
4557245551
JSValueConst args[2];
45552+
if (magic == GEN_MAGIC_RETURN)
45553+
goto close;
4557345554
flat_map_again:
4557445555
if (JS_IsUndefined(it->inner)) {
45575-
if (magic == GEN_MAGIC_NEXT) {
45576-
method = js_dup(it->next);
45577-
} else {
45578-
method = JS_GetProperty(ctx, it->obj, JS_ATOM_return);
45579-
if (JS_IsException(method))
45580-
goto fail;
45581-
}
45582-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45583-
JS_FreeValue(ctx, method);
45556+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
4558445557
if (JS_IsException(item))
4558545558
goto fail;
45586-
if (*pdone || magic == GEN_MAGIC_RETURN) {
45559+
if (*pdone) {
4558745560
ret = item;
4558845561
goto done;
4558945562
}
@@ -45619,27 +45592,19 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4561945592
it->inner = iter;
4562045593
}
4562145594

45622-
if (magic == GEN_MAGIC_NEXT)
45623-
method = JS_GetProperty(ctx, it->inner, JS_ATOM_next);
45624-
else
45625-
method = JS_GetProperty(ctx, it->inner, JS_ATOM_return);
45595+
method = JS_GetProperty(ctx, it->inner, JS_ATOM_next);
4562645596
if (JS_IsException(method)) {
4562745597
inner_fail:
4562845598
JS_IteratorClose(ctx, it->inner, false);
4562945599
JS_FreeValue(ctx, it->inner);
4563045600
it->inner = JS_UNDEFINED;
4563145601
goto fail;
4563245602
}
45633-
if (magic == GEN_MAGIC_RETURN && (JS_IsUndefined(method) || JS_IsNull(method))) {
45634-
goto inner_end;
45635-
} else {
45636-
item = JS_IteratorNext(ctx, it->inner, method, 0, NULL, pdone);
45637-
JS_FreeValue(ctx, method);
45638-
if (JS_IsException(item))
45639-
goto inner_fail;
45640-
}
45603+
item = JS_IteratorNext(ctx, it->inner, method, 0, NULL, pdone);
45604+
JS_FreeValue(ctx, method);
45605+
if (JS_IsException(item))
45606+
goto inner_fail;
4564145607
if (*pdone) {
45642-
inner_end:
4564345608
*pdone = false; // The outer iterator must continue.
4564445609
JS_IteratorClose(ctx, it->inner, false);
4564545610
JS_FreeValue(ctx, it->inner);
@@ -45652,20 +45617,14 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4565245617
break;
4565345618
case JS_ITERATOR_HELPER_KIND_MAP:
4565445619
{
45655-
JSValue item, method, index_val;
45620+
JSValue item, index_val;
4565645621
JSValueConst args[2];
45657-
if (magic == GEN_MAGIC_NEXT) {
45658-
method = js_dup(it->next);
45659-
} else {
45660-
method = JS_GetProperty(ctx, it->obj, JS_ATOM_return);
45661-
if (JS_IsException(method))
45662-
goto fail;
45663-
}
45664-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45665-
JS_FreeValue(ctx, method);
45622+
if (magic == GEN_MAGIC_RETURN)
45623+
goto close;
45624+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
4566645625
if (JS_IsException(item))
4566745626
goto fail;
45668-
if (*pdone || magic == GEN_MAGIC_RETURN) {
45627+
if (*pdone) {
4566945628
ret = item;
4567045629
goto done;
4567145630
}
@@ -45682,36 +45641,33 @@ static JSValue js_iterator_helper_next(JSContext *ctx, JSValueConst this_val,
4568245641
break;
4568345642
case JS_ITERATOR_HELPER_KIND_TAKE:
4568445643
{
45685-
JSValue item, method;
45686-
if (it->count > 0) {
45687-
if (magic == GEN_MAGIC_NEXT) {
45688-
method = js_dup(it->next);
45689-
} else {
45690-
method = JS_GetProperty(ctx, it->obj, JS_ATOM_return);
45691-
if (JS_IsException(method))
45692-
goto fail;
45693-
}
45694-
it->count--;
45695-
item = JS_IteratorNext(ctx, it->obj, method, 0, NULL, pdone);
45696-
JS_FreeValue(ctx, method);
45697-
if (JS_IsException(item))
45698-
goto fail;
45699-
ret = item;
45700-
goto done;
45701-
}
45702-
45703-
*pdone = true;
45704-
if (JS_IteratorClose(ctx, it->obj, false))
45705-
ret = JS_EXCEPTION;
45706-
else
45707-
ret = JS_UNDEFINED;
45644+
JSValue item;
45645+
if (magic == GEN_MAGIC_RETURN || it->count == 0)
45646+
goto close;
45647+
it->count--;
45648+
item = JS_IteratorNext(ctx, it->obj, it->next, 0, NULL, pdone);
45649+
if (JS_IsException(item))
45650+
goto fail;
45651+
ret = item;
4570845652
goto done;
4570945653
}
4571045654
break;
4571145655
default:
4571245656
abort();
4571345657
}
4571445658

45659+
close:
45660+
*pdone = true;
45661+
ret = JS_UNDEFINED;
45662+
if (!JS_IsUndefined(it->inner)) {
45663+
if (JS_IteratorClose(ctx, it->inner, false))
45664+
ret = JS_EXCEPTION;
45665+
JS_FreeValue(ctx, it->inner);
45666+
it->inner = JS_UNDEFINED;
45667+
}
45668+
if (JS_IteratorClose(ctx, it->obj, JS_IsException(ret)))
45669+
ret = JS_EXCEPTION;
45670+
4571545671
done:
4571645672
it->done = magic == GEN_MAGIC_NEXT ? *pdone : 1;
4571745673
it->executing = 0;

‎tests/bug1557.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// https://github.com/quickjs-ng/quickjs/issues/1557
2+
import { assert, assertArrayEquals } from "./assert.js";
3+
4+
assertArrayEquals(
5+
Iterator.from([1, 2, 3, 4]).filter(x => x > 1).take(1).toArray(),
6+
[2]
7+
);
8+
9+
assertArrayEquals(Iterator.from([1, 2, 3, 4]).map(x => x * 2).take(2).toArray(), [2, 4]);
10+
assertArrayEquals(Iterator.from([1, 2, 3, 4, 5]).drop(1).take(2).toArray(), [2, 3]);
11+
assertArrayEquals(Iterator.from([1, 2, 3]).flatMap(x => [x, x * 10]).take(3).toArray(), [1, 10, 2]);
12+
assertArrayEquals(Iterator.from([1, 2, 3, 4, 5, 6]).filter(x => x % 2 === 0).map(x => x + 1).take(2).toArray(), [3, 5]);
13+
14+
// Calling `.return()` explicitly on a helper over a `return`-less source must
15+
// not throw and must report completion with an undefined value.
16+
for (const make of [
17+
it => it.map(x => x),
18+
it => it.filter(() => true),
19+
it => it.take(5),
20+
it => it.drop(0),
21+
it => it.flatMap(x => [x]),
22+
]) {
23+
// `[][Symbol.iterator]()` is a plain array iterator: it has no `return`.
24+
const helper = make([1, 2, 3][Symbol.iterator]());
25+
const r1 = helper.return();
26+
assert(r1.done, true);
27+
assert(r1.value, undefined);
28+
// A second `.return()` is a no-op and also reports done.
29+
const r2 = helper.return();
30+
assert(r2.done, true);
31+
assert(r2.value, undefined);
32+
}
33+
34+
// When the source *does* have a `return`, closing the helper must still
35+
// forward to it exactly once.
36+
for (const make of [
37+
it => it.map(x => x),
38+
it => it.filter(() => true),
39+
it => it.take(5),
40+
it => it.drop(0),
41+
it => it.flatMap(x => [x]),
42+
]) {
43+
let returnCount = 0;
44+
class Src extends Iterator {
45+
next() { return { done: false, value: 1 }; }
46+
return(value) { returnCount++; return { done: true, value }; }
47+
}
48+
const helper = make(new Src());
49+
helper.next();
50+
assert(returnCount, 0);
51+
helper.return();
52+
assert(returnCount, 1);
53+
helper.return(); // already closed: not forwarded again
54+
assert(returnCount, 1);
55+
}

0 commit comments

Comments
 (0)