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

Parser error on Asynchronous iteration and Asynchronous generators #183

Closed
adams85 opened this issue Aug 15, 2021 · 19 comments
Closed

Parser error on Asynchronous iteration and Asynchronous generators #183

adams85 opened this issue Aug 15, 2021 · 19 comments

Comments

@adams85
Copy link
Collaborator

adams85 commented Aug 15, 2021

The parser has issues with the aforementioned ES2018 features:

  • Asynchronous iteration works only when ParserOptions.Tolerant is set to false, otherwise it results in ParserException. Sample input:

    for await (const x of syncToAsyncIterable(['a', 'b'])) {
      console.log(x);
    }

    results in error as expected but is parsed fine in async contexts:

     async function f() {
       for await (const x of syncToAsyncIterable(['a', 'b'])) {
         console.log(x);
       }
     }
  • Asynchronous generators results in in ParserException regardless the ParserOptions.Tolerant setting. Sample input:

    async function* asyncGen() {
      yield someValue;
    }

Library version: 2.0.2

@lahma
Copy link
Collaborator

lahma commented Aug 15, 2021

I think this is a problem also in the original Esprima JS project so maybe need to file an issue there.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 16, 2021

@lahma You mean it has to be fixed in the original Esprima first? That'd be a bummer because this is kinda blocking for my lib...

BTW, have you seen this comment? Could you re-open that issue? (Unfortunately I can't.)

@lahma
Copy link
Collaborator

lahma commented Aug 16, 2021

@lahma You mean it has to be fixed in the original Esprima first? That'd be a bummer because this is kinda blocking for my lib...

Can be fixed on either side first, just a good practice to get the fix ported also to the other side to keep code consistent between the implementations.

BTW, have you seen this comment? Could you re-open that issue? (Unfortunately I can't.)

Would you like to submit a PR to fix remaining issue(s)?

(sorry I fat-fingered and edited your comment first)

@adams85
Copy link
Collaborator Author

adams85 commented Aug 16, 2021

@lahma I can take the other issue (the minor strict mode bug) but I'm not familiar enough with the parser to tackle this one.

@lahma
Copy link
Collaborator

lahma commented Aug 16, 2021

Seems that there's already a work item on Esprima side.

@lahma
Copy link
Collaborator

lahma commented Aug 16, 2021

By a cursory look it's enough to change:

var isGenerator = isAsync ? false : Match("*");

to

var isGenerator = Match("*");

in two places. Breaks two (now invalid) test cases and lacks test cases and JSON trees for results.

Edit: requires some more changes for async generator methods.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 19, 2021

@lahma Now this is the only blocking issue left for me. Are you working on this? If not, I might give it a shot.

@lahma
Copy link
Collaborator

lahma commented Aug 19, 2021

I have some work done, it should be pretty close to be PR ready against the JS version. Got to get my thoughts back together and check where I left the work..

@adams85
Copy link
Collaborator Author

adams85 commented Aug 19, 2021

Great news!

BTW, in the meantime I realized that I was mistaken about the Asynchronous iterations bug. It's parsed alright in async contexts, just the error message is not too helpful. I'll update the issue description.

However, I noticed that the parser skates over invalid usages of await in the case of for and for...in loops even when tolerant parsing is switched off:

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

is accepted without an error.

I've been making some progress on this though, I'll submit it as a PR soon.

@lahma
Copy link
Collaborator

lahma commented Aug 19, 2021

Here's the draft PR on C# side, I'm working on both JS and C# to see how the implementation works: #190

@sebastienros
Copy link
Owner

It's not even implemented on esprima's side?

@lahma
Copy link
Collaborator

lahma commented Aug 19, 2021

Hey welcome back @sebastienros ! Not yet implemented, I'm working on both sides to see what is missing: jquery/esprima#2101

@sebastienros
Copy link
Owner

That's awesome!

@lahma
Copy link
Collaborator

lahma commented Aug 22, 2021

I've completed the changes on JS side and also have the relevant code on .NET side ready (actually developed on .NET sided as debugging is waaaay easier). Waiting for acceptance there.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 22, 2021

@lahma May I expect a 2.x release after these changes get accepted and merged? I'd like to know that to be able to decide whether or not to stall releasing one of my libs depending on Esprima.NET.

@lahma
Copy link
Collaborator

lahma commented Aug 22, 2021

I'd be happy to have a 2.x release with these fixes that have landed and including this too (unless @sebastienros objects). If we don't hear back from JS side for a while we can also consider merging beforehand and fixing later if needed.

@adams85
Copy link
Collaborator Author

adams85 commented Aug 22, 2021

Excellent, thank you!

@lahma
Copy link
Collaborator

lahma commented Aug 29, 2021

I've merged in the changes as the JS PR seems to be in standstill. Your three examples provided in summary now parse without errors. Let's iterate more later if needed...

@lahma lahma closed this as completed Aug 29, 2021
@lahma
Copy link
Collaborator

lahma commented Aug 29, 2021

And the JS PR was also just merged.

lahma pushed a commit that referenced this issue Oct 16, 2021
* minor code readability improvements
* improves error reporting of 'for await' loops (#183)

Co-authored-by: Sébastien Ros <[email protected]>
JohnWinston329 added a commit to JohnWinston329/esprima-dotnet that referenced this issue Dec 28, 2023
* minor code readability improvements
* improves error reporting of 'for await' loops (sebastienros/esprima-dotnet#183)

Co-authored-by: Sébastien Ros <[email protected]>
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

No branches or pull requests

3 participants