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
Improve output of for await
#15943
base: main
Are you sure you want to change the base?
Improve output of for await
#15943
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55411/ |
} finally { | ||
try { | ||
if (ITERATOR_ABRUPT_COMPLETION && ITERATOR_KEY.return != null) { | ||
if (!STEP_KEY.done && ITERATOR_KEY.return != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This accesses .done
on the object twice, which is observable if it is a getter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something like this?
async function wrapper() {
try {
for (
var ITERATOR_KEY = GET_ITERATOR(OBJECT), STEP_KEY, NOT_DONE_KEY;
NOT_DONE_KEY = !(STEP_KEY = await ITERATOR_KEY.next()).done;
) {
}
} finally {
try {
if (NOT_DONE_KEY && ITERATOR_KEY.return != null) {
await ITERATOR_KEY.return();
}
} catch (e) {}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to set NOT_DONE_KEY
to false after the loop ends to avoid executing return()
when it throws in next()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if this test behaves properly:
function* gen() {
try {
yield 1;
} finally {
throw 2;
}
}
let err;
try {
for await (const _ of gen()) break;
} catch (e) {
err = e;
}
expect(err).toBe(2);
Yes, this PR breaks the test. |
We should submit that test to test262, since it didn't catch the bug! |
var ITERATOR_HAD_ERROR_KEY = false; | ||
var ITERATOR_ERROR_KEY; | ||
const buildForAwait = template.statements(` | ||
var ITERATOR_KEY = GET_ITERATOR(OBJECT), STEP_KEY = {}, NOT_DONE_KEY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an upside to initializing STEP_KEY here? It'll be overwritten before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's no longer needed, thank you!
} | ||
} catch (e) { | ||
if (STEP_KEY) throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (STEP_KEY) throw e; | |
if (STEP_KEY != null) throw e; |
.next()
might set Boolean.prototype.done
to true
and return false
:)
Could you also rename _notDone
to _iteratorAbruptCompletion
, to reduce the diff and make it easier to follow the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: .next()
must return an Object; otherwise agree explicit null-check looks clearer.
6.c. If nextResult is not an Object, throw a TypeError exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.next()
might setBoolean.prototype.done
totrue
and returnfalse
:)
I'm not sure if it's worth doing, it's really weird usage. :)
Could you also rename _notDone to _iteratorAbruptCompletion, to reduce the diff and make it easier to follow the changes?
Since I completely changed the position of variable declarations and the logic of throwing exceptions, the changes may not be less, _notDone
is slightly shorter and easier to read in the output.
var _iteratorAbruptCompletion = false; | ||
var _didIteratorError = false; | ||
var _iteratorError; | ||
var _iterator = babelHelpers.asyncIterator(y), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If calling y[Symbol.asyncIterator]()
throws, does this still behave the same?
} finally { | ||
try { | ||
if (_iteratorAbruptCompletion && _iterator.return != null) { | ||
if (_notDone && _iterator.return) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If _iterator.return
is falsy but not nullish, we should still call it to get the proper TypeError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only undefined should skip the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been thinking about this more, and I think we can avoid the catch
block. Does this code lead to smaller minified size, and still passes all the tests?
const buildForAwait = template.statements(`
var ITERATOR_KEY = GET_ITERATOR(OBJECT), STEP_KEY, NOT_DONE_KEY, ITERATOR_HAD_NO_ERROR_KEY;
try {
for (
;
NOT_DONE_KEY = !(STEP_KEY = await ITERATOR_KEY.next()).done;
NOT_DONE_KEY = false
) {
}
ITERATOR_HAD_NO_ERROR_KEY = true;
} finally {
try {
if (NOT_DONE_KEY && ITERATOR_KEY.return) {
await ITERATOR_KEY.return();
}
} catch (e) {
if (!ITERATOR_HAD_NO_ERROR_KEY) throw e;
}
}
`);
It might have problems if |
Yes, to follow the spec, you need to know whether the loop body completed with
And the only way to tell the difference, is to catch the exception. |
Fixes #1, Fixes #2