Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call return() on the iterator when a for-of loop exits abruptly #2958

Open
shicks opened this issue Jun 1, 2018 · 2 comments
Open

Call return() on the iterator when a for-of loop exits abruptly #2958

shicks opened this issue Jun 1, 2018 · 2 comments

Comments

@shicks
Copy link
Member

shicks commented Jun 1, 2018

This breaks out one part of #2899.

I looked into this briefly today. In order to do it right, we end up bloating our for-of transpilation quite a bit. Currently we transpile

for (var x of y) {
  ...
}

to

for (var iter=$jscomp.makeIterator(y), key=iter.next(); !key.done; key=iter.next()) {
  let x = key.value;
  ...
}

which isn't so bad. We can handle break, continue, and return pretty easily by inserting the return call immediately before each, and it doesn't cost anything if you don't use them. But throw can be invisible (i.e. it's likely not coming from a THROW node in the block's AST), so the only way to handle it correctly is to wrap the whole thing in a catch:

for (var iter=$jscomp.makeIterator(y), key=iter.next(); !key.done; key=iter.next()) {
  let x = key.value;
  try {
    ...
  } catch (e) {
    if (iter.return) iter.return();
    throw e;
  }
}

Even this only gets most of the way there - if return throws then we end up with the wrong error thrown. To get that correct, we need an additional try-finally around the return. If we're dealing with breaks as well, at this point, it probably makes sense to handle those at the same time, rather than mutating the AST inside the body:

try {
  for (var iter=$jscomp.makeIterator(y), key=iter.next(); !key.done; key=iter.next()) {
    let x = key.value;
    ...
  }
} catch (e) {
  key.thrown = {thrown: e};
} finally {
  try {
    if (!key.done && iter.return) iter.return();
  } finally {
    if (key.thrown) throw key.thrown.thrown;
  }
}

I estimate that this will add somewhere between 50 and 80 extra gzipped bytes to every for-of loop.

@shicks
Copy link
Member Author

shicks commented Jun 1, 2018

If we wrap the iterator in a wrapper that swallows any exception, we can get it down to

try {
  for (var iter=$jscomp.makeIterator(y), key=iter.next(); !key.done; key=iter.next()) {
    let x = key.value;
    ...
  }
} finally {
  iter.close();
}

with a little extra runtime library. This only amounts to 10-15 extra gzipped bytes, which is a lot better.

@trxcllnt
Copy link

Any update on this issue? Seems related to #3971.

Here's is a simple repro I used to verify:

$ npx google-closure-compiler --js test.js --js_output_file test.min.js --language_in ECMASCRIPT_NEXT --language_out ECMASCRIPT5 --assume_function_wrapper --compilation_level ADVANCED --third_party true --formatting PRETTY_PRINT --debug
$ node test.js 
> return called: true
$ node test.min.js 
> return called: false
test.js:
function first(xs) {
for (let x of xs) {
return x;
}
}

let count = 0;
let returnCalled = false;

const xs = {
[Symbol.iterator]() { return xs; },
next() {
if (count < 3) {
return { value: count++, done: false };
}
return { done: true };
},
return() {
returnCalled = true;
return { done: true };
}
};

first(xs);

