diff --git a/core/engine/src/bytecompiler/mod.rs b/core/engine/src/bytecompiler/mod.rs index 3159e2f062d..996e514d197 100644 --- a/core/engine/src/bytecompiler/mod.rs +++ b/core/engine/src/bytecompiler/mod.rs @@ -2659,19 +2659,31 @@ impl<'ctx> ByteCompiler<'ctx> { unreachable!("with binding cannot be local") } }; + // Resolve the binding exactly once. `GetNameAndLocator` fetches + // the callee value and records the resolved locator on the + // binding stack; `ThisForObjectEnvironmentName` then derives the + // `this` value (`WithBaseObject`) from that same locator without + // re-scanning the environment chain. This guarantees `HasBinding` + // (and the binding object's `[[HasProperty]]` trap) runs once, as + // required by the specification. + let this = self.register_allocator.alloc(); let value = self.register_allocator.alloc(); self.bytecode - .emit_this_for_object_environment_name(value.variable(), index.into()); + .emit_get_name_and_locator(value.variable(), index.into()); + self.bytecode + .emit_this_for_object_environment_name(this.variable()); + self.push_from_register(&this); self.push_from_register(&value); self.register_allocator.dealloc(value); + self.register_allocator.dealloc(this); } else { self.push_from_register(&CallFrame::undefined_register()); + self.compile_expr_to_stack(expr); } } else { self.push_from_register(&CallFrame::undefined_register()); + self.compile_expr_to_stack(expr); } - - self.compile_expr_to_stack(expr); } expr => { let value = self.register_allocator.alloc(); diff --git a/core/engine/src/environments/runtime/mod.rs b/core/engine/src/environments/runtime/mod.rs index ac2ece5276c..bfad6350744 100644 --- a/core/engine/src/environments/runtime/mod.rs +++ b/core/engine/src/environments/runtime/mod.rs @@ -1,5 +1,6 @@ use crate::{ Context, JsResult, JsString, JsSymbol, JsValue, + error::JsNativeError, object::{JsObject, PrivateName}, }; use boa_ast::scope::{BindingLocator, BindingLocatorScope, Scope}; @@ -543,53 +544,25 @@ impl Context { Ok(()) } - /// Finds the object environment that contains the binding and returns the `this` value of the object environment. - pub(crate) fn this_from_object_environment_binding( - &mut self, + /// Returns the `this` value (`WithBaseObject`) for an already-resolved binding + /// locator, without performing another lookup on the environment chain. + /// + /// Per the specification, the `this` value of a function call whose callee is a + /// reference with an Environment Record base is obtained from that already + /// resolved record via `WithBaseObject`. Reusing the resolution avoids + /// re-running `HasBinding` (and therefore avoids invoking the binding object's + /// `[[HasProperty]]` trap a second time). + pub(crate) fn this_from_resolved_object_environment_binding( + &self, locator: &BindingLocator, - ) -> JsResult> { - let global = self.vm.frame().realm.environment(); - if let Some(env) = self.vm.frame().environments.current_declarative_ref(global) - && !env.with() + ) -> Option { + if let BindingLocatorScope::Stack(index) = locator.scope() + && let Environment::Object(o) = self.environment_expect(index) { - return Ok(None); + return Some(o.clone()); } - let min_index = match locator.scope() { - BindingLocatorScope::GlobalObject | BindingLocatorScope::GlobalDeclarative => 0, - BindingLocatorScope::Stack(index) => index, - }; - let max_index = self.vm.frame().environments.len() as u32; - - for index in (min_index..max_index).rev() { - match self.environment_expect(index) { - Environment::Declarative(env) => { - if env.poisoned() { - if let Some(env) = env.kind().as_function() - && env.compile().get_binding(locator.name()).is_some() - { - break; - } - } else if !env.with() { - break; - } - } - Environment::Object(o) => { - let o = o.clone(); - let key = locator.name().clone(); - if o.has_property(key.clone(), self)? { - if let Some(unscopables) = o.get(JsSymbol::unscopables(), self)?.as_object() - && unscopables.get(key.clone(), self)?.to_boolean() - { - continue; - } - return Ok(Some(o)); - } - } - } - } - - Ok(None) + None } /// Checks if the binding pointed by `locator` is initialized. @@ -659,7 +632,23 @@ impl Context { Environment::Object(obj) => { let key = locator.name().clone(); let obj = obj.clone(); - obj.get(key, self).map(Some) + // `GetBindingValue` (9.1.1.2.6): step 2 performs `HasProperty` + // before step 4's `Get`. If the binding is gone (e.g. it was + // deleted by an `@@unscopables` getter during `HasBinding`), + // step 3 returns `undefined` in sloppy mode or throws a + // `ReferenceError` in strict mode. + if obj.has_property(key.clone(), self)? { + obj.get(key, self).map(Some) + } else if self.vm.frame().code_block.strict() { + Err(JsNativeError::reference() + .with_message(format!( + "{} is not defined", + locator.name().to_std_string_escaped() + )) + .into()) + } else { + Ok(Some(JsValue::undefined())) + } } }, } diff --git a/core/engine/src/vm/code_block.rs b/core/engine/src/vm/code_block.rs index ae209f7363b..7bc04229522 100644 --- a/core/engine/src/vm/code_block.rs +++ b/core/engine/src/vm/code_block.rs @@ -390,6 +390,7 @@ impl CodeBlock { | Instruction::StoreUndefined { dst } | Instruction::Exception { dst } | Instruction::This { dst } + | Instruction::ThisForObjectEnvironmentName { dst } | Instruction::NewTarget { dst } | Instruction::ImportMeta { dst } | Instruction::CreateMappedArgumentsObject { dst } @@ -461,7 +462,6 @@ impl CodeBlock { format!("value:{value}, dst:{dst}") } Instruction::StoreLiteral { index, dst } - | Instruction::ThisForObjectEnvironmentName { index, dst } | Instruction::GetFunction { index, dst } | Instruction::GetArgument { index, dst } => { format!("index:{index}, dst:{dst}") diff --git a/core/engine/src/vm/opcode/environment/mod.rs b/core/engine/src/vm/opcode/environment/mod.rs index b6a44373288..5f55ff5ef27 100644 --- a/core/engine/src/vm/opcode/environment/mod.rs +++ b/core/engine/src/vm/opcode/environment/mod.rs @@ -94,13 +94,20 @@ pub(crate) struct ThisForObjectEnvironmentName; impl ThisForObjectEnvironmentName { #[inline(always)] - pub(super) fn operation( - (dst, index): (RegisterOperand, IndexOperand), - context: &mut Context, - ) -> JsResult<()> { - let binding_locator = context.vm.frame().code_block.bindings[usize::from(index)].clone(); + pub(super) fn operation(dst: RegisterOperand, context: &mut Context) -> JsResult<()> { + // The preceding `GetNameAndLocator` opcode already resolved the binding and + // pushed the resolved locator onto the binding stack. Reuse that resolution + // to derive the `this` value (`WithBaseObject`) instead of scanning the + // environment chain again, so `HasBinding` (and the binding object's + // `[[HasProperty]]` trap) runs exactly once, as required by the specification. + let binding_locator = context + .vm + .frame_mut() + .binding_stack + .pop() + .js_expect("locator should have been pushed by GetNameAndLocator")?; let this = context - .this_from_object_environment_binding(&binding_locator)? + .this_from_resolved_object_environment_binding(&binding_locator) .map_or(JsValue::undefined(), Into::into); context.vm.set_register(dst.into(), this); Ok(()) diff --git a/core/engine/src/vm/opcode/mod.rs b/core/engine/src/vm/opcode/mod.rs index 67990049f99..f762de0540f 100644 --- a/core/engine/src/vm/opcode/mod.rs +++ b/core/engine/src/vm/opcode/mod.rs @@ -1770,11 +1770,9 @@ generate_opcodes! { /// Pushes `this` value that is related to the object environment of the given binding /// - /// - Operands: - /// - index: `IndexOperand` /// - Registers: /// - Output: dst - ThisForObjectEnvironmentName { dst: RegisterOperand, index: IndexOperand }, + ThisForObjectEnvironmentName { dst: RegisterOperand }, /// Execute the `super()` method. /// diff --git a/core/engine/src/vm/tests.rs b/core/engine/src/vm/tests.rs index 9df53703732..73213d320ea 100644 --- a/core/engine/src/vm/tests.rs +++ b/core/engine/src/vm/tests.rs @@ -593,3 +593,90 @@ fn recursion_in_setter_throws_uncatchable_error() { ), ]); } + +#[test] +fn with_object_environment_call_single_lookup_and_this() { + run_test_actions([ + TestAction::run(indoc! {r#" + let emptyHasCount = 0; + const emptyProxy = new Proxy({}, { + has(t, p) { + if (p === "Object") { + emptyHasCount++; + } + return Reflect.has(t, p); + } + }); + with (emptyProxy) { + Object(); + } + + let hasCount = 0; + let callThis = null; + const target = { + fn() { + callThis = this; + } + }; + const proxy = new Proxy(target, { + has(t, p) { + if (p === "fn") { + hasCount++; + } + return Reflect.has(t, p); + } + }); + with (proxy) { + fn(); + } + "#}), + TestAction::assert_eq("emptyHasCount", 1), + TestAction::assert_eq("hasCount", 2), + TestAction::assert("callThis === proxy"), + ]); +} + +#[test] +fn with_object_environment_binding_deleted_in_unscopables() { + run_test_actions([ + TestAction::run(indoc! {r#" + let unscopablesCalled = 0; + const env = { + binding: 42, + get [Symbol.unscopables]() { + unscopablesCalled++; + delete env.binding; + return null; + } + }; + let sloppyResult = null; + with (env) { + sloppyResult = binding; + } + + let strictThrew = false; + const envStrict = { + binding: 42, + get [Symbol.unscopables]() { + delete envStrict.binding; + return null; + } + }; + with (envStrict) { + try { + (function() { + "use strict"; + return binding; + })(); + } catch (e) { + if (e instanceof ReferenceError) { + strictThrew = true; + } + } + } + "#}), + TestAction::assert_eq("unscopablesCalled", 1), + TestAction::assert("sloppyResult === undefined"), + TestAction::assert("strictThrew === true"), + ]); +} diff --git a/tests/insta-bytecode/scripts/with-statement-call.js b/tests/insta-bytecode/scripts/with-statement-call.js new file mode 100644 index 00000000000..c93af933871 --- /dev/null +++ b/tests/insta-bytecode/scripts/with-statement-call.js @@ -0,0 +1,15 @@ +// Snapshot pins the bytecode for a function call whose callee is resolved +// through an object Environment Record (a `with` statement). PR #5507 makes +// the compiler emit `GetNameAndLocator` to resolve the binding/locator once, +// then `ThisForObjectEnvironmentName dst:rNN` (dst-only; the old `index` +// operand was removed) to derive the call's `this` (WithBaseObject) from that +// same resolved locator, so HasBinding/[[HasProperty]] runs exactly once. +with ( + { + fn() { + return this; + }, + } +) { + fn(); +} diff --git a/tests/insta-bytecode/src/snapshots/insta_bytecode__compile_bytecode@with-statement-call.js.snap b/tests/insta-bytecode/src/snapshots/insta_bytecode__compile_bytecode@with-statement-call.js.snap new file mode 100644 index 00000000000..b77069a1633 --- /dev/null +++ b/tests/insta-bytecode/src/snapshots/insta_bytecode__compile_bytecode@with-statement-call.js.snap @@ -0,0 +1,33 @@ +--- +source: tests/insta-bytecode/src/lib.rs +expression: output +input_file: tests/insta-bytecode/scripts/with-statement-call.js +--- +-------------------------- Compiled Output: '
' --------------------------- +Location Handler Opcode Operands + 000000 StoreEmptyObject dst:r01 + 000005 GetFunction index:0, dst:r02 + 00000e SetHomeObject function:r02, home:r01 + 000017 DefineOwnPropertyByName object:r01, value:r02, name_index:1 + 000024 PushObjectEnvironment src:r01 + 000029 GetNameAndLocator dst:r03, binding_index:0 + 000032 ThisForObjectEnvironmentName dst:r02 + 000037 PushFromRegister src:r02 + 00003c PushFromRegister src:r03 + 000041 Call argument_count:0 + 000046 PopIntoRegister dst:r01 + 00004b SetAccumulator src:r01 + 000050 PopEnvironment + 000051 CheckReturn + 000052 Return + +Register Count: 4, Flags: CodeBlockFlags(HAS_PROTOTYPE_PROPERTY) +Constants: + 0000: [FUNCTION] name: 'fn' (length: 0) + 0001: [STRING] "fn" + 0002: [SCOPE] index: 1, bindings: 0 +Bindings: + 0000: fn, scope: GlobalObject +Handlers: +Source Map: + 0000: 65..75: (14, 5)