diff --git a/core/ast/src/scope_analyzer.rs b/core/ast/src/scope_analyzer.rs index c586e78f341..b9dd8a669dd 100644 --- a/core/ast/src/scope_analyzer.rs +++ b/core/ast/src/scope_analyzer.rs @@ -2284,10 +2284,10 @@ pub struct EvalDeclarationBindings { pub new_annex_b_function_names: Vec, /// New function names created during the declaration of an eval ast node. - pub new_function_names: FxHashMap, + pub new_function_names: FxHashMap, - /// New variable names created during the declaration of an eval ast node. - pub new_var_names: Vec, + /// Variable names declared during the declaration of an eval ast node. + pub declared_var_names: Vec, } /// `EvalDeclarationInstantiation ( body, varEnv, lexEnv, privateEnv, strict )` @@ -2532,10 +2532,7 @@ pub(crate) fn eval_declaration_instantiation_scope( let binding = var_env.set_mutable_binding(n).expect("must not fail"); result.new_function_names.insert( name, - ( - IdentifierReference::new(binding.locator(), !var_env.is_function(), true), - true, - ), + IdentifierReference::new(binding.locator(), !var_env.is_function(), true), ); } else { // 1. NOTE: The following invocation cannot return an abrupt completion because of the validation preceding step 14. @@ -2544,10 +2541,7 @@ pub(crate) fn eval_declaration_instantiation_scope( let binding = var_env.create_mutable_binding(n, !strict); result.new_function_names.insert( name, - ( - IdentifierReference::new(binding, !var_env.is_function(), true), - false, - ), + IdentifierReference::new(binding, !var_env.is_function(), true), ); } } @@ -2568,13 +2562,17 @@ pub(crate) fn eval_declaration_instantiation_scope( // 1. NOTE: The following invocation cannot return an abrupt completion because of the validation preceding step 14. // 2. Perform ! varEnv.CreateMutableBinding(vn, true). // 3. Perform ! varEnv.InitializeBinding(vn, undefined). - let binding = var_env.create_mutable_binding(name, true); - result.new_var_names.push(IdentifierReference::new( - binding, - !var_env.is_function(), - true, - )); + drop(var_env.create_mutable_binding(name.clone(), true)); } + + let binding = var_env + .get_binding_reference(&name) + .expect("binding must exist"); + result.declared_var_names.push(IdentifierReference::new( + binding.locator(), + !var_env.is_function(), + true, + )); } } diff --git a/core/engine/src/bytecompiler/declarations.rs b/core/engine/src/bytecompiler/declarations.rs index 8f27206ac0b..d44a2691fc3 100644 --- a/core/engine/src/bytecompiler/declarations.rs +++ b/core/engine/src/bytecompiler/declarations.rs @@ -838,24 +838,19 @@ impl ByteCompiler<'_> { self.emit_get_function(&dst, index); // i. Let bindingExists be ! varEnv.HasBinding(fn). - let (binding, binding_exists) = bindings + let binding = bindings .new_function_names .get(&name) .expect("binding must exist"); // ii. If bindingExists is false, then + // 1. NOTE: The following invocation cannot return an abrupt completion because of the validation preceding step 14. + // 2. Perform ! varEnv.CreateMutableBinding(fn, true). + // 3. Perform ! varEnv.InitializeBinding(fn, fo). // iii. Else, - if *binding_exists { - // 1. Perform ! varEnv.SetMutableBinding(fn, fo, false). - let index = self.insert_binding(binding.clone()); - self.emit_binding_access(BindingAccessOpcode::SetName, &index, &dst); - } else { - // 1. NOTE: The following invocation cannot return an abrupt completion because of the validation preceding step 14. - // 2. Perform ! varEnv.CreateMutableBinding(fn, true). - // 3. Perform ! varEnv.InitializeBinding(fn, fo). - let index = self.insert_binding(binding.clone()); - self.emit_binding_access(BindingAccessOpcode::DefInitVar, &index, &dst); - } + // 1. Perform ! varEnv.SetMutableBinding(fn, fo, false). + let index = self.insert_binding(binding.clone()); + self.emit_binding_access(BindingAccessOpcode::DefInitVar, &index, &dst); self.register_allocator.dealloc(dst); } } @@ -872,7 +867,7 @@ impl ByteCompiler<'_> { } } // 18.b - for binding in bindings.new_var_names { + for binding in bindings.declared_var_names { // i. Let bindingExists be ! varEnv.HasBinding(vn). // ii. If bindingExists is false, then // 1. NOTE: The following invocation cannot return an abrupt completion because of the validation preceding step 14. @@ -880,7 +875,7 @@ impl ByteCompiler<'_> { // 3. Perform ! varEnv.InitializeBinding(vn, undefined). let index = self.insert_binding(binding); self.emit_binding_access( - BindingAccessOpcode::DefInitVar, + BindingAccessOpcode::DefEvalVar, &index, &CallFrame::undefined_register(), ); diff --git a/core/engine/src/bytecompiler/mod.rs b/core/engine/src/bytecompiler/mod.rs index b06bd1e8921..b093f3c0008 100644 --- a/core/engine/src/bytecompiler/mod.rs +++ b/core/engine/src/bytecompiler/mod.rs @@ -449,6 +449,7 @@ pub(crate) enum BindingAccessOpcode { DeleteName, GetLocator, DefVar, + DefEvalVar, } /// Manages the source position scope, push on creation, pop on drop. @@ -918,6 +919,9 @@ impl<'ctx> ByteCompiler<'ctx> { } BindingAccessOpcode::GetLocator => self.bytecode.emit_get_locator((*index).into()), BindingAccessOpcode::DefVar => self.bytecode.emit_def_var((*index).into()), + BindingAccessOpcode::DefEvalVar => { + self.bytecode.emit_def_eval_var((*index).into()); + } BindingAccessOpcode::PutLexicalValue => self .bytecode .emit_put_lexical_value(value.variable(), (*index).into()), @@ -943,6 +947,9 @@ impl<'ctx> ByteCompiler<'ctx> { } BindingAccessOpcode::GetLocator => self.bytecode.emit_get_locator((*index).into()), BindingAccessOpcode::DefVar => self.bytecode.emit_def_var((*index).into()), + BindingAccessOpcode::DefEvalVar => { + self.bytecode.emit_def_eval_var((*index).into()); + } BindingAccessOpcode::PutLexicalValue => self .bytecode .emit_put_lexical_value(value.variable(), (*index).into()), @@ -978,7 +985,9 @@ impl<'ctx> ByteCompiler<'ctx> { | BindingAccessOpcode::GetNameAndLocator => { self.bytecode.emit_move(value.variable(), (*index).into()); } - BindingAccessOpcode::GetLocator | BindingAccessOpcode::DefVar => {} + BindingAccessOpcode::GetLocator + | BindingAccessOpcode::DefVar + | BindingAccessOpcode::DefEvalVar => {} BindingAccessOpcode::SetName | BindingAccessOpcode::DefInitVar | BindingAccessOpcode::PutLexicalValue diff --git a/core/engine/src/environments/runtime/declarative/function.rs b/core/engine/src/environments/runtime/declarative/function.rs index 9e198882740..aec836b0425 100644 --- a/core/engine/src/environments/runtime/declarative/function.rs +++ b/core/engine/src/environments/runtime/declarative/function.rs @@ -1,11 +1,16 @@ use boa_ast::scope::Scope; use boa_gc::{Finalize, GcRefCell, Trace, custom_trace}; +use std::cell::RefCell; use crate::{JsNativeError, JsObject, JsResult, JsValue, builtins::function::OrdinaryFunction}; #[derive(Debug, Trace, Finalize)] pub(crate) struct FunctionEnvironment { bindings: GcRefCell>>, + #[unsafe_ignore_trace] + deletable_bindings: RefCell>, + #[unsafe_ignore_trace] + deleted_bindings: RefCell>, slots: Box, // Safety: Nothing in `Scope` needs tracing. @@ -18,6 +23,8 @@ impl FunctionEnvironment { pub(crate) fn new(bindings_count: u32, slots: FunctionSlots, scope: Scope) -> Self { Self { bindings: GcRefCell::new(vec![None; bindings_count as usize]), + deletable_bindings: RefCell::new(vec![false; bindings_count as usize]), + deleted_bindings: RefCell::new(vec![false; bindings_count as usize]), slots: Box::new(slots), scope, } @@ -53,9 +60,69 @@ impl FunctionEnvironment { self.bindings.borrow_mut()[index as usize] = Some(value); } - /// Gets the bindings of this poisonable environment. - pub(crate) const fn bindings(&self) -> &GcRefCell>> { - &self.bindings + pub(crate) fn extend_from_compile(&self) { + let compile_bindings_len = self.scope.num_bindings() as usize; + let mut bindings = self.bindings.borrow_mut(); + let bindings_len = bindings.len(); + + if compile_bindings_len <= bindings_len { + return; + } + + bindings.resize(compile_bindings_len, None); + + let mut deletable_bindings = self.deletable_bindings.borrow_mut(); + deletable_bindings.resize(compile_bindings_len, false); + deletable_bindings[bindings_len..].fill(true); + + self.deleted_bindings + .borrow_mut() + .resize(compile_bindings_len, false); + } + + pub(crate) fn is_deleted_binding(&self, index: u32) -> bool { + self.deleted_bindings + .borrow() + .get(index as usize) + .copied() + .unwrap_or_default() + } + + #[track_caller] + pub(crate) fn restore_deleted_binding(&self, index: u32) { + let index = index as usize; + if let Some(deleted) = self.deleted_bindings.borrow_mut().get_mut(index) { + *deleted = false; + } + } + + #[track_caller] + pub(crate) fn delete_binding(&self, index: u32) -> bool { + let index = index as usize; + + if self + .deleted_bindings + .borrow() + .get(index) + .copied() + .unwrap_or_default() + { + return true; + } + + if !self + .deletable_bindings + .borrow() + .get(index) + .copied() + .unwrap_or_default() + { + return false; + } + + self.bindings.borrow_mut()[index] = None; + self.deleted_bindings.borrow_mut()[index] = true; + true } /// `BindThisValue` diff --git a/core/engine/src/environments/runtime/declarative/mod.rs b/core/engine/src/environments/runtime/declarative/mod.rs index fbb1df51c9d..4e5bb9aec34 100644 --- a/core/engine/src/environments/runtime/declarative/mod.rs +++ b/core/engine/src/environments/runtime/declarative/mod.rs @@ -90,6 +90,21 @@ impl DeclarativeEnvironment { self.kind.set(index, value); } + #[track_caller] + pub(crate) fn delete_binding(&self, index: u32) -> bool { + self.kind.delete_binding(index) + } + + #[track_caller] + pub(crate) fn is_deleted_binding(&self, index: u32) -> bool { + self.kind.is_deleted_binding(index) + } + + #[track_caller] + pub(crate) fn restore_deleted_binding(&self, index: u32) { + self.kind.restore_deleted_binding(index); + } + /// `GetThisBinding` /// /// Returns the `this` binding of this environment. @@ -132,11 +147,7 @@ impl DeclarativeEnvironment { /// Extends the environment with the bindings from the compile time environment. pub(crate) fn extend_from_compile(&self) { if let Some(env) = self.kind().as_function() { - let compile_bindings_number = env.compile().num_bindings() as usize; - let mut bindings = env.bindings().borrow_mut(); - if compile_bindings_number > bindings.len() { - bindings.resize(compile_bindings_number, None); - } + env.extend_from_compile(); } } } @@ -212,6 +223,29 @@ impl DeclarativeEnvironmentKind { } } + #[track_caller] + pub(crate) fn delete_binding(&self, index: u32) -> bool { + match self { + Self::Function(inner) => inner.delete_binding(index), + Self::Lexical(_) | Self::Global(_) | Self::Module(_) => false, + } + } + + #[track_caller] + pub(crate) fn is_deleted_binding(&self, index: u32) -> bool { + match self { + Self::Function(inner) => inner.is_deleted_binding(index), + Self::Lexical(_) | Self::Global(_) | Self::Module(_) => false, + } + } + + #[track_caller] + pub(crate) fn restore_deleted_binding(&self, index: u32) { + if let Self::Function(inner) = self { + inner.restore_deleted_binding(index); + } + } + /// `GetThisBinding` /// /// Returns the `this` binding of this environment. diff --git a/core/engine/src/environments/runtime/mod.rs b/core/engine/src/environments/runtime/mod.rs index 6657a5496fb..ac2ece5276c 100644 --- a/core/engine/src/environments/runtime/mod.rs +++ b/core/engine/src/environments/runtime/mod.rs @@ -460,16 +460,20 @@ impl Context { /// are completely removed of runtime checks because the specification guarantees that runtime /// semantics cannot add or remove lexical bindings. pub(crate) fn find_runtime_binding(&mut self, locator: &mut BindingLocator) -> JsResult<()> { + let deleted_binding = self.is_deleted_binding(locator); + let global = self.vm.frame().realm.environment(); if let Some(env) = self.vm.frame().environments.current_declarative_ref(global) && !env.with() && !env.poisoned() + && !deleted_binding { return Ok(()); } let (global, min_index) = match locator.scope() { BindingLocatorScope::GlobalObject | BindingLocatorScope::GlobalDeclarative => (true, 0), + BindingLocatorScope::Stack(_) if deleted_binding => (false, 0), BindingLocatorScope::Stack(index) => (false, index), }; let max_index = self.vm.frame().environments.len() as u32; @@ -477,10 +481,13 @@ impl Context { for index in (min_index..max_index).rev() { match self.environment_expect(index) { Environment::Declarative(env) => { - if env.poisoned() { + if env.poisoned() || deleted_binding { if let Some(env) = env.kind().as_function() && let Some(b) = env.compile().get_binding(locator.name()) { + if env.is_deleted_binding(b.binding_index()) { + continue; + } locator.set_scope(b.scope()); locator.set_binding_index(b.binding_index()); return Ok(()); @@ -505,6 +512,26 @@ impl Context { } } + if deleted_binding + && let BindingLocatorScope::Stack(index) = locator.scope() + && let Environment::Declarative(env) = self.environment_expect(index) + && let Some(function_env) = env.kind().as_function() + { + let mut scope = function_env.compile().outer(); + while let Some(current_scope) = scope { + if let Some(binding) = current_scope.get_binding(locator.name()) { + locator.set_scope(binding.scope()); + locator.set_binding_index(binding.binding_index()); + return Ok(()); + } + scope = current_scope.outer(); + } + + locator.set_scope(BindingLocatorScope::GlobalObject); + locator.set_binding_index(0); + return Ok(()); + } + if global && self.realm().environment().poisoned() && let Some(b) = self.realm().scope().get_binding(locator.name()) @@ -592,6 +619,24 @@ impl Context { } } + pub(crate) fn is_deleted_binding(&self, locator: &BindingLocator) -> bool { + match locator.scope() { + BindingLocatorScope::Stack(index) => matches!( + self.environment_expect(index), + Environment::Declarative(env) if env.is_deleted_binding(locator.binding_index()) + ), + BindingLocatorScope::GlobalObject | BindingLocatorScope::GlobalDeclarative => false, + } + } + + pub(crate) fn restore_deleted_binding(&self, locator: &BindingLocator) { + if let BindingLocatorScope::Stack(index) = locator.scope() + && let Environment::Declarative(env) = self.environment_expect(index) + { + env.restore_deleted_binding(locator.binding_index()); + } + } + /// Get the value of a binding. /// /// # Panics @@ -672,7 +717,7 @@ impl Context { } BindingLocatorScope::GlobalDeclarative => Ok(false), BindingLocatorScope::Stack(index) => match self.environment_expect(index) { - Environment::Declarative(_) => Ok(false), + Environment::Declarative(env) => Ok(env.delete_binding(locator.binding_index())), Environment::Object(obj) => { let key = locator.name().clone(); let obj = obj.clone(); diff --git a/core/engine/src/tests/env.rs b/core/engine/src/tests/env.rs index aefe1c883da..66bab06c3e2 100644 --- a/core/engine/src/tests/env.rs +++ b/core/engine/src/tests/env.rs @@ -94,3 +94,109 @@ fn indirect_eval_function_var_binding_4350() { js_str!("[object Object],[object Object],[object Object]"), )]); } + +#[test] +// https://github.com/boa-dev/boa/issues/5333 +fn eval_created_bindings_can_be_deleted_5333() { + run_test_actions([ + TestAction::assert_eq( + indoc! {r#" + (function() { + var initial = null; + var deleted = null; + var postDeletion; + eval('initial = x; deleted = delete x; postDeletion = function() { x; }; var x;'); + try { + postDeletion(); + return 'no throw'; + } catch (e) { + return String(initial) + ':' + String(deleted) + ':' + e.name; + } + }()); + "#}, + js_str!("undefined:true:ReferenceError"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + var initial; + var deleted = null; + var postDeletion; + eval('initial = f; deleted = delete f; postDeletion = function() { f; }; function f() { return 33; }'); + try { + postDeletion(); + return 'no throw'; + } catch (e) { + return typeof initial + ':' + String(initial()) + ':' + String(deleted) + ':' + e.name; + } + }()); + "#}, + js_str!("function:33:true:ReferenceError"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + delete globalThis.x; + eval('delete x; var x = 1;'); + var result = typeof globalThis.x + ':' + String(globalThis.x); + delete globalThis.x; + return result; + }()); + "#}, + js_str!("number:1"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + delete globalThis.x; + var result = eval('var x = delete x; x;'); + var global = globalThis.x; + delete globalThis.x; + return String(result) + ':' + String(global); + }()); + "#}, + js_str!("true:true"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + delete globalThis.x; + var x = 'outer'; + var result = (function() { + return eval('var x = delete x; x;'); + }()); + var global = globalThis.x; + delete globalThis.x; + return String(result) + ':' + String(x) + ':' + String(global); + }()); + "#}, + js_str!("true:true:undefined"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + delete globalThis.x; + eval('var x; delete x;'); + eval('var x = 2;'); + var result = String(x) + ':' + String(globalThis.x); + delete globalThis.x; + return result; + }()); + "#}, + js_str!("2:undefined"), + ), + TestAction::assert_eq( + indoc! {r#" + (function() { + delete globalThis.f; + eval('function f() {}; delete f;'); + eval('function f() { return 2; }'); + var result = String(f()) + ':' + String(globalThis.f); + delete globalThis.f; + return result; + }()); + "#}, + js_str!("2:undefined"), + ), + ]); +} diff --git a/core/engine/src/vm/code_block.rs b/core/engine/src/vm/code_block.rs index 07ba1d66245..8d98737fef2 100644 --- a/core/engine/src/vm/code_block.rs +++ b/core/engine/src/vm/code_block.rs @@ -513,9 +513,9 @@ impl CodeBlock { | Instruction::SuperCall { argument_count } => { format!("argument_count:{argument_count}") } - Instruction::DefVar { binding_index } | Instruction::GetLocator { binding_index } => { - format!("binding_index:{binding_index}") - } + Instruction::DefVar { binding_index } + | Instruction::DefEvalVar { binding_index } + | Instruction::GetLocator { binding_index } => format!("binding_index:{binding_index}"), Instruction::DefInitVar { src, binding_index } | Instruction::PutLexicalValue { src, binding_index } | Instruction::SetName { src, binding_index } => { @@ -933,8 +933,7 @@ impl CodeBlock { | Instruction::Reserved56 | Instruction::Reserved57 | Instruction::Reserved58 - | Instruction::Reserved59 - | Instruction::Reserved60 => unreachable!("Reserved opcodes are unreachable"), + | Instruction::Reserved59 => unreachable!("Reserved opcodes are unreachable"), } } } diff --git a/core/engine/src/vm/flowgraph/mod.rs b/core/engine/src/vm/flowgraph/mod.rs index 2f96e6edcbd..a25f4297ac2 100644 --- a/core/engine/src/vm/flowgraph/mod.rs +++ b/core/engine/src/vm/flowgraph/mod.rs @@ -186,6 +186,7 @@ impl CodeBlock { graph.add_edge(previous_pc, pc, None, Color::None, EdgeStyle::Line); } Instruction::DefVar { .. } + | Instruction::DefEvalVar { .. } | Instruction::DefInitVar { .. } | Instruction::PutLexicalValue { .. } | Instruction::GetName { .. } @@ -432,8 +433,7 @@ impl CodeBlock { | Instruction::Reserved56 | Instruction::Reserved57 | Instruction::Reserved58 - | Instruction::Reserved59 - | Instruction::Reserved60 => unreachable!("Reserved opcodes are unreachable"), + | Instruction::Reserved59 => unreachable!("Reserved opcodes are unreachable"), } } diff --git a/core/engine/src/vm/opcode/define/mod.rs b/core/engine/src/vm/opcode/define/mod.rs index 984e4a774b2..27477d93b97 100644 --- a/core/engine/src/vm/opcode/define/mod.rs +++ b/core/engine/src/vm/opcode/define/mod.rs @@ -39,6 +39,37 @@ impl Operation for DefVar { const COST: u8 = 3; } +#[derive(Debug, Clone, Copy)] +pub(crate) struct DefEvalVar; + +impl DefEvalVar { + #[inline(always)] + pub(super) fn operation(index: IndexOperand, context: &mut Context) { + let binding_locator = context.vm.frame().code_block.bindings[usize::from(index)].clone(); + + if context.is_deleted_binding(&binding_locator) { + context.restore_deleted_binding(&binding_locator); + } + + { + let frame = context.vm.frame_mut(); + let global = frame.realm.environment(); + frame.environments.put_value_if_uninitialized( + binding_locator.scope(), + binding_locator.binding_index(), + JsValue::undefined(), + global, + ); + } + } +} + +impl Operation for DefEvalVar { + const NAME: &'static str = "DefEvalVar"; + const INSTRUCTION: &'static str = "INST - DefEvalVar"; + const COST: u8 = 3; +} + /// `DefInitVar` implements the Opcode Operation for `Opcode::DefInitVar` /// /// Operation: @@ -56,6 +87,11 @@ impl DefInitVar { let frame = context.vm.frame(); let strict = frame.code_block.strict(); let mut binding_locator = frame.code_block.bindings[usize::from(index)].clone(); + + if context.is_deleted_binding(&binding_locator) { + context.restore_deleted_binding(&binding_locator); + } + context.find_runtime_binding(&mut binding_locator)?; context.set_binding(&binding_locator, value.clone(), strict)?; diff --git a/core/engine/src/vm/opcode/mod.rs b/core/engine/src/vm/opcode/mod.rs index f01c7afecbe..3c6771f8a82 100644 --- a/core/engine/src/vm/opcode/mod.rs +++ b/core/engine/src/vm/opcode/mod.rs @@ -2270,6 +2270,9 @@ generate_opcodes! { Reserved58 => Reserved, /// Reserved [`Opcode`]. Reserved59 => Reserved, - /// Reserved [`Opcode`]. - Reserved60 => Reserved, + /// Declare `var` type variable during eval declaration instantiation. + /// + /// - Operands: + /// - binding_index: `IndexOperand` + DefEvalVar { binding_index: IndexOperand }, } diff --git a/core/engine/src/vm/opcode/set/name.rs b/core/engine/src/vm/opcode/set/name.rs index abf1cd4c5fe..79379f4dd3e 100644 --- a/core/engine/src/vm/opcode/set/name.rs +++ b/core/engine/src/vm/opcode/set/name.rs @@ -83,12 +83,16 @@ impl SetNameByLocator { pub(crate) fn operation(value: RegisterOperand, context: &mut Context) -> JsResult<()> { let frame = context.vm.frame_mut(); let strict = frame.code_block.strict(); - let binding_locator = frame + let mut binding_locator = frame .binding_stack .pop() .js_expect("locator should have been popped before")?; let value = context.vm.get_register(value.into()).clone(); + if context.is_deleted_binding(&binding_locator) { + context.find_runtime_binding(&mut binding_locator)?; + } + verify_initialized(&binding_locator, context)?; context.set_binding(&binding_locator, value.clone(), strict)?;