diff --git a/crates/oxc_transformer/src/es2022/class_properties/instance_prop_init.rs b/crates/oxc_transformer/src/es2022/class_properties/instance_prop_init.rs index 11ba8ac85e0ca..56f65a5d61705 100644 --- a/crates/oxc_transformer/src/es2022/class_properties/instance_prop_init.rs +++ b/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, + /// 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, /// Clashing symbols - clashing_constructor_symbols: &'v mut FxHashMap>, + clashing_symbols: &'v mut FxHashMap>, /// `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,39 +72,69 @@ 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>) { - 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 }; + self.check_for_symbol_clash(ident, constructor_scope_id); + } +} + +impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { + /// Update parent of scope. + 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(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 - - self.clashing_constructor_symbols.entry(symbol_id).or_insert(ident.name.clone()); - } -} + // 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; + } + } -impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { - /// Update parent of scope. - fn reparent_scope(&mut self, scope_id: ScopeId) { - self.ctx.scopes_mut().change_parent_id(scope_id, Some(self.parent_scope_id)); + // Record the symbol clash. Symbol in constructor needs to be renamed. + self.clashing_symbols.entry(constructor_symbol_id).or_insert(ident.name.clone()); } } diff --git a/tasks/transform_conformance/snapshots/oxc.snap.md b/tasks/transform_conformance/snapshots/oxc.snap.md index 7ed7ea1b6816f..3e0d9819555a4 100644 --- a/tasks/transform_conformance/snapshots/oxc.snap.md +++ b/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)] diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/input.js b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/input.js new file mode 100644 index 0000000000000..db1d954a803bd --- /dev/null +++ b/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); + } +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/output.js b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/output.js new file mode 100644 index 0000000000000..25601e1f1cd35 --- /dev/null +++ b/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); + } +}