fix(engine): run derived class field initializers after super() - #992
fix(engine): run derived class field initializers after super()#992simonyang08 wants to merge 1 commit into
Conversation
…ova#948) A user-written derived class constructor that declared instance fields used to throw ReferenceError: Uninitialized this binding because the field initializer prelude was emitted at the start of the constructor body, before super() had bound this. For derived classes, the prelude is now built as a separate executable and stored on the function. It is invoked from step 11 of EvaluateSuper after super() has bound this, mirroring the behaviour of default constructors. Base-class constructors keep the existing prelude-inside-body path because OrdinaryCallBindThis runs before the user body and so this is already initialized. Includes a regression script under tests/ that covers the original issue, single/multi-field cases, grand-child fields, and a base-class no-regression check. Signed-off-by: simonyang08 <ppt5928@gmail.com>
aapoalas
left a comment
There was a problem hiding this comment.
Hello and welcome to the project! Thank you for the interest in this - this is a really hard bug to fix I must say, but also one of the most important ones and one that I've had in the back of my mind for a good long while :)
Unfortunately, I don't think this direction of attack is acceptable. We cannot grow the ECMAScriptFunctionHeapData object for this, as this is a fairly uncommon feature to be used. I think the optimal solution would be for the initializers to "get swallowed up" by the constructor bytecode itself, but that is non-trivial. The ExecutableHeapData does already contain a class_initializer_bytecodes field which could possibly be reused here to avoid the large-ish refactoring needed for the (possibly) optimal solution, but it may not be an entirely trivial choice either.
| ) -> JsResult<'a, ()> { | ||
| // Read everything we need before mutating the agent. | ||
| let bytecode = f.get(agent).class_field_initializer_bytecode; | ||
| let bytecode = match bytecode { |
There was a problem hiding this comment.
issue: b.unbind() is definitely wrong - all GC handles must be bound using .bind(gc.nogc()) and only unbound in special cases (eg. when having to unbind them temporarily for let gc = gc.into_nogc() reasons).
You can find some examples about this in https://trynova.dev/blog/guide-to-nova-gc
| /// 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<Executable<'a>>, |
There was a problem hiding this comment.
issue: Under no circumstances can we add 4 bytes to every function for this. A different solution must be found.
| // `OrdinaryCallBindThis` runs before the user body and so | ||
| // `this` is already initialized. | ||
| if has_constructor_parent { | ||
| let initializer_executable = constructor_ctx.finish(); |
There was a problem hiding this comment.
thought: I'd honestly prefer to build the initializers as part of the constructor bytecode directly. That would probably need one or two new VM bytecode instructions but that's kinda cheap still.
The harder part is the refactoring of moving the initialization bytecode compilation out of ClassDefinitionEvaluation and into the super() instruction.
Fixes #948
A user-written derived class constructor that declared instance fields used to throw
ReferenceError: Uninitialized this bindingbecause the field-initializer prelude was emitted at the start of the constructor body, beforesuper()had boundthis.Change
For derived classes, the field-initializer prelude is now built as a separate executable stored on the function and invoked from
EvaluateSuperaftersuper()has boundthis, mirroring how default constructors already behave. Base-class constructors keep the existing prelude-inside-body path becauseOrdinaryCallBindThisruns before the user body.The new helper
initialize_ecmascript_function_class_field_initializersmirrorsinitialize_instance_elements(used by default constructors) so future field-init environment changes should apply to both paths.Regression test
tests/class-field-init-in-derived.jscovers the original issue repro, single/multi-field cases, grand-child fields, a base-class no-regression check, and field visibility aftersuper()inside the constructor.Verification
cargo build --bin nova_cli --profile dev-fast— cleancargo clippy --bin nova_cli --profile dev-fast— 0 warningscargo fmt --check— cleanReferenceErrorbefore the change → completes normally afterScope note
This fix inherently threads new state through the bytecode compiler, VM, executable, and function data structures, so the diff spans 10 files (+233/-30) — larger than a typical focused patch, but it is a single semantic change. The GC mark/sweep handling for the new
class_field_initializer_bytecodefield is included.Signed-off-by: simonyang08 ppt5928@gmail.com