-
-
Notifications
You must be signed in to change notification settings - Fork 879
Description
Class properties transform is now complete except for some edge cases.
Conformance now runs without panic, and monitor-oxc seems to show only remaining issues relate to the transforms we've not done yet.
We have 2 more class transforms to implement:
- Private methods
#prop in object
We should NOT enable class properties transform (#7750) until these other 2 transforms are complete, because they need to work in concert with each other.
How the 3 transforms work together
It looks to me like private methods transform and #prop in object transform could run alone. But class properties transform requires the other 2 transforms to work correctly.
Otherwise you have cases like this where properties transform is enabled, and private methods transform is not:
// Input
class C {
#privateMethod() {}
static prop = this.#privateMethod;
}// Output
var _C;
class C {
#privateMethod() {}
}
_C = C;
_defineProperty(C, "prop", _C.#privateMethod);We now have _C.#privateMethod outside the class, which is a syntax error.
We could fix that and only transform methods which need to be transformed. But it'd be very slow because you don't know if a method needs to be transformed or not until you find a reference to it in a static prop, and then you'd need to re-visit the whole class again to find any other references to this private method and transform them too.
So I suggest that we enable private methods + #prop in object transforms automatically if class properties transform is enabled.
Testing
To make Babel's tests for the 2 new transforms work, change the update_fixtures.mjs script to add class-properties to plugins if private-methods or private-property-in-object plugin is there.
Implementation: #prop in object
This is much easier than private methods, but it depends on private methods, because #method in object is also valid. So in one way it makes more sense to do private methods first. BUT, on other hand, doing this transform first would be a good way to explore all the parts of the transform, and get a feel for it.
I think the existing structures PrivateProp and ClassBindings contain all the info needed for this transform, without any alteration.
Implementation: Private methods
I think best to build this inside the class properties transform. It can use the same data structures.
#8042 adds an is_method property to PrivateProp, and records private methods in ClassDetails::private_props hash map, same as private properties:
| pub(super) struct PrivateProp<'a> { | |
| pub binding: BoundIdentifier<'a>, | |
| pub is_static: bool, | |
| pub is_method: bool, | |
| pub is_accessor: bool, | |
| } |
Transforming this.#method
object.#method needs to be transformed anywhere where ClassesStack::find_private_prop is called. Currently there's always an if is_method { return; } line after that call. e.g.:
| let ResolvedPrivateProp { | |
| prop_binding, | |
| class_bindings, | |
| is_static, | |
| is_method, | |
| is_accessor, | |
| is_declaration, | |
| } = self.classes_stack.find_private_prop(&field_expr.field); | |
| if is_method || is_accessor { | |
| return None; | |
| }; |
That's where transform of this.#method would slot in.
"Brand" temp var
If class has any instance private methods (not static), then an extra temp var is created var _Class_brand = new WeakSet();. There is 1 "brand" temp var per class, not 1 per method. Then transpiled #object.method uses this var.
I suggest:
- Record the binding in
ClassBindingsas abrand: Option<BoundIdentifier<'a>>property, along withnameandtemp.
| pub(super) struct ClassBindings<'a> { | |
| /// Binding for class name, if class has name | |
| pub name: Option<BoundIdentifier<'a>>, | |
| /// Temp var for class. | |
| /// e.g. `_Class` in `_Class = class {}, _Class.x = 1, _Class` | |
| pub temp: Option<BoundIdentifier<'a>>, | |
| /// `ScopeId` of hoist scope outside class (which temp `var` binding would be created in) | |
| pub outer_hoist_scope_id: ScopeId, | |
| /// `true` if should use temp binding for references to class in transpiled static private fields, | |
| /// `false` if can use name binding | |
| pub static_private_fields_use_temp: bool, | |
| /// `true` if temp var for class has been inserted | |
| pub temp_var_is_created: bool, | |
| } |
- Generate the binding in entry phase of transform (
ClassProperties::transform_class_body_on_entry). - Insert
_classPrivateMethodInitSpec(this, _Class_brand);into class constructor in entry phase, by adding it toinstance_inits:
oxc/crates/oxc_transformer/src/es2022/class_properties/class.rs
Lines 252 to 274 in c46793e
| let mut instance_inits = Vec::with_capacity(instance_prop_count); | |
| let mut constructor = None; | |
| for element in body.body.iter_mut() { | |
| #[expect(clippy::match_same_arms)] | |
| match element { | |
| ClassElement::PropertyDefinition(prop) => { | |
| if !prop.r#static { | |
| self.convert_instance_property(prop, &mut instance_inits, ctx); | |
| } | |
| } | |
| ClassElement::MethodDefinition(method) => { | |
| if method.kind == MethodDefinitionKind::Constructor | |
| && method.value.body.is_some() | |
| { | |
| constructor = Some(method.value.as_mut()); | |
| } | |
| } | |
| ClassElement::AccessorProperty(_) | ClassElement::TSIndexSignature(_) => { | |
| // TODO: Need to handle these? | |
| } | |
| ClassElement::StaticBlock(_) => {} | |
| } | |
| } |
- Insert the
varstatement for it in exit phase of the transform. This is in 2 places (transform_class_declaration_on_exit_implandtransform_class_expression_on_exit_impl). - To match Babel's output (order of where
var _Class_brand = ...;is inserted), will need to insert it in same loop as where temp vars for private properties are inserted. When you find the firstPrivatePropwithis_method == true, insertvar _Class_brandthen.
Transforming methods themselves
Don't do anything to private methods in entry phase of transform. Do it all in exit phase (ClassProperties::transform_class_elements), same as for static properties + static blocks. That will allow other transforms to run on the method's body before it's moved to outside of the class.
The body of the method needs to be transformed as well to replace references to class name with temp var, and transform super:
// Input
class C {
#method() {
return [this, C, super.prop];
}
}// Output
var _C_brand = /*#__PURE__*/ new WeakSet();
class C {
constructor() {
_classPrivateMethodInitSpec(this, _C_brand);
}
}
_C = C;
function _method() {
return [this, _C, _superPropGet(_C.prototype, "prop", this)];
}The _method function can just be moved, and its parent scope ID updated. If the context outside the class is not strict mode, then all scopes within the method need to have ScopeFlags::StrictMode flag removed.
Luckily, this is all extremely similar to the transform that happens on static property initializers, using StaticVisitor.
The differences are:
thisdoes not need to be transformed.- Scopes within method body (e.g. nested blocks or functions) do not need their parent scope updated (because they remain within the same function).
super.propis transformed slightly differently if it's an instance method.
Could either:
- Copy-and-paste
StaticVisitorand amend it. or - Alter
StaticVisitorto also cover private methods.
Eventually, we should move all this into the main visitor, to remove these extra visits. But I strongly suggest not to do that initially. It will be challenging to track all the required state. Will be easier to do once tests are passing.
Summary
There's quite a lot to this transform, but on positive side, it mostly follows same patterns as we already have in class properties transform.
And if any consolation, at least no need to deal with computed keys for private methods!