Implement Async Generator Parsing #1669

nekevss · 2021-10-15T02:49:42Z

This Pull Request addresses #1558.

There were a lot of files and different small changes here and there with some larger chunks as well. Let me know what you think. While I was in implementing the AsyncGeneratorMethod on the object initializer. I also added the AsyncMethod as well...hopefully that's not stepping on anyone's toes, so to speak. There were a lot of different changes across quite a few files, so please let me know if I missed anything.

It changes the following:

Implement AsyncGeneratorExpression Parsing
Implement AsyncGeneratorDeclaration Parsing
Implement AsyncGeneratorMethod on Object Initializing Parsing
Implement AsyncMethod on Object Initializing Parsing

…atorExpression

…ion parse

raskad

Great work! Sorry for the late review. I have some small changes.

It would also be great if you could rebase this. Right now It is difficult to see which new passing/failing tests are because of your changes and which are because of the diff to main and the submodule update.

boa/src/syntax/ast/node/declaration/async_generator_expr/mod.rs

boa/src/syntax/parser/statement/declaration/hoistable/mod.rs

…atorExpression

raskad · 2021-10-20T19:21:11Z

@nekevss All the failing tests that have redeclaration in their name can be ignored. They should be fixed by #1391.

nekevss · 2021-10-22T03:27:23Z

I was able to make a change that fixed the tests for test/language/expressions/object/method-definition/early-errors-object-method-async-lineterminator.js. It passed when I ran the tests on my computer. I'm not entirely sure how the rest of the tests are breaking off adding in the async method parsing and async gen parsing.

I can definitely look into it further, but I might need some direction on where to look.

Razican

Hi there, sorry it took me some time to review this, and I wasn't able to review all of it. For now, I can tell a couple of things:

Thank you for your contribution! This looks very, very promising, and I think we are very close to have it ready :)
I see there are changes to GitHub workflows and so on, that are unrelated to this PR. I guess this is due to a strange re-base. No worries, you should merge the main branch into this one, and do a git submodule update to make sure you have the latest Test262 submodule. We will squash the commits when merging, so the list of commits is not very important, the important part is the diff, and it should be as clean as possible to make the review easier.
I saw that you were not fully following some parsers, probably because some of the checks for await, then no LineTerminator, then function were done beforehand. This is not a good practice, and I would prefer if you changed this so that each parser follows its spec correctly. This makes it much easier to develop each parser on its own (you only need to check for the spec of that parser), and it's easier to maintain. If someone in the future would like to change the AsyncGeneratorDeclaration parser, for example, it shouldn't need to be aware of where is it being called from and what tokens that are part of it are already consumed. It should just follow the parser specification for that particular non-terminal.

Let me know if any of my comments are not clear, or if you would like to have some guidance on this, and thank you again! :)

boa/src/syntax/parser/expression/primary/async_generator_expression/mod.rs

boa/src/syntax/parser/statement/declaration/hoistable/async_function_decl/mod.rs

boa/src/syntax/parser/statement/declaration/hoistable/async_generator_decl/mod.rs

boa/src/syntax/parser/expression/primary/async_function_expression/mod.rs

nekevss · 2021-10-23T01:18:10Z

I went through and made a bunch of the changes. Thought I would push what I have so far even though I still have a question regarding PrimaryExpression.

@Razican I merged the main branch into this one and ran git submodule update. If there's anything I did wrong please feel free to let me know. I've mainly only used git on personal projects before contributing here, so there's definitely some gaps in knowledge that videos and reading don't always help 😃

Razican

Looks very good now! There are only a couple of things (and a small formatting recommendation):

Try running the following:

cd test262 && git pull origin 12e9d67bb529474ad1ffddca92361512ab30c4c0

This should update the test262 suite to the one in the main branch, hopefully. It seems this one is using a different commit of the tests.

Then, it seems that these changes were not taken into account for our (in progress) virtual machine. To check this, you can compile the code with the "vm" feature:

cargo check --features "vm"

This way you will get the error in that piece of code. It should be pretty easy, it's just a match that doesn't take into account the new AST nodes.

Razican · 2021-10-23T08:12:04Z

boa/src/syntax/parser/statement/declaration/hoistable/async_generator_decl/mod.rs

+    fn error_context(&self) -> &'static str {
+        "async generator declaration"
+    }
+    fn is_default(&self) -> bool {
+        self.is_default.0
+    }
+    fn name_allow_yield(&self) -> bool {
+        self.allow_yield.0
+    }
+    fn name_allow_await(&self) -> bool {
+        self.allow_await.0
+    }
+    fn parameters_allow_yield(&self) -> bool {
+        true
+    }
+    fn parameters_allow_await(&self) -> bool {
+        true
+    }
+    fn body_allow_yield(&self) -> bool {
+        true
+    }
+    fn body_allow_await(&self) -> bool {
+        true
+    }


Could we add the #[inline] attribute to all of these, and separate each function with an empty line?

Suggested change

fn error_context(&self) -> &'static str {

"async generator declaration"

}

fn is_default(&self) -> bool {

self.is_default.0

}

fn name_allow_yield(&self) -> bool {

self.allow_yield.0

}

fn name_allow_await(&self) -> bool {

self.allow_await.0

}

fn parameters_allow_yield(&self) -> bool {

true

}

fn parameters_allow_await(&self) -> bool {

true

}

fn body_allow_yield(&self) -> bool {

true

}

fn body_allow_await(&self) -> bool {

true

}

#[inline]

fn error_context(&self) -> &'static str {

"async generator declaration"

}

#[inline]

fn is_default(&self) -> bool {

self.is_default.0

}

#[inline]

fn name_allow_yield(&self) -> bool {

self.allow_yield.0

}

#[inline]

fn name_allow_await(&self) -> bool {

self.allow_await.0

}

#[inline]

fn parameters_allow_yield(&self) -> bool {

true

}

#[inline]

fn parameters_allow_await(&self) -> bool {

true

}

#[inline]

fn body_allow_yield(&self) -> bool {

true

}

#[inline]

fn body_allow_await(&self) -> bool {

true

}

Sounds good. Should this also be applied to all other hoistable declarations as well?

I think we do not have to add #[inline] to theses functions. The compiler should always inline oneliners like this without the hint as far as I know.

I'll leave it off for now since it can always be added in across all the hoistable declarations if need be.

Oh, sorry, I didn't read these and I added it anyway. Feel free to remove them. It should inline them in --release mode without any issue, but with the #[inline] keyword it should also inline them in debug mode.

@nekevss Sorry, this discussion gets a bit off topic ;)

@Razican My impression is that the #[inline] hints are mainly for the LLVM codegen. And in debug mode rustc is called with -C opt-level=0 so no LLVM optimizations would run at all.

I just tried it on godbolt:

-C opt-level=0 => not inlined

-C opt-level=0 + #[inline] => not inlined

-C opt-level=1 => not inlined

-C opt-level=1 + #[inline] => inlined

-C opt-level=2 => inlined

We have no [profile.dev], but we have a [profile.test] where opt level is set to 1. So it would only matter for running cargo test.
I honestly don't know if it's worth it, because imo all the #[inline]s really clutter the codebase. I think we should decide how we want to deal with it and maybe document it, so the question does not come up again. What do you think?

Makes sense, let's open a discussion about it and see what gets decided :)

boa/src/syntax/ast/node/declaration/async_generator_expr/mod.rs

boa/src/syntax/ast/node/declaration/async_generator_decl/mod.rs

Razican · 2021-10-23T08:46:01Z

boa/src/syntax/ast/node/declaration/async_generator_decl/mod.rs

+///  - [ECMAScript reference][spec]
+///
+/// [spec]: https://tc39.es/ecma262/#prod-AsyncGeneratorMethod
+


Suggested change

boa/src/syntax/parser/statement/declaration/hoistable/async_generator_decl/mod.rs

…nerator_decl/mod.rs

Razican

This looks now ready to merge from my side! :)

nekevss added 15 commits October 11, 2021 19:53

initial commit of attempt at AsyncGeneratorDeclaration and AsyncGener…

92e5a05

…atorExpression

fixed misspelled AsyncGeneratorDeclaration

c80080c

adding AsyncGeneratorDeclaration to hoistable

059cd48

removing unused Keyword from async_function_expression

3a6ff05

AsyncGeneratorExpr tests and cargo fmt

86326c7

fixing tests

8b03415

added tests to mod and adding to module declarations

94fbdad

Added AsyncGenerator and Async to MethodDefinitionKind

6dfeafe

allwo await and yield set to true in async_gen_decl - cargo fmt

2b81c58

added new method kinds to object node

28b7e82

forgot cargo fmt

0bb3853

Added some better comments implementations

2b5ecb5

added async and async generator methods to object_initializer

0391d57

Added note to uniqueformalparameters:formalparameters error check

3124783

Opps, removed cursor ops that now happen before AsyncFunctionDeclarat…

f47b79f

…ion parse

RageKnify requested review from raskad, jedel1043 and Razican October 15, 2021 14:13

RageKnify added the parser Issues surrounding the parser label Oct 15, 2021

RageKnify added this to the v0.14.0 milestone Oct 15, 2021

RageKnify linked an issue Oct 15, 2021 that may be closed by this pull request

Implement Async Generator Parsing #1558

Closed

3 tasks

raskad requested changes Oct 19, 2021

View reviewed changes

boa/src/syntax/ast/node/declaration/async_generator_expr/mod.rs Show resolved Hide resolved

boa/src/syntax/parser/statement/declaration/hoistable/mod.rs Outdated Show resolved Hide resolved

nekevss added 8 commits October 19, 2021 18:30

made requested review changes

557d5c0

initial commit of attempt at AsyncGeneratorDeclaration and AsyncGener…

83e3092

…atorExpression

fixed misspelled AsyncGeneratorDeclaration

408e3fc

adding AsyncGeneratorDeclaration to hoistable

f9b2183

removing unused Keyword from async_function_expression

6ff0cc6

AsyncGeneratorExpr tests and cargo fmt

adbcb41

fixing tests

3578a6e

added tests to mod and adding to module declarations

d926ad2

nekevss added 2 commits October 20, 2021 22:19

fixing issue in async methods in object_initializer + tests

b37268a

added a comment

343d4f6

Razican requested changes Oct 22, 2021

View reviewed changes

Razican added ast Issue surrounding the abstract syntax tree enhancement New feature or request labels Oct 22, 2021

nekevss added 4 commits October 22, 2021 19:21

Merge branch 'master' into impl-async-gen-parser

74dedf8

review changes to match parser spec/clean up

4cb76b2

changes to comment blocks on async gen decl

a59405b

cargo fmt

35704be

Razican requested changes Oct 23, 2021

View reviewed changes

nekevss and others added 5 commits October 23, 2021 09:36

Added to ByteCompiler & review changes

7abd657

Added TODO to PrimaryExpression

ac55423

Removed inline attribute

58b32dd

Added more to comment blocks

5edcba5

Update boa/src/syntax/ast/node/declaration/async_generator_expr/mod.rs

7c89348

Razican reviewed Oct 23, 2021

View reviewed changes

boa/src/syntax/parser/statement/declaration/hoistable/async_generator_decl/mod.rs Outdated Show resolved Hide resolved

Update boa/src/syntax/parser/statement/declaration/hoistable/async_ge…

1892ecd

…nerator_decl/mod.rs

Razican approved these changes Oct 23, 2021

View reviewed changes

Razican requested a review from raskad October 23, 2021 16:48

raskad approved these changes Oct 23, 2021

View reviewed changes

raskad merged commit 33e2205 into boa-dev:main Oct 23, 2021

nekevss deleted the impl-async-gen-parser branch October 23, 2021 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Async Generator Parsing #1669

Implement Async Generator Parsing #1669

nekevss commented Oct 15, 2021 •

edited

Loading

raskad left a comment

raskad commented Oct 20, 2021

nekevss commented Oct 22, 2021

Razican left a comment

nekevss commented Oct 23, 2021

Razican left a comment

Razican Oct 23, 2021

nekevss Oct 23, 2021

raskad Oct 23, 2021

nekevss Oct 23, 2021

Razican Oct 23, 2021

raskad Oct 23, 2021

Razican Oct 23, 2021

Razican Oct 23, 2021

Razican left a comment

Implement Async Generator Parsing #1669

Implement Async Generator Parsing #1669

Conversation

nekevss commented Oct 15, 2021 • edited Loading

raskad left a comment

Choose a reason for hiding this comment

raskad commented Oct 20, 2021

nekevss commented Oct 22, 2021

Razican left a comment

Choose a reason for hiding this comment

nekevss commented Oct 23, 2021

Razican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

nekevss commented Oct 15, 2021 •

edited

Loading