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

feat(transformer/class-properties): transform super assignment expressions that are inside static prop initializer #7959

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/oxc_ast/src/ast_impl/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1515,3 +1515,9 @@ impl ImportPhase {
}
}
}

impl<'a> From<IdentifierName<'a>> for StringLiteral<'a> {
fn from(ident: IdentifierName<'a>) -> Self {
Self { span: ident.span, value: ident.name.clone(), raw: None }
}
}
2 changes: 2 additions & 0 deletions crates/oxc_transformer/src/common/helper_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ pub enum Helper {
ClassPrivateFieldLooseKey,
ClassPrivateFieldLooseBase,
SuperPropGet,
SuperPropSet,
}

impl Helper {
Expand All @@ -183,6 +184,7 @@ impl Helper {
Self::ClassPrivateFieldLooseKey => "classPrivateFieldLooseKey",
Self::ClassPrivateFieldLooseBase => "classPrivateFieldLooseBase",
Self::SuperPropGet => "superPropGet",
Self::SuperPropSet => "superPropSet",
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,16 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
let AssignmentExpression { span, operator, right: value, left } = assign_expr;
let object = match left {
AssignmentTarget::PrivateFieldExpression(field_expr) => field_expr.unbox().object,
_ => unreachable!(),
// For super assignment expression
AssignmentTarget::StaticMemberExpression(member_expr) => Expression::StringLiteral(
ctx.ast.alloc(StringLiteral::from(member_expr.property.clone())),
),
AssignmentTarget::ComputedMemberExpression(member_expr) => {
member_expr.unbox().expression
}
_ => {
unreachable!();
}
};

if operator == AssignmentOperator::Assign {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ impl<'a, 'ctx, 'v> VisitMut<'a> for StaticInitializerVisitor<'a, 'ctx, 'v> {
// `object.#prop = value`, `object.#prop += value`, `object.#prop ??= value` etc
Expression::AssignmentExpression(_) => {
self.class_properties.transform_assignment_expression(expr, self.ctx);
self.class_properties.transform_super_assignment_expression(expr, self.ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the cause of the panic in conformance. Problem is that transform_assignment_expression sometimes mutates expr so it's not an AssignmentExpression any more. So then transform_super_assignment_expression enters unreachable!().

So we need to check that it's still an AssignmentExpression:

Suggested change
self.class_properties.transform_super_assignment_expression(expr, self.ctx);
if matches!(expr, Expression::AssignmentExpression(_)) {
self.class_properties.transform_super_assignment_expression(expr, self.ctx);
}

Having to do this check again is inefficient, but don't worry - this visitor will be removed anyway in my forthcoming refactoring, so we can change it then.

}
// `object.#prop++`, `--object.#prop`
Expression::UpdateExpression(_) => {
Expand Down
93 changes: 90 additions & 3 deletions crates/oxc_transformer/src/es2022/class_properties/supers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
is_callee: bool,
ctx: &mut TraverseCtx<'a>,
) -> Expression<'a> {
let property = &member.property;
let property =
ctx.ast.expression_string_literal(property.span, property.name.clone(), None);
let property = Expression::StringLiteral(ctx.ast.alloc(member.property.clone().into()));
Copy link
Contributor

@overlookmotel overlookmotel Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shorter, but I'm not really a fan of impl From<IdentifierName> for StringLiteral.

Reason is: You're dealing with an owned type on the stack. Usually, it's better to allocate nodes into the arena as quickly as possible, and then only hold smaller types like:

  • Box<T> (8 bytes)
  • References &T or &mut T (8 bytes)
  • Enum types e.g. Expression, Statement (16 bytes)
  • Atom (16 bytes)

Generally, if you find yourself having to use ctx.ast.alloc, you're not in an optimal situation, because it means you have a large type on the stack.

In the original version, you only had to clone the Atom (which is essentially a Copy), whereas in this new version you have to clone the whole IdentifierName to be able to pass it to from.

In this case, it's not a big deal, because IdentifierName is only 24 bytes, but in general in my opinion it's not the best pattern.

If you wanted to, you could implement this conversion instead as a method on AstBuilder - AstBuilder:: expression_string_literal_from_identifier_name. But personally I think that'd be overkill unless we need to do that conversion in many places.

self.create_super_prop_get(member.span, property, is_callee, ctx)
}

Expand Down Expand Up @@ -125,6 +123,71 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
arguments.push(Argument::from(array));
}

/// Transform assignment expression where left-hand side contains `super`.
///
/// * `object.#prop = value`
/// -> `_superPropSet(_Class, prop, value, _Class)`
/// * `object.#prop += value`
/// -> `_superPropSet(_Class, prop, _superPropGet(_Class, prop, _Class) + value, _Class)`
/// * `object.#prop &&= value`
/// -> `_superPropGet(_Class, prop, _Class) && _superPropSet(_Class, prop, value, _Class)`
//
// `#[inline]` so that compiler sees that `expr` is an `Expression::AssignmentExpression`
pub(super) fn transform_super_assignment_expression(
&mut self,
expr: &mut Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) {
let Expression::AssignmentExpression(assign_expr) = expr else { unreachable!() };
match &assign_expr.left {
AssignmentTarget::StaticMemberExpression(member) if member.object.is_super() => {
self.transform_assignment_expression_for_super_static_member_expr(expr, ctx);
}
AssignmentTarget::ComputedMemberExpression(member) if member.object.is_super() => {
self.transform_assignment_expression_for_super_computed_member_expr(expr, ctx);
}
_ => {}
};
}

fn transform_assignment_expression_for_super_static_member_expr(
&mut self,
expr: &mut Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) {
let getter =
|s: &mut Self, object: Expression<'a>, span: Span, ctx: &mut TraverseCtx<'a>| {
s.create_super_prop_get(span, object, false, ctx)
};
let setter = |s: &mut Self,
object: Expression<'a>,
value: Expression<'a>,
span: Span,
ctx: &mut TraverseCtx<'a>| {
s.create_super_prop_set(span, object, value, ctx)
};
self.transform_instance_assignment_expression(expr, getter, setter, ctx);
}

fn transform_assignment_expression_for_super_computed_member_expr(
&mut self,
expr: &mut Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) {
let getter =
|s: &mut Self, object: Expression<'a>, span: Span, ctx: &mut TraverseCtx<'a>| {
s.create_super_prop_get(span, object, false, ctx)
};
let setter = |s: &mut Self,
object: Expression<'a>,
value: Expression<'a>,
span: Span,
ctx: &mut TraverseCtx<'a>| {
s.create_super_prop_set(span, object, value, ctx)
};
self.transform_instance_assignment_expression(expr, getter, setter, ctx);
}

