-
-
Notifications
You must be signed in to change notification settings - Fork 640
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
ClassBindings
as abrand: Option<BoundIdentifier<'a>>
property, along withname
andtemp
.
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
var
statement for it in exit phase of the transform. This is in 2 places (transform_class_declaration_on_exit_impl
andtransform_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 firstPrivateProp
withis_method == true
, insertvar _Class_brand
then.
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:
this
does 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.prop
is transformed slightly differently if it's an instance method.
Could either:
- Copy-and-paste
StaticVisitor
and amend it. or - Alter
StaticVisitor
to 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!