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

Missing tests for for await (async of #2979

Closed
Zalathar opened this issue May 3, 2021 · 4 comments · Fixed by #2982
Closed

Missing tests for for await (async of #2979

Zalathar opened this issue May 3, 2021 · 4 comments · Fixed by #2982
Labels

Comments

@Zalathar
Copy link
Contributor

Zalathar commented May 3, 2021

It was recently noticed (babel/babel#13235) that Babel failed to parse this code:

async function f() {
    for await (async of []);
}

Unlike a regular for-of loop there is no ambiguity with for (async of => {};;), and an async function expression is not legal here, so async can be used as the loop variable.

But it's easy for the implementation to mess up by parsing async of as an async function anyway, or by incorrectly sharing code that prohibits for (async of in a regular for-of.

@jugglinmike
Copy link
Contributor

That sounds like a meaningful test case to me. Thanks for the report!

Would you like to write the test? I'm available to help you get started if you need a hand.

@Zalathar
Copy link
Contributor Author

Zalathar commented May 3, 2021

I was able to write some test cases based on the existing templates in src/dstr-binding-for-await/default/, but I'm a bit lost on where to put them (since they aren't destructuring?) or what documentation they need.

I guess I should just submit a draft PR with what I have so far?

@jugglinmike
Copy link
Contributor

Great! I'm happy to give feedback on a work-in-progress.

Since this issue is orthogonal to the parsing of destructuring patterns, using those templates may produce a lot of tests which aren't meaningfully distinctive for implementers. This sounds like a case for hand-written tests, but we can discuss that in the context of your patch if you prefer.

@bakkot
Copy link
Contributor

bakkot commented May 4, 2021

The PR which added tests for other cases may be useful context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants