[Merged by Bors] - Implement `CompletionRecords` for the Vm #2618

nekevss · 2023-02-26T02:11:16Z

This goal of this draft is to begin to implement CompletionRecords. This draft switches the Vm's Opcode from JsResult<ShouldExit> to CompletionType where the value is the top of the stack and returns a CompletionRecord. Primarily submitting as a draft for now for feedback.

It changes the following:

Adds CompletionRecord and CompletionType to Vm.
Changes the evaluation loop from while to loop that returns a CompletionType.
Adapts Context::run to handle CompletionType and CompletionRecord.
Removes FinallyAddresses in favor of just checking the env_stack.

github-actions · 2023-02-26T02:20:07Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	94,181	94,181	0
Passed	71,950	71,950	0
Ignored	17,218	17,218	0
Failed	5,013	5,013	0
Panics	12	12	0
Conformance	76.40%	76.40%	0.00%

codecov · 2023-02-26T02:22:07Z

Codecov Report

Merging #2618 (34eccb4) into main (7f6af7b) will decrease coverage by 0.06%.
The diff coverage is 63.72%.

@@            Coverage Diff             @@
##             main    #2618      +/-   ##
==========================================
- Coverage   49.63%   49.57%   -0.06%     
==========================================
  Files         386      388       +2     
  Lines       39176    39280     +104     
==========================================
+ Hits        19445    19475      +30     
- Misses      19731    19805      +74

Impacted Files	Coverage Δ
boa_engine/src/builtins/async_generator/mod.rs	`5.07% <0.00%> (ø)`
boa_engine/src/builtins/generator/mod.rs	`9.31% <0.00%> (ø)`
boa_engine/src/vm/opcode/await_stm/mod.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/define/class/getter.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/define/class/method.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/define/class/setter.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/generator/mod.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/generator/yield_stm.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/get/generator.rs	`0.00% <0.00%> (ø)`
boa_engine/src/vm/opcode/get/private.rs	`0.00% <0.00%> (ø)`
... and 65 more

... and 13 files with indirect coverage changes

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

jedel1043 · 2023-02-26T02:43:56Z

Nice work! Does this close #1542, #1543 and #1544? If that's the case, it would be good to link them :)

nekevss · 2023-02-26T03:18:31Z

Does this close #1542, #1543 and #1544?

I think it does in a way. It's a bit hard to say as I think those issues may predate the move to the VM.

It does have Context::run return a CompletionRecord, but it is only a CompletionRecord and not a JsResult<CompletionRecord>, which is one of the main reasons I submitted this as a draft first for feedback. ~~Also, the Operations trait would now return a CompletionType where the top of the stack would essentially be the [[value]] field~~.

Essentially, this should allow us to use JsResult<JsValue> for any method or operation that returns only a Normal or Throw Completion. But for the operations or value that can return an abrupt completion, we should be able to use the CompletionRecord.

jedel1043 · 2023-02-28T17:55:52Z

boa_engine/src/vm/completion_record.rs

+    pub(crate) fn convert(self, context: &mut Context<'_>) -> JsResult<JsValue> {
+        match self.completion_type {
+            CompletionType::Throw => {
+                let err = JsError::from_opaque(self.value);
+                if let Ok(native) = err.try_native(context) {
+                    Err(JsError::from_native(native))
+                } else {
+                    Err(err)
+                }
+            }
+            _ => Ok(self.value),
+        }
+    }


Would it be possible to instead make CompletionRecord something like:

enum CompletionRecord { Normal(JsValue), Return(JsValue), Throw(JsError) }

?

That way we don't need to convert JsErrors to opaque errors back and forth.

HalidOdat · 2023-03-08T07:51:45Z

boa_engine/src/vm/completion_record.rs

@@ -0,0 +1,24 @@
+use crate::{JsError, JsResult, JsValue};
+
+#[derive(Debug, Clone)]


Suggested change

#[derive(Debug, Clone)]

// spec: <https://tc39.es/ecma262/#sec-completion-record-specification-type>

#[derive(Debug, Clone)]

Would be nice to have a link to spec :)

HalidOdat · 2023-03-08T08:08:53Z

boa_engine/src/vm/completion_record.rs

+    }
+
+    /// This function will consume the current `CompletionRecord` and return a `JsResult<JsValue>`
+    #[allow(clippy::missing_const_for_fn)]


Why is this lint allowed? If there is a valid reason could we have a comment explaining it :)

HalidOdat · 2023-03-08T08:15:29Z

boa_engine/src/vm/opcode/push/class/mod.rs

        } else {
-            Err(JsNativeError::typ()
+            return Err(JsNativeError::typ()


Why explicitly use return keyword here?

HalidOdat · 2023-03-08T18:59:57Z

Looks good! Great work @nekevss , Will try to do a more thorough review tomorrow! :)

HalidOdat

Looks good to me! :)

raskad

I like the new logic.
If @jedel1043 agrees and you want to merge this as is lets do it @nekevss.

jedel1043

Good work! The changes look almost perfect :)

boa_engine/src/builtins/async_generator/mod.rs

boa_engine/src/vm/completion_record.rs

jedel1043

Looks great to me!

nekevss · 2023-03-16T02:35:37Z

bors r+

This goal of this draft is to begin to implement `CompletionRecords`. This draft switches the `Vm`'s `Opcode` from `JsResult<ShouldExit>` to `CompletionType` where the value is the top of the stack and returns a `CompletionRecord`. Primarily submitting as a draft for now for feedback. It changes the following: - Adds `CompletionRecord` and `CompletionType` to `Vm`. - Changes the evaluation loop from `while` to `loop` that returns a `CompletionType`. - Adapts `Context::run` to handle `CompletionType` and `CompletionRecord`. - Removes `FinallyAddresses` in favor of just checking the `env_stack`.

bors · 2023-03-16T02:47:02Z

Pull request successfully merged into main.

Build succeeded:

nekevss requested review from raskad, jedel1043, HalidOdat, jasonwilliams, RageKnify and Razican February 28, 2023 15:51

jedel1043 reviewed Feb 28, 2023

View reviewed changes

HalidOdat reviewed Mar 8, 2023

View reviewed changes

HalidOdat approved these changes Mar 9, 2023

View reviewed changes

nekevss added 16 commits March 10, 2023 22:41

Initial changes to return and modules structure

feb5e3c

Removed FinallyReturn and TryAddresses

11d3f14

Add CompletionRecord and convert vm executionloop and enums

b45ea45

All non-doc tests passing/fp added to call frame

d67caf5

Removed commented out old execution loop

6415130

Move throw logic into Opcode

6494812

Fixes to logic handling await and yield

8d8b087

Fixed all panics - first draft complete

b30baee

Revert to set_frame_pointer

2f5cb85

Rearrange EarlyReturn and trace logging

0f1e296

Fixed FinallyEnd And GeneratorResume handling

26f72e1

Run Rustfmt and Clippy

f61bf55

Remove dead function and revert some lines back to original state

7502c85

Post rebase fmt and clippy

ae4fc7d

Fix fuzz error block

7a9fe5b

Cargo fmt

eb3413a

nekevss added 7 commits March 10, 2023 22:41

return -> break

0388d6f

Fix move error

fa24e7a

Fix move error p2

bdbef95

Update CompletionRecords to enum

c50f28a

Revert to JsResult

6970095

Add err-register and preserve JsNativeErrors

b4e68bc

Remove unneeded return stmt and add more comments/docs

34eccb4

nekevss force-pushed the impl-completion-records branch from 3aaa136 to 34eccb4 Compare March 11, 2023 03:42

raskad approved these changes Mar 13, 2023

View reviewed changes

jedel1043 requested changes Mar 13, 2023

View reviewed changes

boa_engine/src/builtins/async_generator/mod.rs Show resolved Hide resolved

boa_engine/src/vm/completion_record.rs Show resolved Hide resolved

jedel1043 approved these changes Mar 16, 2023

View reviewed changes

nekevss added this to the v0.17.0 milestone Mar 16, 2023

bors bot changed the title ~~Implement CompletionRecords for the Vm~~ [Merged by Bors] - Implement CompletionRecords for the Vm Mar 16, 2023

bors bot closed this Mar 16, 2023

bors bot deleted the impl-completion-records branch March 16, 2023 02:47

nekevss mentioned this pull request Mar 16, 2023

Evaluate/Reevaluate the execution semantics of CompletionRecord #2675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Implement `CompletionRecords` for the Vm #2618

[Merged by Bors] - Implement `CompletionRecords` for the Vm #2618

nekevss commented Feb 26, 2023

github-actions bot commented Feb 26, 2023 •

edited

Loading

codecov bot commented Feb 26, 2023 •

edited

Loading

jedel1043 commented Feb 26, 2023

nekevss commented Feb 26, 2023 •

edited

Loading

jedel1043 Feb 28, 2023

HalidOdat Mar 8, 2023 •

edited

Loading

HalidOdat Mar 8, 2023 •

edited

Loading

HalidOdat Mar 8, 2023

HalidOdat commented Mar 8, 2023

HalidOdat left a comment

raskad left a comment

jedel1043 left a comment

jedel1043 left a comment

nekevss commented Mar 16, 2023

bors bot commented Mar 16, 2023

		@@ -0,0 +1,24 @@
		use crate::{JsError, JsResult, JsValue};

		#[derive(Debug, Clone)]

	#[derive(Debug, Clone)]
	// spec: <https://tc39.es/ecma262/#sec-completion-record-specification-type>
	#[derive(Debug, Clone)]

[Merged by Bors] - Implement CompletionRecords for the Vm #2618

[Merged by Bors] - Implement CompletionRecords for the Vm #2618

Conversation

nekevss commented Feb 26, 2023

github-actions bot commented Feb 26, 2023 • edited Loading

Test262 conformance changes

codecov bot commented Feb 26, 2023 • edited Loading

Codecov Report

jedel1043 commented Feb 26, 2023

nekevss commented Feb 26, 2023 • edited Loading

jedel1043 Feb 28, 2023

Choose a reason for hiding this comment

HalidOdat Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

HalidOdat Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

HalidOdat Mar 8, 2023

Choose a reason for hiding this comment

HalidOdat commented Mar 8, 2023

HalidOdat left a comment

Choose a reason for hiding this comment

raskad left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

nekevss commented Mar 16, 2023

bors bot commented Mar 16, 2023

[Merged by Bors] - Implement `CompletionRecords` for the Vm #2618

[Merged by Bors] - Implement `CompletionRecords` for the Vm #2618

github-actions bot commented Feb 26, 2023 •

edited

Loading

codecov bot commented Feb 26, 2023 •

edited

Loading

nekevss commented Feb 26, 2023 •

edited

Loading

HalidOdat Mar 8, 2023 •

edited

Loading

HalidOdat Mar 8, 2023 •

edited

Loading