Skip to content
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

Implement remaining class transforms #8052

Open
overlookmotel opened this issue Dec 20, 2024 · 0 comments
Open

Implement remaining class transforms #8052

overlookmotel opened this issue Dec 20, 2024 · 0 comments
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Dec 20, 2024

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 a brand: Option<BoundIdentifier<'a>> property, along with name and temp.

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,
}

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 and transform_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 first PrivateProp with is_method == true, insert var _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:

  1. Copy-and-paste StaticVisitor and amend it. or
  2. 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!

@overlookmotel overlookmotel added C-enhancement Category - New feature or request A-transformer Area - Transformer / Transpiler labels Dec 20, 2024
@Dunqing Dunqing self-assigned this Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants