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

fix: Class Field Initializer should not allow await expression as immediate child #10946

Merged
merged 3 commits into from
Dec 31, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Dec 30, 2019

Q                       A
Fixed Issues? Fixes #6687
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR fixes the isAwaitAllowed logic by excluding await identifier reference immediately nested in a class property initializer.

Per https://tc39.es/proposal-class-fields/#sec-new-syntax

FieldDefinition[Yield, Await]:
   ClassElementName[?Yield, ?Await] Initializer[In, ~Yield, ~Await]opt

the spec dictates that the Initializer can not contain IdentifierReference_Yield or IdentifierReference_Await, which means await should be considered as identifier reference here, unless its [Await] parameter is enabled by other language structures.

The original snippet in #6687

async function a() {
  class Foo {
    bar = await baz();
  }
}

will still throw because await is an identifier instead of await expression, but not because the constructor is not an async function. Think of the following example, which should still throw even the static property does not need a constructor to initialize.

async function a() {
  class Foo {
    static bar = await baz();
  }
}

Note that we have a similar issue regarding to the generators:
Parsing the following code should throw because yield is an identifier, which turns out to be a reserved word since it resides in a class declaration where all parts are in strict code mode.

function* f() {
  return class { p = yield + 42 }
}

I will address this in a separate PR.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Class Fields pkg: parser labels Dec 30, 2019
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Do we have tests for await inside computed keys? I tried searching [await but couldn't find them.

async function f() {
  class A {
    [await 1]() {}
  }
}

@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 31, 2019

@nicolo-ribaudo Test is added. The computed properties are not affected here because it is never in the class scope. The class scope are applied to the property initializer and the method body.

But I feel like if we really want to be 100% spec compliant, we should introduce noAwait and noYield as acorn did in noIn, and align these context states to the grammar. Checking noAwait via scope and awaitPos still looks ad-hoc to me.

@JLHwung JLHwung merged commit 26eb891 into babel:master Dec 31, 2019
@JLHwung JLHwung deleted the fix-6687 branch December 31, 2019 14:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Class Fields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong await parsing
3 participants