-
-
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
[Merged by Bors] - Try-catch-block control flow fix/refactor #2568
Conversation
1405510
to
e9694c6
Compare
Codecov Report
@@ Coverage Diff @@
## main #2568 +/- ##
==========================================
+ Coverage 48.52% 48.80% +0.28%
==========================================
Files 387 391 +4
Lines 38603 38919 +316
==========================================
+ Hits 18733 18996 +263
- Misses 19870 19923 +53
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Fixed tests (4):
Broken tests (34):
|
@nekevss Are the new broken tests expected? Where they previously false positives? Btw, you should be able to push your branches to the boa-dev/boa origin instead of your own fork, that way the test results are posted automatically. |
Not entirely, I had actually missed some logic on compiling continues that broke some tests. There's probably some of these that could be valid bugs/missed logic, so I can look at those this weekend and test them. There were some of these tests however that I do think may have been false positives (although I don't have any evidence), especially due to some tests specifically testing I mostly wanted to get some eyes on this early since it takes a different approach as to how environments are tracked by the Here's the most recent results (Updated to current).
Fixed tests (6):
Broken tests (2):
|
7a092ce
to
b23f8fb
Compare
I did not have time to look at these change in depth, but I have some thoughts on the topic of possible errors and The first thing is that we do not have to implement We currently have some bugs related to completions and control flow that are closeley related to // boa
eval('if (true) {"a";}'); // undefined
eval('do {"a";} while (false)'); // undefined
// Node
eval('if (true) {"a";}'); // "a"
eval('do {"a";} while (false)'); // "a" This in itself is not a big problem, but when combined with control flow breaking statements it gets a little tricky: eval('do {"a"; if (true) { "b"; continue }; "c";} while (false);'); // "b" In this example we have to somehow make sure that we return either the Also if there are some false positives that you find while working on this, it would not surprise me if they are related to these bugs @nekevss |
@raskad Agreed on not implementing the I do think that there is a way to implement something akin to The approach I thought might work is to first ensure that control flow is stable and predictable. That way we can guarantee when a break occurs that it will end in x place. Once that's done, then we could either store the value in a record on the I think this branch does address some of/if not most of the control flow bugs. |
ad5d6b6
to
03500d3
Compare
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 like the logic changes. It's still hard to understand, but way better than the old logic.
I've got some suggestions, mainly to avoid panics and to make some things more readable.
@@ -22,7 +22,7 @@ impl ByteCompiler<'_, '_> { | |||
configurable_globals: bool, | |||
) -> JsResult<()> { | |||
self.context.push_compile_time_environment(false); | |||
let push_env = self.emit_and_track_decl_env(); | |||
let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment); | |||
self.push_empty_loop_jump_control(); |
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 did not change in the PR, but I just noticed that pushing the loop control info here, before the initializer, is theoretically wrong, right? It should only be pushed after the Opcode::LoopStart
where we currently do set_label
and set_start_address
.
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 was a small change that was added during the JumpControlInfo
being added. Set label and start address occur down further from lines 46-51. The initial idea was to make everything read a bit more uniform and not insert pushing the jump info 15+ lines into the compilation. But I think it could be taken or left either way.
03500d3
to
d6f3f44
Compare
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 think this is a great improvement. We can start working on some of the more tricky control flow issues when this is merged.
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.
Very nice work! Just some suggestions and questions
6aeef16
to
75dfdbe
Compare
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.
Found some whitespace you missed
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.
Everything looks good! Nice work!
bors r+ |
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel necessary. ---> This Pull Request is meant to address #1900. While working on it, there was a decent amount of refactoring/restructuring. Initially, I had kept with the current approach of just keeping track of a kind and counter on the environment stack, especially because that kept the size of each stack entry to a minimum. I did, however, make a switch to having the opcode create the `EnvStackEntry` with a start address and exit address for a bit more precision. It changes the following: - Consolidates `loop_env_stack` and `try_env_stack` into one `EnvStackEntry` struct. - Changes `Continue`, `Break`, `LoopStart`, `LoopContinue`, `FinallyStart`, `FinallyEnd` and various others. Most of this primarily revolves around the creating of `EnvStackEntry` and interacting with the `env_stack`. - Changes/updates the try-catch-finally, break and continue statement compilations as necessary - Adds an `AbruptCompletionRecord` in place of the `finally_jump` vector. - Adds some tests for try-catch-finally blocks with breaks.
Pull request successfully merged into main. Build succeeded: |
This Pull Request is meant to address #1900. While working on it, there was a decent amount of refactoring/restructuring. Initially, I had kept with the current approach of just keeping track of a kind and counter on the environment stack, especially because that kept the size of each stack entry to a minimum.
I did, however, make a switch to having the opcode create the
EnvStackEntry
with a start address and exit address for a bit more precision.It changes the following:
loop_env_stack
andtry_env_stack
into oneEnvStackEntry
struct.Continue
,Break
,LoopStart
,LoopContinue
,FinallyStart
,FinallyEnd
and various others. Most of this primarily revolves around the creating ofEnvStackEntry
and interacting with theenv_stack
.AbruptCompletionRecord
in place of thefinally_jump
vector.