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

Async functions and await expressions #446

Closed
wants to merge 10 commits into from

Conversation

mysticatea
Copy link
Contributor

@mysticatea mysticatea commented Aug 5, 2016

Async functions proposal arrived stage 4.
I'm investigating to implement async/await.
This is work in progress, but I'm happy to be reviewed and asked any concerns.

If acorn will not consider accepting stage 4 features, please close this PR.

  • basic
    • async function declaration
    • async function expression
    • async arrow function expression
    • async method
    • await expression
  • early errors
    • async function declaration
    • async function expression
    • async arrow function expression
    • async method
    • await expression
  • tests
    • async function declaration
    • async function expression
    • async arrow function expression
    • async method
    • await expression
  • for loose parser
    • async function declaration
    • async function expression
    • async arrow function expression
    • async method
    • await expression

@mysticatea mysticatea force-pushed the asyncawait branch 6 times, most recently from e79ed22 to 607f0e0 Compare August 7, 2016 13:17
@mysticatea
Copy link
Contributor Author

mysticatea commented Aug 7, 2016

@marijnh @RReverser

I think I have finished the implementation of async/await for the normal parser of Acorn.
(Except early errors for super. Those of normal functions also have not been addressed (#448, #449), so I'd like to separate PR.)

If Acorn will support the new stage 4 feature, I'm happy if you review this PR.
(CI build is failed since I have not implemented the loose parser.)

@mysticatea
Copy link
Contributor Author

I have done implementing for the loose parser.

@mysticatea mysticatea changed the title [WIP] Async functions and await expressions Async functions and await expressions Aug 7, 2016
@mysticatea
Copy link
Contributor Author

@marijnh @RReverser Could you reply any response, please?

this.parsePropertyName(prop)
this.parsePropertyValue(prop, isPattern, isGenerator, startPos, startLoc, refDestructuringErrors)
this.parsePropertyName(prop, refDestructuringErrors)
if (!isPattern && this.options.ecmaVersion >= 8 && !isGenerator && !prop.computed && prop.key.type === "Identifier" && prop.key.name === "async" && (this.type === tt.name || this.type === tt.bracketL) && !this.canInsertSemicolon()) {
Copy link
Contributor

@xiemaisi xiemaisi Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.type === tt.name || this.type === tt.bracketL

This doesn't seem quite sufficient; according to the spec, async methods can have arbitrary PropertyNames as their name, so the following should probably be allowed:

({
  async "m"() {},
  async 42() {}
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the condition by this.type !== tt.parenL.
Thank you!

@mysticatea
Copy link
Contributor Author

I tweaked the history of this PR to make reviewing easy.

@@ -71,7 +71,7 @@ export class Parser {
this.potentialArrowAt = -1

// Flags to track whether we are in a function, a generator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ", an async function"

@mysticatea
Copy link
Contributor Author

I fixed the taught problem.
And I rebased to resolve conflicts.

@rictic rictic mentioned this pull request Aug 22, 2016
@mysticatea
Copy link
Contributor Author

@marijnh @RReverser Could you tell me what I should do to advance this PR? I will do accordingly.

node.computed = true
this.expect(tt.bracketR)
base = this.finishNode(node, "MemberExpression")
} else if (!noCalls && this.eat(tt.parenL)) {
let refDestructuringErrors = new DestructuringErrors
let exprList = this.parseExprList(tt.parenR, false, false, refDestructuringErrors)
if (maybeAsyncArrow && !this.canInsertSemicolon() && this.eat(tt.arrow)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is possible for maybeAsyncArrow to be true (which depends on base.name == "async") and this branch, whose conditional uses this.eat(tt.parentL), to also be taken, since both would be referring to the same token, which can't both be async and (. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, never mind, the original expression is inspecting the node, not the token state. Ignore me.

@marijnh
Copy link
Member

marijnh commented Sep 5, 2016

I've merged this as c3cac9c...7304648 and followed up with some small patches 7304648...78e8aa7. Please take a look at those and see if you agree.

I'm not very happy with the further complexity around the refDestructuringErrors kludge, but that was already bad before, you just made it a little more complex (and caused it to allocate a bunch more objects). We should figure out a cleaner and less error-prone way to express this at some point, but I don't think that's something this PR should wait for.

@marijnh marijnh closed this Sep 5, 2016
@marijnh
Copy link
Member

marijnh commented Sep 5, 2016

(I should add that my benchmarks don't notice a measurable slowdown due to those extra error-tracking objects that are allocated.)

@RReverser
Copy link
Member

@marijnh Small short-lived objects are allocated from a "fast storage" in most modern engine GCs, so at least performance-wise it shouldn't be a problem.

@marijnh
Copy link
Member

marijnh commented Sep 5, 2016

@RReverser That's true, but such objects aren't free -- they'll still cause more minor GCs to happen, which has a cost.

@RReverser
Copy link
Member

@marijnh Unless an optimizer detects that those objects can be used directly from the stack. Although this is implementation-dependant and probably not something to be relied upon (as well as any performance guesses).

@mysticatea mysticatea deleted the asyncawait branch September 5, 2016 20:26
@TrySound
Copy link

TrySound commented Sep 6, 2016

What's a plan for release?
This DestructuringErrors.call makes me sad.

@marijnh
Copy link
Member

marijnh commented Sep 7, 2016

What's a plan for release?

See this.

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

Successfully merging this pull request may close these issues.

None yet

5 participants