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

[Transforms] Down-level transformations for Async Functions #9175

Merged
merged 31 commits into from Jul 20, 2016

Conversation

Projects
None yet
10 participants
@rbuckton
Member

rbuckton commented Jun 15, 2016

This change adds support for transforming a subset of generator function features to a down-level representation to support async functions when targeting ES5 or ES3.

@rbuckton

This comment has been minimized.

Show comment
Hide comment
@@ -139,7 +139,7 @@ namespace ts {
return node;
}
export function createTempVariable(recordTempVariable: (node: Identifier) => void, location?: TextRange): Identifier {
export function createTempVariable(recordTempVariable: ((node: Identifier) => void) | undefined, location?: TextRange): Identifier {

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

Why not recordTempVariable?: (node: Identifier) => void?

@weswigham

weswigham Jun 17, 2016

Member

Why not recordTempVariable?: (node: Identifier) => void?

This comment has been minimized.

@rbuckton

rbuckton Jun 17, 2016

Member

To catch mistakes. Its acceptable to not record the temp variable, but you almost always want to. The only cases where we don't are when we're creating a temp parameter, or we are going to add the temp variable name to a VariableDeclarationList ourselves.

@rbuckton

rbuckton Jun 17, 2016

Member

To catch mistakes. Its acceptable to not record the temp variable, but you almost always want to. The only cases where we don't are when we're creating a temp parameter, or we are going to add the temp variable name to a VariableDeclarationList ourselves.

@@ -324,17 +324,19 @@ namespace ts {
const node = <ArrayLiteralExpression>createNode(SyntaxKind.ArrayLiteralExpression, location);
node.elements = parenthesizeListElements(createNodeArray(elements));
if (multiLine) {

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

node.multiLine = !!multiLine? Or does it really need to not exist when not set?

@weswigham

weswigham Jun 17, 2016

Member

node.multiLine = !!multiLine? Or does it really need to not exist when not set?

This comment has been minimized.

@rbuckton

rbuckton Jun 17, 2016

Member

Setting it unnecessarily could introduce a new hidden class and cause deoptimizations, and increases the number of bytes per node.

@rbuckton

rbuckton Jun 17, 2016

Member

Setting it unnecessarily could introduce a new hidden class and cause deoptimizations, and increases the number of bytes per node.

}
return node;
}
export function createObjectLiteral(properties?: ObjectLiteralElement[], location?: TextRange, multiLine?: boolean) {
const node = <ObjectLiteralExpression>createNode(SyntaxKind.ObjectLiteralExpression, location);
node.properties = createNodeArray(properties);
if (multiLine) {

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

Same question as above.

@weswigham

weswigham Jun 17, 2016

Member

Same question as above.

This comment has been minimized.

@rbuckton

rbuckton Jun 17, 2016

Member

Same answer as above ;)

@rbuckton

rbuckton Jun 17, 2016

Member

Same answer as above ;)

return node;
}
export function createTryCatchFinally(tryBlock: Block, catchClause: CatchClause, finallyBlock: Block, location?: TextRange) {

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

Since catchClause and finallyBlock are both optional/undefinable (as done below) - why not annotate them both with a ??

@weswigham

weswigham Jun 17, 2016

Member

Since catchClause and finallyBlock are both optional/undefinable (as done below) - why not annotate them both with a ??

This comment has been minimized.

@rbuckton

rbuckton Jun 17, 2016

Member

Because a try with neither a catch nor a finally is illegal. I could make finallyBlock optional, but there are fewer places where that matters.

@rbuckton

rbuckton Jun 17, 2016

Member

Because a try with neither a catch nor a finally is illegal. I could make finallyBlock optional, but there are fewer places where that matters.

Show outdated Hide outdated src/compiler/factory.ts
@@ -870,7 +923,7 @@ namespace ts {
return <Expression>createBinary(left, SyntaxKind.LessThanToken, right, location);
}
export function createAssignment(left: Expression, right: Expression, location?: TextRange) {
export function createAssignment(left: Expression, right: Expression, location?: TextRange, original?: Node) {

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

The new optional parameter original appears to be unused?

@weswigham

weswigham Jun 17, 2016

Member

The new optional parameter original appears to be unused?

Show outdated Hide outdated src/compiler/transformers/es6.ts
@@ -1684,6 +1686,119 @@ namespace ts {
return convertIterationStatementBodyIfNecessary(node);
}
// // TODO(rbuckton): Switch to using __values helper for for..of?

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

I mean, it looks pretty nice/clean - though I imagine the difference in capabilities/output size/runtime speed matter more. In any case, this block should either be un-commented and used to replace the other visitForOfStatement or removed. ;)

Though, if it's not strictly required for downlevel generators, I imagine it needs to be moved into a followup PR.

@weswigham

weswigham Jun 17, 2016

Member

I mean, it looks pretty nice/clean - though I imagine the difference in capabilities/output size/runtime speed matter more. In any case, this block should either be un-commented and used to replace the other visitForOfStatement or removed. ;)

Though, if it's not strictly required for downlevel generators, I imagine it needs to be moved into a followup PR.

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jun 17, 2016

Member

We need to handle directive prologues ("use strict";) when transforming async functions - I looked at the output of the tests/cases/conformance/async/es6/await[Binary,Call]Expression_es6 tests on this branch (which conveniently always have a string as their first statement!) and noticed that we move prologues into the generator function we synthesize - this is likely incorrect (even if it's our current behavior) if the consumer is using custom prologues (if they were just using "use strict" they may never notice). In any case, generators, also, need to special case retaining prologues as the first statement in the original outer function declaration (though, maybe not applicable right now since generators only arise as a consequence of async functions).

Also, would it be possible to just see the results of those same conformance tests running vs es5/es3? I know you added a bunch of new tests, but simply replicating the tests/cases/conformance/async/es6 directory for es5 and maybe es3 would be pertinent with this change. (And allow useful comparisons) #Resolved

Member

weswigham commented Jun 17, 2016

We need to handle directive prologues ("use strict";) when transforming async functions - I looked at the output of the tests/cases/conformance/async/es6/await[Binary,Call]Expression_es6 tests on this branch (which conveniently always have a string as their first statement!) and noticed that we move prologues into the generator function we synthesize - this is likely incorrect (even if it's our current behavior) if the consumer is using custom prologues (if they were just using "use strict" they may never notice). In any case, generators, also, need to special case retaining prologues as the first statement in the original outer function declaration (though, maybe not applicable right now since generators only arise as a consequence of async functions).

Also, would it be possible to just see the results of those same conformance tests running vs es5/es3? I know you added a bunch of new tests, but simply replicating the tests/cases/conformance/async/es6 directory for es5 and maybe es3 would be pertinent with this change. (And allow useful comparisons) #Resolved

@weswigham

This comment has been minimized.

Show comment
Hide comment
@weswigham

weswigham Jun 17, 2016

Member

I don't think there's any issues, but I couldn't find a test to confirm - I don't see any tests (existing or otherwise) covering constructs such as class extends (await whatever) (which should be valid within an async function). Given we check our emit for await pretty much everywhere else, we should probably add tests in an extends clause.

Member

weswigham commented Jun 17, 2016

I don't think there's any issues, but I couldn't find a test to confirm - I don't see any tests (existing or otherwise) covering constructs such as class extends (await whatever) (which should be valid within an async function). Given we check our emit for await pretty much everywhere else, we should probably add tests in an extends clause.

Show outdated Hide outdated tests/baselines/reference/exportEqualsAmd.js
@@ -4,6 +4,6 @@ export = { ["hi"]: "there" };
//// [exportEqualsAmd.js]
define(["require", "exports"], function (require, exports) {
"use strict";
return (_a = {}, _a["hi"] = "there", _a);
return _a = {}, _a["hi"] = "there", _a;

This comment has been minimized.

@weswigham

weswigham Jun 17, 2016

Member

Is the removal of the parenthesis in this test output and the one below intentional? #Resolved

@weswigham

weswigham Jun 17, 2016

Member

Is the removal of the parenthesis in this test output and the one below intentional? #Resolved

This comment has been minimized.

@rbuckton

rbuckton Jun 22, 2016

Member

Yes. The node factories will automatically add parenthesis if they are needed. As a result, many places where we explicitly added parentheses now instead leverage this behavior. In this instance, the createReturn factory does not need to parenthesize, so they are not added.

@rbuckton

rbuckton Jun 22, 2016

Member

Yes. The node factories will automatically add parenthesis if they are needed. As a result, many places where we explicitly added parentheses now instead leverage this behavior. In this instance, the createReturn factory does not need to parenthesize, so they are not added.

Show outdated Hide outdated src/compiler/binder.ts
@@ -2587,9 +2593,14 @@ namespace ts {
// If a FunctionDeclaration has an asterisk token, is exported, or its

This comment has been minimized.

@yuit

yuit Jun 20, 2016

Contributor

The comment need to be updated I think

@yuit

yuit Jun 20, 2016

Contributor

The comment need to be updated I think

Show outdated Hide outdated src/compiler/binder.ts
@@ -2608,10 +2619,15 @@ namespace ts {
// If a FunctionExpression contains an asterisk token, or its subtree has marked the container
// as needing to capture the lexical this, then this node is ES6 syntax.
if (asteriskToken || (subtreeFlags & TransformFlags.ES6FunctionSyntaxMask)) {
if (subtreeFlags & TransformFlags.ES6FunctionSyntaxMask) {

This comment has been minimized.

@yuit

yuit Jun 20, 2016

Contributor

I think comment need to be updated?

@yuit

yuit Jun 20, 2016

Contributor

I think comment need to be updated?

Show outdated Hide outdated src/compiler/binder.ts
transformFlags |= TransformFlags.AssertES6;
}
// Currently, we only support generators that were originally async function bodies.
if (asteriskToken && node.emitFlags & NodeEmitFlags.AsyncFunctionBody) {

This comment has been minimized.

@yuit

yuit Jun 20, 2016

Contributor

how can this node.emitFlags & NodeEmitFlags.AsyncFunctionBody be observed here since the flag is set in the transformer's factory.

Also what happen to the case of asteriskToken but not AsyncFunctionBody?

@yuit

yuit Jun 20, 2016

Contributor

how can this node.emitFlags & NodeEmitFlags.AsyncFunctionBody be observed here since the flag is set in the transformer's factory.

Also what happen to the case of asteriskToken but not AsyncFunctionBody?

This comment has been minimized.

@rbuckton

rbuckton Jun 22, 2016

Member

TransformFlags aggregation happens both during the bind phase and during each successive transformation phase (though usually it has less work to do as it can reuse the flags from unchanged subtrees).

In this case, we only add the flag if the generator was created as a result of an async function transformation.

We do not down-level generators that are not the result of an async function transformation.

@rbuckton

rbuckton Jun 22, 2016

Member

TransformFlags aggregation happens both during the bind phase and during each successive transformation phase (though usually it has less work to do as it can reuse the flags from unchanged subtrees).

In this case, we only add the flag if the generator was created as a result of an async function transformation.

We do not down-level generators that are not the result of an async function transformation.

rbuckton added some commits Jun 28, 2016

Added additional es5 conformance tests, better emit for functions ret…
…urning global promise, pass this to __generator
Added es5 conformance tests for async arrow functions, add error for …
…referencing 'arguments' in a generator.
@rbuckton

This comment has been minimized.

Show comment
Hide comment
@rbuckton

rbuckton Jun 28, 2016

Member

@yuit, @weswigham: Please take another look following the recent commits.
@mhegazy, @vladima, @DanielRosenwasser: Any comments?

Member

rbuckton commented Jun 28, 2016

@yuit, @weswigham: Please take another look following the recent commits.
@mhegazy, @vladima, @DanielRosenwasser: Any comments?

@rbuckton rbuckton merged commit d4ad7f3 into transforms Jul 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@rbuckton rbuckton deleted the transforms-generators branch Jul 20, 2016

@rbuckton rbuckton restored the transforms-generators branch Jul 20, 2016

@olee

This comment has been minimized.

Show comment
Hide comment
@olee

olee Jul 22, 2016

Does this PR being merged mean we can use async/await now with transpilation to < ES6?

olee commented Jul 22, 2016

Does this PR being merged mean we can use async/await now with transpilation to < ES6?

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Jul 22, 2016

Member

It's merged with the transforms branch, not master. It won't ship until transforms merges into master.

Member

sandersn commented Jul 22, 2016

It's merged with the transforms branch, not master. It won't ship until transforms merges into master.

@weswigham weswigham deleted the transforms-generators branch Aug 11, 2016

@cime

This comment has been minimized.

Show comment
Hide comment
@cime

cime Sep 1, 2016

Anything new on this one? When will it be available in typescript@next?

cime commented Sep 1, 2016

Anything new on this one? When will it be available in typescript@next?

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Sep 1, 2016

Contributor

should start showing up in typescript@next next week.

Contributor

mhegazy commented Sep 1, 2016

should start showing up in typescript@next next week.

@nippur72

This comment has been minimized.

Show comment
Hide comment
@nippur72

nippur72 Sep 18, 2016

those using tslib and noEmitHelpers must install tslib from github as the npm package is not updated yet (and thus does not contain __generator).

those using tslib and noEmitHelpers must install tslib from github as the npm package is not updated yet (and thus does not contain __generator).

@tony19 tony19 referenced this pull request Oct 2, 2016

Merged

Add support for http2 #98

atsu85 referenced this pull request in aurelia/template-lint Oct 9, 2016

@heycalmdown heycalmdown referenced this pull request Oct 10, 2016

Closed

ES5 지원 #10

@jkobylec

This comment has been minimized.

Show comment
Hide comment
@jkobylec

jkobylec Oct 14, 2016

I got excited enough by @mghegazy's comment earlier that I went ahead and installed typescript@next on my project to start refactoring some of my more labyrinthine promise usage into async/await. And did not bother to check whether transforms had actually been merged into master 😄

And I was like, whoa, this already works when targeting ES5! But then I realized the output is still using ES6 generators, which just happen to work in Chrome and the iOS 10 webview I was testing in 🤐

I got excited enough by @mghegazy's comment earlier that I went ahead and installed typescript@next on my project to start refactoring some of my more labyrinthine promise usage into async/await. And did not bother to check whether transforms had actually been merged into master 😄

And I was like, whoa, this already works when targeting ES5! But then I realized the output is still using ES6 generators, which just happen to work in Chrome and the iOS 10 webview I was testing in 🤐

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Oct 15, 2016

Contributor

And I was like, whoa, this already works when targeting ES5! But then I realized the output is still using ES6 generators, which just happen to work in Chrome and the iOS 10 webview I was testing in

@jkobylec i do not think this is accurate. Emitting async functions for ES3/ES5 is supported in typescript@next.

here is a demonstration.

c:\test>tsc --v
Version 2.1.0-dev.20161014

c:\test>type a.ts
async function test() {
    await bar();
}

c:\test>tsc --target ES5 --lib es5,es2015.promise a.ts
a.ts(2,11): error TS2304: Cannot find name 'bar'.

c:\test>type a.js
var __awaiter =...
var __generator = ...

function test() {
    return __awaiter(this, void 0, void 0, function () {
        return __generator(this, function (_a) {
            switch (_a.label) {
                case 0: return [4 /*yield*/, bar()];
                case 1:
                    _a.sent();
                    return [2 /*return*/];
            }
        });
    });
}

Also i would recommend creating a new issue instead of commenting on a closed PR. thanks.

Contributor

mhegazy commented Oct 15, 2016

And I was like, whoa, this already works when targeting ES5! But then I realized the output is still using ES6 generators, which just happen to work in Chrome and the iOS 10 webview I was testing in

@jkobylec i do not think this is accurate. Emitting async functions for ES3/ES5 is supported in typescript@next.

here is a demonstration.

c:\test>tsc --v
Version 2.1.0-dev.20161014

c:\test>type a.ts
async function test() {
    await bar();
}

c:\test>tsc --target ES5 --lib es5,es2015.promise a.ts
a.ts(2,11): error TS2304: Cannot find name 'bar'.

c:\test>type a.js
var __awaiter =...
var __generator = ...

function test() {
    return __awaiter(this, void 0, void 0, function () {
        return __generator(this, function (_a) {
            switch (_a.label) {
                case 0: return [4 /*yield*/, bar()];
                case 1:
                    _a.sent();
                    return [2 /*return*/];
            }
        });
    });
}

Also i would recommend creating a new issue instead of commenting on a closed PR. thanks.

@jkobylec

This comment has been minimized.

Show comment
Hide comment
@jkobylec

jkobylec Oct 15, 2016

My assumption was that the incorporation into master was still in progress
which is why I didn't open a new issue, but seems like the behavior I'm
encountering may be an oddity in my build tool chain I'll need to track
down, then. Apologies if I caused any alarm.
On Fri, Oct 14, 2016 at 5:35 PM Mohamed Hegazy notifications@github.com
wrote:

And I was like, whoa, this already works when targeting ES5! But then I
realized the output is still using ES6 generators, which just happen to
work in Chrome and the iOS 10 webview I was testing in

@jkobylec https://github.com/jkobylec i do not think this is accurate.
Emitting async functions for ES3/ES5 is supported in typescript@next.

here is a demonstration.

c:\test>tsc --v
Version 2.1.0-dev.20161014

c:\test>type a.ts
async function test() {
await bar();
}

c:\test>tsc --target ES5 --lib es5,es2015.promise a.ts
a.ts(2,11): error TS2304: Cannot find name 'bar'.

c:\test>type a.js
var __awaiter =...
var __generator = ...

function test() {
return __awaiter(this, void 0, void 0, function () {
return __generator(this, function (_a) {
switch (_a.label) {
case 0: return [4 /yield/, bar()];
case 1:
_a.sent();
return [2 /return/];
}
});
});
}

Also i would recommend creating a new issue instead of commenting on a
closed PR. thanks.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9175 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHtc0jzelV11ieHBGLK28go-4apOf1c1ks5q0B-8gaJpZM4I16H6
.

My assumption was that the incorporation into master was still in progress
which is why I didn't open a new issue, but seems like the behavior I'm
encountering may be an oddity in my build tool chain I'll need to track
down, then. Apologies if I caused any alarm.
On Fri, Oct 14, 2016 at 5:35 PM Mohamed Hegazy notifications@github.com
wrote:

And I was like, whoa, this already works when targeting ES5! But then I
realized the output is still using ES6 generators, which just happen to
work in Chrome and the iOS 10 webview I was testing in

@jkobylec https://github.com/jkobylec i do not think this is accurate.
Emitting async functions for ES3/ES5 is supported in typescript@next.

here is a demonstration.

c:\test>tsc --v
Version 2.1.0-dev.20161014

c:\test>type a.ts
async function test() {
await bar();
}

c:\test>tsc --target ES5 --lib es5,es2015.promise a.ts
a.ts(2,11): error TS2304: Cannot find name 'bar'.

c:\test>type a.js
var __awaiter =...
var __generator = ...

function test() {
return __awaiter(this, void 0, void 0, function () {
return __generator(this, function (_a) {
switch (_a.label) {
case 0: return [4 /yield/, bar()];
case 1:
_a.sent();
return [2 /return/];
}
});
});
}

Also i would recommend creating a new issue instead of commenting on a
closed PR. thanks.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9175 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHtc0jzelV11ieHBGLK28go-4apOf1c1ks5q0B-8gaJpZM4I16H6
.

@kitsonk kitsonk referenced this pull request Nov 15, 2016

Closed

Migrate to TypeScript 2.1 #111

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.