console.log('return called:', returnCalled);
test.min.js:
function $$jscomp$arrayIteratorImpl$$($array$jscomp$6$$) {
var $index$jscomp$101$$ = 0;
return function() {
return $index$jscomp$101$$ < $array$jscomp$6$$.length ? {done:!1, value:$array$jscomp$6$$[$index$jscomp$101$$++]} : {done:!0};
};
}
var $$jscomp$defineProperty$$ = "function" == typeof Object.defineProperties ? Object.defineProperty : function($target$jscomp$92$$, $property$jscomp$5$$, $descriptor$jscomp$1$$) {
if ($target$jscomp$92$$ == Array.prototype || $target$jscomp$92$$ == Object.prototype) {
return $target$jscomp$92$$;
}
$target$jscomp$92$$[$property$jscomp$5$$] = $descriptor$jscomp$1$$.value;
return $target$jscomp$92$$;
};
function $$jscomp$getGlobal$$($passedInThis_possibleGlobals$$) {
$passedInThis_possibleGlobals$$ = ["object" == typeof globalThis && globalThis, $passedInThis_possibleGlobals$$, "object" == typeof window && window, "object" == typeof self && self, "object" == typeof global && global];
for (var $i$jscomp$3$$ = 0; $i$jscomp$3$$ < $passedInThis_possibleGlobals$$.length; ++$i$jscomp$3$$) {
var $maybeGlobal$$ = $passedInThis_possibleGlobals$$[$i$jscomp$3$$];
if ($maybeGlobal$$ && $maybeGlobal$$.Math == Math) {
return $maybeGlobal$$;
}
}
throw Error("Cannot find global object");
}
var $$jscomp$global$$ = $$jscomp$getGlobal$$(this);
function $$jscomp$polyfill$$($property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$, $impl$jscomp$inline_13_polyfill$jscomp$1$$) {
if ($impl$jscomp$inline_13_polyfill$jscomp$1$$) {
a: {
var $obj$jscomp$inline_7$$ = $$jscomp$global$$;
$property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$ = $property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$.split(".");
for (var $i$jscomp$inline_9_orig$jscomp$inline_12$$ = 0; $i$jscomp$inline_9_orig$jscomp$inline_12$$ < $property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$.length - 1; $i$jscomp$inline_9_orig$jscomp$inline_12$$++) {
var $key$jscomp$inline_10$$ = $property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$[$i$jscomp$inline_9_orig$jscomp$inline_12$$];
if (!($key$jscomp$inline_10$$ in $obj$jscomp$inline_7$$)) {
break a;
}
$obj$jscomp$inline_7$$ = $obj$jscomp$inline_7$$[$key$jscomp$inline_10$$];
}
$property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$ = $property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$[$property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$.length - 1];
$i$jscomp$inline_9_orig$jscomp$inline_12$$ = $obj$jscomp$inline_7$$[$property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$];
$impl$jscomp$inline_13_polyfill$jscomp$1$$ = $impl$jscomp$inline_13_polyfill$jscomp$1$$($i$jscomp$inline_9_orig$jscomp$inline_12$$);
$impl$jscomp$inline_13_polyfill$jscomp$1$$ != $i$jscomp$inline_9_orig$jscomp$inline_12$$ && null != $impl$jscomp$inline_13_polyfill$jscomp$1$$ && $$jscomp$defineProperty$$($obj$jscomp$inline_7$$, $property$jscomp$inline_11_split$jscomp$inline_8_target$jscomp$94$$, {configurable:!0, writable:!0, value:$impl$jscomp$inline_13_polyfill$jscomp$1$$});
}
}
}
$$jscomp$polyfill$$("Symbol", function($orig$jscomp$1$$) {
function $symbolPolyfill$$($opt_description$jscomp$2$$) {
if (this instanceof $symbolPolyfill$$) {
throw new TypeError("Symbol is not a constructor");
}
return new $SymbolClass$$($SYMBOL_PREFIX$$ + ($opt_description$jscomp$2$$ || "") + "_" + $counter$$++, $opt_description$jscomp$2$$);
}
function $SymbolClass$$($id$jscomp$5$$, $opt_description$jscomp$1$$) {
this.$g$ = $id$jscomp$5$$;
$$jscomp$defineProperty$$(this, "description", {configurable:!0, writable:!0, value:$opt_description$jscomp$1$$});
}
if ($orig$jscomp$1$$) {
return $orig$jscomp$1$$;
}
$SymbolClass$$.prototype.toString = function() {
return this.$g$;
};
var $SYMBOL_PREFIX$$ = "jscomp_symbol_" + (1E9 * Math.random() >>> 0) + "_", $counter$$ = 0;
return $symbolPolyfill$$;
});
$$jscomp$polyfill$$("Symbol.iterator", function($orig$jscomp$2_symbolIterator$$) {
if ($orig$jscomp$2_symbolIterator$$) {
return $orig$jscomp$2_symbolIterator$$;
}
$orig$jscomp$2_symbolIterator$$ = Symbol("Symbol.iterator");
for (var $arrayLikes$$ = "Array Int8Array Uint8Array Uint8ClampedArray Int16Array Uint16Array Int32Array Uint32Array Float32Array Float64Array".split(" "), $i$jscomp$6$$ = 0; $i$jscomp$6$$ < $arrayLikes$$.length; $i$jscomp$6$$++) {
var $ArrayLikeCtor$$ = $$jscomp$global$$[$arrayLikes$$[$i$jscomp$6$$]];
"function" === typeof $ArrayLikeCtor$$ && "function" != typeof $ArrayLikeCtor$$.prototype[$orig$jscomp$2_symbolIterator$$] && $$jscomp$defineProperty$$($ArrayLikeCtor$$.prototype, $orig$jscomp$2_symbolIterator$$, {configurable:!0, writable:!0, value:function() {
return $$jscomp$iteratorPrototype$$($$jscomp$arrayIteratorImpl$$(this));
}});
}
return $orig$jscomp$2_symbolIterator$$;
});
function $$jscomp$iteratorPrototype$$($iterator$jscomp$6_next$$) {
$iterator$jscomp$6_next$$ = {next:$iterator$jscomp$6_next$$};
$iterator$jscomp$6_next$$[Symbol.iterator] = function() {
return this;
};
return $iterator$jscomp$6_next$$;
}
var $count$$ = 0, $returnCalled$$ = !1, $$jscomp$compprop0$$ = {}, $xs$$ = ($$jscomp$compprop0$$[Symbol.iterator] = function() {
return $xs$$;
}, $$jscomp$compprop0$$.next = function() {
return 3 > $count$$ ? {value:$count$$++, done:!1} : {done:!0};
}, $$jscomp$compprop0$$.return = function() {
$returnCalled$$ = !0;
return {done:!0};
}, $$jscomp$compprop0$$);
a: {
var $JSCompiler_inline_result$jscomp$18$$;
var $iteratorFunction$jscomp$inline_20$$ = "undefined" != typeof Symbol && Symbol.iterator && $xs$$[Symbol.iterator];
if ($iteratorFunction$jscomp$inline_20$$) {
$JSCompiler_inline_result$jscomp$18$$ = $iteratorFunction$jscomp$inline_20$$.call($xs$$);
} else {
if ("number" == typeof $xs$$.length) {
$JSCompiler_inline_result$jscomp$18$$ = {next:$$jscomp$arrayIteratorImpl$$($xs$$)};
} else {
throw Error(String($xs$$) + " is not an iterable or ArrayLike");
}
}
for (var $$jscomp$key$m1422502875$0$x$jscomp$inline_16$$ = $JSCompiler_inline_result$jscomp$18$$.next(); !$$jscomp$key$m1422502875$0$x$jscomp$inline_16$$.done;) {
break a;
}
}
console.log("return called:", $returnCalled$$);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants