From 0024a9ef33f959591ef4b3bf8a2f49b5fa6959eb Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Sat, 14 Dec 2024 19:11:43 +0000 Subject: [PATCH] feat(transformer/class-properties): only rename symbols if necessary --- .../class_properties/instance_prop_init.rs | 33 ++++++++++++++----- .../snapshots/oxc.snap.md | 4 +-- .../input.js | 20 +++++++++++ .../output.js | 22 +++++++++++++ 4 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/input.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-var-clash/output.js 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 5a78f1c3c2069b..ebfb71a0dba2de 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 @@ -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, /// 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()); } } diff --git a/tasks/transform_conformance/snapshots/oxc.snap.md b/tasks/transform_conformance/snapshots/oxc.snap.md index 7ed7ea1b6816f0..3e0d9819555a40 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 00000000000000..db1d954a803bd9 --- /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 00000000000000..25601e1f1cd35d --- /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); + } +}