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

Add loop and switch return values #2828

Merged
merged 2 commits into from
May 8, 2023
Merged

Add loop and switch return values #2828

merged 2 commits into from
May 8, 2023

Conversation

raskad
Copy link
Member

@raskad raskad commented Apr 17, 2023

This Pull Request changes the following:

  • Return values for all loops and switch statements.
  • Explicitly capture and update loop return values on the env_stack of the CallFrames.
  • Update loop return values on break and continue.
  • Adjust bytecode to handle statements that do not return any values.

@raskad raskad added bug Something isn't working execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine. labels Apr 17, 2023
@raskad raskad added this to the v0.17.0 milestone Apr 17, 2023
@github-actions
Copy link

github-actions bot commented Apr 17, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,601 94,601 0
Passed 73,755 73,831 +76
Ignored 17,505 17,505 0
Failed 3,341 3,265 -76
Panics 0 0 0
Conformance 77.96% 78.04% +0.08%
Fixed tests (76):
test/language/statements/for-of/cptn-expr-itr.js [strict mode] (previously Failed)
test/language/statements/for-of/cptn-expr-itr.js (previously Failed)
test/language/statements/for-of/cptn-decl-itr.js [strict mode] (previously Failed)
test/language/statements/for-of/cptn-decl-itr.js (previously Failed)
test/language/statements/try/completion-values.js [strict mode] (previously Failed)
test/language/statements/try/completion-values.js (previously Failed)
test/language/statements/while/cptn-iter.js [strict mode] (previously Failed)
test/language/statements/while/cptn-iter.js (previously Failed)
test/language/statements/while/S12.6.2_A8.js [strict mode] (previously Failed)
test/language/statements/while/S12.6.2_A8.js (previously Failed)
test/language/statements/while/S12.6.2_A7.js [strict mode] (previously Failed)
test/language/statements/while/S12.6.2_A7.js (previously Failed)
test/language/statements/while/S12.6.2_A5.js [strict mode] (previously Failed)
test/language/statements/while/S12.6.2_A5.js (previously Failed)
test/language/statements/while/cptn-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/while/cptn-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-fall-thru-nrml.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-fall-thru-nrml.js (previously Failed)
test/language/statements/switch/cptn-b-final.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-b-final.js (previously Failed)
test/language/statements/switch/cptn-a-fall-thru-nrml.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-a-fall-thru-nrml.js (previously Failed)
test/language/statements/switch/cptn-b-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-b-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-dflt-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-final.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-no-dflt-match-final.js (previously Failed)
test/language/statements/switch/cptn-b-fall-thru-nrml.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-b-fall-thru-nrml.js (previously Failed)
test/language/statements/switch/cptn-dflt-fall-thru-nrml.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-fall-thru-nrml.js (previously Failed)
test/language/statements/switch/cptn-a-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-a-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-dflt-final.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-final.js (previously Failed)
test/language/statements/switch/cptn-dflt-b-fall-thru-nrml.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-b-fall-thru-nrml.js (previously Failed)
test/language/statements/switch/cptn-dflt-b-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-b-abrupt-empty.js (previously Failed)
test/language/statements/switch/cptn-dflt-b-final.js [strict mode] (previously Failed)
test/language/statements/switch/cptn-dflt-b-final.js (previously Failed)
test/language/statements/if/cptn-no-else-true-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/if/cptn-no-else-true-abrupt-empty.js (previously Failed)
test/language/statements/do-while/cptn-normal.js [strict mode] (previously Failed)
test/language/statements/do-while/cptn-normal.js (previously Failed)
test/language/statements/do-while/S12.6.1_A5.js [strict mode] (previously Failed)
test/language/statements/do-while/S12.6.1_A5.js (previously Failed)
test/language/statements/do-while/S12.6.1_A8.js [strict mode] (previously Failed)
test/language/statements/do-while/S12.6.1_A8.js (previously Failed)
test/language/statements/do-while/S12.6.1_A3.js [strict mode] (previously Failed)
test/language/statements/do-while/S12.6.1_A3.js (previously Failed)
test/language/statements/do-while/S12.6.1_A7.js [strict mode] (previously Failed)
test/language/statements/do-while/S12.6.1_A7.js (previously Failed)
test/language/statements/do-while/cptn-abrupt-empty.js [strict mode] (previously Failed)
test/language/statements/do-while/cptn-abrupt-empty.js (previously Failed)
test/language/statements/for-in/S12.6.4_A4.1.js [strict mode] (previously Failed)
test/language/statements/for-in/S12.6.4_A4.1.js (previously Failed)
test/language/statements/for-in/S12.6.4_A4.js [strict mode] (previously Failed)
test/language/statements/for-in/S12.6.4_A4.js (previously Failed)
test/language/statements/for-in/S12.6.4_A3.js [strict mode] (previously Failed)
test/language/statements/for-in/S12.6.4_A3.js (previously Failed)
test/language/statements/for-in/S12.6.4_A3.1.js [strict mode] (previously Failed)
test/language/statements/for-in/S12.6.4_A3.1.js (previously Failed)
test/language/statements/for-in/cptn-expr-itr.js [strict mode] (previously Failed)
test/language/statements/for-in/cptn-expr-itr.js (previously Failed)
test/language/statements/for-in/cptn-decl-itr.js [strict mode] (previously Failed)
test/language/statements/for-in/cptn-decl-itr.js (previously Failed)
test/language/statements/for/cptn-expr-expr-iter.js [strict mode] (previously Failed)
test/language/statements/for/cptn-expr-expr-iter.js (previously Failed)
test/language/statements/for/cptn-decl-expr-iter.js [strict mode] (previously Failed)
test/language/statements/for/cptn-decl-expr-iter.js (previously Failed)
test/language/statements/with/cptn-nrml.js (previously Failed)
test/language/statements/with/cptn-abrupt-empty.js (previously Failed)

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #2828 (d547395) into main (70b0d49) will increase coverage by 0.06%.
The diff coverage is 86.29%.

@@            Coverage Diff             @@
##             main    #2828      +/-   ##
==========================================
+ Coverage   51.90%   51.96%   +0.06%     
==========================================
  Files         431      431              
  Lines       43562    43657      +95     
==========================================
+ Hits        22609    22686      +77     
- Misses      20953    20971      +18     
Impacted Files Coverage Δ
boa_engine/src/bytecompiler/statement/with.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/call_frame/mod.rs 89.47% <ø> (ø)
boa_engine/src/vm/code_block.rs 56.57% <ø> (ø)
boa_engine/src/vm/flowgraph/mod.rs 0.00% <0.00%> (ø)
boa_engine/src/vm/opcode/mod.rs 45.45% <ø> (ø)
boa_engine/src/bytecompiler/statement/labelled.rs 65.21% <40.00%> (ø)
boa_engine/src/vm/opcode/control_flow/continue.rs 82.50% <64.70%> (-8.81%) ⬇️
boa_engine/src/vm/opcode/control_flow/break.rs 88.88% <76.47%> (-6.57%) ⬇️
boa_ast/src/statement/mod.rs 76.82% <80.00%> (+0.20%) ⬆️
boa_engine/src/bytecompiler/statement/mod.rs 94.11% <85.71%> (ø)
... and 7 more

... and 2 files with indirect coverage changes

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.

Nice work! Looks good to me! :)

@raskad raskad force-pushed the loop-return-values branch 2 times, most recently from c9a4df9 to 2d91f7a Compare April 25, 2023 22:20
@raskad raskad force-pushed the loop-return-values branch from 2d91f7a to b14e9f7 Compare May 5, 2023 02:04
@raskad
Copy link
Member Author

raskad commented May 5, 2023

Rebased. I had to add a compile_statement_list_without_declarations function but that should be able to be removed when #2887 is merged.

@raskad raskad force-pushed the loop-return-values branch from b14e9f7 to 5359549 Compare May 5, 2023 18:52
@jedel1043 jedel1043 enabled auto-merge May 7, 2023 17:28
@raskad raskad force-pushed the loop-return-values branch from 5359549 to 58dbd4c Compare May 7, 2023 22:26
@raskad
Copy link
Member Author

raskad commented May 7, 2023

@jedel1043 I dont know if you already reviewed this or just enabled auto merge earlier, but now this is rebased again and ready.

@@ -113,7 +80,45 @@ impl Operation for LoopEnd {
const INSTRUCTION: &'static str = "INST - LoopEnd";

fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
cleanup_loop_environment(context);
//cleanup_loop_environment(context);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//cleanup_loop_environment(context);

/// Set the loop return value for the current `EnvStackEntry`.
pub(crate) fn set_loop_return_value(&mut self, value: &JsValue) -> bool {
if let EnvEntryKind::Loop { value: v, .. } = &mut self.kind {
*v = value.clone();
Copy link
Member

Choose a reason for hiding this comment

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

If we're assigning v anyways, maybe this should take value by value then.

@jedel1043
Copy link
Member

@jedel1043 I dont know if you already reviewed this or just enabled auto merge earlier, but now this is rebased again and ready.

Just enabled auto merge, but left my review moments ago

@jedel1043 jedel1043 added this pull request to the merge queue May 8, 2023
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.

All good, nice job!

Merged via the queue into main with commit 7605453 May 8, 2023
@raskad raskad deleted the loop-return-values branch May 8, 2023 10:57
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 vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants