From 80d0b3e10f472734d30017dba80e68c3e504e69f Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Sun, 15 Dec 2024 01:53:13 +0000 Subject: [PATCH] perf(transformer/class-properties): fast path for instance prop initializer scope re-parenting (#7901) Add a fast path for inserting instance property initializers into constructor, when no existing constructor or constructor has no bindings. This should be reasonably common. The `Scope flags mismatch` errors are due to #7900. --- .../class_properties/instance_prop_init.rs | 119 +++++++++++++++--- .../snapshots/oxc.snap.md | 21 +++- .../input.js | 33 +++++ .../output.js | 29 +++++ 4 files changed, 180 insertions(+), 22 deletions(-) create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/input.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/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 56f65a5d61705..697b3dc199bf7 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 @@ -26,14 +26,18 @@ impl<'a, 'ctx> ClassProperties<'a, 'ctx> { value: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>, ) { - let mut updater = InstanceInitializerVisitor::new(self, ctx); - updater.visit_expression(value); + if let Some(constructor_scope_id) = self.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); + updater.visit_expression(value); + } else { + // No symbol clashes possible. Just re-parent first-level scopes (faster). + let mut updater = FastInstanceInitializerVisitor::new(self, ctx); + updater.visit_expression(value); + }; } } -// TODO: If no `constructor_scope_id`, then don't need to traverse beyond first-level scope, -// as all we need to do is update scopes. Add a faster visitor for this more limited traversal. - /// Visitor to change parent scope of first-level scopes in instance property initializer, /// and find any `IdentifierReference`s which would be shadowed by bindings in constructor, /// once initializer moves into constructor body. @@ -43,10 +47,8 @@ struct InstanceInitializerVisitor<'a, 'v> { 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, + /// Constructor's scope, for checking symbol clashes against + constructor_scope_id: ScopeId, /// Clashing symbols clashing_symbols: &'v mut FxHashMap>, /// `TraverseCtx` object. @@ -55,6 +57,7 @@ struct InstanceInitializerVisitor<'a, 'v> { impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { fn new( + constructor_scope_id: ScopeId, class_properties: &'v mut ClassProperties<'a, '_>, ctx: &'v mut TraverseCtx<'a>, ) -> Self { @@ -63,7 +66,7 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { // 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, + constructor_scope_id, clashing_symbols: &mut class_properties.clashing_constructor_symbols, ctx, } @@ -90,12 +93,10 @@ impl<'a, 'v> Visit<'a> for InstanceInitializerVisitor<'a, 'v> { self.scope_ids_stack.pop(); } - // `#[inline]` because this is a hot path + // `#[inline]` because this function just delegates to `check_for_symbol_clash` #[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); + self.check_for_symbol_clash(ident); } } @@ -106,17 +107,13 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { } /// 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, - ) { + fn check_for_symbol_clash(&mut self, ident: &IdentifierReference<'a>) { // 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) + self.ctx.scopes().get_binding(self.constructor_scope_id, &ident.name) else { return; }; @@ -138,3 +135,85 @@ impl<'a, 'v> InstanceInitializerVisitor<'a, 'v> { self.clashing_symbols.entry(constructor_symbol_id).or_insert(ident.name.clone()); } } + +/// Visitor to change parent scope of first-level scopes in instance property initializer. +/// +/// Unlike `InstanceInitializerVisitor`, does not check for symbol clashes. +/// +/// Therefore only needs to walk until find a node which has a scope. No point continuing to traverse +/// inside that scope, as by definition and nested scopes can't be first level. +/// +/// The visitors here are for the only types which can be the first scope reached when starting +/// traversal from an `Expression`. +struct FastInstanceInitializerVisitor<'a, 'v> { + /// Parent scope + parent_scope_id: ScopeId, + /// `TraverseCtx` object. + ctx: &'v mut TraverseCtx<'a>, +} + +impl<'a, 'v> FastInstanceInitializerVisitor<'a, 'v> { + fn new( + class_properties: &'v mut ClassProperties<'a, '_>, + ctx: &'v mut TraverseCtx<'a>, + ) -> Self { + Self { parent_scope_id: class_properties.instance_inits_scope_id, ctx } + } +} + +impl<'a, 'v> Visit<'a> for FastInstanceInitializerVisitor<'a, 'v> { + #[inline] + fn visit_function(&mut self, func: &Function<'a>, _flags: ScopeFlags) { + self.reparent_scope(&func.scope_id); + } + + #[inline] + fn visit_arrow_function_expression(&mut self, func: &ArrowFunctionExpression<'a>) { + self.reparent_scope(&func.scope_id); + } + + #[inline] + fn visit_class(&mut self, class: &Class<'a>) { + // `decorators` is outside `Class`'s scope and can contain scopes itself + self.visit_decorators(&class.decorators); + + self.reparent_scope(&class.scope_id); + } + + #[inline] + fn visit_ts_conditional_type(&mut self, conditional: &TSConditionalType<'a>) { + // `check_type` is outside `TSConditionalType`'s scope and can contain scopes itself + self.visit_ts_type(&conditional.check_type); + + self.reparent_scope(&conditional.scope_id); + + // `false_type` is outside `TSConditionalType`'s scope and can contain scopes itself + self.visit_ts_type(&conditional.false_type); + } + + #[inline] + fn visit_ts_method_signature(&mut self, signature: &TSMethodSignature<'a>) { + self.reparent_scope(&signature.scope_id); + } + + #[inline] + fn visit_ts_construct_signature_declaration( + &mut self, + signature: &TSConstructSignatureDeclaration<'a>, + ) { + self.reparent_scope(&signature.scope_id); + } + + #[inline] + fn visit_ts_mapped_type(&mut self, mapped: &TSMappedType<'a>) { + self.reparent_scope(&mapped.scope_id); + } +} + +impl<'a, 'v> FastInstanceInitializerVisitor<'a, 'v> { + /// Update parent of scope. + fn reparent_scope(&mut self, scope_id: &Cell>) { + let scope_id = scope_id.get().unwrap(); + self.ctx.scopes_mut().change_parent_id(scope_id, Some(self.parent_scope_id)); + } +} diff --git a/tasks/transform_conformance/snapshots/oxc.snap.md b/tasks/transform_conformance/snapshots/oxc.snap.md index 3e0d9819555a4..cb6bf018b4c40 100644 --- a/tasks/transform_conformance/snapshots/oxc.snap.md +++ b/tasks/transform_conformance/snapshots/oxc.snap.md @@ -1,6 +1,6 @@ commit: 54a8389f -Passed: 110/124 +Passed: 110/125 # All Passed: * babel-plugin-transform-class-static-block @@ -16,7 +16,24 @@ Passed: 110/124 * regexp -# babel-plugin-transform-class-properties (11/13) +# babel-plugin-transform-class-properties (11/14) +* instance-prop-initializer-no-existing-constructor/input.js +Scope flags mismatch: +after transform: ScopeId(12): ScopeFlags(StrictMode) +rebuilt : ScopeId(13): ScopeFlags(StrictMode | Constructor) +Scope flags mismatch: +after transform: ScopeId(13): ScopeFlags(StrictMode) +rebuilt : ScopeId(14): ScopeFlags(StrictMode | Constructor) +Scope flags mismatch: +after transform: ScopeId(14): ScopeFlags(StrictMode) +rebuilt : ScopeId(15): ScopeFlags(StrictMode | Constructor) +Scope flags mismatch: +after transform: ScopeId(15): ScopeFlags(StrictMode) +rebuilt : ScopeId(16): ScopeFlags(StrictMode | Constructor) +Scope flags mismatch: +after transform: ScopeId(20): ScopeFlags(StrictMode) +rebuilt : ScopeId(21): ScopeFlags(StrictMode | Constructor) + * 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-no-existing-constructor/input.js b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/input.js new file mode 100644 index 0000000000000..6af629830d317 --- /dev/null +++ b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/input.js @@ -0,0 +1,33 @@ +class C { + function = function() {}; + functions = [ + function() { + function foo() {} + }, + function() { + function foo() {} + } + ]; + arrow = () => {}; + arrows = [() => () => {}, () => () => {}]; + klass = class {}; + classExtends = class extends class {} {}; + classes = [ + class { + method() { + class D {} + } + method2() { + class E {} + } + }, + class { + method() { + class D {} + } + method2() { + class E {} + } + } + ]; +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/output.js b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/output.js new file mode 100644 index 0000000000000..548b4403c8300 --- /dev/null +++ b/tasks/transform_conformance/tests/babel-plugin-transform-class-properties/test/fixtures/instance-prop-initializer-no-existing-constructor/output.js @@ -0,0 +1,29 @@ +class C { + constructor() { + babelHelpers.defineProperty(this, "function", function() {}); + babelHelpers.defineProperty(this, "functions", [function() { + function foo() {} + }, function() { + function foo() {} + }]); + babelHelpers.defineProperty(this, "arrow", () => {}); + babelHelpers.defineProperty(this, "arrows", [() => () => {}, () => () => {}]); + babelHelpers.defineProperty(this, "klass", class {}); + babelHelpers.defineProperty(this, "classExtends", class extends class {} {}); + babelHelpers.defineProperty(this, "classes", [class { + method() { + class D {} + } + method2() { + class E {} + } + }, class { + method() { + class D {} + } + method2() { + class E {} + } + }]); + } +}