/// Member:
/// `_superPropGet(_Class, prop, _Class)`
///
Expand Down Expand Up @@ -155,4 +218,28 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
// `_superPropGet(_Class, prop, _Class)` or `_superPropGet(_Class, prop, _Class, 2)`
self.ctx.helper_call_expr(Helper::SuperPropGet, span, arguments, ctx)
}

/// `_superPropSet(_Class, prop, value, _Class)`
fn create_super_prop_set(
&mut self,
span: Span,
property: Expression<'a>,
value: Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Expression<'a> {
let temp_binding = self.current_class_mut().bindings.get_or_init_temp_binding(ctx);
let arguments = ctx.ast.vec_from_array([
Argument::from(temp_binding.create_read_expression(ctx)),
Argument::from(property),
Argument::from(value),
Argument::from(temp_binding.create_read_expression(ctx)),
Argument::from(ctx.ast.expression_numeric_literal(
SPAN,
1.0,
None,
NumberBase::Decimal,
)),
]);
self.ctx.helper_call_expr(Helper::SuperPropSet, span, arguments, ctx)
}
}
4 changes: 2 additions & 2 deletions tasks/transform_conformance/snapshots/oxc.snap.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
commit: 54a8389f

Passed: 111/125
Passed: 113/127

# All Passed:
* babel-plugin-transform-class-static-block
Expand All @@ -16,7 +16,7 @@ Passed: 111/125
* regexp


# babel-plugin-transform-class-properties (12/14)
# babel-plugin-transform-class-properties (14/16)
* typescript/optional-call/input.ts
Symbol reference IDs mismatch for "X":
after transform: SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), ReferenceId(11), ReferenceId(16)]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const ident = "A";
class Outer {
static A = 0;
static B = () => {
super.A += 1;
super.A -= 1;
super.A &&= 1;
super.A ||= 1;
super.A = 1;

super[ident] += 1;
super[ident] -= 1;
super[ident] &&= 1;
super[ident] ||= 1;
super[ident] = 1;
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
var _Outer;
const ident = "A";
class Outer {}
_Outer = Outer;
babelHelpers.defineProperty(Outer, "A", 0);
babelHelpers.defineProperty(Outer, "B", () => {
babelHelpers.superPropSet(_Outer, "A", babelHelpers.superPropGet(_Outer, "A", _Outer) + 1, _Outer, 1);
babelHelpers.superPropSet(_Outer, "A", babelHelpers.superPropGet(_Outer, "A", _Outer) - 1, _Outer, 1);
babelHelpers.superPropGet(_Outer, "A", _Outer) &&
babelHelpers.superPropSet(_Outer, "A", 1, _Outer, 1);
babelHelpers.superPropGet(_Outer, "A", _Outer) ||
babelHelpers.superPropSet(_Outer, "A", 1, _Outer, 1);
babelHelpers.superPropSet(_Outer, "A", 1, _Outer, 1);
babelHelpers.superPropSet(
_Outer,
ident,
babelHelpers.superPropGet(_Outer, ident, _Outer) + 1,
_Outer,
1,
);
babelHelpers.superPropSet(
_Outer,
ident,
babelHelpers.superPropGet(_Outer, ident, _Outer) - 1,
_Outer,
1,
);
babelHelpers.superPropGet(_Outer, ident, _Outer) &&
babelHelpers.superPropSet(_Outer, ident, 1, _Outer, 1);
babelHelpers.superPropGet(_Outer, ident, _Outer) ||
babelHelpers.superPropSet(_Outer, ident, 1, _Outer, 1);
babelHelpers.superPropSet(_Outer, ident, 1, _Outer, 1);
});
Loading