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] - Fix panic in do while #1968

Closed
wants to merge 2 commits into from
Closed

Conversation

pdogr
Copy link
Contributor

@pdogr pdogr commented Mar 22, 2022

Node::DoWhileLoop ast node had a buggy bytecode generation where self.patch_jump(exit) was called after emitting LoopEnd opcode. This would patch the loop exit to the instruction following the do while code, which would panic in cases where do while was enclosed in a block statement.

This Pull Request fixes #1929.

It changes the following:

  • Patch jump before emitting Opcode::LoopEnd
  • Add test which has do while statement inside a block statement to demonstrate that the change fixes the panic.

- fix bug in do while bytecode generation
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1968 (66d06ed) into main (5fa1668) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1968   +/-   ##
=======================================
  Coverage   45.87%   45.87%           
=======================================
  Files         206      206           
  Lines       17102    17102           
=======================================
  Hits         7846     7846           
  Misses       9256     9256           
Impacted Files Coverage Δ
boa_engine/src/bytecompiler.rs 37.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fa1668...66d06ed. Read the comment docs.

@HalidOdat HalidOdat added bug Something isn't working execution Issues or PRs related to code execution labels Mar 22, 2022
@HalidOdat HalidOdat added this to the v0.15.0 milestone Mar 22, 2022
@Razican
Copy link
Member

Razican commented Mar 22, 2022

Hi! Thanks for the contribution!! It seems it's missing a cargo fmt run :)

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

LGTM

@jedel1043
Copy link
Member

VM implementation

Test result main count PR count difference
Total 88,428 88,428 0
Passed 43,986 43,986 0
Ignored 21,495 21,495 0
Failed 22,947 22,947 0
Panics 0 0 0
Conformance 49.74% 49.74% 0.00%

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thank you!

@HalidOdat
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 22, 2022
 `Node::DoWhileLoop` ast node had a buggy bytecode generation where `self.patch_jump(exit)` was called after emitting `LoopEnd` opcode. This would patch the loop exit to the instruction following the do while code, which would panic in cases where do while was enclosed in a block statement.

This Pull Request fixes #1929.

It changes the following:
- Patch jump before emitting `Opcode::LoopEnd`
- Add test which has do while statement inside a block statement to demonstrate that the change fixes the panic.
@bors
Copy link

bors bot commented Mar 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix panic in do while [Merged by Bors] - Fix panic in do while Mar 22, 2022
@bors bors bot closed this Mar 22, 2022
Razican pushed a commit that referenced this pull request Jun 8, 2022
 `Node::DoWhileLoop` ast node had a buggy bytecode generation where `self.patch_jump(exit)` was called after emitting `LoopEnd` opcode. This would patch the loop exit to the instruction following the do while code, which would panic in cases where do while was enclosed in a block statement.

This Pull Request fixes #1929.

It changes the following:
- Patch jump before emitting `Opcode::LoopEnd`
- Add test which has do while statement inside a block statement to demonstrate that the change fixes the panic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do-whiles within any block cause a panic
5 participants