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 parsing of optional chaining in new expressions and decorators #393

Merged
merged 5 commits into from
Aug 7, 2023

Conversation

adams85
Copy link
Collaborator

@adams85 adams85 commented Aug 6, 2023

Takes a crack at #322 (However, addresses only new expressions because decorators seem to have bigger issues ATM).

The fix involves keeping track of the last processed token, which I'm not completely comfortably with TBH. But other approaches look much more complicated. Could someone more familiar with the parser take a look at this?

@adams85 adams85 linked an issue Aug 6, 2023 that may be closed by this pull request
@adams85 adams85 force-pushed the fix/optional-chaining-in-new branch 2 times, most recently from d6ac5a5 to 80defbd Compare August 6, 2023 12:25
@lahma
Copy link
Collaborator

lahma commented Aug 6, 2023

Now sure if I can be of much help here, always appreciate you improving the parsing though! I've usually looked at acorn for ideas on how to accomplish some specific parsing scenarios in the past.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 7, 2023

Thanks for the tip. Acorn just passes a flag down the call stack to implement the necessary check. In Esprima it's a bit more complicated because ParseLeftHandSideExpression is not called directly but through IsolateCoverGrammar. So I had to introduce a new flag in JavaScriptParser.Context, which I wanted to avoid. However, this looks to be the correct way.

And while being at it, I also fixed #394.

@adams85 adams85 linked an issue Aug 7, 2023 that may be closed by this pull request
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

LGTM, seems naturally follow common patterns like previous-reset for flags and reads well. Nicely done once again!

@adams85 adams85 force-pushed the fix/optional-chaining-in-new branch from 46d357e to ffc967a Compare August 7, 2023 18:26
@adams85 adams85 merged commit 93ddbe6 into main Aug 7, 2023
2 checks passed
@adams85 adams85 deleted the fix/optional-chaining-in-new branch August 7, 2023 18:34
@adams85
Copy link
Collaborator Author

adams85 commented Aug 7, 2023

How about pushing out a new RC? :) If you could do it, I could finish the regex story in Jint.

@lahma
Copy link
Collaborator

lahma commented Aug 7, 2023

I of course can, but would you like to do the honors this time? Just to make sure we don't have dependency on single entities (like me or Sebastien). Just create the tag with the name following earlier tags against latest main, push against origin and rock'n'roll 🚀

Just say so and I'll do it, just testing how you would feel about it (convo about whether to push is of course nice, but you master the features at the moment).

@adams85
Copy link
Collaborator Author

adams85 commented Aug 7, 2023

If it's that simple, no problem. :) I'll give it a shot.

@adams85 adams85 changed the title Fix parsing of optional chaining in new expressions Fix parsing of optional chaining in new expressions and decorators Aug 7, 2023
@lahma
Copy link
Collaborator

lahma commented Aug 7, 2023

What is this sorcery, you have accomplished the task of publishing an Esprima release to nuget.org, congrats! 🚀

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

Successfully merging this pull request may close these issues.

Decorator parsing is too forgiving Optional chaining-related parser issues
2 participants