-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
Async arrow parse #1066
Async arrow parse #1066
Conversation
Test262 conformance changes:
|
Codecov Report
@@ Coverage Diff @@
## master #1066 +/- ##
==========================================
- Coverage 58.70% 58.70% -0.01%
==========================================
Files 176 180 +4
Lines 12391 12468 +77
==========================================
+ Hits 7274 7319 +45
- Misses 5117 5149 +32
Continue to review full report at Codecov.
|
Benchmark for 73c9795Click to view benchmark
|
Benchmark for 97327deClick to view benchmark
|
Benchmark for a2d334fClick to view benchmark
|
Looking good! A couple of things:
I will do a proper review later this week. |
I put a comment on the #818 issue that it might make sense to split into multiple issues rather than one big issue but either/or is good with me. Since this is still draft: I've left the todo!() since I plan to implement them before marking this ready |
Benchmark for 2cfa3bfClick to view benchmark
|
I'm unsure why the test262 failures have gone up, I'd guess its to do with the lack of execution implementation but will need to look into it - implementing unique formal params has reduced the failure count, perhaps implementing async without returning a Promise has caused enough failures to offset anything which is now passing |
Benchmark for afafedcClick to view benchmark
|
I've started looking at the execution stuff in the async_exec branch - I'm much less familiar with the execution side of boa so I have a lot of learning to do |
Benchmark for 1976cb4Click to view benchmark
|
Benchmark for c82081cClick to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, just needs a rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for my delay reviewing this. I saw some things that could be improved, though.
/// | ||
/// More information: | ||
/// - [ECMAScript reference][spec] | ||
/// - [MDN documentation][mdn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this link is broken.
//! Async Arrow function parsing. | ||
//! | ||
//! More information: | ||
//! - [MDN documentation][mdn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation link seems broken.
/// Arrow function parsing. | ||
/// | ||
/// More information: | ||
/// - [MDN documentation][mdn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation link seems broken.
/// - [ECMAScript specification][spec] | ||
/// | ||
/// Async More information: | ||
/// - [MDN documentation][async-mdn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link seems broken.
/// As these are effectively the same we just use a flag to reduce code duplication. | ||
/// | ||
/// More information: | ||
/// - [MDN documentation][mdn] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link seems broken.
Ok(body) | ||
} | ||
_ => Ok(StatementList::from(vec![Return::new( | ||
ExpressionBody::new(self.allow_in, self.is_async).parse(cursor)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it seems that this depends on this self.is_async
parameter, but the spec doesn't say so.
match cursor.peek(0)?.ok_or(ParseError::AbruptEnd)?.kind() { | ||
TokenKind::Punctuator(Punctuator::OpenBlock) => { | ||
let _ = cursor.next(); | ||
let body = FunctionBody::new(false, self.is_async).parse(cursor)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the spec correctly, both options here should be false
.
/// UniqueFormalParameters / FormalParameters parsing. | ||
/// | ||
/// Due to the similarity between these productions we use a unique flag rather than | ||
/// duplicating the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I would prefer having the former FormalParameters
, and then having another UniqueFormalParameters
structure that would basically call FormalParameters::parse()
, and would then do a check on the generated parameter list before returning the result.
if self.unique && params.iter().any(|v| v.name() == next_param.name()) { | ||
// https://tc39.es/ecma262/#prod-UniqueFormalParameters | ||
return Err(ParseError::lex(LexError::Syntax( | ||
"Invalid duplicate formal parameter name".into(), | ||
pos, | ||
))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be a lexing error. It's a normal parsing error, the lexing was correct, but in this context, it's a syntax error, so a parsing error.
fn check_arrow_assignment_2arg() { | ||
check_parser( | ||
"let foo = (a, b) => { return a };", | ||
vec![LetDeclList::from(vec![LetDecl::new::<_, Option<Node>>( | ||
Identifier::from("foo"), | ||
Some( | ||
ArrowFunctionDecl::new( | ||
vec![ | ||
FormalParameter::new("a", None, false), | ||
FormalParameter::new("b", None, false), | ||
], | ||
vec![Return::new::<Node, Option<_>, Option<_>>( | ||
Some(Identifier::from("a").into()), | ||
None, | ||
) | ||
.into()], | ||
) | ||
.into(), | ||
), | ||
)]) | ||
.into()], | ||
); | ||
} | ||
|
||
#[test] | ||
fn check_arrow_assignment_2arg_nobrackets() { | ||
check_parser( | ||
"let foo = (a, b) => a;", | ||
vec![LetDeclList::from(vec![LetDecl::new::<_, Option<Node>>( | ||
Identifier::from("foo"), | ||
Some( | ||
ArrowFunctionDecl::new( | ||
vec![ | ||
FormalParameter::new("a", None, false), | ||
FormalParameter::new("b", None, false), | ||
], | ||
vec![Return::new::<Node, Option<_>, Option<_>>( | ||
Some(Identifier::from("a").into()), | ||
None, | ||
) | ||
.into()], | ||
) | ||
.into(), | ||
), | ||
)]) | ||
.into()], | ||
); | ||
} | ||
|
||
#[test] | ||
fn check_arrow_assignment_3arg() { | ||
check_parser( | ||
"let foo = (a, b, c) => { return a };", | ||
vec![LetDeclList::from(vec![LetDecl::new::<_, Option<Node>>( | ||
Identifier::from("foo"), | ||
Some( | ||
ArrowFunctionDecl::new( | ||
vec![ | ||
FormalParameter::new("a", None, false), | ||
FormalParameter::new("b", None, false), | ||
FormalParameter::new("c", None, false), | ||
], | ||
vec![Return::new::<Node, Option<_>, Option<_>>( | ||
Some(Identifier::from("a").into()), | ||
None, | ||
) | ||
.into()], | ||
) | ||
.into(), | ||
), | ||
)]) | ||
.into()], | ||
); | ||
} | ||
|
||
#[test] | ||
fn check_arrow_assignment_3arg_nobrackets() { | ||
check_parser( | ||
"let foo = (a, b, c) => a;", | ||
vec![LetDeclList::from(vec![LetDecl::new::<_, Option<Node>>( | ||
Identifier::from("foo"), | ||
Some( | ||
ArrowFunctionDecl::new( | ||
vec![ | ||
FormalParameter::new("a", None, false), | ||
FormalParameter::new("b", None, false), | ||
FormalParameter::new("c", None, false), | ||
], | ||
vec![Return::new::<Node, Option<_>, Option<_>>( | ||
Some(Identifier::from("a").into()), | ||
None, | ||
) | ||
.into()], | ||
) | ||
.into(), | ||
), | ||
)]) | ||
.into()], | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were those tests moved?
@Lan2u do you think you'll be coming back to this PR? |
Unfortunately probably not, I haven't had much time over the last year. |
I'm closing this PR, as this is now very outdated. I will create issues in order to be able to re-do this again. |
Do you think this code is relevant to implementing #1805? I'm new to the codebase and just gathering info |
Things have changed a bit, but the general idea should still be valid :) so I think it can be useful to check the diff |
This Pull Request finishes the lex/parsing part of #818.
It changes the following: