From bc2dd9c4bdab1b2af7ea03c3949e4a006ce1fa17 Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 17:17:55 +0000 Subject: [PATCH] Fix order dependent execution in assignment. (#2378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1917 (fixes the code example given with the hashes) Fixes the order dependent execution of assignment, so the following code works correctly: ```javascript function f(x) { console.log(x) } // used to check the order of execution let a = [[]] (f(1), a)[(f(2), 0)][(f(3), 0)] = (f(4), 123) // 🤮 ``` Prints out: ```bash 1 2 3 4 123 ``` As expected This introduces some opcodes: - ~~`Swap3`: currently used only to keep some previous code working that needs refactoring.~~ - ~~`RotateRight n`: Rotates the `n` top values from the top of the stack to the right by `1`.~~ Already added by #2390 ~~Besides the new opcodes,~~ Some opcodes pop and push order of values on the stack have been changed. To eliminate many swaps and to make this change easier. ~~This PR is still a WIP and needs more refactoring~~ This is now ready for review/merge :) --- boa_engine/src/bytecompiler/mod.rs | 238 +++++++++++------- .../src/vm/opcode/define/class/getter.rs | 2 +- .../src/vm/opcode/define/class/method.rs | 2 +- .../src/vm/opcode/define/class/setter.rs | 2 +- .../src/vm/opcode/define/own_property.rs | 2 +- boa_engine/src/vm/opcode/delete/mod.rs | 2 +- boa_engine/src/vm/opcode/get/property.rs | 4 +- boa_engine/src/vm/opcode/mod.rs | 24 +- boa_engine/src/vm/opcode/set/private.rs | 6 +- boa_engine/src/vm/opcode/set/property.rs | 16 +- .../src/vm/opcode/unary_ops/decrement.rs | 6 +- .../src/vm/opcode/unary_ops/increment.rs | 6 +- boa_engine/src/vm/tests.rs | 37 ++- 13 files changed, 225 insertions(+), 122 deletions(-) diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index 28ad4eff429..d865e977599 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -678,8 +678,8 @@ impl<'b> ByteCompiler<'b> { self.emit(Opcode::GetPropertyByName, &[index]); } PropertyAccessField::Expr(expr) => { - self.compile_expr(expr, true)?; self.compile_expr(access.target(), true)?; + self.compile_expr(expr, true)?; self.emit(Opcode::GetPropertyByValue, &[]); } }, @@ -695,8 +695,8 @@ impl<'b> ByteCompiler<'b> { self.emit(Opcode::GetPropertyByName, &[index]); } PropertyAccessField::Expr(expr) => { - self.compile_expr(&**expr, true)?; self.emit_opcode(Opcode::Super); + self.compile_expr(&**expr, true)?; self.emit_opcode(Opcode::GetPropertyByValue); } }, @@ -712,62 +712,95 @@ impl<'b> ByteCompiler<'b> { Ok(()) } - #[inline] - fn access_set( - &mut self, - access: Access<'_>, - expr: Option<&Expression>, - use_expr: bool, - ) -> JsResult<()> { - if let Some(expr) = expr { - self.compile_expr(expr, true)?; - } - - if use_expr { - self.emit(Opcode::Dup, &[]); + // The wrap is needed so it can match the function signature. + #[allow(clippy::unnecessary_wraps)] + fn access_set_top_of_stack_expr_fn(compiler: &mut ByteCompiler<'_>, level: u8) -> JsResult<()> { + match level { + 0 => {} + 1 => compiler.emit_opcode(Opcode::Swap), + _ => { + compiler.emit_opcode(Opcode::RotateLeft); + compiler.emit_u8(level + 1); + } } + Ok(()) + } + #[inline] + fn access_set(&mut self, access: Access<'_>, use_expr: bool, expr_fn: F) -> JsResult + where + F: FnOnce(&mut ByteCompiler<'_>, u8) -> JsResult, + { match access { Access::Variable { name } => { + let result = expr_fn(self, 0); + if use_expr { + self.emit(Opcode::Dup, &[]); + } let binding = self.context.set_mutable_binding(name); let index = self.get_or_insert_binding(binding); self.emit(Opcode::SetName, &[index]); + result } Access::Property { access } => match access { PropertyAccess::Simple(access) => match access.field() { PropertyAccessField::Const(name) => { self.compile_expr(access.target(), true)?; + let result = expr_fn(self, 1); let index = self.get_or_insert_name((*name).into()); + self.emit(Opcode::SetPropertyByName, &[index]); + if !use_expr { + self.emit(Opcode::Pop, &[]); + } + result } PropertyAccessField::Expr(expr) => { - self.compile_expr(expr, true)?; self.compile_expr(access.target(), true)?; + self.compile_expr(expr, true)?; + let result = expr_fn(self, 2); self.emit(Opcode::SetPropertyByValue, &[]); + if !use_expr { + self.emit(Opcode::Pop, &[]); + } + result } }, PropertyAccess::Private(access) => { self.compile_expr(access.target(), true)?; - self.emit_opcode(Opcode::Swap); + let result = expr_fn(self, 1); let index = self.get_or_insert_name(access.field().into()); self.emit(Opcode::AssignPrivateField, &[index]); + if !use_expr { + self.emit(Opcode::Pop, &[]); + } + result } PropertyAccess::Super(access) => match access.field() { PropertyAccessField::Const(name) => { self.emit(Opcode::Super, &[]); + let result = expr_fn(self, 1); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::SetPropertyByName, &[index]); + if !use_expr { + self.emit(Opcode::Pop, &[]); + } + result } PropertyAccessField::Expr(expr) => { - self.compile_expr(expr, true)?; self.emit(Opcode::Super, &[]); + self.compile_expr(expr, true)?; + let result = expr_fn(self, 0); self.emit(Opcode::SetPropertyByValue, &[]); + if !use_expr { + self.emit(Opcode::Pop, &[]); + } + result } }, }, Access::This => todo!("access_set `this`"), } - Ok(()) } #[inline] @@ -781,8 +814,8 @@ impl<'b> ByteCompiler<'b> { self.emit(Opcode::DeletePropertyByName, &[index]); } PropertyAccessField::Expr(expr) => { - self.compile_expr(expr, true)?; self.compile_expr(access.target(), true)?; + self.compile_expr(expr, true)?; self.emit(Opcode::DeletePropertyByValue, &[]); } }, @@ -869,47 +902,61 @@ impl<'b> ByteCompiler<'b> { Expression::Unary(unary) => { let opcode = match unary.op() { UnaryOp::IncrementPre => { - self.compile_expr(unary.target(), true)?; - self.emit(Opcode::Inc, &[]); - + // TODO: promote to an early error. let access = Access::from_expression(unary.target()).ok_or_else(|| { JsNativeError::syntax().with_message("Invalid increment operand") })?; - self.access_set(access, None, true)?; + + self.access_set(access, true, |compiler, _| { + compiler.compile_expr(unary.target(), true)?; + compiler.emit(Opcode::Inc, &[]); + Ok(()) + })?; + None } UnaryOp::DecrementPre => { - self.compile_expr(unary.target(), true)?; - self.emit(Opcode::Dec, &[]); - // TODO: promote to an early error. let access = Access::from_expression(unary.target()).ok_or_else(|| { JsNativeError::syntax().with_message("Invalid decrement operand") })?; - self.access_set(access, None, true)?; + + self.access_set(access, true, |compiler, _| { + compiler.compile_expr(unary.target(), true)?; + compiler.emit(Opcode::Dec, &[]); + Ok(()) + })?; None } UnaryOp::IncrementPost => { - self.compile_expr(unary.target(), true)?; - self.emit(Opcode::IncPost, &[]); - // TODO: promote to an early error. let access = Access::from_expression(unary.target()).ok_or_else(|| { JsNativeError::syntax().with_message("Invalid increment operand") })?; - self.access_set(access, None, false)?; + + self.access_set(access, false, |compiler, level| { + compiler.compile_expr(unary.target(), true)?; + compiler.emit(Opcode::IncPost, &[]); + compiler.emit_opcode(Opcode::RotateRight); + compiler.emit_u8(level + 2); + Ok(()) + })?; None } UnaryOp::DecrementPost => { - self.compile_expr(unary.target(), true)?; - self.emit(Opcode::DecPost, &[]); - // TODO: promote to an early error. let access = Access::from_expression(unary.target()).ok_or_else(|| { JsNativeError::syntax().with_message("Invalid decrement operand") })?; - self.access_set(access, None, false)?; + + self.access_set(access, false, |compiler, level| { + compiler.compile_expr(unary.target(), true)?; + compiler.emit(Opcode::DecPost, &[]); + compiler.emit_opcode(Opcode::RotateRight); + compiler.emit_u8(level + 2); + Ok(()) + })?; None } @@ -1038,7 +1085,10 @@ impl<'b> ByteCompiler<'b> { } Expression::Assign(assign) if assign.op() == AssignOp::Assign => { match Access::from_assign_target(assign.lhs()) { - Ok(access) => self.access_set(access, Some(assign.rhs()), use_expr)?, + Ok(access) => self.access_set(access, use_expr, |compiler, _| { + compiler.compile_expr(assign.rhs(), true)?; + Ok(()) + })?, Err(pattern) => { self.compile_expr(assign.rhs(), true)?; if use_expr { @@ -1051,7 +1101,30 @@ impl<'b> ByteCompiler<'b> { Expression::Assign(assign) => { let access = Access::from_assign_target(assign.lhs()) .expect("patterns should throw early errors on complex assignment operators"); - self.access_get(access, true)?; + + let shortcircuit_operator_compilation = + |compiler: &mut ByteCompiler<'_>, opcode: Opcode| -> JsResult<()> { + let (early_exit, pop_count) = + compiler.access_set(access, use_expr, |compiler, level| { + compiler.access_get(access, true)?; + let early_exit = compiler.emit_opcode_with_operand(opcode); + compiler.compile_expr(assign.rhs(), true)?; + Ok((early_exit, level)) + })?; + if pop_count == 0 { + compiler.patch_jump(early_exit); + } else { + let exit = compiler.emit_opcode_with_operand(Opcode::Jump); + compiler.patch_jump(early_exit); + for _ in 0..pop_count { + compiler.emit_opcode(Opcode::Swap); + compiler.emit_opcode(Opcode::Pop); + } + compiler.patch_jump(exit); + } + Ok(()) + }; + let opcode = match assign.op() { AssignOp::Assign => unreachable!(), AssignOp::Add => Opcode::Add, @@ -1067,31 +1140,25 @@ impl<'b> ByteCompiler<'b> { AssignOp::Shr => Opcode::ShiftRight, AssignOp::Ushr => Opcode::UnsignedShiftRight, AssignOp::BoolAnd => { - let exit = self.emit_opcode_with_operand(Opcode::LogicalAnd); - self.compile_expr(assign.rhs(), true)?; - self.access_set(access, None, use_expr)?; - self.patch_jump(exit); + shortcircuit_operator_compilation(self, Opcode::LogicalAnd)?; return Ok(()); } AssignOp::BoolOr => { - let exit = self.emit_opcode_with_operand(Opcode::LogicalOr); - self.compile_expr(assign.rhs(), true)?; - self.access_set(access, None, use_expr)?; - self.patch_jump(exit); + shortcircuit_operator_compilation(self, Opcode::LogicalOr)?; return Ok(()); } AssignOp::Coalesce => { - let exit = self.emit_opcode_with_operand(Opcode::Coalesce); - self.compile_expr(assign.rhs(), true)?; - self.access_set(access, None, use_expr)?; - self.patch_jump(exit); + shortcircuit_operator_compilation(self, Opcode::Coalesce)?; return Ok(()); } }; - self.compile_expr(assign.rhs(), true)?; - self.emit(opcode, &[]); - self.access_set(access, None, use_expr)?; + self.access_set(access, use_expr, |compiler, _| { + compiler.access_get(access, true)?; + compiler.compile_expr(assign.rhs(), true)?; + compiler.emit(opcode, &[]); + Ok(()) + })?; } Expression::ObjectLiteral(object) => { self.emit_opcode(Opcode::PushEmptyObject); @@ -1105,7 +1172,6 @@ impl<'b> ByteCompiler<'b> { PropertyDefinition::Property(name, expr) => match name { PropertyName::Literal(name) => { self.compile_expr(expr, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -1121,7 +1187,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Get(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::SetPropertyGetterByName, &[index]); } @@ -1136,7 +1201,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Set(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::SetPropertySetterByName, &[index]); } @@ -1150,7 +1214,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Ordinary(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -1164,7 +1227,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Async(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -1178,7 +1240,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Generator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -1192,7 +1253,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::AsyncGenerator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -1365,7 +1425,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(field) => { self.compile_expr(field, true)?; - self.emit(Opcode::Swap, &[]); self.emit(Opcode::GetPropertyByValue, &[]); } } @@ -1404,9 +1463,9 @@ impl<'b> ByteCompiler<'b> { self.emit_opcode(Opcode::PushValueToArray); } - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name(Sym::RAW.into()); self.emit(Opcode::SetPropertyByName, &[index]); + self.emit(Opcode::Pop, &[]); for expr in template.exprs() { self.compile_expr(expr, true)?; @@ -1491,7 +1550,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(field) => { self.compile_expr(field, true)?; - self.emit_opcode(Opcode::Swap); self.emit_opcode(Opcode::GetPropertyByValue); } } @@ -1512,7 +1570,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(expr) => { self.compile_expr(expr, true)?; - self.emit_opcode(Opcode::Swap); self.emit_opcode(Opcode::GetPropertyByValue); } } @@ -1602,7 +1659,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(expr) => { self.compile_expr(expr, true)?; - self.emit(Opcode::Swap, &[]); self.emit(Opcode::GetPropertyByValue, &[]); } } @@ -1858,7 +1914,11 @@ impl<'b> ByteCompiler<'b> { self.emit(Opcode::DefInitVar, &[index]); } IterableLoopInitializer::Access(access) => { - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; } IterableLoopInitializer::Var(declaration) => match declaration { Binding::Identifier(ident) => { @@ -1979,7 +2039,11 @@ impl<'b> ByteCompiler<'b> { self.emit(Opcode::DefInitVar, &[index]); } IterableLoopInitializer::Access(access) => { - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; } IterableLoopInitializer::Var(declaration) => match declaration { Binding::Identifier(ident) => { @@ -2649,7 +2713,6 @@ impl<'b> ByteCompiler<'b> { } PropertyName::Computed(node) => { self.compile_expr(node, true)?; - self.emit_opcode(Opcode::Swap); if rest_exits { self.emit_opcode(Opcode::GetPropertyByValuePush); } else { @@ -2702,7 +2765,11 @@ impl<'b> ByteCompiler<'b> { )); } self.emit(Opcode::CopyDataProperties, &[excluded_keys.len() as u32, 0]); - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; } AssignmentPropertyAccess { name, @@ -2717,7 +2784,6 @@ impl<'b> ByteCompiler<'b> { } PropertyName::Computed(node) => { self.compile_expr(node, true)?; - self.emit_opcode(Opcode::Swap); if rest_exits { self.emit_opcode(Opcode::GetPropertyByValuePush); } else { @@ -2733,7 +2799,11 @@ impl<'b> ByteCompiler<'b> { self.patch_jump(skip); } - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; if rest_exits && name.computed().is_some() { self.emit_opcode(Opcode::Swap); @@ -2753,7 +2823,6 @@ impl<'b> ByteCompiler<'b> { } PropertyName::Computed(node) => { self.compile_expr(node, true)?; - self.emit_opcode(Opcode::Swap); self.emit_opcode(Opcode::GetPropertyByValue); } } @@ -2806,7 +2875,11 @@ impl<'b> ByteCompiler<'b> { } PropertyAccess { access } => { self.emit_opcode(Opcode::IteratorNext); - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; } // BindingElement : BindingPattern Initializer[opt] Pattern { @@ -2831,7 +2904,11 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessRest { access } => { self.emit_opcode(Opcode::IteratorToArray); - self.access_set(Access::Property { access }, None, false)?; + self.access_set( + Access::Property { access }, + false, + Self::access_set_top_of_stack_expr_fn, + )?; } // BindingRestElement : ... BindingPattern PatternRest { pattern } => { @@ -3128,7 +3205,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Get(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassGetterByName, &[index]); } @@ -3142,7 +3218,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Set(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassSetterByName, &[index]); } @@ -3156,7 +3231,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Ordinary(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3170,7 +3244,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Async(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3184,7 +3257,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Generator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3198,7 +3270,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::AsyncGenerator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3333,7 +3404,6 @@ impl<'b> ByteCompiler<'b> { } else { self.emit_opcode(Opcode::PushUndefined); } - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineOwnPropertyByName, &[index]); } @@ -3432,7 +3502,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Get(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassGetterByName, &[index]); } @@ -3446,7 +3515,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Set(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassSetterByName, &[index]); } @@ -3460,7 +3528,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Ordinary(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3474,7 +3541,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Async(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3488,7 +3554,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::Generator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } @@ -3502,7 +3567,6 @@ impl<'b> ByteCompiler<'b> { MethodDefinition::AsyncGenerator(expr) => match name { PropertyName::Literal(name) => { self.function(expr.into(), NodeKind::Expression, true)?; - self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name((*name).into()); self.emit(Opcode::DefineClassMethodByName, &[index]); } diff --git a/boa_engine/src/vm/opcode/define/class/getter.rs b/boa_engine/src/vm/opcode/define/class/getter.rs index 52f75b87ed4..1b7a8c0b249 100644 --- a/boa_engine/src/vm/opcode/define/class/getter.rs +++ b/boa_engine/src/vm/opcode/define/class/getter.rs @@ -17,8 +17,8 @@ impl Operation for DefineClassGetterByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = object.to_object(context)?; value .as_object() diff --git a/boa_engine/src/vm/opcode/define/class/method.rs b/boa_engine/src/vm/opcode/define/class/method.rs index e9d2886240c..f6a8c4fda01 100644 --- a/boa_engine/src/vm/opcode/define/class/method.rs +++ b/boa_engine/src/vm/opcode/define/class/method.rs @@ -17,8 +17,8 @@ impl Operation for DefineClassMethodByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { diff --git a/boa_engine/src/vm/opcode/define/class/setter.rs b/boa_engine/src/vm/opcode/define/class/setter.rs index bd76ba87c06..43873a3c133 100644 --- a/boa_engine/src/vm/opcode/define/class/setter.rs +++ b/boa_engine/src/vm/opcode/define/class/setter.rs @@ -17,8 +17,8 @@ impl Operation for DefineClassSetterByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = object.to_object(context)?; value .as_object() diff --git a/boa_engine/src/vm/opcode/define/own_property.rs b/boa_engine/src/vm/opcode/define/own_property.rs index c89c063b757..cea5ea8349f 100644 --- a/boa_engine/src/vm/opcode/define/own_property.rs +++ b/boa_engine/src/vm/opcode/define/own_property.rs @@ -17,8 +17,8 @@ impl Operation for DefineOwnPropertyByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { diff --git a/boa_engine/src/vm/opcode/delete/mod.rs b/boa_engine/src/vm/opcode/delete/mod.rs index 6b0d5f3d793..a6f6b1c11b6 100644 --- a/boa_engine/src/vm/opcode/delete/mod.rs +++ b/boa_engine/src/vm/opcode/delete/mod.rs @@ -47,8 +47,8 @@ impl Operation for DeletePropertyByValue { const INSTRUCTION: &'static str = "INST - DeletePropertyByValue"; fn execute(context: &mut Context) -> JsResult { - let object = context.vm.pop(); let key = context.vm.pop(); + let object = context.vm.pop(); let result = object .to_object(context)? .__delete__(&key.to_property_key(context)?, context)?; diff --git a/boa_engine/src/vm/opcode/get/property.rs b/boa_engine/src/vm/opcode/get/property.rs index 87c01c4a72d..f467f4597ec 100644 --- a/boa_engine/src/vm/opcode/get/property.rs +++ b/boa_engine/src/vm/opcode/get/property.rs @@ -50,8 +50,8 @@ impl Operation for GetPropertyByValue { const INSTRUCTION: &'static str = "INST - GetPropertyByValue"; fn execute(context: &mut Context) -> JsResult { - let object = context.vm.pop(); let key = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { @@ -78,8 +78,8 @@ impl Operation for GetPropertyByValuePush { const INSTRUCTION: &'static str = "INST - GetPropertyByValuePush"; fn execute(context: &mut Context) -> JsResult { - let object = context.vm.pop(); let key = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index ae10eb5f18b..861e15ea836 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -702,7 +702,7 @@ generate_impl! { /// /// Operands: /// - /// Stack: key, object **=>** value + /// Stack: object, key **=>** value GetPropertyByValue, /// Get a property by value from an object an push the key and value on the stack. @@ -711,7 +711,7 @@ generate_impl! { /// /// Operands: /// - /// Stack: key, object **=>** key, value + /// Stack: object, key **=>** key, value GetPropertyByValuePush, /// Sets a property by name of an object. @@ -720,21 +720,21 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** value SetPropertyByName, /// Defines a own property of an object by name. /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** DefineOwnPropertyByName, /// Defines a class method by name. /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** DefineClassMethodByName, /// Sets a property by value of an object. @@ -743,7 +743,7 @@ generate_impl! { /// /// Operands: /// - /// Stack: value, key, object **=>** + /// Stack: object, key, value **=>** value SetPropertyByValue, /// Defines a own property of an object by value. @@ -766,7 +766,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** SetPropertyGetterByName, /// Defines a getter class method by name. @@ -775,7 +775,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** DefineClassGetterByName, /// Sets a getter property by value of an object. @@ -802,7 +802,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** SetPropertySetterByName, /// Defines a setter class method by name. @@ -811,7 +811,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** DefineClassSetterByName, /// Sets a setter property by value of an object. @@ -838,7 +838,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: object, value **=>** + /// Stack: object, value **=>** value AssignPrivateField, /// Set a private property of a class constructor by it's name. @@ -936,7 +936,7 @@ generate_impl! { /// /// Operands: /// - /// Stack: key, object **=>** + /// Stack: object, key **=>** DeletePropertyByValue, /// Copy all properties of one object to another object. diff --git a/boa_engine/src/vm/opcode/set/private.rs b/boa_engine/src/vm/opcode/set/private.rs index 5c46de0e4f9..2f5a3a978e5 100644 --- a/boa_engine/src/vm/opcode/set/private.rs +++ b/boa_engine/src/vm/opcode/set/private.rs @@ -25,7 +25,8 @@ impl Operation for AssignPrivateField { let mut object_borrow_mut = object.borrow_mut(); match object_borrow_mut.get_private_element(name.sym()) { Some(PrivateElement::Field(_)) => { - object_borrow_mut.set_private_element(name.sym(), PrivateElement::Field(value)); + object_borrow_mut + .set_private_element(name.sym(), PrivateElement::Field(value.clone())); } Some(PrivateElement::Method(_)) => { return Err(JsNativeError::typ() @@ -38,7 +39,7 @@ impl Operation for AssignPrivateField { }) => { let setter = setter.clone(); drop(object_borrow_mut); - setter.call(&object.clone().into(), &[value], context)?; + setter.call(&object.clone().into(), &[value.clone()], context)?; } None => { return Err(JsNativeError::typ() @@ -56,6 +57,7 @@ impl Operation for AssignPrivateField { .with_message("cannot set private property on non-object") .into()); } + context.vm.push(value); Ok(ShouldExit::False) } } diff --git a/boa_engine/src/vm/opcode/set/property.rs b/boa_engine/src/vm/opcode/set/property.rs index f1f88551843..e76685eabea 100644 --- a/boa_engine/src/vm/opcode/set/property.rs +++ b/boa_engine/src/vm/opcode/set/property.rs @@ -18,8 +18,8 @@ impl Operation for SetPropertyByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { @@ -33,7 +33,8 @@ impl Operation for SetPropertyByName { .into_common::(false) .into(); - object.set(name, value, context.vm.frame().code.strict, context)?; + object.set(name, value.clone(), context.vm.frame().code.strict, context)?; + context.vm.stack.push(value); Ok(ShouldExit::False) } } @@ -50,9 +51,9 @@ impl Operation for SetPropertyByValue { const INSTRUCTION: &'static str = "INST - SetPropertyByValue"; fn execute(context: &mut Context) -> JsResult { - let object = context.vm.pop(); - let key = context.vm.pop(); let value = context.vm.pop(); + let key = context.vm.pop(); + let object = context.vm.pop(); let object = if let Some(object) = object.as_object() { object.clone() } else { @@ -60,7 +61,8 @@ impl Operation for SetPropertyByValue { }; let key = key.to_property_key(context)?; - object.set(key, value, context.vm.frame().code.strict, context)?; + object.set(key, value.clone(), context.vm.frame().code.strict, context)?; + context.vm.stack.push(value); Ok(ShouldExit::False) } } @@ -78,8 +80,8 @@ impl Operation for SetPropertyGetterByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = object.to_object(context)?; let name = context.vm.frame().code.names[index as usize]; let name = context @@ -155,8 +157,8 @@ impl Operation for SetPropertySetterByName { fn execute(context: &mut Context) -> JsResult { let index = context.vm.read::(); - let object = context.vm.pop(); let value = context.vm.pop(); + let object = context.vm.pop(); let object = object.to_object(context)?; let name = context.vm.frame().code.names[index as usize]; let name = context diff --git a/boa_engine/src/vm/opcode/unary_ops/decrement.rs b/boa_engine/src/vm/opcode/unary_ops/decrement.rs index a09396eb09d..82fd5c5ba8a 100644 --- a/boa_engine/src/vm/opcode/unary_ops/decrement.rs +++ b/boa_engine/src/vm/opcode/unary_ops/decrement.rs @@ -41,13 +41,13 @@ impl Operation for DecPost { fn execute(context: &mut Context) -> JsResult { let value = context.vm.pop(); let value = value.to_numeric(context)?; - context.vm.push(value.clone()); match value { Numeric::Number(number) => context.vm.push(number - 1f64), - Numeric::BigInt(bigint) => { - context.vm.push(JsBigInt::sub(&bigint, &JsBigInt::one())); + Numeric::BigInt(ref bigint) => { + context.vm.push(JsBigInt::sub(bigint, &JsBigInt::one())); } } + context.vm.push(value); Ok(ShouldExit::False) } } diff --git a/boa_engine/src/vm/opcode/unary_ops/increment.rs b/boa_engine/src/vm/opcode/unary_ops/increment.rs index f9771206915..0fedbf099b5 100644 --- a/boa_engine/src/vm/opcode/unary_ops/increment.rs +++ b/boa_engine/src/vm/opcode/unary_ops/increment.rs @@ -41,13 +41,13 @@ impl Operation for IncPost { fn execute(context: &mut Context) -> JsResult { let value = context.vm.pop(); let value = value.to_numeric(context)?; - context.vm.push(value.clone()); match value { Numeric::Number(number) => context.vm.push(number + 1f64), - Numeric::BigInt(bigint) => { - context.vm.push(JsBigInt::add(&bigint, &JsBigInt::one())); + Numeric::BigInt(ref bigint) => { + context.vm.push(JsBigInt::add(bigint, &JsBigInt::one())); } } + context.vm.push(value); Ok(ShouldExit::False) } } diff --git a/boa_engine/src/vm/tests.rs b/boa_engine/src/vm/tests.rs index cb5738afcfb..106c17001cd 100644 --- a/boa_engine/src/vm/tests.rs +++ b/boa_engine/src/vm/tests.rs @@ -1,4 +1,4 @@ -use crate::{exec, Context, JsValue}; +use crate::{check_output, exec, Context, JsValue, TestAction}; #[test] fn typeof_string() { @@ -189,3 +189,38 @@ fn get_reference_by_super() { JsValue::from("ab") ); } + +#[test] +fn order_of_execution_in_assigment() { + let scenario = r#" + let i = 0; + let array = [[]]; + + array[i++][i++] = i++; + "#; + + check_output(&[ + TestAction::Execute(scenario), + TestAction::TestEq("i", "3"), + TestAction::TestEq("array.length", "1"), + TestAction::TestEq("array[0].length", "2"), + ]); +} + +#[test] +fn order_of_execution_in_assigment_with_comma_expressions() { + let scenario = r#" + let result = ""; + function f(i) { + result += i; + } + let a = [[]]; + + (f(1), a)[(f(2), 0)][(f(3), 0)] = (f(4), 123); + "#; + + check_output(&[ + TestAction::Execute(scenario), + TestAction::TestEq("result", "\"1234\""), + ]); +}