Fix destructive for-of loop assignments (#2803)

This Pull Request changes the following:

- Fix the assignment of variables in destructive for-of initializers.
- Fix the environment handling in the compilation of `for-of/in` loops.
- Close iterators in destructive for-of loop assignments, when the code throws during the assignment.

boa_engine/src/bytecompiler/declaration/declaration_pattern.rs

@@ -176,6 +176,7 @@ impl ByteCompiler<'_, '_> {
             Pattern::Array(pattern) => {
                 self.emit_opcode(Opcode::ValueNotNullOrUndefined);
                 self.emit_opcode(Opcode::GetIterator);
+                self.emit_opcode(Opcode::IteratorClosePush);
                 match pattern.bindings().split_last() {
                     None => self.emit_opcode(Opcode::PushFalse),
                     Some((last, rest)) => {
@@ -185,6 +186,7 @@ impl ByteCompiler<'_, '_> {
                         self.compile_array_pattern_element(last, def, true);
                     }
                 }
+                self.emit_opcode(Opcode::IteratorClosePop);
                 self.iterator_close(false);
             }
         }
@@ -209,7 +211,7 @@ impl ByteCompiler<'_, '_> {
         match element {
             // ArrayBindingPattern : [ Elision ]
             Elision => {
-                self.emit_opcode(Opcode::IteratorNext);
+                self.emit_opcode(Opcode::IteratorNextSetDone);
                 if with_done {
                     self.emit_opcode(Opcode::IteratorUnwrapNext);
                 }
@@ -220,7 +222,7 @@ impl ByteCompiler<'_, '_> {
                 ident,
                 default_init,
             } => {
-                self.emit_opcode(Opcode::IteratorNext);
+                self.emit_opcode(Opcode::IteratorNextSetDone);
                 self.emit_opcode(unwrapping);
                 if let Some(init) = default_init {
                     let skip = self.emit_opcode_with_operand(Opcode::JumpIfNotUndefined);
@@ -230,20 +232,31 @@ impl ByteCompiler<'_, '_> {
                 self.emit_binding(def, *ident);
             }
             PropertyAccess { access } => {
-                self.emit_opcode(Opcode::IteratorNext);
-                self.emit_opcode(unwrapping);
-                self.access_set(
-                    Access::Property { access },
-                    false,
-                    ByteCompiler::access_set_top_of_stack_expr_fn,
-                );
+                self.access_set(Access::Property { access }, false, |compiler, level| {
+                    if level != 0 {
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 2);
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 2);
+                    }
+                    compiler.emit_opcode(Opcode::IteratorNextSetDone);
+                    compiler.emit_opcode(unwrapping);
+                    if level != 0 {
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 3 + u8::from(with_done));
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 3 + u8::from(with_done));
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 1);
+                    }
+                });
             }
             // BindingElement : BindingPattern Initializer[opt]
             Pattern {
                 pattern,
                 default_init,
             } => {
-                self.emit_opcode(Opcode::IteratorNext);
+                self.emit_opcode(Opcode::IteratorNextSetDone);
                 self.emit_opcode(unwrapping);
 
                 if let Some(init) = default_init {
@@ -263,12 +276,23 @@ impl ByteCompiler<'_, '_> {
                 }
             }
             PropertyAccessRest { access } => {
-                self.emit_opcode(Opcode::IteratorToArray);
-                self.access_set(
-                    Access::Property { access },
-                    false,
-                    ByteCompiler::access_set_top_of_stack_expr_fn,
-                );
+                self.access_set(Access::Property { access }, false, |compiler, level| {
+                    if level != 0 {
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 2);
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 2);
+                    }
+                    compiler.emit_opcode(Opcode::IteratorToArray);
+                    if level != 0 {
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 3);
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 3);
+                        compiler.emit_opcode(Opcode::RotateLeft);
+                        compiler.emit_u8(level + 1);
+                    }
+                });
                 if with_done {
                     self.emit_opcode(Opcode::PushTrue);
                 }

boa_engine/src/bytecompiler/statement/loop.rs

