From 8eedf879838f890d3839a0f394264e79c308bd6c Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Wed, 18 Dec 2024 02:44:41 +0000 Subject: [PATCH] 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. --- .../src/es2022/class_properties/class.rs | 11 ++- .../es2022/class_properties/class_bindings.rs | 78 ++++++++++--------- .../es2022/class_properties/private_field.rs | 8 +- .../class_properties/static_prop_init.rs | 26 +------ .../src/es2022/class_properties/supers.rs | 2 +- 5 files changed, 55 insertions(+), 70 deletions(-) diff --git a/crates/oxc_transformer/src/es2022/class_properties/class.rs b/crates/oxc_transformer/src/es2022/class_properties/class.rs index 242f7cad3d0cbe..50d1bbfeb9afd4 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/class.rs +++ b/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")` diff --git a/crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs b/crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs index efaca62c690ad0..01bfa32a216067 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs +++ b/crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs @@ -22,20 +22,19 @@ 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 @@ -43,10 +42,9 @@ pub(super) struct ClassBindings<'a> { /// Temp var for class. /// e.g. `_Class` in `_Class = class {}, _Class.x = 1, _Class` pub temp: Option>, - /// `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,12 +53,7 @@ impl<'a> ClassBindings<'a> { name_binding: Option>, temp_binding: Option>, ) -> 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. @@ -68,24 +61,35 @@ impl<'a> ClassBindings<'a> { 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>, diff --git a/crates/oxc_transformer/src/es2022/class_properties/private_field.rs b/crates/oxc_transformer/src/es2022/class_properties/private_field.rs index 48fdd0e8c0eabb..cd2f257c8d780e 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/private_field.rs +++ b/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); diff --git a/crates/oxc_transformer/src/es2022/class_properties/static_prop_init.rs b/crates/oxc_transformer/src/es2022/class_properties/static_prop_init.rs index 5dce0179336d0c..77e835a3059084 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/static_prop_init.rs +++ b/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; - } } } @@ -471,7 +449,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); } } @@ -492,7 +470,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(); diff --git a/crates/oxc_transformer/src/es2022/class_properties/supers.rs b/crates/oxc_transformer/src/es2022/class_properties/supers.rs index 1616c6810d6bdd..cbeabac882dc42 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/supers.rs +++ b/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));