diff --git a/crates/oxc_transformer/src/es2022/class_properties/computed_key.rs b/crates/oxc_transformer/src/es2022/class_properties/computed_key.rs index 958ad310e..3a088e7e9 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/computed_key.rs +++ b/crates/oxc_transformer/src/es2022/class_properties/computed_key.rs @@ -16,21 +16,32 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { method: &mut MethodDefinition<'a>, ctx: &mut TraverseCtx<'a>, ) { - // TODO: Don't alter numeric literal key e.g. `class C { 123() {} }` - // TODO: Don't re-create key if it doesn't need to be altered - let Some(key) = method.key.as_expression_mut() else { return }; + // Exit if key is not an `Expression` + // (`PropertyKey::StaticIdentifier` or `PropertyKey::PrivateIdentifier`) + let Some(key) = method.key.as_expression_mut() else { + return; + }; + + // Exit if evaluating key cannot have side effects. + // This check also results in exit for non-computed keys e.g. `class C { 'x'() {} 123() {} }`. + if !key_needs_temp_var(key, ctx) { + return; + } - // TODO: Don't alter key if it's provable evaluating it has no side effects. // TODO(improve-on-babel): It's unnecessary to create temp vars for method keys unless: // 1. Properties also have computed keys. // 2. Some of those properties' computed keys have side effects and require temp vars. // 3. At least one property satisfying the above is after this method, // or class contains a static block which is being transformed // (static blocks are always evaluated after computed keys, regardless of order) - method.key = PropertyKey::from(self.create_computed_key_temp_var(key, ctx)); + let key = ctx.ast.move_expression(key); + let temp_var = self.create_computed_key_temp_var(key, ctx); + method.key = PropertyKey::from(temp_var); } - /// Convert computed property/method key to a temp var. + /// Convert computed property/method key to a temp var, if a temp var is required. + /// + /// If no temp var is required, take ownership of key, and return it. /// /// Transformation is: /// * Class declaration: @@ -40,56 +51,31 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { /// /// This function: /// * Creates the `let _x;` statement and inserts it. - /// * Creates the `_x = x()` assignments. - /// * Inserts assignments before class declaration, or adds to `state` if class expression. + /// * Creates the `_x = x()` assignment. + /// * Inserts assignment before class. /// * Returns `_x`. - pub(super) fn create_computed_key_temp_var( + pub(super) fn create_computed_key_temp_var_if_required( &mut self, key: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>, ) -> Expression<'a> { let key = ctx.ast.move_expression(key); - - // Bound vars and literals do not need temp var - return unchanged. - // e.g. `let x = 'x'; class C { [x] = 1; }` or `class C { ['x'] = 1; }` - // - // `this` does not have side effects, but it needs a temp var anyway, because `this` in computed - // key and `this` within class constructor resolve to different `this` bindings. - // So we need to create a temp var outside of the class to get the correct `this`. - // `class C { [this] = 1; }` - // -> `let _this; _this = this; class C { constructor() { this[_this] = 1; } }` - // - // TODO(improve-on-babel): Can avoid the temp var if key is for a static prop/method, - // as in that case the usage of `this` stays outside the class. - // - // TODO: Do fuller analysis to detect expressions which cannot have side effects e.g. `'x' + 'y'`. - let cannot_have_side_effects = match &key { - Expression::BooleanLiteral(_) - | Expression::NullLiteral(_) - | Expression::NumericLiteral(_) - | Expression::BigIntLiteral(_) - | Expression::RegExpLiteral(_) - | Expression::StringLiteral(_) => true, - Expression::Identifier(ident) => { - // Cannot have side effects if is bound. - // Check that the var is not mutated is required for cases like - // `let x = 1; class { [x] = 1; [++x] = 2; }` - // `++x` is hoisted to before class in output, so `x` in 1st key would get the wrong - // value unless it's hoisted out too. - // TODO: Add an exec test for this odd case. - // TODO(improve-on-babel): That case is rare. - // Test for it in first pass over class elements, and avoid temp vars where possible. - match ctx.symbols().get_reference(ident.reference_id()).symbol_id() { - Some(symbol_id) => !ctx.symbols().symbol_is_mutated(symbol_id), - None => false, - } - } - _ => false, - }; - if cannot_have_side_effects { - return key; + if key_needs_temp_var(&key, ctx) { + self.create_computed_key_temp_var(key, ctx) + } else { + key } + } + /// * Create `let _x;` statement and insert it. + /// * Create `_x = x()` assignment. + /// * Insert assignment before class. + /// * Return `_x`. + fn create_computed_key_temp_var( + &mut self, + key: Expression<'a>, + ctx: &mut TraverseCtx<'a>, + ) -> Expression<'a> { // We entered transform via `enter_expression` or `enter_statement`, // so `ctx.current_scope_id()` is the scope outside the class let parent_scope_id = ctx.current_scope_id(); @@ -105,3 +91,46 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { binding.create_read_expression(ctx) } } + +/// Check if temp var is required for `key`. +/// +/// `this` does not have side effects, but in this context, it needs a temp var anyway, because `this` +/// in computed key and `this` within class constructor resolve to different `this` bindings. +/// So we need to create a temp var outside of the class to get the correct `this`. +/// `class C { [this] = 1; }` +/// -> `let _this; _this = this; class C { constructor() { this[_this] = 1; } }` +// +// TODO(improve-on-babel): Can avoid the temp var if key is for a static prop/method, +// as in that case the usage of `this` stays outside the class. +fn key_needs_temp_var(key: &Expression, ctx: &TraverseCtx) -> bool { + match key { + // Literals cannot have side effects. + // e.g. `let x = 'x'; class C { [x] = 1; }` or `class C { ['x'] = 1; }`. + Expression::BooleanLiteral(_) + | Expression::NullLiteral(_) + | Expression::NumericLiteral(_) + | Expression::BigIntLiteral(_) + | Expression::RegExpLiteral(_) + | Expression::StringLiteral(_) => false, + // `IdentifierReference`s can have side effects if is unbound. + // + // If var is mutated, it also needs a temp var, because of cases like + // `let x = 1; class { [x] = 1; [++x] = 2; }` + // `++x` is hoisted to before class in output, so `x` in 1st key would get the wrong value + // unless it's hoisted out too. + // + // TODO: Add an exec test for this odd case. + // TODO(improve-on-babel): That case is rare. + // Test for it in first pass over class elements, and avoid temp vars where possible. + Expression::Identifier(ident) => { + match ctx.symbols().get_reference(ident.reference_id()).symbol_id() { + Some(symbol_id) => ctx.symbols().symbol_is_mutated(symbol_id), + None => true, + } + } + // Treat any other expression as possibly having side effects e.g. `foo()`. + // TODO: Do fuller analysis to detect expressions which cannot have side effects. + // e.g. `"x" + "y"`. + _ => true, + } +} diff --git a/crates/oxc_transformer/src/es2022/class_properties/prop_decl.rs b/crates/oxc_transformer/src/es2022/class_properties/prop_decl.rs index a90b36794..bf359315d 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/prop_decl.rs +++ b/crates/oxc_transformer/src/es2022/class_properties/prop_decl.rs @@ -242,8 +242,12 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { return self.create_init_assignment_not_loose(prop, value, assignee, ctx); } key @ match_expression!(PropertyKey) => { - // TODO: This can also be a numeric key (non-computed). Maybe other key types? - let key = self.create_computed_key_temp_var(key.to_expression_mut(), ctx); + let key = key.to_expression_mut(); + // Note: Key can also be static `StringLiteral` or `NumericLiteral`. + // `class C { 'x' = true; 123 = false; }` + // No temp var is created for these. + // TODO: Any other possible static key types? + let key = self.create_computed_key_temp_var_if_required(key, ctx); ctx.ast.member_expression_computed(SPAN, assignee, key, false) } PropertyKey::PrivateIdentifier(_) => { @@ -274,8 +278,12 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { ctx.ast.expression_string_literal(ident.span, ident.name.clone(), None) } key @ match_expression!(PropertyKey) => { - // TODO: This can also be a numeric key (non-computed). Maybe other key types? - self.create_computed_key_temp_var(key.to_expression_mut(), ctx) + let key = key.to_expression_mut(); + // Note: Key can also be static `StringLiteral` or `NumericLiteral`. + // `class C { 'x' = true; 123 = false; }` + // No temp var is created for these. + // TODO: Any other possible static key types? + self.create_computed_key_temp_var_if_required(key, ctx) } PropertyKey::PrivateIdentifier(_) => { // Handled in `convert_instance_property` and `convert_static_property`