@@ -89,15 +89,19 @@ impl ByteCompiler<'_, '_> {
         label: Option<Sym>,
         configurable_globals: bool,
     ) {
-        let init_bound_names = bound_names(for_in_loop.initializer());
-        if init_bound_names.is_empty() {
+        let initializer_bound_names = match for_in_loop.initializer() {
+            IterableLoopInitializer::Let(declaration)
+            | IterableLoopInitializer::Const(declaration) => bound_names(declaration),
+            _ => Vec::new(),
+        };
+        if initializer_bound_names.is_empty() {
             self.compile_expr(for_in_loop.target(), true);
         } else {
             self.push_compile_environment(false);
             let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-            for name in init_bound_names {
-                self.create_mutable_binding(name, false, false);
+            for name in &initializer_bound_names {
+                self.create_mutable_binding(*name, false, false);
             }
             self.compile_expr(for_in_loop.target(), true);
 
@@ -119,12 +123,17 @@ impl ByteCompiler<'_, '_> {
         self.patch_jump_with_target(continue_label, start_address);
         self.patch_jump_with_target(loop_start, start_address);
 
-        self.push_compile_environment(false);
-        let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
         self.emit_opcode(Opcode::Pop); // pop the `done` value.
         self.emit_opcode(Opcode::IteratorNext);
         let exit = self.emit_opcode_with_operand(Opcode::IteratorUnwrapNextOrJump);
 
+        let iteration_environment = if initializer_bound_names.is_empty() {
+            None
+        } else {
+            self.push_compile_environment(false);
+            Some(self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment))
+        };
+
         match for_in_loop.initializer() {
             IterableLoopInitializer::Identifier(ident) => {
                 self.create_mutable_binding(*ident, true, true);
@@ -176,19 +185,18 @@ impl ByteCompiler<'_, '_> {
                 }
             },
             IterableLoopInitializer::Pattern(pattern) => {
-                for ident in bound_names(pattern) {
-                    self.create_mutable_binding(ident, true, true);
-                }
-                self.compile_declaration_pattern(pattern, BindingOpcode::InitVar);
+                self.compile_declaration_pattern(pattern, BindingOpcode::SetName);
             }
         }
 
         self.compile_stmt(for_in_loop.body(), false, configurable_globals);
 
-        let env_info = self.pop_compile_environment();
-        self.patch_jump_with_target(push_env.0, env_info.num_bindings as u32);
-        self.patch_jump_with_target(push_env.1, env_info.index as u32);
-        self.emit_opcode(Opcode::PopEnvironment);
+        if let Some(iteration_environment) = iteration_environment {
+            let env_info = self.pop_compile_environment();
+            self.patch_jump_with_target(iteration_environment.0, env_info.num_bindings as u32);
+            self.patch_jump_with_target(iteration_environment.1, env_info.index as u32);
+            self.emit_opcode(Opcode::PopEnvironment);
+        }
 
         self.emit(Opcode::Jump, &[start_address]);
 
@@ -197,7 +205,9 @@ impl ByteCompiler<'_, '_> {
         self.patch_jump(cont_exit_label);
         self.pop_loop_control_info();
         self.emit_opcode(Opcode::LoopEnd);
-        self.iterator_close(false);
+        self.emit_opcode(Opcode::Pop);
+        self.emit_opcode(Opcode::Pop);
+        self.emit_opcode(Opcode::Pop);
 
         self.patch_jump(early_exit);
     }
@@ -208,15 +218,19 @@ impl ByteCompiler<'_, '_> {
         label: Option<Sym>,
         configurable_globals: bool,
     ) {
-        let init_bound_names = bound_names(for_of_loop.initializer());
-        if init_bound_names.is_empty() {
+        let initializer_bound_names = match for_of_loop.initializer() {
+            IterableLoopInitializer::Let(declaration)
+            | IterableLoopInitializer::Const(declaration) => bound_names(declaration),
+            _ => Vec::new(),
+        };
+        if initializer_bound_names.is_empty() {
             self.compile_expr(for_of_loop.iterable(), true);
         } else {
             self.push_compile_environment(false);
             let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
 
-            for name in init_bound_names {
-                self.create_mutable_binding(name, false, false);
+            for name in &initializer_bound_names {
+                self.create_mutable_binding(*name, false, false);
             }
             self.compile_expr(for_of_loop.iterable(), true);
 
@@ -241,9 +255,6 @@ impl ByteCompiler<'_, '_> {
         self.patch_jump_with_target(loop_start, start_address);
         self.patch_jump_with_target(cont_label, start_address);
 
-        self.push_compile_environment(false);
-        let push_env = self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment);
-
         self.emit_opcode(Opcode::Pop); // pop the `done` value.
         self.emit_opcode(Opcode::IteratorNext);
         if for_of_loop.r#await() {
@@ -252,6 +263,13 @@ impl ByteCompiler<'_, '_> {
         }
         let exit = self.emit_opcode_with_operand(Opcode::IteratorUnwrapNextOrJump);
 
+        let iteration_environment = if initializer_bound_names.is_empty() {
+            None
+        } else {
+            self.push_compile_environment(false);
+            Some(self.emit_opcode_with_two_operands(Opcode::PushDeclarativeEnvironment))
+        };
+
         match for_of_loop.initializer() {
             IterableLoopInitializer::Identifier(ref ident) => {
                 self.create_mutable_binding(*ident, true, true);
@@ -303,19 +321,18 @@ impl ByteCompiler<'_, '_> {
                 }
             },
             IterableLoopInitializer::Pattern(pattern) => {
-                for ident in bound_names(pattern) {
-                    self.create_mutable_binding(ident, true, true);
-                }
-                self.compile_declaration_pattern(pattern, BindingOpcode::InitVar);
+                self.compile_declaration_pattern(pattern, BindingOpcode::SetName);
             }
         }
 
         self.compile_stmt(for_of_loop.body(), false, configurable_globals);
 
-        let env_info = self.pop_compile_environment();
-        self.patch_jump_with_target(push_env.0, env_info.num_bindings as u32);
-        self.patch_jump_with_target(push_env.1, env_info.index as u32);
-        self.emit_opcode(Opcode::PopEnvironment);
+        if let Some(iteration_environment) = iteration_environment {
+            let env_info = self.pop_compile_environment();
+            self.patch_jump_with_target(iteration_environment.0, env_info.num_bindings as u32);
+            self.patch_jump_with_target(iteration_environment.1, env_info.index as u32);
+            self.emit_opcode(Opcode::PopEnvironment);
+        }
 
         self.emit(Opcode::Jump, &[start_address]);
 

boa_engine/src/vm/call_frame/mod.rs

@@ -2,14 +2,16 @@
 //!
 //! This module will provides everything needed to implement the `CallFrame`
 
-use crate::{object::JsObject, vm::CodeBlock};
-use boa_gc::{Finalize, Gc, Trace};
-
 mod abrupt_record;
 mod env_stack;
 
+use crate::{object::JsObject, vm::CodeBlock};
+use boa_gc::{Finalize, Gc, Trace};
+use thin_vec::ThinVec;
+
 pub(crate) use abrupt_record::AbruptCompletionRecord;
 pub(crate) use env_stack::EnvStackEntry;
+
 /// A `CallFrame` holds the state of a function call.
 #[derive(Clone, Debug, Finalize, Trace)]
 pub struct CallFrame {
@@ -33,6 +35,9 @@ pub struct CallFrame {
     // When an async generator is resumed, the generator object is needed
     // to fulfill the steps 4.e-j in [AsyncGeneratorStart](https://tc39.es/ecma262/#sec-asyncgeneratorstart).
     pub(crate) async_generator: Option<JsObject>,
+
+    // Iterators and their `[[Done]]` flags that must be closed when an abrupt completion is thrown.
+    pub(crate) iterators: ThinVec<(JsObject, bool)>,
 }
 
 /// ---- `CallFrame` creation methods ----
@@ -52,6 +57,7 @@ impl CallFrame {
             arg_count: 0,
             generator_resume_kind: GeneratorResumeKind::Normal,
             async_generator: None,
+            iterators: ThinVec::new(),
         }
     }
 

boa_engine/src/vm/code_block.rs

@@ -448,9 +448,12 @@ impl CodeBlock {
             | Opcode::GetIterator
             | Opcode::GetAsyncIterator
             | Opcode::IteratorNext
+            | Opcode::IteratorNextSetDone
             | Opcode::IteratorUnwrapNext
             | Opcode::IteratorUnwrapValue
             | Opcode::IteratorToArray
+            | Opcode::IteratorClosePush
+            | Opcode::IteratorClosePop
             | Opcode::RequireObjectCoercible
             | Opcode::ValueNotNullOrUndefined
             | Opcode::RestParameterInit

boa_engine/src/vm/flowgraph/mod.rs

@@ -502,9 +502,12 @@ impl CodeBlock {
                 | Opcode::GetIterator
                 | Opcode::GetAsyncIterator
                 | Opcode::IteratorNext
+                | Opcode::IteratorNextSetDone
                 | Opcode::IteratorUnwrapNext
                 | Opcode::IteratorUnwrapValue
                 | Opcode::IteratorToArray
+                | Opcode::IteratorClosePush
+                | Opcode::IteratorClosePop
                 | Opcode::RequireObjectCoercible
                 | Opcode::ValueNotNullOrUndefined
                 | Opcode::RestParameterInit

boa_engine/src/vm/opcode/control_flow/throw.rs

@@ -1,7 +1,9 @@
 use crate::{
+    js_string,
     vm::{call_frame::AbruptCompletionRecord, opcode::Operation, CompletionType},
     Context, JsError, JsNativeError, JsResult,
 };
+use thin_vec::ThinVec;
 
 /// `Throw` implements the Opcode Operation for `Opcode::Throw`
 ///
@@ -20,6 +22,20 @@ impl Operation for Throw {
         } else {
             JsError::from_opaque(context.vm.pop())
         };
+
+        // Close all iterators that are still open.
+        let mut iterators = ThinVec::new();
+        std::mem::swap(&mut iterators, &mut context.vm.frame_mut().iterators);
+        for (iterator, done) in iterators {
+            if done {
+                continue;
+            }
+            if let Ok(Some(f)) = iterator.get_method(js_string!("return"), context) {
+                drop(f.call(&iterator.into(), &[], context));
+            }
+        }
+        context.vm.err.take();
+
         // 1. Find the viable catch and finally blocks
         let current_address = context.vm.frame().pc;
         let viable_catch_candidates = context

boa_engine/src/vm/opcode/iteration/iterator.rs

@@ -29,6 +29,57 @@ impl Operation for IteratorNext {
     }
 }
 
+/// `IteratorNextSetDone` implements the Opcode Operation for `Opcode::IteratorNextSetDone`
+///
+/// Operation:
+///  - Calls the `next` method of `iterator`, puts its return value on the stack
+///    and sets the `[[Done]]` value of the iterator on the call frame.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct IteratorNextSetDone;
+
+impl Operation for IteratorNextSetDone {
+    const NAME: &'static str = "IteratorNextSetDone";
+    const INSTRUCTION: &'static str = "INST - IteratorNextSetDone";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let next_method = context.vm.pop();
+        let iterator = context.vm.pop();
+        let mut done = true;
+        let result = next_method
+            .call(&iterator, &[], context)
+            .and_then(|next_result| {
+                next_method
+                    .as_object()
+                    .cloned()
+                    .map(IteratorResult::new)
+                    .ok_or_else(|| {
+                        JsNativeError::typ()
+                            .with_message("next value should be an object")
+                            .into()
+                    })
+                    .and_then(|iterator_result| {
+                        iterator_result.complete(context).map(|d| {
+                            done = d;
+                            context.vm.push(iterator);
+                            context.vm.push(next_method);
+                            context.vm.push(next_result);
+                            CompletionType::Normal
+                        })
+                    })
+            });
+
+        context
+            .vm
+            .frame_mut()
+            .iterators
+            .last_mut()
+            .expect("iterator on the call frame must exist")
+            .1 = done;
+
+        result
+    }
+}
+
 /// `IteratorUnwrapNext` implements the Opcode Operation for `Opcode::IteratorUnwrapNext`
 ///
 /// Operation:
@@ -105,8 +156,6 @@ impl Operation for IteratorUnwrapNextOrJump {
 
         if next_result.complete(context)? {
             context.vm.frame_mut().pc = address as usize;
-            context.vm.frame_mut().dec_frame_env_stack();
-            context.vm.environments.pop();
             context.vm.push(true);
         } else {
             context.vm.push(false);
@@ -136,8 +185,26 @@ impl Operation for IteratorToArray {
         let iterator_record = IteratorRecord::new(iterator.clone(), next_method.clone(), false);
         let mut values = Vec::new();
 
-        while let Some(result) = iterator_record.step(context)? {
-            values.push(result.value(context)?);
+        let err = loop {
+            match iterator_record.step(context) {
+                Ok(Some(result)) => match result.value(context) {
+                    Ok(value) => values.push(value),
+                    Err(err) => break Some(err),
+                },
+                Ok(None) => break None,
+                Err(err) => break Some(err),
+            }
+        };
+
+        context
+            .vm
+            .frame_mut()
+            .iterators
+            .last_mut()
+            .expect("should exist")
+            .1 = true;
+        if let Some(err) = err {
+            return Err(err);
         }
 
         let array = Array::create_array_from_list(values, context);
@@ -148,3 +215,46 @@ impl Operation for IteratorToArray {
         Ok(CompletionType::Normal)
     }
 }
+
+/// `IteratorClosePush` implements the Opcode Operation for `Opcode::IteratorClosePush`
+///
+/// Operation:
+///  - Push an iterator to the call frame close iterator stack.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct IteratorClosePush;
+
+impl Operation for IteratorClosePush {
+    const NAME: &'static str = "IteratorClosePush";
+    const INSTRUCTION: &'static str = "INST - IteratorClosePush";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        let next_method = context.vm.pop();
+        let iterator = context.vm.pop();
+        let iterator_object = iterator.as_object().expect("iterator was not an object");
+        context
+            .vm
+            .frame_mut()
+            .iterators
+            .push((iterator_object.clone(), false));
+        context.vm.push(iterator);
+        context.vm.push(next_method);
+        Ok(CompletionType::Normal)
+    }
+}
+
+/// `IteratorClosePop` implements the Opcode Operation for `Opcode::IteratorClosePop`
+///
+/// Operation:
+///  - Pop an iterator from the call frame close iterator stack.
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct IteratorClosePop;
+
+impl Operation for IteratorClosePop {
+    const NAME: &'static str = "IteratorClosePop";
+    const INSTRUCTION: &'static str = "INST - IteratorClosePop";
+
+    fn execute(context: &mut Context<'_>) -> JsResult<CompletionType> {
+        context.vm.frame_mut().iterators.pop();
+        Ok(CompletionType::Normal)
+    }
+}

boa_engine/src/vm/opcode/mod.rs

@@ -1423,6 +1423,14 @@ generate_impl! {
         /// Stack: iterator, next_method **=>** iterator, next_method, next_value
         IteratorNext,
 
+        /// Calls the `next` method of `iterator`, puts its return value on the stack
+        /// and sets the `[[Done]]` value of the iterator on the call frame.
+        ///
+        /// Operands:
+        ///
+        /// Stack: iterator, next_method **=>** iterator, next_method, next_value
+        IteratorNextSetDone,
+
         /// Gets the `value` and `done` properties of an iterator result.
         ///
         /// Stack: next_result **=>** done, next_value
@@ -1448,6 +1456,20 @@ generate_impl! {
         /// Stack: iterator, next_method **=>** iterator, next_method, array
         IteratorToArray,
 
+        /// Push an iterator to the call frame close iterator stack.
+        ///
+        /// Operands:
+        ///
+        /// Stack: iterator, next_method => iterator, next_method
+        IteratorClosePush,
+
+        /// Pop an iterator from the call frame close iterator stack.
+        ///
+        /// Operands:
+        ///
+        /// Stack:
+        IteratorClosePop,
+
         /// Concat multiple stack objects into a string.
         ///
         /// Operands: value_count: `u32`