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 top-level await parsing (#38483) #38518

Closed
wants to merge 1 commit into from
Closed

Fix top-level await parsing (#38483) #38518

wants to merge 1 commit into from

Conversation

lissein
Copy link
Contributor

@lissein lissein commented May 13, 2020

The previous implementation wasn't able to handle case like await (async () => {}) outside of "async" functions.

One problem is that the syntax can be exactly the same for two usecases:

Use case 1

await (async () => {})()

An await expression on the self-calling async function

Use case 2

function await(fn) {
  return () => {};
}
await (async () => {})(); // equivalent to: await((async () => {})())

A call to the function returned by a call to the "await" user defined function with the async () => {} argument.


We have no way to know which case we want to handle. This pull request handles Use case 2 first. If there is some identifier named "await" defined, we make this statement a function call, otherwise, we make it an await expression.

I can update this PR if someone has a better idea to handle this ambiguous case.

Fixes #38483

@Kingwl Kingwl mentioned this pull request May 13, 2020
The previous implementation wasn't able to handle case like `await (async () => {})`
outside of "async" functions.
@sandersn sandersn added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 20, 2020
@rbuckton
Copy link
Member

I plan to have a more comprehensive solution involving a cover grammar for TLA once #35282 merges.

// One drawback of this implementation is that we cannot use this form in top-level
// if there is an "await" identifier as we cannot differentiate this two cases
const isCallLike = () => {
return (nextToken() === SyntaxKind.OpenParenToken && !identifiers.has("await")) && !scanner.hasPrecedingLineBreak();
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this approach is function hoisting. Your example has this:

function await(fn) {
  return () => {};
}
await (async () => {})(); // equivalent to: await((async () => {})())

However, it should also fail in this case:

await (async () => {})(); // equivalent to: await((async () => {})())
function await(fn) {
  return () => {};
}

I don't think this is the correct solution to the underlying problem.

@orta orta closed this May 31, 2021
@orta orta deleted the branch microsoft:master May 31, 2021 09:44
@orta
Copy link
Contributor

orta commented May 31, 2021

Hi @lissein - this PR didn't have 'allow maintainers to make changes' checked, and so when we migrated TypeScript's primary branch from 'master' to 'main' this PR couldn't have its branch re-targeted.

After playing with the code samples in this thread, I do think this is still not quite handled by TypeScript, at least the error @rbuckton mentioned should raise still doesn't raise in the 4.3 beta

await (async () => {})(); // equivalent to: await((async () => {})())
function await(fn) {
  return () => {};
}

So, there's value in re-opening IMO 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Incorrect lexing of TLA await in try...catch
4 participants