-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement Async Functions #692
Conversation
Accidentally submitted (still working on the TOC)... |
314761c
to
9db23b2
Compare
Ok good to go! |
Added changes to evalute body: removed first steps from https://bterlson.github.io/ecma262/#sec-ordinarycallevaluatebody and moved to corresponding EvaluateBodys (eg. https://bterlson.github.io/ecma262/#sec-function-definitions-runtime-semantics-evaluatebody). Rationale in this note. |
Consider moving CreateDynamicFunction somewhere else, as it's now being used from multiple places and it's less appropriate to nest it under Function. On the other hand, I can't find any good place to put it. I think the section "Async Function Objects" should be consistent with "GeneratorFunction Objects", i.e. space or no space. |
"Note: Async functions are not constructable and do not have a prototype property." seems to contradict the line above it where you say "Else [if not a generator], perform MakeConstructor(F)". |
The note below also would need adjusting: "A prototype property is automatically created for every function created using CreateDynamicFunction, to provide for the possibility that the function will be used as a constructor." |
Indeed! I put the note but didn't fix up the else leg. Will fix that plus the note. |
Minor issue I noticed in debugging our async/await implementation: in https://bterlson.github.io/ecma262/#abstract-ops-async-function-await , you have to set |
It would be really nice if you could factor out a version of PerformPromiseThen which did not need to deal with the throwawayCapability business. It's shown up in multiple specs now. I think the idea would be to let the [[Capability]] field of PromiseReaction be undefined (noting that this is for spec-internal situations), add a check in PromiseReactionJob, and tweak PerformPromiseThen so that promiseCapability is an optional argument that defaults to undefined (i.e. change the assert and return value). |
@domenic I think such a refactoring could be left to a later patch. |
@domenic yeah I'm leaning toward a separate refactoring patch for that. Can you open an issue on it (hopefully with an example I can take a look at)? |
<a href="#async-function-definitions-awaited-fulfilled">AsyncFunction Awaited Fulfilled</a> this should use emu-xref instead like the other closures do. |
this is very terse compared to existing closure declarations, and should probably be expanded to be more like those (intro paragraph, internal slot list, when called, length specifier). |
Also the section header should include "Functions" |
per recent changes, should be "Let asyncContext be F.[[AsyncContext]]". I'd suggest doing a search for "the value of" to find other instances. |
In reviewing this further, I think the last steps of AsyncFunction Awaited Fulfilled/Rejected, where it returns the completion value, do not make much sense. It's not returning to anyone interesting; it would only impact the throwawayCapability.[[Promise]], which is, well, a throwaway. |
Note to self: I also need to define the [[AsyncContext]] internal slot somewhere. |
This spec has not been updated for the great uppercasing of record fields. In particular completion record [[value]], [[target]], and [[type]] are all now uppercase. |
…romises This patch implements a bug fix to the async/await specification described at tc39/ecma262#692 (comment) Namely, the intermediate values of Promises may be rejected, and they do not have .then called on them anymore (now that the memory leak is fixed), but they do not correspond do unhandled rejections. This change has been tested manually with integration with Blink; once it is checked in and rolled, then further tests can be added on the Blink side for the uncaught rejection handler and async/await. BUG=v8:4483 Review-Url: https://codereview.chromium.org/2338273007 Cr-Commit-Position: refs/heads/master@{#39480}
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 editorial tweaks, but I don't know how to submit a PR to a PR.
@jmdyck: send the PR to this repo + branch: https://github.com/bterlson/ecma262/tree/async-functions. Then once the PR is accepted there it will show up here. |
Alternatively you can just push your fixes someplace and I can pull them in outside of the GitHub interface. Whichever is easiest for you! |
@jmdyck are your commits already ready to go? If so I can wait a bit before making further changes so as not to create rebase headaches... |
I've just finished making all the changes on my local branch, but I haven't started doing any commits yet. If you've got stuff ready to push, you should probably just push it, and I'll deal with the headaches. In order to submit a PR against your repo, I'd have to have a fork of it on github, right? But jmdyck/ecma262 already exists as a fork of tc39/ecma262, so I'm not sure how I could have another fork of ecma262. |
I also don't have commits yet, just started on the rest of Domenic's issues. I'll probably want to push something later today. If you already have a fork, easiest way using the GitHub UI is to push your commits and hit the pull request button on your repo. On the next screen you can change the "base fork" to bterlson/ecma262 and base branch to async-functions, then create the PR. |
Okay, I've finished all my commits. I'll see if I can make a PR. |
…to true in AsyncFunctionAwait
@littledan Right, added that plus the definition of [[AsyncContext]]. |
LGTM (AFAICT!) |
Editorial nit: I think section 19.2.1 will no longer be accurate after this is merged.
Maybe that section should mention |
@not-an-aardvark you are correct, I will fix that up! |
Alright, gonna pull this in now. We can of course make further updates up 'til we freeze for ES2017. |
The nonterminals: - FunctionExpression - ClassExpression - GeneratorExpression - AsyncGeneratorExpression are all defined with a single RHS involving an optional BindingIdentifier. But AsyncFunctionExpression is defined with two RHSs, one with a BindingIdentifier and one without. (It's been that way since it was introduced in PR tc39#692.) I can't see any reason for it to be not like the others.
The nonterminals: - FunctionExpression - ClassExpression - GeneratorExpression - AsyncGeneratorExpression are all defined with a single RHS involving an optional BindingIdentifier. But AsyncFunctionExpression is defined with two RHSs, one with a BindingIdentifier and one without. (It's been that way since it was introduced in PR tc39#692.) I can't see any reason for it to be not like the others.
The nonterminals: - FunctionExpression - ClassExpression - GeneratorExpression - AsyncGeneratorExpression are all defined with a single RHS involving an optional BindingIdentifier. But AsyncFunctionExpression is defined with two RHSs, one with a BindingIdentifier and one without. (It's been that way since it was introduced in PR tc39#692) I can't see any reason for it to be not like the others.
Hi there, I'm curious about why the link up there is not working anymore: https://tc39.github.io/ecmascript-asyncawait Hope that doesn't mean the proposal was somehow reverted from the ES spec :) |
@alkthegit https://github.com/tc39/proposal-async-await and that same content rendered available here as well: https://web.archive.org/web/20201113005044/https://tc39.es/ecmascript-asyncawait/ |
Thanks, but I've already been there and the page also contains the same broken link. What I was hoping to find is a kind of a "long explaner", with an example being here on the pipe operator: Thought it was moved to the resource under the broken link but maybe it just never existed? |
Indeed the proposal repo, and https://tc39.es/proposal-async-await/, are all that exists. Not every proposal chooses the same ways to explain itself. |
Ok, got it. |
Thanks, Chris! |
it was the wrong link. updated above. there are others, at least one more, but that's what I was able to find quickly |
This PR implements async functions as specified in the Async Functions Proposal with minor modifications. This is a big PR with lots of fiddly pieces. As such, I'm sure there are minor editorial and likely technical issues.
To aid in reviewing, I've published a build of the spec with async funtions. Below is a list of changes with deep links. Please help me review this!
await
is a new keyword 11.6.2.1await
added as alternative for various Identifier kinds 12.1 and appropriate early errors 12.1.1.Additionally, any production of the grammar that took a yield parameter now also takes an await parameter. An RHS non-terminal that previously passed a ?Yield parameter now also pass ?Await. RHSes that passed +Yield or
Yield were updated to pass +//?Await as appropriate.