refactor(transformer/class-properties): streamline handling scope of instance property initializer (#10492)

Move the `instance_inits_scope_id` and `instance_inits_constructor_scope_id` fields from the `ClassProperties` struct to local variables.

Using struct-level fields as temporary variables is unnecessary within three function depths, and you also need to consider when to reset the states, otherwise, you may cause a bug due to using previous temporary information. Moreover, I think it also improves a bit readability.

NOTE: This is not only a refactor, but also along with #10495 and #10493 to prepare #10491.

Author: Dunqing

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

@@ -239,23 +239,33 @@ impl<'a> ClassProperties<'a, '_> {
             return;
         }
 
+        // Scope that instance property initializers will be inserted into.
+        // This is usually class constructor, but can also be a `_super` function which is created.
+        let instance_inits_scope_id;
+        // Scope of class constructor, if instance property initializers will be inserted into constructor.
+        // Used for checking for variable name clashes.
+        // e.g. `class C { prop = x(); constructor(x) {} }`
+        // - `x` in constructor needs to be renamed when `x()` is moved into constructor body.
+        // `None` if class has no existing constructor, as then there can't be any clashes.
+        let mut instance_inits_constructor_scope_id = None;
+
         // Determine where to insert instance property initializers in constructor
         let instance_inits_insert_location = if let Some(constructor) = constructor {
             // Existing constructor
             let constructor = constructor.value.as_mut();
             if has_super_class {
                 let (insert_scopes, insert_location) =
                     Self::replace_super_in_constructor(constructor, ctx);
-                self.instance_inits_scope_id = insert_scopes.insert_in_scope_id;
-                self.instance_inits_constructor_scope_id = insert_scopes.constructor_scope_id;
+                instance_inits_scope_id = insert_scopes.insert_in_scope_id;
+                instance_inits_constructor_scope_id = insert_scopes.constructor_scope_id;
                 insert_location
             } else {
                 let constructor_scope_id = constructor.scope_id();
-                self.instance_inits_scope_id = constructor_scope_id;
+                instance_inits_scope_id = constructor_scope_id;
                 // Only record `constructor_scope_id` if constructor's scope has some bindings.
                 // If it doesn't, no need to check for shadowed symbols in instance prop initializers,
                 // because no bindings to clash with.
-                self.instance_inits_constructor_scope_id =
+                instance_inits_constructor_scope_id =
                     if ctx.scoping().get_bindings(constructor_scope_id).is_empty() {
                         None
                     } else {
@@ -270,8 +280,7 @@ impl<'a> ClassProperties<'a, '_> {
                 NodeId::DUMMY,
                 ScopeFlags::Function | ScopeFlags::Constructor | ScopeFlags::StrictMode,
             );
-            self.instance_inits_scope_id = constructor_scope_id;
-            self.instance_inits_constructor_scope_id = None;
+            instance_inits_scope_id = constructor_scope_id;
             InstanceInitsInsertLocation::NewConstructor
         };
 
@@ -300,7 +309,13 @@ impl<'a> ClassProperties<'a, '_> {
             match element {
                 ClassElement::PropertyDefinition(prop) => {
                     if !prop.r#static {
-                        self.convert_instance_property(prop, &mut instance_inits, ctx);
+                        self.convert_instance_property(
+                            prop,
+                            &mut instance_inits,
+                            instance_inits_scope_id,
+                            instance_inits_constructor_scope_id,
+                            ctx,
+                        );
                     }
                 }
                 ClassElement::MethodDefinition(method) => {
@@ -321,7 +336,13 @@ impl<'a> ClassProperties<'a, '_> {
         // Create a constructor if there isn't one.
         match instance_inits_insert_location {
             InstanceInitsInsertLocation::NewConstructor => {
-                self.insert_constructor(body, instance_inits, has_super_class, ctx);
+                Self::insert_constructor(
+                    body,
+                    instance_inits,
+                    has_super_class,
+                    instance_inits_scope_id,
+                    ctx,
+                );
             }
             InstanceInitsInsertLocation::ExistingConstructor(stmt_index) => {
                 self.insert_inits_into_constructor_as_statements(
@@ -336,11 +357,17 @@ impl<'a> ClassProperties<'a, '_> {
                     constructor.as_mut().unwrap(),
                     instance_inits,
                     &super_binding,
+                    instance_inits_scope_id,
                     ctx,
                 );
             }
             InstanceInitsInsertLocation::SuperFnOutsideClass(super_binding) => {
-                self.create_super_function_outside_constructor(instance_inits, &super_binding, ctx);
+                self.create_super_function_outside_constructor(
+                    instance_inits,
+                    &super_binding,
+                    instance_inits_scope_id,
+                    ctx,
+                );
             }
         }
     }

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

@@ -215,18 +215,17 @@ impl<'a> ClassProperties<'a, '_> {
 
     /// Add a constructor to class containing property initializers.
     pub(super) fn insert_constructor(
-        &self,
         body: &mut ClassBody<'a>,
         inits: Vec<Expression<'a>>,
         has_super_class: bool,
+        constructor_scope_id: ScopeId,
         ctx: &mut TraverseCtx<'a>,
     ) {
         // Create statements to go in function body
         let mut stmts = ctx.ast.vec_with_capacity(inits.len() + usize::from(has_super_class));
 
         // Add `super(..._args);` statement and `..._args` param if class has a super class.
         // `constructor(..._args) { super(..._args); /* prop initialization */ }`
-        let constructor_scope_id = self.instance_inits_scope_id;
         let mut params_rest = None;
         if has_super_class {
             let args_binding =
@@ -301,6 +300,7 @@ impl<'a> ClassProperties<'a, '_> {
         constructor: &mut Function<'a>,
         inits: Vec<Expression<'a>>,
         super_binding: &BoundIdentifier<'a>,
+        super_func_scope_id: ScopeId,
         ctx: &mut TraverseCtx<'a>,
     ) {
         // Rename any symbols in constructor which clash with references in inits
@@ -313,7 +313,6 @@ impl<'a> ClassProperties<'a, '_> {
         // rather than an additional `return this` statement.
         // Actually this wouldn't work at present, as `_classPrivateFieldInitSpec(this, _prop, value)`
         // does not return `this`. We could alter it so it does when we have our own helper package.
-        let super_func_scope_id = self.instance_inits_scope_id;
         let args_binding =
             ctx.generate_uid("args", super_func_scope_id, SymbolFlags::FunctionScopedVariable);
         let super_call = create_super_call(&args_binding, ctx);
@@ -371,6 +370,7 @@ impl<'a> ClassProperties<'a, '_> {
         &mut self,
         inits: Vec<Expression<'a>>,
         super_binding: &BoundIdentifier<'a>,
+        super_func_scope_id: ScopeId,
         ctx: &mut TraverseCtx<'a>,
     ) {
         // Add `"use strict"` directive if outer scope is not strict mode
@@ -387,7 +387,6 @@ impl<'a> ClassProperties<'a, '_> {
         // `<inits>; return this;`
         let body_stmts = ctx.ast.vec_from_iter(exprs_into_stmts(inits, ctx).chain([return_stmt]));
         // `function() { <inits>; return this; }`
-        let super_func_scope_id = self.instance_inits_scope_id;
         let super_func = ctx.ast.expression_function_with_scope_id_and_pure(
             SPAN,
             FunctionType::FunctionExpression,

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

@@ -25,15 +25,22 @@ impl<'a> ClassProperties<'a, '_> {
     pub(super) fn transform_instance_initializer(
         &mut self,
         value: &Expression<'a>,
+        instance_inits_scope_id: ScopeId,
+        instance_inits_constructor_scope_id: Option<ScopeId>,
         ctx: &mut TraverseCtx<'a>,
     ) {
-        if let Some(constructor_scope_id) = self.instance_inits_constructor_scope_id {
+        if let Some(constructor_scope_id) = instance_inits_constructor_scope_id {
             // Re-parent first-level scopes, and check for symbol clashes
-            let mut updater = InstanceInitializerVisitor::new(constructor_scope_id, self, ctx);
+            let mut updater = InstanceInitializerVisitor::new(
+                instance_inits_scope_id,
+                constructor_scope_id,
+                self,
+                ctx,
+            );
             updater.visit_expression(value);
         } else {
             // No symbol clashes possible. Just re-parent first-level scopes (faster).
-            let mut updater = FastInstanceInitializerVisitor::new(self, ctx);
+            let mut updater = FastInstanceInitializerVisitor::new(instance_inits_scope_id, ctx);
             updater.visit_expression(value);
         }
     }
@@ -58,6 +65,7 @@ struct InstanceInitializerVisitor<'a, 'v> {
 
 impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> {
     fn new(
+        instance_inits_scope_id: ScopeId,
         constructor_scope_id: ScopeId,
         class_properties: &'v mut ClassProperties<'a, '_>,
         ctx: &'v mut TraverseCtx<'a>,
@@ -66,7 +74,7 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> {
             // Most initializers don't contain any scopes, so best default is 0 capacity
             // to avoid an allocation in most cases
             scope_ids_stack: Stack::new(),
-            parent_scope_id: class_properties.instance_inits_scope_id,
+            parent_scope_id: instance_inits_scope_id,
             constructor_scope_id,
             clashing_symbols: &mut class_properties.clashing_constructor_symbols,
             ctx,
@@ -154,8 +162,8 @@ struct FastInstanceInitializerVisitor<'a, 'v> {
 }
 
 impl<'a, 'v> FastInstanceInitializerVisitor<'a, 'v> {
-    fn new(class_properties: &'v ClassProperties<'a, '_>, ctx: &'v mut TraverseCtx<'a>) -> Self {
-        Self { parent_scope_id: class_properties.instance_inits_scope_id, ctx }
+    fn new(instance_inits_scope_id: ScopeId, ctx: &'v mut TraverseCtx<'a>) -> Self {
+        Self { parent_scope_id: instance_inits_scope_id, ctx }
     }
 }
 

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

@@ -203,7 +203,7 @@ use serde::Deserialize;
 
 use oxc_ast::ast::*;
 use oxc_span::Atom;
-use oxc_syntax::{scope::ScopeId, symbol::SymbolId};
+use oxc_syntax::symbol::SymbolId;
 use oxc_traverse::{Traverse, TraverseCtx};
 
 use crate::TransformCtx;
@@ -266,15 +266,6 @@ pub struct ClassProperties<'a, 'ctx> {
 
     // ----- State used only during enter phase -----
     //
-    /// Scope that instance property initializers will be inserted into.
-    /// This is usually class constructor, but can also be a `_super` function which is created.
-    instance_inits_scope_id: ScopeId,
-    /// Scope of class constructor, if instance property initializers will be inserted into constructor.
-    /// Used for checking for variable name clashes.
-    /// e.g. `class C { prop = x(); constructor(x) {} }`
-    /// - `x` in constructor needs to be renamed when `x()` is moved into constructor body.
-    /// `None` if class has no existing constructor, as then there can't be any clashes.
-    instance_inits_constructor_scope_id: Option<ScopeId>,
     /// Symbols in constructor which clash with instance prop initializers.
     /// Keys are symbols' IDs.
     /// Values are initially the original name of binding, later on the name of new UID name.
@@ -310,9 +301,6 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
             ctx,
             classes_stack: ClassesStack::new(),
             private_field_count: 0,
-            // Temporary values - overwritten when entering class
-            instance_inits_scope_id: ScopeId::new(0),
-            instance_inits_constructor_scope_id: None,
             // `Vec`s and `FxHashMap`s which are reused for every class being transformed
             clashing_constructor_symbols: FxHashMap::default(),
             insert_before: vec![],

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

@@ -2,6 +2,7 @@
 //! Transform of class property declarations (instance or static properties).
 
 use oxc_ast::{NONE, ast::*};
+use oxc_semantic::ScopeId;
 use oxc_span::SPAN;
 use oxc_syntax::reference::ReferenceFlags;
 use oxc_traverse::TraverseCtx;
@@ -21,12 +22,19 @@ impl<'a> ClassProperties<'a, '_> {
         &mut self,
         prop: &mut PropertyDefinition<'a>,
         instance_inits: &mut Vec<Expression<'a>>,
+        instance_inits_scope_id: ScopeId,
+        instance_inits_constructor_scope_id: Option<ScopeId>,
         ctx: &mut TraverseCtx<'a>,
     ) {
         // Get value
         let value = match prop.value.take() {
             Some(value) => {
-                self.transform_instance_initializer(&value, ctx);
+                self.transform_instance_initializer(
+                    &value,
+                    instance_inits_scope_id,
+                    instance_inits_constructor_scope_id,
+                    ctx,
+                );
                 value
             }
             None => ctx.ast.void_0(SPAN),