Refactor compile time environment handling #3365

raskad · 2023-10-09T01:51:24Z

This Pull Request changes the following:

Refactor compile time environment handling.
Align compile time environment handling with the spec.
Remove useless modifications to ByteCompiler before the ByteCompiler::finish().
Simplify compile time environment methods and remove duplicates.
Fix some bugs with declarations in eval calls.

github-actions · 2023-10-09T02:01:11Z

Test262 conformance changes

Test result	main count	PR count	difference
Total	95,574	95,574	0
Passed	75,273	75,276	+3
Ignored	19,482	19,482	0
Failed	819	816	-3
Panics	0	0	0
Conformance	78.76%	78.76%	+0.00%

Fixed tests (3):

test/language/statements/variable/12.2.1-11.js (previously Failed)
test/language/eval-code/direct/arrow-fn-body-cntns-arguments-func-decl-arrow-func-declare-arguments-assign-incl-def-param-arrow-arguments.js (previously Failed)
test/language/eval-code/direct/arrow-fn-body-cntns-arguments-var-bind-arrow-func-declare-arguments-assign-incl-def-param-arrow-arguments.js (previously Failed)

codecov · 2023-10-09T02:07:49Z

Codecov Report

Attention: 104 lines in your changes are missing coverage. Please review.

Comparison is base (348c757) 49.55% compared to head (e389020) 49.49%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3365      +/-   ##
==========================================
- Coverage   49.55%   49.49%   -0.06%     
==========================================
  Files         446      446              
  Lines       43705    43643      -62     
==========================================
- Hits        21656    21602      -54     
+ Misses      22049    22041       -8

Files	Coverage Δ
boa_engine/src/builtins/json/mod.rs	`84.90% <100.00%> (+0.05%)`	⬆️
boa_engine/src/bytecompiler/env.rs	`100.00% <100.00%> (+5.12%)`	⬆️
boa_engine/src/bytecompiler/expression/assign.rs	`37.83% <100.00%> (-0.42%)`	⬇️
boa_engine/src/bytecompiler/expression/unary.rs	`100.00% <100.00%> (ø)`
boa_engine/src/bytecompiler/expression/update.rs	`55.00% <100.00%> (-0.45%)`	⬇️
boa_engine/src/bytecompiler/function.rs	`100.00% <100.00%> (ø)`
boa_engine/src/bytecompiler/statement/block.rs	`100.00% <100.00%> (ø)`
boa_engine/src/bytecompiler/statement/switch.rs	`100.00% <100.00%> (ø)`
boa_engine/src/bytecompiler/statement/try.rs	`91.20% <100.00%> (+0.29%)`	⬆️
boa_engine/src/bytecompiler/statement/with.rs	`100.00% <100.00%> (ø)`
... and 14 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

boa_engine/src/builtins/eval/mod.rs

HalidOdat

Nice work! @raskad

Besides @jedel1043 comment this looks good to me! :)

jedel1043 · 2023-10-10T17:52:44Z

boa_engine/src/bytecompiler/mod.rs

@@ -1180,7 +1171,6 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
    }

    /// Compile a [`Declaration`].
-    #[allow(unused_variables)]


This is showing warnings because block is not used if the feature annex-b is disabled. Maybe rename block to _block?

Renaming to _block does not seem to work as we get a different lint for usage of _* named variables. I just added the allow back.

jedel1043

Really nice improvements! Just have some suggestions.

jedel1043 · 2023-10-10T17:57:58Z

boa_engine/src/environments/compile.rs

-    pub(crate) fn is_lex_binding(&self, name: Identifier) -> bool {
+    /// Get the binding locator for a binding with the given name.
+    /// Fall back to the global environment if the binding is not found.
+    pub(crate) fn get_identifier_reference(&self, name: Identifier) -> (BindingLocator, bool) {


Can you document that the bool returned represents the type of binding found? An alternative would be to return an object instead of a tuple:

struct IdentifierReference { locator: BindingLocator, lexical: bool }

jedel1043 · 2023-10-10T18:01:44Z

boa_engine/src/environments/compile.rs

+impl PartialEq for CompileTimeEnvironment {
+    fn eq(&self, other: &Self) -> bool {
+        self.environment_index == other.environment_index
+    }
+}
+


Hmm, it seems wrong to allow comparing environments that may only share the same index as equals (e.g. environments from two different script compilations). Maybe we just expose environment_index and compare that?

jedel1043 · 2023-10-10T18:08:11Z

boa_engine/src/bytecompiler/declarations.rs

+        let lex_env = if strict {
+            // a. Let lexEnv be varEnv.
+            var_env
+        } else {
+            // a. Let lexEnv be NewDeclarativeEnvironment(varEnv).
+            // b. NOTE: Non-strict functions use a separate Environment Record for top-level lexical
+            //    declarations so that a direct eval can determine whether any var scoped declarations
+            //    introduced by the eval code conflict with pre-existing top-level lexically scoped declarations.
+            //    This is not needed for strict functions because a strict direct eval always
+            //    places all declarations into a new Environment Record.
+            let env_index = self.push_compile_environment(false);
+            self.emit_with_varying_operand(Opcode::PushDeclarativeEnvironment, env_index);
+            self.lexical_environment.clone()
+        };


Really liking this! Makes it very easy to implement things per the spec :)

boa_engine/src/bytecompiler/declarations.rs

jedel1043

LGTM!

* Refactor compile time environment handling * Apply review * Apply review

Refactor compile time environment handling

569c21f

raskad added execution Issues or PRs related to code execution Internal Category for changelog labels Oct 9, 2023

raskad added this to the v0.18.0 milestone Oct 9, 2023

jedel1043 requested a review from a team October 10, 2023 00:31

jedel1043 reviewed Oct 10, 2023

View reviewed changes

boa_engine/src/builtins/eval/mod.rs Outdated Show resolved Hide resolved

HalidOdat approved these changes Oct 10, 2023

View reviewed changes

HalidOdat added the technical debt label Oct 10, 2023

Apply review

e8042b3

jedel1043 reviewed Oct 10, 2023

View reviewed changes

Apply review

e389020

jedel1043 approved these changes Oct 10, 2023

View reviewed changes

jedel1043 added this pull request to the merge queue Oct 10, 2023

Merged via the queue into main with commit f654ad1 Oct 10, 2023
14 checks passed

sam-finch-tezos pushed a commit to trilitech/boa that referenced this pull request Nov 29, 2023

Refactor compile time environment handling (boa-dev#3365)

89c02a2

* Refactor compile time environment handling * Apply review * Apply review

raskad deleted the refactor-compile-time-environments branch December 5, 2023 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor compile time environment handling #3365

Refactor compile time environment handling #3365

raskad commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

codecov bot commented Oct 9, 2023 •

edited

Loading

HalidOdat left a comment

jedel1043 Oct 10, 2023

raskad Oct 10, 2023

jedel1043 left a comment

jedel1043 Oct 10, 2023

jedel1043 Oct 10, 2023

jedel1043 Oct 10, 2023

jedel1043 left a comment

Refactor compile time environment handling #3365

Refactor compile time environment handling #3365

Conversation

raskad commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

Test262 conformance changes

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

HalidOdat left a comment

Choose a reason for hiding this comment

jedel1043 Oct 10, 2023

Choose a reason for hiding this comment

raskad Oct 10, 2023

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

jedel1043 Oct 10, 2023

Choose a reason for hiding this comment

jedel1043 Oct 10, 2023

Choose a reason for hiding this comment

jedel1043 Oct 10, 2023

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2023 •

edited

Loading