refactor(transformer/class-properties): simplify logic for when to create temp binding (#7977)

Remove the hack of overwriting temp binding with name binding before traversing class body. Instead decide which binding to use in `ClassBindings::get_or_init_static_binding` based on a flag.

This less performant than what we had before. But it simplifies some confusing logic, and prepares the ground for changes to come where we'll need to duck in and out of static context repeatedly while traversing the class body.

Author: overlookmotel

crates/oxc_transformer/src/es2022/class_properties/class.rs

@@ -257,17 +257,20 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
                 .insert_many_after(&stmt_address, self.insert_after_stmts.drain(..));
         }
 
-        // Update class bindings prior to traversing class body and insertion of statements/expressions
-        // before/after the class. See comments on `ClassBindings`.
+        // Flag that static private fields should be transpiled using name binding,
+        // while traversing class body.
+        //
         // Static private fields reference class name (not temp var) in class declarations.
         // `class Class { static #prop; method() { return obj.#prop; } }`
         // -> `method() { return _assertClassBrand(Class, obj, _prop)._; }`
         // (note `Class` in `_assertClassBrand(Class, ...)`, not `_Class`)
-        // So set "temp" binding to actual class name while visiting class body.
+        //
+        // Also see comments on `ClassBindings`.
+        //
         // Note: If declaration is `export default class {}` with no name, and class has static props,
         // then class has had name binding created already in `transform_class`.
         // So name binding is always `Some`.
-        class_details.bindings.temp = class_details.bindings.name.clone();
+        class_details.bindings.static_private_fields_use_temp = false;
     }
 
     /// `_classPrivateFieldLooseKey("prop")`

crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs

@@ -22,31 +22,29 @@ use oxc_traverse::{BoundIdentifier, TraverseCtx};
 /// is unfortunately rather complicated.
 ///
 /// Transpiled private fields referring to a static private prop use:
-/// * Class name when field is within class body and class has a name
-///   e.g. `class C { static #x; method() { return obj.#x; } }`
-/// * Temp var when field is within class body and class has no name
-///   e.g. `C = class { static #x; method() { return obj.#x; } }`
-/// * Temp var when field is within a static prop initializer.
-///   e.g. `class C { static #x; y = obj.#x; }`
-///
-/// To cover all these cases, the meaning of `temp` binding here changes while traversing the class body.
-/// [`ClassProperties::transform_class_declaration`] sets `temp` binding to be a copy of the
-/// `name` binding before that traversal begins. So the name `temp` is misleading at that point.
 ///
-/// Debug assertions are used to make sure this complex logic is correct.
+/// * Class name when field is within body of class declaration
+///   e.g. `class C { static #x; method() { return obj.#x; } }`
+///   -> `_assertClassBrand(C, obj, _x)._`
+/// * Temp var when field is within body of class expression
+///   e.g. `C = class C { static #x; method() { return obj.#x; } }`
+///   -> `_assertClassBrand(_C, obj, _x)._`
+/// * Temp var when field is within a static prop initializer
+///   e.g. `class C { static #x; static y = obj.#x; }`
+///   -> `_assertClassBrand(_C, obj, _x)._`
 ///
-/// [`ClassProperties::transform_class_declaration`]: super::ClassProperties::transform_class_declaration
+/// `static_private_fields_use_temp` is updated as transform moves through the class,
+/// to indicate which binding to use.
 #[derive(Default, Clone)]
 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>>,
-    /// `true` if currently transforming static property initializers.
-    /// Only used in debug builds to check logic is correct.
-    #[cfg(debug_assertions)]
-    pub currently_transforming_static_property_initializers: bool,
+    /// `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,
 }
 
 impl<'a> ClassBindings<'a> {
@@ -55,37 +53,43 @@ impl<'a> ClassBindings<'a> {
         name_binding: Option<BoundIdentifier<'a>>,
         temp_binding: Option<BoundIdentifier<'a>>,
     ) -> Self {
-        Self {
-            name: name_binding,
-            temp: temp_binding,
-            #[cfg(debug_assertions)]
-            currently_transforming_static_property_initializers: false,
-        }
+        Self { name: name_binding, temp: temp_binding, static_private_fields_use_temp: true }
     }
 
     /// Get `SymbolId` of name binding.
     pub fn name_symbol_id(&self) -> Option<SymbolId> {
         self.name.as_ref().map(|binding| binding.symbol_id)
     }
 
-    /// Create a binding for temp var, if there isn't one already.
-    pub fn get_or_init_temp_binding(&mut self, ctx: &mut TraverseCtx<'a>) -> &BoundIdentifier<'a> {
-        if self.temp.is_none() {
-            // This should only be possible if we are currently transforming static prop initializers
-            #[cfg(debug_assertions)]
-            {
-                assert!(
-                    self.currently_transforming_static_property_initializers,
-                    "Should be unreachable"
-                );
-            }
-
-            self.temp = Some(Self::create_temp_binding(self.name.as_ref(), ctx));
+    /// Get binding to use for referring to class in transpiled static private fields.
+    ///
+    /// e.g. `Class` in `_assertClassBrand(Class, object, _prop)._` (class name)
+    /// or `_Class` in `_assertClassBrand(_Class, object, _prop)._` (temp var)
+    ///
+    /// * In class expressions, this is always be temp binding.
+    /// * In class declarations, it's the name binding when code is inside class body,
+    ///   and temp binding when code is outside class body.
+    ///
+    /// `static_private_fields_use_temp` is set accordingly at the right moments
+    /// elsewhere in this transform.
+    ///
+    /// If a temp binding is required, and one doesn't already exist, a temp binding is created.
+    pub fn get_or_init_static_binding(
+        &mut self,
+        ctx: &mut TraverseCtx<'a>,
+    ) -> &BoundIdentifier<'a> {
+        if self.static_private_fields_use_temp {
+            // Create temp binding if doesn't already exist
+            self.temp.get_or_insert_with(|| Self::create_temp_binding(self.name.as_ref(), ctx))
+        } else {
+            // `static_private_fields_use_temp` is always `true` for class expressions.
+            // Class declarations always have a name binding if they have any static props.
+            // So `unwrap` here cannot panic.
+            self.name.as_ref().unwrap()
         }
-        self.temp.as_ref().unwrap()
     }
 
-    /// Generate a binding for temp var.
+    /// Generate binding for temp var.
     pub fn create_temp_binding(
         name_binding: Option<&BoundIdentifier<'a>>,
         ctx: &mut TraverseCtx<'a>,

crates/oxc_transformer/src/es2022/class_properties/private_field.rs

@@ -88,7 +88,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
                 Self::create_underscore_member_expression(prop_ident, span, ctx)
             } else {
                 // `_assertClassBrand(Class, object, _prop)._`
-                let class_binding = class_bindings.get_or_init_temp_binding(ctx);
+                let class_binding = class_bindings.get_or_init_static_binding(ctx);
                 let class_ident = class_binding.create_read_expression(ctx);
 
                 self.create_assert_class_brand_underscore(
@@ -282,7 +282,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
                     Self::create_underscore_member_expression(prop_ident, field_expr.span, ctx);
                 (callee, object)
             } else {
-                let class_binding = class_bindings.get_or_init_temp_binding(ctx);
+                let class_binding = class_bindings.get_or_init_static_binding(ctx);
                 let class_ident = class_binding.create_read_expression(ctx);
 
                 // Make 2 copies of `object`
@@ -383,7 +383,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
             // for shortcut/no shortcut and do the "can we shortcut?" check here.
             // Then only create temp var for the "no shortcut" branch, and clone the resulting binding
             // before passing it to the "no shortcut" method. What a palaver!
-            let class_binding = class_bindings.get_or_init_temp_binding(ctx);
+            let class_binding = class_bindings.get_or_init_static_binding(ctx);
             let class_binding = class_binding.clone();
             let class_symbol_id = class_bindings.name_symbol_id();
 
@@ -792,7 +792,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
                 let get_expr = Self::create_underscore_member_expression(prop_ident, SPAN, ctx);
                 (get_expr, object, None)
             } else {
-                let class_binding = class_bindings.get_or_init_temp_binding(ctx);
+                let class_binding = class_bindings.get_or_init_static_binding(ctx);
                 let class_ident = class_binding.create_read_expression(ctx);
                 let class_ident2 = class_binding.create_read_expression(ctx);
 

crates/oxc_transformer/src/es2022/class_properties/static_prop_init.rs

@@ -22,30 +22,8 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
         value: &mut Expression<'a>,
         ctx: &mut TraverseCtx<'a>,
     ) {
-        self.set_is_transforming_static_property_initializers(true);
-
         let mut replacer = StaticInitializerVisitor::new(self, ctx);
         replacer.visit_expression(value);
-
-        self.set_is_transforming_static_property_initializers(false);
-    }
-
-    /// Set flag on `ClassBindings` that we are/are not currently transforming static prop initializers.
-    ///
-    /// The logic around which bindings are used for transforming private fields is complex,
-    /// so we use this to make sure the logic is correct.
-    ///
-    /// In debug builds, `ClassBindings::get_or_init_temp_binding` will panic if we end up transforming
-    /// a static private field, and there's no `temp` binding - which should be impossible.
-    #[inline(always)] // `#[inline(always)]` because is no-op in release builds
-    #[allow(clippy::inline_always)]
-    #[cfg_attr(not(debug_assertions), expect(unused_variables, clippy::unused_self))]
-    fn set_is_transforming_static_property_initializers(&mut self, is_it: bool) {
-        #[cfg(debug_assertions)]
-        {
-            let class_details = self.current_class_mut();
-            class_details.bindings.currently_transforming_static_property_initializers = is_it;
-        }
     }
 }
 
@@ -470,7 +448,7 @@ impl<'a, 'ctx, 'v> StaticInitializerVisitor<'a, 'ctx, 'v> {
     fn replace_this_with_temp_var(&mut self, expr: &mut Expression<'a>, span: Span) {
         if self.this_depth == 0 {
             let class_details = self.class_properties.current_class_mut();
-            let temp_binding = class_details.bindings.get_or_init_temp_binding(self.ctx);
+            let temp_binding = class_details.bindings.get_or_init_static_binding(self.ctx);
             *expr = temp_binding.create_spanned_read_expression(span, self.ctx);
         }
     }
@@ -491,7 +469,7 @@ impl<'a, 'ctx, 'v> StaticInitializerVisitor<'a, 'ctx, 'v> {
         }
 
         // Identifier is reference to class name. Rename it.
-        let temp_binding = class_details.bindings.get_or_init_temp_binding(self.ctx);
+        let temp_binding = class_details.bindings.get_or_init_static_binding(self.ctx);
         ident.name = temp_binding.name.clone();
 
         let symbols = self.ctx.symbols_mut();

crates/oxc_transformer/src/es2022/class_properties/supers.rs

@@ -137,7 +137,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
         is_callee: bool,
         ctx: &mut TraverseCtx<'a>,
     ) -> Expression<'a> {
-        let temp_binding = self.current_class_mut().bindings.get_or_init_temp_binding(ctx);
+        let temp_binding = self.current_class_mut().bindings.get_or_init_static_binding(ctx);
 
         let ident1 = Argument::from(temp_binding.create_read_expression(ctx));
         let ident2 = Argument::from(temp_binding.create_read_expression(ctx));