perf(transformer/class-properties): do not re-generate same method key

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

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`