Skip to content

Improve parsing in await and yield context #44464

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

Closed

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Jun 6, 2021

Fixes #44459

It works. But I don't know why 🤷‍♂️.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 6, 2021
@sandersn
Copy link
Member

I need to run through it in the debugger to understand it myself, but my guess is that isBindingIdentifier accepts await and yield as keywords where isIdentifier doesn't (when not in an await/yeild context). Then something in parseVarExpression explicitly rejects await/yield. Not sure why it incorrectly works today.

@sandersn sandersn self-assigned this Jun 18, 2021
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 18, 2021
@sandersn
Copy link
Member

Well, I was sort of right. It's actually an error in the binder, from checkContextualIdentifier, not parseVarExpression. A better fix would be to change the definition of inYieldContext and inAwaitContext in isIdentifier to skip let await/let yield as well.

@sandersn
Copy link
Member

Now that I've checked how to clear Yield/AwaitContext, I see that isBindingIdentifier is the same as isIdentifier without the special cases for yield/await.

I'd like to see two things:

  1. Check whether the special case for yield/await is even needed. Try deleting it from isIdentifier and report back how baselines change. It could be that the binder has better error messages in all cases.
  2. If that doesn't work out, put a small comment above isBindingIdentifier saying // let await/let yield are allowed here and disallowed in the binder (or something similar)

I think it's probably fine to call isBindingIdentifier instead of isIdentifier; isIdentifier hasn't changed for 5 years, and I assume @rbuckton created a isBindingIdentifier last year instead of passing a boolean to isIdentifier for performance reasons.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

See comments above.

@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 21, 2021

Moved to #44680.

@Kingwl Kingwl closed this Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Binding identifier should care about await/yield context
3 participants