-
Notifications
You must be signed in to change notification settings - Fork 80
fix(engine): run derived class field initializers after super() #992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,10 @@ pub(crate) struct ECMAScriptFunctionHeapData<'a> { | |
| /// Stores the compiled bytecode of an ECMAScript function. | ||
| pub(crate) compiled_bytecode: Option<Executable<'a>>, | ||
| pub(crate) name: Option<String<'a>>, | ||
| /// 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>>, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: Under no circumstances can we add 4 bytes to every function for this. A different solution must be found. |
||
| } | ||
|
|
||
| unsafe impl Send for ECMAScriptFunctionHeapData<'_> {} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| 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(), | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forlet gc = gc.into_nogc()reasons).You can find some examples about this in https://trynova.dev/blog/guide-to-nova-gc