feat(transformer/class-properties): only rename symbols if necessary (#7896)

#7872 implements renaming symbols in constructor which shadow references in instance property initializers. But we don't need to rename where the reference in initializer references a symbol which is bound within the initializer itself.

Input:

```js
class C {
  double = n => n * 2;
  constructor(n) {
    console.log(n);
  }
}
```

Output:

```js
class C {
  constructor(n) { // <-- not renamed
    this.double = n => n * 2; // <-- moved into constructor
    console.log(n); // <-- not renamed
  }
}
```

This produces better output, and avoids a traversal of constructor's AST renaming symbols.

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

@@ -6,6 +6,7 @@ use std::cell::Cell;
 use rustc_hash::FxHashMap;
 
 use oxc_ast::{ast::*, visit::Visit};
+use oxc_data_structures::stack::Stack;
 use oxc_span::Atom;
 use oxc_syntax::{
     scope::{ScopeFlags, ScopeId},
@@ -37,17 +38,17 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
 /// and find any `IdentifierReference`s which would be shadowed by bindings in constructor,
 /// once initializer moves into constructor body.
 struct InstanceInitializerVisitor<'a, 'v> {
-    /// Incremented when entering a scope, decremented when exiting it.
-    /// Parent `ScopeId` should be updated when `scope_depth == 0`.
-    scope_depth: u32,
-    /// Parent scope
+    /// Pushed to when entering a scope, popped when exiting it.
+    /// Parent `ScopeId` should be updated when stack is empty (i.e. this is a first-level scope).
+    scope_ids_stack: Stack<ScopeId>,
+    /// New parent scope for first-level scopes in initializer
     parent_scope_id: ScopeId,
     /// Constructor scope, if need to check for clashing bindings with constructor.
     /// `None` if constructor is newly created, or inits are being inserted in `_super` function
     /// outside class, because in those cases there are no bindings which can clash.
     constructor_scope_id: Option<ScopeId>,
     /// Clashing symbols
-    clashing_constructor_symbols: &'v mut FxHashMap<SymbolId, Atom<'a>>,
+    clashing_symbols: &'v mut FxHashMap<SymbolId, Atom<'a>>,
     /// `TraverseCtx` object.
     ctx: &'v mut TraverseCtx<'a>,
 }
@@ -58,10 +59,12 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> {
         ctx: &'v mut TraverseCtx<'a>,
     ) -> Self {
         Self {
-            scope_depth: 0,
+            // 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,
             constructor_scope_id: class_properties.instance_inits_constructor_scope_id,
-            clashing_constructor_symbols: &mut class_properties.clashing_constructor_symbols,
+            clashing_symbols: &mut class_properties.clashing_constructor_symbols,
             ctx,
         }
     }
@@ -69,33 +72,30 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> {
 
 impl<'a, 'v> Visit<'a> for InstanceInitializerVisitor<'a, 'v> {
     /// Update parent scope for first level of scopes.
+    /// Convert scope to sloppy mode if `self.make_sloppy_mode == true`.
+    //
+    // `#[inline]` because this function is small and called from many `walk` functions
+    #[inline]
     fn enter_scope(&mut self, _flags: ScopeFlags, scope_id: &Cell<Option<ScopeId>>) {
-        if self.scope_depth == 0 {
-            let scope_id = scope_id.get().unwrap();
+        let scope_id = scope_id.get().unwrap();
+        if self.scope_ids_stack.is_empty() {
             self.reparent_scope(scope_id);
         }
-        self.scope_depth += 1;
+        self.scope_ids_stack.push(scope_id);
     }
 
+    // `#[inline]` because this function is tiny and called from many `walk` functions
+    #[inline]
     fn leave_scope(&mut self) {
-        self.scope_depth -= 1;
+        self.scope_ids_stack.pop();
     }
 
+    // `#[inline]` because this is a hot path
+    #[inline]
     fn visit_identifier_reference(&mut self, ident: &IdentifierReference<'a>) {
         let Some(constructor_scope_id) = self.constructor_scope_id else { return };
 
-        // TODO: It would be ideal if could get reference `&Bindings` for constructor
-        // in `InstanceInitializerVisitor::new` rather than indexing into `ScopeTree::bindings`
-        // with same `ScopeId` every time here, but `ScopeTree` doesn't allow that, and we also
-        // take a `&mut ScopeTree` in `reparent_scope`, so borrow-checker doesn't allow that.
-        let Some(symbol_id) = self.ctx.scopes().get_binding(constructor_scope_id, &ident.name)
-        else {
-            return;
-        };
-
-        // TODO: Optimization: Exit if reference is bound to symbol within initializer
-
-        self.clashing_constructor_symbols.entry(symbol_id).or_insert(ident.name.clone());
+        self.check_for_symbol_clash(ident, constructor_scope_id);
     }
 }
 
@@ -104,4 +104,37 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> {
     fn reparent_scope(&mut self, scope_id: ScopeId) {
         self.ctx.scopes_mut().change_parent_id(scope_id, Some(self.parent_scope_id));
     }
+
+    /// Check if symbol referenced by `ident` is shadowed by a binding in constructor's scope.
+    fn check_for_symbol_clash(
+        &mut self,
+        ident: &IdentifierReference<'a>,
+        constructor_scope_id: ScopeId,
+    ) {
+        // TODO: It would be ideal if could get reference `&Bindings` for constructor
+        // in `InstanceInitializerVisitor::new` rather than indexing into `ScopeTree::bindings`
+        // with same `ScopeId` every time here, but `ScopeTree` doesn't allow that, and we also
+        // take a `&mut ScopeTree` in `reparent_scope`, so borrow-checker doesn't allow that.
+        let Some(constructor_symbol_id) =
+            self.ctx.scopes().get_binding(constructor_scope_id, &ident.name)
+        else {
+            return;
+        };
+
+        // Check the symbol this identifier refers to is bound outside of the initializer itself.
+        // If it's bound within the initializer, there's no clash, so exit.
+        // e.g. `class C { double = (n) => n * 2; constructor(n) {} }`
+        // Even though there's a binding `n` in constructor, it doesn't shadow the use of `n` in init.
+        // This is an improvement over Babel.
+        let reference_id = ident.reference_id();
+        if let Some(ident_symbol_id) = self.ctx.symbols().get_reference(reference_id).symbol_id() {
+            let scope_id = self.ctx.symbols().get_scope_id(ident_symbol_id);
+            if self.scope_ids_stack.contains(&scope_id) {
+                return;
+            }
+        }
+
+        // Record the symbol clash. Symbol in constructor needs to be renamed.
+        self.clashing_symbols.entry(constructor_symbol_id).or_insert(ident.name.clone());
+    }
 }

tasks/transform_conformance/snapshots/oxc.snap.md

@@ -1,6 +1,6 @@
 commit: 54a8389f
 
-Passed: 109/123
+Passed: 110/124
 
 # All Passed:
 * babel-plugin-transform-class-static-block
@@ -16,7 +16,7 @@ Passed: 109/123
 * regexp
 
 
-# babel-plugin-transform-class-properties (10/12)
+# babel-plugin-transform-class-properties (11/13)
 * typescript/optional-call/input.ts
 Symbol reference IDs mismatch for "X":
 after transform: SymbolId(0): [ReferenceId(0), ReferenceId(2), ReferenceId(6), ReferenceId(11), ReferenceId(16)]

tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/input.js

@@ -0,0 +1,20 @@
+let bound, bound2, bound3;
+
+class C {
+  clash1 = bound;
+  clash2 = unbound;
+  noClash1 = bound2;
+  noClash2 = unbound2;
+  noClash3 = bound3;
+  noClash4 = unbound3;
+  noClash5 = x => x;
+  noClash6 = () => { let y; return y; };
+
+  constructor(bound, x, y) {
+    {
+      var unbound;
+      let bound3, unbound3;
+    }
+    console.log(bound, unbound, x, y);
+  }
+}

tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/output.js

@@ -0,0 +1,22 @@
+let bound, bound2, bound3;
+
+class C {
+  constructor(_bound, x, y) {
+    babelHelpers.defineProperty(this, "clash1", bound);
+    babelHelpers.defineProperty(this, "clash2", unbound);
+    babelHelpers.defineProperty(this, "noClash1", bound2);
+    babelHelpers.defineProperty(this, "noClash2", unbound2);
+    babelHelpers.defineProperty(this, "noClash3", bound3);
+    babelHelpers.defineProperty(this, "noClash4", unbound3);
+    babelHelpers.defineProperty(this, "noClash5", (x) => x);
+    babelHelpers.defineProperty(this, "noClash6", () => {
+      let y;
+      return y;
+    });
+    {
+      var _unbound;
+      let bound3, unbound3;
+    }
+    console.log(_bound, _unbound, x, y);
+  }
+}