From 10f36a05738bac57c412fec069b9e0d817d6dad0 Mon Sep 17 00:00:00 2001 From: Monti-27 Date: Wed, 22 Apr 2026 14:49:40 +0530 Subject: [PATCH] fix eval delete bindings --- core/engine/src/builtins/eval/mod.rs | 19 +++++++ .../runtime/declarative/function.rs | 47 +++++++++++++++++ .../environments/runtime/declarative/mod.rs | 35 +++++++++++++ core/engine/src/environments/runtime/mod.rs | 42 +++++++++++++++- core/engine/src/tests/env.rs | 50 +++++++++++++++++++ core/engine/src/vm/opcode/define/mod.rs | 9 ++++ 6 files changed, 200 insertions(+), 2 deletions(-) diff --git a/core/engine/src/builtins/eval/mod.rs b/core/engine/src/builtins/eval/mod.rs index f8fc8f458f0..06521a204eb 100644 --- a/core/engine/src/builtins/eval/mod.rs +++ b/core/engine/src/builtins/eval/mod.rs @@ -316,6 +316,24 @@ impl Eval { ) .map_err(|e| JsNativeError::syntax().with_message(e))?; + let deletable_binding_indices = + bindings + .new_annex_b_function_names + .iter() + .map(|binding| binding.locator().binding_index()) + .chain(bindings.new_function_names.values().filter_map( + |(binding, binding_exists)| { + (!*binding_exists).then_some(binding.locator().binding_index()) + }, + )) + .chain( + bindings + .new_var_names + .iter() + .map(|binding| binding.locator().binding_index()), + ) + .collect::>(); + compiler.eval_declaration_instantiation(&body, strict, &variable_scope, bindings); compiler.compile_statement_list(body.statements(), true, false); @@ -326,6 +344,7 @@ impl Eval { // function environment before evaluating. if !strict { var_environment.extend_from_compile(); + var_environment.mark_deletable_bindings(deletable_binding_indices); } let env_fp = context.vm.frame().environments.len() as u32; diff --git a/core/engine/src/environments/runtime/declarative/function.rs b/core/engine/src/environments/runtime/declarative/function.rs index 9e198882740..cc4c73e0544 100644 --- a/core/engine/src/environments/runtime/declarative/function.rs +++ b/core/engine/src/environments/runtime/declarative/function.rs @@ -1,8 +1,16 @@ use boa_ast::scope::Scope; use boa_gc::{Finalize, GcRefCell, Trace, custom_trace}; +use rustc_hash::FxHashSet; +use std::cell::RefCell; use crate::{JsNativeError, JsObject, JsResult, JsValue, builtins::function::OrdinaryFunction}; +#[derive(Debug, Default)] +struct BindingDeletionState { + deletable_bindings: FxHashSet, + deleted_bindings: FxHashSet, +} + #[derive(Debug, Trace, Finalize)] pub(crate) struct FunctionEnvironment { bindings: GcRefCell>>, @@ -11,6 +19,9 @@ pub(crate) struct FunctionEnvironment { // Safety: Nothing in `Scope` needs tracing. #[unsafe_ignore_trace] scope: Scope, + + #[unsafe_ignore_trace] + binding_deletion_state: Box>, } impl FunctionEnvironment { @@ -20,6 +31,7 @@ impl FunctionEnvironment { bindings: GcRefCell::new(vec![None; bindings_count as usize]), slots: Box::new(slots), scope, + binding_deletion_state: Box::default(), } } @@ -58,6 +70,41 @@ impl FunctionEnvironment { &self.bindings } + pub(crate) fn mark_deletable_bindings(&self, bindings: I) + where + I: IntoIterator, + { + self.binding_deletion_state + .borrow_mut() + .deletable_bindings + .extend(bindings); + } + + pub(crate) fn is_deleted_binding(&self, index: u32) -> bool { + self.binding_deletion_state + .borrow() + .deleted_bindings + .contains(&index) + } + + #[track_caller] + pub(crate) fn delete_binding(&self, index: u32) -> bool { + if !self + .binding_deletion_state + .borrow() + .deletable_bindings + .contains(&index) + { + return false; + } + self.bindings.borrow_mut()[index as usize] = None; + self.binding_deletion_state + .borrow_mut() + .deleted_bindings + .insert(index); + true + } + /// `BindThisValue` /// /// Sets the given value as the `this` binding of the environment. diff --git a/core/engine/src/environments/runtime/declarative/mod.rs b/core/engine/src/environments/runtime/declarative/mod.rs index fbb1df51c9d..67121100d28 100644 --- a/core/engine/src/environments/runtime/declarative/mod.rs +++ b/core/engine/src/environments/runtime/declarative/mod.rs @@ -90,6 +90,16 @@ 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) + } + /// `GetThisBinding` /// /// Returns the `this` binding of this environment. @@ -139,6 +149,15 @@ impl DeclarativeEnvironment { } } } + + pub(crate) fn mark_deletable_bindings(&self, bindings: I) + where + I: IntoIterator, + { + if let Some(env) = self.kind().as_function() { + env.mark_deletable_bindings(bindings); + } + } } /// The kind of the declarative environment. @@ -212,6 +231,22 @@ 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, + } + } + /// `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..43c1cb6c124 100644 --- a/core/engine/src/environments/runtime/mod.rs +++ b/core/engine/src/environments/runtime/mod.rs @@ -460,16 +460,26 @@ 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 = 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, + }; + 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 +487,18 @@ 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 deleted_binding + && self + .environment_expect(index) + .as_declarative() + .is_some_and(|env| env.is_deleted_binding(b.binding_index())) + { + continue; + } locator.set_scope(b.scope()); locator.set_binding_index(b.binding_index()); return Ok(()); @@ -505,6 +523,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()) @@ -672,7 +710,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..bead1f63515 100644 --- a/core/engine/src/tests/env.rs +++ b/core/engine/src/tests/env.rs @@ -94,3 +94,53 @@ 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() { + eval('delete x; var x = 1;'); + return typeof globalThis.x + ':' + String(globalThis.x); + }()); + "#}, + js_str!("number:1"), + ), + ]); +} diff --git a/core/engine/src/vm/opcode/define/mod.rs b/core/engine/src/vm/opcode/define/mod.rs index 984e4a774b2..4e948a93237 100644 --- a/core/engine/src/vm/opcode/define/mod.rs +++ b/core/engine/src/vm/opcode/define/mod.rs @@ -1,5 +1,6 @@ use super::{IndexOperand, RegisterOperand}; use crate::{Context, JsResult, JsValue, vm::opcode::Operation}; +use boa_ast::scope::BindingLocatorScope; pub(crate) mod class; pub(crate) mod own_property; @@ -20,6 +21,14 @@ impl DefVar { // TODO: spec specifies to return `empty` on empty vars, but we're trying to initialize. let binding_locator = context.vm.frame().code_block.bindings[usize::from(index)].clone(); + if let BindingLocatorScope::Stack(index) = binding_locator.scope() + && let crate::environments::Environment::Declarative(env) = + context.environment_expect(index) + && env.is_deleted_binding(binding_locator.binding_index()) + { + return; + } + { let frame = context.vm.frame_mut(); let global = frame.realm.environment();