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..a30148ebf 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,59 @@ 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 a separate executable on the function. 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 bytecode = f.get(agent).class_field_initializer_bytecode; + let bytecode = match bytecode { + Some(b) => b.unbind(), + None => return Ok(()), + }; + let f = f.bind(gc.nogc()); + 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/ecmascript/builtins/ecmascript_function.rs b/nova_vm/src/ecmascript/builtins/ecmascript_function.rs index d2ebc374b..cfbf412c5 100644 --- a/nova_vm/src/ecmascript/builtins/ecmascript_function.rs +++ b/nova_vm/src/ecmascript/builtins/ecmascript_function.rs @@ -890,6 +890,7 @@ pub(crate) fn ordinary_function_create<'gc>( ecmascript_function, compiled_bytecode: None, name: None, + class_field_initializer_bytecode: None, }; if let Some(function_prototype) = params.function_prototype && function_prototype @@ -1226,6 +1227,7 @@ impl HeapMarkAndSweep for ECMAScriptFunctionHeapData<'static> { ecmascript_function, compiled_bytecode, name, + class_field_initializer_bytecode, } = self; let ECMAScriptFunctionObjectHeapData { environment, @@ -1243,6 +1245,7 @@ impl HeapMarkAndSweep for ECMAScriptFunctionHeapData<'static> { object_index.mark_values(queues); compiled_bytecode.mark_values(queues); name.mark_values(queues); + class_field_initializer_bytecode.mark_values(queues); environment.mark_values(queues); private_environment.mark_values(queues); realm.mark_values(queues); @@ -1258,6 +1261,7 @@ impl HeapMarkAndSweep for ECMAScriptFunctionHeapData<'static> { ecmascript_function, compiled_bytecode, name, + class_field_initializer_bytecode, } = self; let ECMAScriptFunctionObjectHeapData { environment, @@ -1275,6 +1279,7 @@ impl HeapMarkAndSweep for ECMAScriptFunctionHeapData<'static> { object_index.sweep_values(compactions); compiled_bytecode.sweep_values(compactions); name.sweep_values(compactions); + class_field_initializer_bytecode.sweep_values(compactions); environment.sweep_values(compactions); private_environment.sweep_values(compactions); realm.sweep_values(compactions); diff --git a/nova_vm/src/ecmascript/types/language/function/data.rs b/nova_vm/src/ecmascript/types/language/function/data.rs index 5eea65b86..b7b1d8f37 100644 --- a/nova_vm/src/ecmascript/types/language/function/data.rs +++ b/nova_vm/src/ecmascript/types/language/function/data.rs @@ -105,6 +105,10 @@ pub(crate) struct ECMAScriptFunctionHeapData<'a> { /// Stores the compiled bytecode of an ECMAScript function. pub(crate) compiled_bytecode: Option>, pub(crate) name: Option>, + /// For a user-written derived class constructor with instance fields, + /// holds the compiled bytecode that initializes those fields. It is run + /// after `super()` has bound `this` (from `EvaluateSuper` step 11). + pub(crate) class_field_initializer_bytecode: Option>, } unsafe impl Send for ECMAScriptFunctionHeapData<'_> {} diff --git a/nova_vm/src/engine/bytecode/bytecode_compiler.rs b/nova_vm/src/engine/bytecode/bytecode_compiler.rs index a3929e690..a5f7ee921 100644 --- a/nova_vm/src/engine/bytecode/bytecode_compiler.rs +++ b/nova_vm/src/engine/bytecode/bytecode_compiler.rs @@ -1237,6 +1237,7 @@ impl<'a, 's, 'gc, 'scope> CompileEvaluation<'a, 's, 'gc, 'scope> for ast::Functi }), identifier, compiled_bytecode: None, + class_field_initializer_bytecode: None, }, ); } @@ -1436,6 +1437,7 @@ impl<'a, 's, 'gc, 'scope> CompileEvaluation<'a, 's, 'gc, 'scope> for ast::Object }), identifier, compiled_bytecode: None, + class_field_initializer_bytecode: None, }, // enumerable: true, true.into(), @@ -1484,6 +1486,7 @@ impl<'a, 's, 'gc, 'scope> CompileEvaluation<'a, 's, 'gc, 'scope> for ast::Object }), identifier: None, compiled_bytecode: None, + class_field_initializer_bytecode: None, }, // enumerable: true, true.into(), 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..e8d35089c 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,44 @@ 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 register it so it + // runs 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), + }; + body_ctx.compile_function_body(constructor_data); + let body_executable = body_ctx.finish(); + ctx.set_function_expression_class_field_initializer_bytecode( + constructor_index, + initializer_executable, + ); + 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); @@ -854,6 +882,7 @@ fn define_constructor_method( // CompileContext holds a name identifier for us if this is NamedEvaluation. identifier: None, compiled_bytecode: None, + class_field_initializer_bytecode: None, }, has_constructor_parent.into(), ) @@ -915,6 +944,7 @@ fn define_method<'s>( // Note: method name is always found in the result register. identifier: Some(NamedEvaluationParameter::Result), compiled_bytecode: None, + class_field_initializer_bytecode: None, }, // enumerable: false, false.into(), @@ -998,6 +1028,7 @@ fn define_private_method<'s>( }), identifier: Some(NamedEvaluationParameter::Result), compiled_bytecode: None, + class_field_initializer_bytecode: None, }, immediate.into(), ); diff --git a/nova_vm/src/engine/bytecode/bytecode_compiler/compile_context.rs b/nova_vm/src/engine/bytecode/bytecode_compiler/compile_context.rs index 3878367c1..242a36fd5 100644 --- a/nova_vm/src/engine/bytecode/bytecode_compiler/compile_context.rs +++ b/nova_vm/src/engine/bytecode/bytecode_compiler/compile_context.rs @@ -1258,6 +1258,15 @@ impl<'agent, 'script, 'gc, 'scope> CompileContext<'agent, 'script, 'gc, 'scope> .set_function_expression_bytecode(index, executable); } + pub(super) fn set_function_expression_class_field_initializer_bytecode( + &mut self, + index: IndexType, + executable: Executable<'gc>, + ) { + self.executable + .set_function_expression_class_field_initializer_bytecode(index, executable); + } + pub(super) fn add_class_initializer_bytecode( &mut self, executable: Executable<'gc>, diff --git a/nova_vm/src/engine/bytecode/bytecode_compiler/executable_context.rs b/nova_vm/src/engine/bytecode/bytecode_compiler/executable_context.rs index 8b7a6f3d2..b92c649cc 100644 --- a/nova_vm/src/engine/bytecode/bytecode_compiler/executable_context.rs +++ b/nova_vm/src/engine/bytecode/bytecode_compiler/executable_context.rs @@ -474,6 +474,15 @@ impl<'agent, 'gc, 'scope> ExecutableContext<'agent, 'gc, 'scope> { self.function_expressions[index as usize].compiled_bytecode = Some(executable); } + pub(super) fn set_function_expression_class_field_initializer_bytecode( + &mut self, + index: IndexType, + executable: Executable<'gc>, + ) { + self.function_expressions[index as usize].class_field_initializer_bytecode = + Some(executable); + } + pub(super) fn add_class_initializer_bytecode( &mut self, executable: Executable<'gc>, diff --git a/nova_vm/src/engine/bytecode/executable.rs b/nova_vm/src/engine/bytecode/executable.rs index c9708d172..6b5c0a4f8 100644 --- a/nova_vm/src/engine/bytecode/executable.rs +++ b/nova_vm/src/engine/bytecode/executable.rs @@ -64,6 +64,11 @@ pub(crate) struct FunctionExpression<'a> { pub(crate) identifier: Option, /// Optionally eagerly compile the FunctionExpression into bytecode. pub(crate) compiled_bytecode: Option>, + /// For a class constructor with instance fields defined on a derived + /// class, holds a separate executable that runs the field initializers + /// after `super()` has bound `this`. The executable is invoked from + /// `EvaluateSuper` step 11 (InitializeInstanceElements). + pub(crate) class_field_initializer_bytecode: Option>, } bindable_handle!(FunctionExpression); @@ -74,8 +79,10 @@ impl HeapMarkAndSweep for FunctionExpression<'static> { expression: _, identifier: _, compiled_bytecode, + class_field_initializer_bytecode, } = self; compiled_bytecode.mark_values(queues); + class_field_initializer_bytecode.mark_values(queues); } fn sweep_values(&mut self, compactions: &CompactionLists) { @@ -83,8 +90,10 @@ impl HeapMarkAndSweep for FunctionExpression<'static> { expression: _, identifier: _, compiled_bytecode, + class_field_initializer_bytecode, } = self; compiled_bytecode.sweep_values(compactions); + class_field_initializer_bytecode.sweep_values(compactions); } } diff --git a/nova_vm/src/engine/bytecode/vm/execute_instructions.rs b/nova_vm/src/engine/bytecode/vm/execute_instructions.rs index df9b663c5..7c6def3e2 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, @@ -1220,10 +1220,12 @@ pub(super) fn execute_class_define_constructor<'gc>( let FunctionExpression { expression, compiled_bytecode, + class_field_initializer_bytecode, .. } = executable.fetch_function_expression(agent, instr.get_first_index(), gc.nogc()); let function_expression = expression.get(); let compiled_bytecode = *compiled_bytecode; + let class_field_initializer_bytecode = *class_field_initializer_bytecode; let has_constructor_parent = instr.get_second_bool(); let function_prototype = if has_constructor_parent { @@ -1252,6 +1254,10 @@ pub(super) fn execute_class_define_constructor<'gc>( if let Some(compiled_bytecode) = compiled_bytecode { function.get_mut(agent).compiled_bytecode = Some(compiled_bytecode.unbind()); } + if let Some(class_field_initializer_bytecode) = class_field_initializer_bytecode { + function.get_mut(agent).class_field_initializer_bytecode = + Some(class_field_initializer_bytecode.unbind()); + } set_function_name(agent, function, class_name.into(), None, gc.nogc()); make_constructor(agent, function, Some(false), Some(proto), gc.nogc()); function.get_mut(agent).ecmascript_function.home_object = Some(proto.into()); @@ -1766,7 +1772,8 @@ pub(super) fn execute_evaluate_super<'gc>( result.unbind().bind(gc.nogc()) }; // 7. Let thisER be GetThisEnvironment(). - let Environment::Function(this_er) = get_this_environment(agent, gc.nogc()) else { + let this_er = get_this_environment(agent, gc.nogc()); + let Environment::Function(this_er) = this_er else { unreachable!(); }; // 8. Perform ? thisER.BindThisValue(result). @@ -1776,12 +1783,24 @@ 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`. They are stored on the function as a separate + // executable and 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..2f716b7c0 --- /dev/null +++ b/tests/class-field-init-in-derived.js @@ -0,0 +1,61 @@ +// 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') +} \ No newline at end of file