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

Support async generators #2101

Merged
merged 2 commits into from
Aug 29, 2021
Merged

Support async generators #2101

merged 2 commits into from
Aug 29, 2021

Conversation

lahma
Copy link
Contributor

@lahma lahma commented Aug 19, 2021

Async generators are valid constructs and went through issues parsing them. Couple issues surfaced on class handling side but unrelated to async generators per se.

Before

✔ 54265 valid programs parsed without error
✔ 6748 invalid programs produced a parsing error
✔ 1101 invalid programs did not produce a parsing error (and allowed by the whitelist file)
✔ 16288 valid programs produced a parsing error (and allowed by the whitelist file)

After

✔ 60262 valid programs parsed without error
✔ 6747 invalid programs produced a parsing error
✔ 1102 invalid programs did not produce a parsing error (and allowed by the whitelist file)
✔ 10291 valid programs produced a parsing error (and allowed by the whitelist file)

fixes #1990

@lahma lahma force-pushed the async-generator branch 7 times, most recently from f4713c9 to c1dcacc Compare August 21, 2021 06:39
@lahma lahma changed the title Async generator Support async generators Aug 21, 2021
@lahma lahma force-pushed the async-generator branch 2 times, most recently from a770bff to 283aa68 Compare August 21, 2021 10:46
@lahma lahma marked this pull request as ready for review August 21, 2021 12:43
@lahma
Copy link
Contributor Author

lahma commented Aug 21, 2021

Hi @ariya ! Here's a PR to allow parsing about 6000 more scripts that were unparseable before, basically allowing async generators and couple more error checks.

@ariya
Copy link
Contributor

ariya commented Aug 21, 2021

Great work @lahma, thanks! I will take a look.

@@ -49,7 +51,6 @@ export const Messages = {
StrictLHSPrefix: 'Prefix increment/decrement may not have eval or arguments operand in strict mode',
StrictModeWith: 'Strict mode code may not include a with statement',
StrictOctalLiteral: 'Octal literals are not allowed in strict mode.',
StrictParamDupe: 'Strict mode function may not have duplicate parameter names',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this in favor of DuplicateParameter which aligns with V8

@@ -20,13 +20,14 @@ interface Context {
allowIn: boolean;
allowStrictDirective: boolean;
allowYield: boolean;
await: boolean;
isAsync: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this as await is keyword and isAsync describes the context

@@ -256,6 +258,12 @@ export class Parser {
this.errorHandler.tolerate(this.unexpectedTokenError(token, message));
}

tolerateInvalidLoopStatement() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to create separate function as for statement parsing started to fail due to cyclomatic complexity

@@ -613,7 +621,7 @@ export class Parser {

switch (this.lookahead.type) {
case Token.Identifier:
if ((this.context.isModule || this.context.await) && this.lookahead.value === 'await') {
if ((this.context.isModule || this.context.isAsync) && this.lookahead.value === 'await') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this now reads better, isModule, isAsync

@@ -785,19 +792,20 @@ export class Parser {
return this.finalize(node, new Node.FunctionExpression(null, params.params, method, isGenerator));
}

parsePropertyMethodAsyncFunction(): Node.FunctionExpression {
parsePropertyMethodAsyncFunction(isGenerator: boolean): Node.FunctionExpression {
Copy link
Contributor Author

@lahma lahma Aug 22, 2021

Choose a reason for hiding this comment

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

Might make sense at some point to put isGenerator into context

Copy link
Contributor

Choose a reason for hiding this comment

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

This is Boolean trap, though we already have some cases like that in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I put this into context as part of this PR or what changes required?

@@ -2125,7 +2146,7 @@ export class Parser {
return this.finalize(node, new Node.Property('init', key, computed, value, method, shorthand));
}

parseRestProperty(params, kind): Node.RestElement {
parseRestProperty(params): Node.RestElement {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the unused parameter as style checks gave warning and couldn't see a reason to keep

@@ -3018,6 +3043,12 @@ export class Parser {
}
this.expect(')');

if (options.hasDuplicateParameterNames) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved checks to single place where can validate against context

@lahma
Copy link
Contributor Author

lahma commented Aug 25, 2021

@ariya friendly ping from here 🙂 it would also be great if you have time to check #2082 , the new fork of Esprima is running far ahead already..

EDIT and the very moment you check, sorry for pinging 🤦🏻‍♂️

@lahma
Copy link
Contributor Author

lahma commented Aug 26, 2021

@ariya anything you want me to tweak here?

@ariya ariya merged commit 512cd66 into jquery:main Aug 29, 2021
@ariya
Copy link
Contributor

ariya commented Aug 29, 2021

Thank you @lahma 👍

@lahma
Copy link
Contributor Author

lahma commented Aug 29, 2021

You're most welcome, thanks for merging!

@jogibear9988
Copy link

@lahma i've merged your changes also in my fork

@@ -1342,6 +1356,10 @@ export class Parser {
expr = this.inheritCoverGrammar(this.matchKeyword('new') ? this.parseNewExpression : this.parsePrimaryExpression);
}

if (isSuper && this.match('(') && !this.context.inClassConstructor) {

Choose a reason for hiding this comment

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

@lahma:
this condition is not correct when the class is not inherited

Choose a reason for hiding this comment

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

In esprima next there is also context.allowSuper.
I got it from this pull req: #2047

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

Successfully merging this pull request may close these issues.

ES2018 Feature: Async Iteration (for-await-of)
3 participants