From 787d4a8bc518d5a76cd9fea1cf6f398481f17e8c Mon Sep 17 00:00:00 2001 From: raskad <32105367+raskad@users.noreply.github.com> Date: Wed, 25 Jan 2023 15:12:57 +0000 Subject: [PATCH] Fix double property access on assignment ops (#2551) Currently the compilation of assignment operators leads to a double object property access, both on the get and set access. While this refactor adds special access handling instead of using the existing `access_set` and `access_get` functions, it fixes the double access and should also make the resulting code more efficient. --- .../src/bytecompiler/expression/assign.rs | 203 ++++++++++++++---- 1 file changed, 160 insertions(+), 43 deletions(-) diff --git a/boa_engine/src/bytecompiler/expression/assign.rs b/boa_engine/src/bytecompiler/expression/assign.rs index 3565936bd5..7b4327ca14 100644 --- a/boa_engine/src/bytecompiler/expression/assign.rs +++ b/boa_engine/src/bytecompiler/expression/assign.rs @@ -1,10 +1,12 @@ -use boa_ast::expression::operator::{assign::AssignOp, Assign}; - use crate::{ bytecompiler::{Access, ByteCompiler}, vm::{BindingOpcode, Opcode}, JsResult, }; +use boa_ast::expression::{ + access::{PropertyAccess, PropertyAccessField}, + operator::{assign::AssignOp, Assign}, +}; impl ByteCompiler<'_, '_> { pub(crate) fn compile_assign(&mut self, assign: &Assign, use_expr: bool) -> JsResult<()> { @@ -26,29 +28,6 @@ impl ByteCompiler<'_, '_> { let access = Access::from_assign_target(assign.lhs()) .expect("patterns should throw early errors on complex assignment operators"); - 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, @@ -63,26 +42,164 @@ impl ByteCompiler<'_, '_> { AssignOp::Shl => Opcode::ShiftLeft, AssignOp::Shr => Opcode::ShiftRight, AssignOp::Ushr => Opcode::UnsignedShiftRight, - AssignOp::BoolAnd => { - shortcircuit_operator_compilation(self, Opcode::LogicalAnd)?; - return Ok(()); - } - AssignOp::BoolOr => { - shortcircuit_operator_compilation(self, Opcode::LogicalOr)?; - return Ok(()); - } - AssignOp::Coalesce => { - shortcircuit_operator_compilation(self, Opcode::Coalesce)?; - return Ok(()); - } + AssignOp::BoolAnd => Opcode::LogicalAnd, + AssignOp::BoolOr => Opcode::LogicalOr, + AssignOp::Coalesce => Opcode::Coalesce, }; - self.access_set(access, use_expr, |compiler, _| { - compiler.access_get(access, true)?; - compiler.compile_expr(assign.rhs(), true)?; - compiler.emit(opcode, &[]); - Ok(()) - })?; + let short_circuit = matches!( + assign.op(), + AssignOp::BoolAnd | AssignOp::BoolOr | AssignOp::Coalesce + ); + let mut pop_count = 0; + let mut early_exit = None; + + match access { + Access::Variable { name } => { + let binding = self.context.get_binding_value(name); + let index = self.get_or_insert_binding(binding); + self.emit(Opcode::GetName, &[index]); + + if short_circuit { + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + if use_expr { + self.emit_opcode(Opcode::Dup); + } + + let binding = self.context.set_mutable_binding(name); + let index = self.get_or_insert_binding(binding); + self.emit(Opcode::SetName, &[index]); + } + Access::Property { access } => match access { + PropertyAccess::Simple(access) => match access.field() { + PropertyAccessField::Const(name) => { + let index = self.get_or_insert_name((*name).into()); + self.compile_expr(access.target(), true)?; + self.emit_opcode(Opcode::Dup); + self.emit_opcode(Opcode::Dup); + + self.emit(Opcode::GetPropertyByName, &[index]); + if short_circuit { + pop_count = 2; + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + + self.emit(Opcode::SetPropertyByName, &[index]); + if !use_expr { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccessField::Expr(expr) => { + self.compile_expr(access.target(), true)?; + self.emit_opcode(Opcode::Dup); + self.compile_expr(expr, true)?; + + self.emit_opcode(Opcode::GetPropertyByValuePush); + if short_circuit { + pop_count = 2; + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + + self.emit_opcode(Opcode::SetPropertyByValue); + if !use_expr { + self.emit_opcode(Opcode::Pop); + } + } + }, + PropertyAccess::Private(access) => { + let index = self.get_or_insert_private_name(access.field()); + self.compile_expr(access.target(), true)?; + self.emit_opcode(Opcode::Dup); + + self.emit(Opcode::GetPrivateField, &[index]); + if short_circuit { + pop_count = 1; + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + + self.emit(Opcode::SetPrivateField, &[index]); + if !use_expr { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccess::Super(access) => match access.field() { + PropertyAccessField::Const(name) => { + let index = self.get_or_insert_name((*name).into()); + self.emit_opcode(Opcode::Super); + self.emit_opcode(Opcode::Dup); + self.emit_opcode(Opcode::This); + self.emit_opcode(Opcode::Swap); + + self.emit(Opcode::GetPropertyByName, &[index]); + if short_circuit { + pop_count = 2; + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + + self.emit(Opcode::SetPropertyByName, &[index]); + if !use_expr { + self.emit_opcode(Opcode::Pop); + } + } + PropertyAccessField::Expr(expr) => { + self.emit_opcode(Opcode::Super); + self.emit_opcode(Opcode::Dup); + self.compile_expr(expr, true)?; + + self.emit_opcode(Opcode::GetPropertyByValuePush); + if short_circuit { + pop_count = 2; + early_exit = Some(self.emit_opcode_with_operand(opcode)); + self.compile_expr(assign.rhs(), true)?; + } else { + self.compile_expr(assign.rhs(), true)?; + self.emit_opcode(opcode); + } + + self.emit(Opcode::SetPropertyByValue, &[]); + if !use_expr { + self.emit_opcode(Opcode::Pop); + } + } + }, + }, + Access::This => unreachable!(), + } + + if let Some(early_exit) = early_exit { + if pop_count == 0 { + self.patch_jump(early_exit); + } else { + let exit = self.emit_opcode_with_operand(Opcode::Jump); + self.patch_jump(early_exit); + for _ in 0..pop_count { + self.emit_opcode(Opcode::Swap); + self.emit_opcode(Opcode::Pop); + } + self.patch_jump(exit); + } + } } Ok(())