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

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

@@ -37,7 +37,7 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> {
 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,
+    scope_ids: Vec<ScopeId>,
     /// Parent scope
     parent_scope_id: ScopeId,
     /// Constructor scope, if need to check for clashing bindings with constructor.
@@ -56,7 +56,9 @@ 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: vec![],
             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,
@@ -77,14 +79,14 @@ impl<'a, 'v> Visit<'a> for InstanceInitializerVisitor<'a, 'v> {
         // inside a function or class. But some TS types with scopes could be first level via
         // e.g. `TaggedTemplateExpression::type_parameters`, which contains `TSType`.
         // Not sure if that matters though, as they'll be stripped out anyway by TS transform.
-        if self.scope_depth == 0 {
+        if self.scope_ids.is_empty() {
             self.reparent_scope(scope_id);
         }
-        self.scope_depth += 1;
+        self.scope_ids.push(scope_id);
     }
 
     fn leave_scope(&mut self) {
-        self.scope_depth -= 1;
+        self.scope_ids.pop();
     }
 
     fn visit_identifier_reference(&mut self, ident: &IdentifierReference<'a>) {
@@ -94,14 +96,29 @@ impl<'a, 'v> Visit<'a> for InstanceInitializerVisitor<'a, 'v> {
         // 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)
+        let Some(constructor_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
+        // 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.contains(&scope_id) {
+                return;
+            }
+        }
 
-        self.clashing_constructor_symbols.entry(symbol_id).or_insert(ident.name.clone());
+        // Record the symbol clash. Symbol in constructor needs to be renamed.
+        self.clashing_constructor_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);
+  }
+}