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 await binding error within static block #13088

Merged
merged 4 commits into from Jul 16, 2021

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 1, 2021

Q                       A
Fixed Issues? Implements tc39/proposal-class-static-block#46
Tests Added + Pass? Yes
License MIT

This PR aligns Babel behaviour to current V8 implementation, which follows Contains Static Semantics. However since this behaviour is not specified in that way, we have ongoing discussions in tc39/proposal-class-static-block#43

I will mark this PR as ready when consensus is reached.

This PR implements tc39/proposal-class-static-block#46. The spec now adds [Await] production parameter to static block and throw if it contains (specified in SDO containsAwait) AwaitExpression and for await. However implementing the SDO containsAwait requires scan of whole AST tree, which is not performant. Babel parses the StaticBlock in [~Await] and uses parser scopes to throw if an await identifier is within the static block.

See scope#inStaticBlock for current implementation detail.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Static Block labels Apr 1, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 1, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47370/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0ce8c72:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jun 9, 2021
@JLHwung JLHwung marked this pull request as ready for review June 9, 2021 14:59
@JLHwung JLHwung force-pushed the fix-static-block-await branch 3 times, most recently from 0aeebe9 to c116d2f Compare June 9, 2021 15:53
@KFlash
Copy link

KFlash commented Jun 13, 2021

@JLHwung Why not set 'this.scope.inStaticBlock' when you enter a static block, and unset this again every time you enter function body, arrow or block statement, and then for await check if this is set and if so, throw.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 14, 2021

@KFlash Besides function body, arrow, we should also take care of the class field initializers. A plain block statement { expr } inherit the await production, so we should not reset for plain block. Essentially it should be maintained in whatever production that does not inherit [Await], which is easily overlooked when new language feature is introduced. A boolean state has fastest access time, however such state inStaticBlock will only be accessed when we see a binding identifier named await, which is rare.

I think the current approach is easier to maintain because we already have to push a lexical scope for static block, so an extra mark on the scope, compacted to bit arrays, does not introduce extra recording tasks. The worst scenario of current approach is an await nested in N block statements within a static block, while the boolean approach certainly performs better in this scenario.

@KFlash
Copy link

KFlash commented Jun 14, 2021

@JLHwung An easy solution for this is to "copy" what the author of this proposal did. Look at the open PR for implementation of static block in the Typescript repo.

@JLHwung JLHwung added this to To review in Nicolò's ideal PR review order list via automation Jun 24, 2021
@JLHwung JLHwung merged commit 6e57617 into babel:main Jul 16, 2021
Nicolò's ideal PR review order list automation moved this from To review to Done Jul 16, 2021
@JLHwung JLHwung deleted the fix-static-block-await branch July 16, 2021 14:35
nicolo-ribaudo pushed a commit to nicolo-ribaudo/babel that referenced this pull request Jul 30, 2021
* fix: allow await within SCOPE_FUNCTION under static block

* perf: scan scopeStack for once

* add new test case

* chore: update allowlist
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2021
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 PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Class Static Block
Development

Successfully merging this pull request may close these issues.

None yet

5 participants