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

allow await #504

Closed
yukulele opened this issue Mar 11, 2024 · 7 comments
Closed

allow await #504

yukulele opened this issue Mar 11, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@yukulele
Copy link

Expression = first:[0-9]+ [+] last[0-9]+ { 
  return await asyncAdd(first, last)
}

get this error :

await is only valid in async functions and the top level bodies of modules

await could be allowed inside expression body function.

@hildjj hildjj added the enhancement New feature or request label Mar 11, 2024
@hildjj
Copy link
Contributor

hildjj commented Mar 11, 2024

I thought I needed this once or twice, but have gotten around it by pre-processing the async class, post-processing a generated AST, or parsing once, processing, and parsing again. The most interesting cases are those where the text the grammar would accept is modified by the results of an async call that uses information parsed from the input.

There will have to be an option that affects code generation.

Discussion:

  • I assume this is going to have a pretty big negative impact on performance, but let's measure it.
  • Does every parse function get marked as async? I don't see any good way around it, since an otherwise-sync rule might call an async one.
  • Determining if a rule should be run in an async context would be difficult without a full parse of the code in result and predicate blocks, or a mechanism for the async flag to a rule's metadata
  • Tree shaking rules that were determined to be async might help, but that's likely to run into edge cases, and the top-level rule will always be async, which means the gains are likely to be minimal.
  • There will need to be a separate set of TypeScript types for async grammars, particularly in Typescript .d.ts for generated grammars #477
  • Initializers and top-level initializers should be run in async contexts as well.

This is going to be a good amount of work to get correct. A compelling use case would help increase its priority.

@vipcxj
Copy link

vipcxj commented Aug 16, 2024

@hildjj I think we can add a async mode. In async mode, all parser methods are async, even without 'await' keyword. And 'await' only allowed in this mode.

@ashokgowtham
Copy link

async as a 'mode' is a bad idea imo. It might seem like easy win, but it will cost the development in the longer run.
It introduces another parallel code path. All of a sudden, it will require new features getting added to be thought through twice, once with async and another with sync.
Also it doesn't scale well. Every type of 'mode' would double the number of code paths.

@yukulele
Copy link
Author

it will require new features getting added to be thought through twice, once with async and another with sync.

Not really, because what works in async also works in sync.

@reverofevil
Copy link

Not really, because what works in async also works in sync.

Unfortunately, no, because even sync code in async function will produce a Promise that resolves in next microtask. If you need truly sync code (for example, in *.config.js file for some library), you're out of luck.

@reverofevil
Copy link

reverofevil commented Sep 13, 2024

I don't think this should be done on peggy side. return async function() { return ...; } or return (some promise) is totally doable with the way it works right now.

Grammar is not a programming language, actions are best described in a separate file (and even better if it's in a typed language), and only valid reasons to use actions in peggy is to clean up AST and, maybe, write some prototype in its online playground. Asynchronous code is certainly something that should be done well outside the grammar.

@hildjj
Copy link
Contributor

hildjj commented Sep 13, 2024

@reverofevil makes a compelling point. I'm going to mark this as "not planned" for the moment. Good arguments could persuade me otherwise, but this seems like a LOT of work and breakage for not enough benefit at the moment.

@hildjj hildjj closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants