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

[Merged by Bors] - Implement Async Generators #2200

Closed
wants to merge 6 commits into from

Conversation

raskad
Copy link
Member

@raskad raskad commented Jul 23, 2022

This Pull Request fixes #1560.

It changes the following:

  • Implement AsyncGeneratorFunction builtin object.
  • Implement AsyncGenerator builtin object.
  • Implement async generator execution.
  • Add some parse errors for async generators.

The AsyncGenerator.prototype.return() method currently does not work correctly with finally blocks, but I think we should merge this first to not increase the complexity of the pr too much.

@raskad raskad added enhancement New feature or request parser Issues surrounding the parser builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution labels Jul 23, 2022
@raskad raskad added this to the v0.16.0 milestone Jul 23, 2022
@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #2200 (5186aba) into main (a8bf59d) will decrease coverage by 0.45%.
The diff coverage is 17.62%.

@@            Coverage Diff             @@
##             main    #2200      +/-   ##
==========================================
- Coverage   41.99%   41.54%   -0.46%     
==========================================
  Files         231      234       +3     
  Lines       21530    21891     +361     
==========================================
+ Hits         9042     9094      +52     
- Misses      12488    12797     +309     
Impacted Files Coverage Δ
boa_engine/src/builtins/function/mod.rs 24.35% <0.00%> (-0.92%) ⬇️
boa_engine/src/builtins/mod.rs 8.95% <0.00%> (-0.28%) ⬇️
boa_engine/src/builtins/promise/mod.rs 18.97% <ø> (ø)
boa_engine/src/context/mod.rs 30.70% <0.00%> (-0.14%) ⬇️
boa_engine/src/object/mod.rs 19.85% <0.00%> (-0.98%) ⬇️
...x/ast/node/declaration/async_generator_decl/mod.rs 10.52% <0.00%> (-2.81%) ⬇️
boa_engine/src/vm/call_frame.rs 100.00% <ø> (ø)
boa_engine/src/vm/code_block.rs 32.80% <0.00%> (-4.78%) ⬇️
boa_engine/src/vm/opcode.rs 50.00% <ø> (ø)
boa_engine/src/builtins/async_generator/mod.rs 7.75% <7.75%> (ø)
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@raskad
Copy link
Member Author

raskad commented Jul 23, 2022

VM implementation

Test result main count PR count difference
Total 91,573 91,573 0
Passed 59,820 64,752 +4,932
Ignored 14,606 14,606 0
Failed 17,147 12,215 -4,932
Panics 0 0 0
Conformance 65.32% 70.71% +5.39%

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This is great! I started the review, but still a lot to go through.

boa_engine/src/builtins/promise/mod.rs Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

This looks really good!

I just have some small nitpicks :)

boa_engine/src/vm/call_frame.rs Outdated Show resolved Hide resolved
boa_engine/src/vm/mod.rs Outdated Show resolved Hide resolved
bors bot pushed a commit that referenced this pull request Aug 5, 2022
Currently async generator class methods are not being parsed correctly. In comparison to the current main branch this only fixes a few tests, but after #2200 is merged, it should be aroud 1000 additional passing tests.
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

It looks great! I had some improvement suggestions, but in general this is great work :) and the coverage increase is impressive

boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved

/// `FunctionCompiler` is used to compile AST functions to bytecode.
#[derive(Debug, Clone, Copy)]
pub(crate) struct FunctionCompiler {
Copy link
Member

Choose a reason for hiding this comment

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

I would move this struct to a sub-module. 3k lines is already too much in this file, and we also have #1808 that mentions dividing this huge file into smaller parts.

Copy link
Member Author

@raskad raskad Aug 13, 2022

Choose a reason for hiding this comment

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

True. I have moved the bytecompiler to a module folder and created a separate file for the function compiler. I think from here we can do further dividing.

@raskad
Copy link
Member Author

raskad commented Aug 13, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 13, 2022
This Pull Request fixes #1560.

It changes the following:

- Implement `AsyncGeneratorFunction` builtin object.
- Implement `AsyncGenerator` builtin object.
- Implement async generator execution.
- Add some parse errors for async generators.

The `AsyncGenerator.prototype.return()` method currently does not work correctly with `finally` blocks, but I think we should merge this first to not increase the complexity of the pr too much.
@bors
Copy link

bors bot commented Aug 13, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Implement Async Generators [Merged by Bors] - Implement Async Generators Aug 13, 2022
@bors bors bot closed this Aug 13, 2022
@bors bors bot deleted the implement-async-generator branch August 13, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request execution Issues or PRs related to code execution parser Issues surrounding the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Async Generator Execution
3 participants