From d3ca0cf037aa04b0c8395e238780c2896f1b52a1 Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 03:40:18 +0100 Subject: [PATCH 1/5] Fix order of execution in assignment --- boa_engine/src/bytecompiler/mod.rs | 233 +++++++++++------- boa_engine/src/vm/code_block.rs | 1 + .../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 | 29 ++- boa_engine/src/vm/opcode/set/property.rs | 16 +- boa_engine/src/vm/opcode/swap/mod.rs | 18 ++ .../src/vm/opcode/unary_ops/decrement.rs | 6 +- .../src/vm/opcode/unary_ops/increment.rs | 6 +- 13 files changed, 206 insertions(+), 117 deletions(-) diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index 28ad4eff429..f5979410ccb 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,92 @@ 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<()> { + if level != 0 { + 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) => { + let result = expr_fn(self, 0); + if use_expr { + self.emit(Opcode::Dup, &[]); + } self.compile_expr(access.target(), true)?; self.emit_opcode(Opcode::Swap); let index = self.get_or_insert_name(access.field().into()); self.emit(Opcode::AssignPrivateField, &[index]); + 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 +811,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 +899,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 +1082,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 +1098,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 +1137,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 +1169,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 +1184,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 +1198,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 +1211,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 +1224,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 +1237,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 +1250,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 +1422,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(field) => { self.compile_expr(field, true)?; - self.emit(Opcode::Swap, &[]); self.emit(Opcode::GetPropertyByValue, &[]); } } @@ -1404,9 +1460,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 +1547,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 +1567,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 +1656,6 @@ impl<'b> ByteCompiler<'b> { } PropertyAccessField::Expr(expr) => { self.compile_expr(expr, true)?; - self.emit(Opcode::Swap, &[]); self.emit(Opcode::GetPropertyByValue, &[]); } } @@ -1858,7 +1911,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 +2036,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 +2710,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 +2762,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 +2781,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 +2796,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 +2820,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 +2872,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 +2901,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 +3202,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 +3215,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 +3228,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 +3241,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 +3254,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 +3267,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 +3401,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 +3499,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 +3512,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 +3525,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 +3538,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 +3551,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 +3564,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/code_block.rs b/boa_engine/src/vm/code_block.rs index e434e41aef0..921ff2f22ee 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -289,6 +289,7 @@ impl CodeBlock { | Opcode::PopIfThrown | Opcode::Dup | Opcode::Swap + | Opcode::Swap3 | Opcode::PushZero | Opcode::PushOne | Opcode::PushNaN 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..b4456e42e35 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -195,6 +195,13 @@ generate_impl! { /// Stack: second, first **=>** first, second Swap, + /// Swap the top three values on the stack. + /// + /// Operands: + /// + /// Stack: third, second, first **=>** first, second, third + Swap3, + /// Rotates the top `n` values of the stack to the left by `1`. /// /// Equivalent to calling [`slice::rotate_left`] with argument `1` on the top `n` values of the @@ -702,7 +709,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 +718,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 +727,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 +750,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 +773,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** SetPropertyGetterByName, /// Defines a getter class method by name. @@ -775,7 +782,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 +809,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** SetPropertySetterByName, /// Defines a setter class method by name. @@ -811,7 +818,7 @@ generate_impl! { /// /// Operands: name_index: `u32` /// - /// Stack: value, object **=>** + /// Stack: object, value **=>** DefineClassSetterByName, /// Sets a setter property by value of an object. @@ -936,7 +943,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/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/swap/mod.rs b/boa_engine/src/vm/opcode/swap/mod.rs index 941812af607..1ecce3a8550 100644 --- a/boa_engine/src/vm/opcode/swap/mod.rs +++ b/boa_engine/src/vm/opcode/swap/mod.rs @@ -24,6 +24,24 @@ impl Operation for Swap { } } +/// `Swap3` implements the Opcode Operation for `Opcode::Swap3` +/// +/// Operation: +/// - Swap the top three values on the stack. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(crate) struct Swap3; + +impl Operation for Swap3 { + const NAME: &'static str = "Swap3"; + const INSTRUCTION: &'static str = "INST - Swap3"; + + fn execute(context: &mut Context) -> JsResult { + let len = context.vm.stack.len(); + context.vm.stack.swap(len - 1, len - 3); + Ok(ShouldExit::False) + } +} + /// `RotateLeft` implements the Opcode Operation for `Opcode::RotateLeft` /// /// Operation: 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) } } From efefaa4c9334ef55381f50184055901e152f6106 Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 13:11:04 +0100 Subject: [PATCH 2/5] Fix Assign Private Field order of execution --- boa_engine/src/bytecompiler/mod.rs | 9 ++++----- boa_engine/src/vm/opcode/mod.rs | 2 +- boa_engine/src/vm/opcode/set/private.rs | 6 ++++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index f5979410ccb..8b488dec605 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -763,14 +763,13 @@ impl<'b> ByteCompiler<'b> { } }, PropertyAccess::Private(access) => { - let result = expr_fn(self, 0); - if use_expr { - self.emit(Opcode::Dup, &[]); - } 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() { diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index b4456e42e35..9235d514eaa 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -845,7 +845,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. 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) } } From c15522165e9b9e52fe1dc59b8fcddce850bb5884 Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 13:19:43 +0100 Subject: [PATCH 3/5] Remove opcode Swap3 --- boa_engine/src/vm/code_block.rs | 1 - boa_engine/src/vm/opcode/mod.rs | 7 ------- boa_engine/src/vm/opcode/swap/mod.rs | 18 ------------------ 3 files changed, 26 deletions(-) diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 921ff2f22ee..e434e41aef0 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -289,7 +289,6 @@ impl CodeBlock { | Opcode::PopIfThrown | Opcode::Dup | Opcode::Swap - | Opcode::Swap3 | Opcode::PushZero | Opcode::PushOne | Opcode::PushNaN diff --git a/boa_engine/src/vm/opcode/mod.rs b/boa_engine/src/vm/opcode/mod.rs index 9235d514eaa..861e15ea836 100644 --- a/boa_engine/src/vm/opcode/mod.rs +++ b/boa_engine/src/vm/opcode/mod.rs @@ -195,13 +195,6 @@ generate_impl! { /// Stack: second, first **=>** first, second Swap, - /// Swap the top three values on the stack. - /// - /// Operands: - /// - /// Stack: third, second, first **=>** first, second, third - Swap3, - /// Rotates the top `n` values of the stack to the left by `1`. /// /// Equivalent to calling [`slice::rotate_left`] with argument `1` on the top `n` values of the diff --git a/boa_engine/src/vm/opcode/swap/mod.rs b/boa_engine/src/vm/opcode/swap/mod.rs index 1ecce3a8550..941812af607 100644 --- a/boa_engine/src/vm/opcode/swap/mod.rs +++ b/boa_engine/src/vm/opcode/swap/mod.rs @@ -24,24 +24,6 @@ impl Operation for Swap { } } -/// `Swap3` implements the Opcode Operation for `Opcode::Swap3` -/// -/// Operation: -/// - Swap the top three values on the stack. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub(crate) struct Swap3; - -impl Operation for Swap3 { - const NAME: &'static str = "Swap3"; - const INSTRUCTION: &'static str = "INST - Swap3"; - - fn execute(context: &mut Context) -> JsResult { - let len = context.vm.stack.len(); - context.vm.stack.swap(len - 1, len - 3); - Ok(ShouldExit::False) - } -} - /// `RotateLeft` implements the Opcode Operation for `Opcode::RotateLeft` /// /// Operation: From 2e321932942a9b66d11d17f78c85f10bf29ba46d Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 14:10:28 +0100 Subject: [PATCH 4/5] Add tests --- boa_engine/src/vm/tests.rs | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) 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\""), + ]); +} From 2e6f5434f3e73d002a5462d8e9c3324ed2336afd Mon Sep 17 00:00:00 2001 From: Halid Odat Date: Sun, 30 Oct 2022 14:18:03 +0100 Subject: [PATCH 5/5] Use Swap opcode if possible instead of RotateLeft --- boa_engine/src/bytecompiler/mod.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/boa_engine/src/bytecompiler/mod.rs b/boa_engine/src/bytecompiler/mod.rs index 8b488dec605..d865e977599 100644 --- a/boa_engine/src/bytecompiler/mod.rs +++ b/boa_engine/src/bytecompiler/mod.rs @@ -715,9 +715,13 @@ impl<'b> ByteCompiler<'b> { // 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<()> { - if level != 0 { - compiler.emit_opcode(Opcode::RotateLeft); - compiler.emit_u8(level + 1); + match level { + 0 => {} + 1 => compiler.emit_opcode(Opcode::Swap), + _ => { + compiler.emit_opcode(Opcode::RotateLeft); + compiler.emit_u8(level + 1); + } } Ok(()) }