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
Ensure call return() for an abrupt completion at for await of loop #52754
Ensure call return() for an abrupt completion at for await of loop #52754
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@microsoft-github-policy-service agree |
I encountered the same problem today. The simplest workarounds include
|
An MCVE:
{
"devDependencies": {
"@types/node": "18.14.0",
"typescript": "4.9.5"
},
"type": "module"
} tsconfig.json {
"compilerOptions": {
"target": "ES2017",
"rootDir": ".",
"outDir": "dist",
"module": "Node16"
}
}
const asyncIterable = {
[Symbol.asyncIterator]() {
return {
next() {
console.log('next()');
return { value: '!', done: false };
},
return(value) {
console.log('return()');
return { value, done: true };
},
};
}
};
for await (const x of asyncIterable) {
console.log(x);
break;
} code generated: var __asyncValues = (this && this.__asyncValues) || function (o) {
if (!Symbol.asyncIterator) throw new TypeError("Symbol.asyncIterator is not defined.");
var m = o[Symbol.asyncIterator], i;
return m ? m.call(o) : (o = typeof __values === "function" ? __values(o) : o[Symbol.iterator](), i = {}, verb("next"), verb("throw"), verb("return"), i[Symbol.asyncIterator] = function () { return this; }, i);
function verb(n) { i[n] = o[n] && function (v) { return new Promise(function (resolve, reject) { v = o[n](v), settle(resolve, reject, v.done, v.value); }); }; }
function settle(resolve, reject, d, v) { Promise.resolve(v).then(function(v) { resolve({ value: v, done: d }); }, reject); }
};
var _a, e_1, _b, _c;
const asyncIterable = {
[Symbol.asyncIterator]() {
return {
next() {
console.log('next()');
return { value: '!', done: false };
},
return(value) {
console.log('return()');
return { value, done: true };
},
};
}
};
try {
for (var _d = true, asyncIterable_1 = __asyncValues(asyncIterable), asyncIterable_1_1; asyncIterable_1_1 = await asyncIterable_1.next(), _a = asyncIterable_1_1.done, !_a;) {
_c = asyncIterable_1_1.value;
_d = false;
try {
const x = _c;
console.log(x);
break;
}
finally {
_d = true;
}
}
}
catch (e_1_1) { e_1 = { error: e_1_1 }; }
finally {
try {
// _d is always true
if (!_d && !_a && (_b = asyncIterable_1.return)) await _b.call(asyncIterable_1);
}
finally { if (e_1) throw e_1.error; }
}
export {}; |
@niceandneat Can you please open a bug first? Please include @kbridge 's repro if it matches your problem. |
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.
Unfortunately, the proposed changes break continue
. I have an alternative solution I'm investigating, however.
I experimented a bit, and we can definitely drop the inner // source
for await (const x of y) {
console.log(x);
}
// generated
var _a, e_1, _b, _c;
try {
for (
// start in non-user code
/*initializer*/ var _d = true, y_1 = __asyncValues(y), y_1_1;
/*condition*/ y_1_1 = await y_1.next(), _a = y_1_1.done, !_a;
/*incrementor*/ _d = true // <- switch back to non-user code
) {
_c = y_1_1.value;
_d = false; // <- switch to user code
const x = _c;
// we can remove the inner try..finally and just inline the original body:
console.log(x);
}
}
catch (e_1_1) { e_1 = { error: e_1_1 }; }
finally {
try {
if (!_d && !_a && (_b = y_1.return)) await _b.call(y_1);
}
finally { if (e_1) throw e_1.error; }
} This will allow local @niceandneat: do you want to update this PR with this change, or should I create a new one? |
@rbuckton Thanks a lot! I will update this PR with your changes and let you know. |
@rbuckton I applied some changes to inline the original body. If It has any problem, please let me know. |
src/compiler/transformers/es2018.ts
Outdated
return setEmitFlags( | ||
factory.createBlock( | ||
setTextRange(factory.createNodeArray(statements), statementsLocation), | ||
/*multiLine*/ true | ||
), | ||
EmitFlags.NoSourceMap | EmitFlags.NoTokenSourceMaps |
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 should preserve bodyLocation
and avoid setting these flags.
assert.isTrue(await result.main()); | ||
}); | ||
|
||
it("don't call return when user code continue (es2015)", async () => { |
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 add tests for break
, return
, and non-local continue
(i.e., a labeled continue
pointing to a label in an outer loop)?
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 also added local label continue
case.
After #51297, early exit(e.g.
throw
) fromfor await...of
statement body does not trigger.return()
of the asyncIterator. This PR removestry/finally
block to prevent anonUserCode
variable from being settrue
when code exit early.Before
After
It makes iterator call
.return()
after for loop exits early and fits more with the specification.Fixes #53106
Fixes #52936