diff --git a/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs b/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs index 69158b974..0b326a3fa 100644 --- a/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs +++ b/nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs @@ -9,11 +9,11 @@ use core::ops::ControlFlow; use crate::{ ecmascript::{ Agent, ArgumentsList, Array, BUILTIN_STRING_MEMORY, BuiltinConstructorFunction, - ECMAScriptCodeEvaluationState, Environment, ExceptionType, ExecutionContext, Function, - InternalMethods, InternalSlots, IteratorRecord, JsError, JsResult, KeyedGroup, Number, - Object, OrdinaryObject, PrivateName, PropertyDescriptor, PropertyKey, PropertyKeySet, - PropertyLookupCache, ProtoIntrinsics, Realm, SetResult, SmallInteger, String, TryError, - TryGetResult, TryHasResult, TryResult, Value, array_create, + ECMAScriptCodeEvaluationState, ECMAScriptFunction, Environment, ExceptionType, + ExecutionContext, Function, InternalMethods, InternalSlots, IteratorRecord, JsError, + JsResult, KeyedGroup, Number, Object, OrdinaryObject, PrivateName, PropertyDescriptor, + PropertyKey, PropertyKeySet, PropertyLookupCache, ProtoIntrinsics, Realm, SetResult, + SmallInteger, String, TryError, TryGetResult, TryHasResult, TryResult, Value, array_create, canonicalize_keyed_collection_key, get_iterator, if_abrupt_close_iterator, is_callable, is_constructor, iterator_close_with_error, iterator_step_value, js_result_into_try, new_class_field_initializer_environment, require_object_coercible, to_length, to_object, @@ -2747,6 +2747,72 @@ pub(crate) fn initialize_instance_elements<'a>( Ok(()) } +/// Runs the deferred class field initializer bytecode associated with a +/// user-written ECMAScript function constructor. +/// +/// For a user-written derived class constructor that has instance fields +/// declared on the class, the field initializers must not run before +/// `super()` (because `this` is uninitialized at that point). The compiler +/// stores them as the body executable's `class_initializer_bytecodes[0]` +/// entry (a slot already present on `ExecutableHeapData`). This helper runs +/// that executable in a new function environment where `this` is bound to +/// the constructed instance, mirroring the behaviour of +/// [`initialize_instance_elements`] for built-in default constructors. +pub(crate) fn initialize_ecmascript_function_class_field_initializers<'a>( + agent: &mut Agent, + f: ECMAScriptFunction, + instance: Object, + gc: GcScope<'a, '_>, +) -> JsResult<'a, ()> { + // Read everything we need before mutating the agent. + let f = f.bind(gc.nogc()); + // The body executable's `class_initializer_bytecodes` always has the + // deferred field initializer at index 0 when one was emitted at compile + // time. For constructors without deferred initializers the slot is + // empty. + let body_executable = f.get(agent).compiled_bytecode; + let bytecode = body_executable + .and_then(|body_exe| { + body_exe + .get(agent) + .class_initializer_bytecodes + .first() + .copied() + }) + .and_then(|(init, _)| init); + let Some(bytecode) = bytecode else { + return Ok(()); + }; + let outer_env = f.get(agent).ecmascript_function.environment; + let outer_priv_env = f.get(agent).ecmascript_function.private_environment; + let source_code = f.get(agent).ecmascript_function.source_code; + let realm = f.get(agent).ecmascript_function.realm; + let instance = instance.bind(gc.nogc()); + let decl_env = new_class_field_initializer_environment( + agent, + Function::ECMAScriptFunction(f), + instance, + outer_env, + gc.nogc(), + ); + agent.push_execution_context(ExecutionContext { + ecmascript_code: Some(ECMAScriptCodeEvaluationState { + lexical_environment: Environment::Function(decl_env.unbind()), + variable_environment: Environment::Function(decl_env.unbind()), + private_environment: outer_priv_env.unbind(), + is_strict_mode: true, + source_code: source_code.unbind(), + }), + function: Some(Function::ECMAScriptFunction(f.unbind())), + realm: realm.unbind(), + script_or_module: None, + }); + let bytecode = bytecode.scope(agent, gc.nogc()); + let result = Vm::execute(agent, bytecode, None, gc).into_js_result(); + agent.pop_execution_context(); + result.map(|_| ()) +} + /// ### [7.3.34 AddValueToKeyedGroup ( groups, key, value )](https://tc39.es/ecma262/#sec-add-value-to-keyed-group) /// The abstract operation AddValueToKeyedGroup takes arguments groups (a List of Records with fields /// [[Key]] (an ECMAScript language value) and [[Elements]] (a List of ECMAScript language values)), diff --git a/nova_vm/src/engine/bytecode/bytecode_compiler/class_definition_evaluation.rs b/nova_vm/src/engine/bytecode/bytecode_compiler/class_definition_evaluation.rs index ed9c4531b..624083d6e 100644 --- a/nova_vm/src/engine/bytecode/bytecode_compiler/class_definition_evaluation.rs +++ b/nova_vm/src/engine/bytecode/bytecode_compiler/class_definition_evaluation.rs @@ -636,16 +636,51 @@ impl<'a, 's, 'gc, 'scope> CompileEvaluation<'a, 's, 'gc, 'scope> for ast::Class< constructor_ctx.add_instruction(Instruction::Store); let source_code = constructor_ctx.get_source_code(); if let Some(constructor) = constructor { - let constructor_data = CompileFunctionBodyData { - source_code, - is_lexical: false, - // Class code is always strict. - is_strict: true, - ast: FunctionAstRef::ClassConstructor(&constructor.value), - }; - constructor_ctx.compile_function_body(constructor_data); - let executable = constructor_ctx.finish(); - ctx.set_function_expression_bytecode(constructor_index, executable); + // For a user-written constructor on a derived class, the + // instance field initializers cannot run before `super()` + // because `this` is uninitialized at that point. Build the + // prelude as a separate executable and attach it to the + // body executable's `class_initializer_bytecodes`. It is then + // invoked from `EvaluateSuper` step 11 after `super()` has + // bound `this`. For base classes the existing + // prelude-inside-body approach is preserved because + // `OrdinaryCallBindThis` runs before the user body and so + // `this` is already initialized. + if has_constructor_parent { + let initializer_executable = constructor_ctx.finish(); + let mut body_ctx = CompileContext::new(agent, source_code, gc); + let constructor_data = CompileFunctionBodyData { + source_code, + is_lexical: false, + // Class code is always strict. + is_strict: true, + ast: FunctionAstRef::ClassConstructor(&constructor.value), + }; + // The slot must be reserved before the body is compiled: + // class definitions nested inside the constructor push + // their own default-constructor initializer entries into + // the same `class_initializer_bytecodes` vec while the + // body compiles, so index 0 is only ours if we claim it + // first. `EvaluateSuper` reads the entry at index 0. + body_ctx.add_class_initializer_bytecode( + initializer_executable, + has_constructor_parent, + ); + body_ctx.compile_function_body(constructor_data); + let body_executable = body_ctx.finish(); + ctx.set_function_expression_bytecode(constructor_index, body_executable); + } else { + let constructor_data = CompileFunctionBodyData { + source_code, + is_lexical: false, + // Class code is always strict. + is_strict: true, + ast: FunctionAstRef::ClassConstructor(&constructor.value), + }; + constructor_ctx.compile_function_body(constructor_data); + let executable = constructor_ctx.finish(); + ctx.set_function_expression_bytecode(constructor_index, executable); + } } else { let executable = constructor_ctx.finish(); ctx.add_class_initializer_bytecode(executable, has_constructor_parent); diff --git a/nova_vm/src/engine/bytecode/vm/execute_instructions.rs b/nova_vm/src/engine/bytecode/vm/execute_instructions.rs index df9b663c5..c4cd85a19 100644 --- a/nova_vm/src/engine/bytecode/vm/execute_instructions.rs +++ b/nova_vm/src/engine/bytecode/vm/execute_instructions.rs @@ -17,17 +17,17 @@ use crate::{ copy_data_properties, copy_data_properties_into_object, create_builtin_constructor, create_data_property_or_throw, create_unmapped_arguments_object, define_property_or_throw, evaluate_import_call, get_this_environment, get_this_value, get_value, has_property, - is_constructor, is_less_than, is_loosely_equal, is_private_reference, - is_property_reference, is_strictly_equal, is_super_reference, is_unresolvable_reference, - iterator_complete, iterator_value, make_constructor, make_method, - new_class_static_element_environment, new_declarative_environment, new_private_environment, - ordinary_function_create, ordinary_object_create_with_intrinsics, perform_eval, - private_element_find, put_value, resolve_binding, resolve_private_identifier, - resolve_this_binding, set, set_function_name, throw_no_proxy_private_names, - throw_read_undefined_or_null_error, to_boolean, to_number, to_number_primitive, to_numeric, - to_numeric_primitive, to_object, to_property_key, to_property_key_complex, - to_property_key_primitive, to_property_key_simple, to_string, to_string_primitive, - try_copy_data_properties_into_object, try_create_data_property, + initialize_ecmascript_function_class_field_initializers, is_constructor, is_less_than, + is_loosely_equal, is_private_reference, is_property_reference, is_strictly_equal, + is_super_reference, is_unresolvable_reference, iterator_complete, iterator_value, + make_constructor, make_method, new_class_static_element_environment, + new_declarative_environment, new_private_environment, ordinary_function_create, + ordinary_object_create_with_intrinsics, perform_eval, private_element_find, put_value, + resolve_binding, resolve_private_identifier, resolve_this_binding, set, set_function_name, + throw_no_proxy_private_names, throw_read_undefined_or_null_error, to_boolean, to_number, + to_number_primitive, to_numeric, to_numeric_primitive, to_object, to_property_key, + to_property_key_complex, to_property_key_primitive, to_property_key_simple, to_string, + to_string_primitive, try_copy_data_properties_into_object, try_create_data_property, try_define_property_or_throw, try_get_value, try_has_property, try_initialize_referenced_binding, try_put_value, try_resolve_binding, try_result_into_js, try_result_into_option_js, unwrap_try, @@ -1776,12 +1776,26 @@ pub(super) fn execute_evaluate_super<'gc>( .bind(gc.nogc()); // 9. Let F be thisER.[[FunctionObject]]. // 10. Assert: F is an ECMAScript function object. - let Function::ECMAScriptFunction(_f) = this_er.get_function_object(agent) else { - unreachable!(); + let f_unbound = match this_er.get_function_object(agent) { + Function::ECMAScriptFunction(f) => f.unbind(), + _ => unreachable!(), }; // 11. Perform ? InitializeInstanceElements(result, F). + // For a user-written derived class constructor with instance fields + // declared on the class, the field initializers must run after `super()` + // has bound `this`. The compiler attaches the deferred initializer + // bytecode to the constructor body's `compiled_bytecode` Executable via + // its `class_initializer_bytecodes` slot (a slot already present on + // `ExecutableHeapData`), and it is invoked here. + let result_object_unbound = result.unbind(); + initialize_ecmascript_function_class_field_initializers( + agent, + f_unbound, + result_object_unbound, + gc, + )?; // 12. Return result. - vm.result = Some(result.unbind().into()); + vm.result = Some(result_object_unbound.into()); Ok(()) } diff --git a/tests/class-field-init-in-derived.js b/tests/class-field-init-in-derived.js new file mode 100644 index 000000000..9425b4d1d --- /dev/null +++ b/tests/class-field-init-in-derived.js @@ -0,0 +1,108 @@ +// Regression test for https://github.com/trynova/nova/issues/948 +// "class field initializers are broken in subclasses" +// +// A user-written derived class constructor used to throw +// `ReferenceError: Uninitialized this binding` because the instance field +// initializer prelude was emitted at the start of the constructor body, +// before `super()` had bound `this`. The fix defers the field initializer +// to a separate executable that runs after `super()` returns. + +class A {} +class B extends A { + b = 2 + constructor() { super() } +} + +const b = new B() +if (b.b !== 2) { + throw new Error('expected b.b === 2, got ' + b.b) +} + +// Field visible to the constructor body after super() returns. +class C extends A { + c = 3 + constructor() { + super() + if (this.c !== 3) { + throw new Error('expected this.c === 3 inside constructor') + } + } +} +new C() + +// Multiple instance fields. +class E extends A { + e1 = 1 + e2 = 2 + constructor() { super() } +} +const e = new E() +if (e.e1 !== 1 || e.e2 !== 2) { + throw new Error('expected e.e1 === 1 and e.e2 === 2') +} + +// Grand-child still inherits fields from both levels. +class I extends B { + i = 'i-field' + constructor() { super() } +} +const i = new I() +if (i.b !== 2 || i.i !== 'i-field') { + throw new Error('expected i.b === 2 and i.i === "i-field"') +} + +// Base class with fields must remain unchanged (no regression). +class G { + g = 42 + constructor() {} +} +if (new G().g !== 42) { + throw new Error('expected new G().g === 42') +} +// Nested classes inside a derived constructor must not displace the +// deferred field-initializer entry: the compiler reserves slot 0 of the +// body executable's class_initializer_bytecodes for it before compiling +// the body, and EvaluateSuper reads slot 0. + +// Variant 1: nested base class with fields and an implicit constructor +// (its default-constructor initializer entry is appended after ours). +class P extends A { + p = 'p-field' + constructor() { + class InnerWithDefault { + inner = 7 + } + if (new InnerWithDefault().inner !== 7) { + throw new Error('expected inner === 7') + } + super() + } +} +if (new P().p !== 'p-field') { + throw new Error('expected p === "p-field"') +} + +// Variant 2: nested derived class with its own user constructor and +// fields (its deferred initializer lives at slot 0 of its own body +// executable, one level down). +class Q extends A { + q = 'q-field' + constructor() { + const Nested = class extends A { + nested = 'nested-field' + constructor() { super() } + } + const inner = new Nested() + if (inner.nested !== 'nested-field') { + throw new Error('expected nested === "nested-field"') + } + super() + if (this.q !== 'q-field') { + throw new Error('expected this.q === "q-field" inside constructor') + } + } +} +const q = new Q() +if (q.q !== 'q-field') { + throw new Error('expected q.q === "q-field"') +}