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

Prevent ForOfStatement from printing the forbidden sequence "for ( async of" #13208

Merged
merged 1 commit into from Apr 26, 2021

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Apr 26, 2021

Q                       A
Fixed Issues? Fixes #13207
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

If we detect that a ForOfStatement's left-hand-side is an identifier named async, print extra parentheses around the identifier to avoid this edge-case in the ECMAScript grammar.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 26, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 26, 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 6faed7b:

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

@Zalathar
Copy link
Contributor Author

After writing this I remembered that there's a whole subsystem dedicated to “does this node need parentheses in light of its parent”. Not sure if it makes more sense to to use that instead, or to stick with this ad-hoc code.

@nicolo-ribaudo nicolo-ribaudo added pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 26, 2021
@Zalathar
Copy link
Contributor Author

The alternative would be something like this in parentheses.ts:

export function Identifier(node: t.Identifier, parent: t.Node): boolean {
  return (
    node.name === "async" && t.isForOfStatement(parent) && node === parent.left
  );
}

@existentialism
Copy link
Member

The alternative would be something like this in parentheses.ts:

export function Identifier(node: t.Identifier, parent: t.Node): boolean {
  return (
    node.name === "async" && t.isForOfStatement(parent) && node === parent.left
  );
}

👍 from me

@JLHwung JLHwung merged commit 026e7f5 into babel:main Apr 26, 2021
@JLHwung
Copy link
Contributor

JLHwung commented Apr 26, 2021

@existentialism Oops I mentally think of your 👍 as ✅.

@existentialism
Copy link
Member

All good, looks great thanks @Zalathar!

@thorn0
Copy link
Contributor

thorn0 commented Apr 29, 2021

Looks like this PR doesn't take into account that in for await (async of []); the parens are not needed.

@Zalathar
Copy link
Contributor Author

Zalathar commented Apr 29, 2021

Looks like this PR doesn't take into account that in for await (async of []); the parens are not needed.

Ah, nice catch.

Thankfully the output still happens to be correct, so it's not a disaster, but this was an oversight and at least deserves explicit tests and comments.

That said, some quick testing reveals that Babel itself doesn't know how to parse for await (async of []);, which is more concerning. So the extra parens turn out to be an unintended mitigation for an edge-case parser bug, as well as a hand-hold for any unfortunate humans who have to read this weird code.

(TODO: File and fix the parser bug!)

So once the parser bug is fixed, I'm inclined to leave the unnecessary parens as-is, or maybe remove them in compact mode only.

@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 Jul 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 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 pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ForOfStatement prints the forbidden token sequence "for ( async of"
6 participants