From 6b7d3ed613f63234e261e9b9d27084d7f1f835a0 Mon Sep 17 00:00:00 2001 From: Dunqing <29533304+Dunqing@users.noreply.github.com> Date: Thu, 26 Sep 2024 15:50:27 +0000 Subject: [PATCH] perf(isolated-declarations): should clone transformed AST rather than original AST (#6078) close: #6074 Performance regression introduced by https://github.com/oxc-project/oxc/pull/5909. After this PR we back to the fold pattern again --- crates/oxc_isolated_declarations/src/class.rs | 112 +++++---- .../src/declaration.rs | 2 +- crates/oxc_isolated_declarations/src/lib.rs | 230 ++++++++++-------- .../oxc_isolated_declarations/src/module.rs | 8 +- .../tests/snapshots/set-get-accessor.snap | 2 +- 5 files changed, 200 insertions(+), 154 deletions(-) diff --git a/crates/oxc_isolated_declarations/src/class.rs b/crates/oxc_isolated_declarations/src/class.rs index f5de96b770eaf..a6fd14ddf3cfb 100644 --- a/crates/oxc_isolated_declarations/src/class.rs +++ b/crates/oxc_isolated_declarations/src/class.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use oxc_allocator::{Box, CloneIn}; #[allow(clippy::wildcard_imports)] use oxc_ast::{ast::*, NONE}; @@ -269,23 +271,26 @@ impl<'a> IsolatedDeclarations<'a> { elements } - /// Transform getter and setter methods + /// Collect return_type of getter and first parma type of setter /// /// ### Getter /// - /// 1. If it has no return type, infer it from the function body - /// 2. If it cannot be inferred from the function body, try to infer it from setter method's first parameter + /// 1. If it has return type, use it + /// 2. If it has no return type, infer it from the function body /// /// ### Setter /// - /// 1. If it has no parameter, create a parameter with the name `value` - /// 2. If it has no parameter type, infer it from the getter method's return type - fn transform_getter_or_setter_methods(&self, decl: &mut Class<'a>) { - let mut method_annotations: FxHashMap<_, (bool, _, _)> = FxHashMap::default(); - for element in decl.body.body.iter_mut() { + /// 1. If it has no parameter type, infer it from the getter method's return type + fn collect_getter_or_setter_annotations( + &self, + decl: &Class<'a>, + ) -> FxHashMap, Box<'a, TSTypeAnnotation<'a>>> { + let mut method_annotations = FxHashMap::default(); + for element in &decl.body.body { if let ClassElement::MethodDefinition(method) = element { - if method.key.is_private_identifier() - && (method.computed && !self.is_literal_key(&method.key)) + if (method.key.is_private_identifier() + || method.accessibility.is_some_and(TSAccessibility::is_private)) + || (method.computed && !self.is_literal_key(&method.key)) { continue; } @@ -296,52 +301,36 @@ impl<'a> IsolatedDeclarations<'a> { match method.kind { MethodDefinitionKind::Set => { - let params = &mut method.value.params; - if params.items.is_empty() { - *params = self.create_formal_parameters( - self.ast.binding_pattern_kind_binding_identifier(SPAN, "value"), - ); - } - let Some(first_param) = method.value.params.items.first_mut() else { + let Some(first_param) = method.value.params.items.first() else { continue; }; - let entry = method_annotations.entry(name).or_default(); - entry.0 |= first_param.pattern.type_annotation.is_none(); - entry.1 = Some(&mut first_param.pattern.type_annotation); + if let Some(annotation) = + first_param.pattern.type_annotation.clone_in(self.ast.allocator) + { + method_annotations.insert(name, annotation); + } } MethodDefinitionKind::Get => { - if method.accessibility.is_some_and(TSAccessibility::is_private) { - continue; + let function = &method.value; + if let Some(annotation) = function + .return_type + .clone_in(self.ast.allocator) + .or_else(|| self.infer_function_return_type(function)) + { + method_annotations.insert(name, annotation); } - - let function = &mut method.value; - if function.return_type.is_none() { - function.return_type = self.infer_function_return_type(function); - }; - let return_type = &mut function.return_type; - let entry = method_annotations.entry(name).or_default(); - entry.0 |= return_type.is_none(); - entry.2 = Some(&mut function.return_type); } _ => continue, }; } } - for (requires_inference, param, return_type) in method_annotations.into_values() { - if requires_inference { - if let (Some(Some(annotation)), Some(option)) - | (Some(option), Some(Some(annotation))) = (param, return_type) - { - option.replace(annotation.clone_in(self.ast.allocator)); - } - } - } + method_annotations } pub fn transform_class( &self, - decl: &mut Class<'a>, + decl: &Class<'a>, declare: Option, ) -> Option>> { if decl.declare { @@ -361,7 +350,7 @@ impl<'a> IsolatedDeclarations<'a> { } } - self.transform_getter_or_setter_methods(decl); + let setter_getter_annotations = self.collect_getter_or_setter_annotations(decl); let mut has_private_key = false; let mut elements = self.ast.vec(); let mut is_function_overloads = false; @@ -391,7 +380,32 @@ impl<'a> IsolatedDeclarations<'a> { let function = &method.value; let params = match method.kind { - MethodDefinitionKind::Set => function.params.clone_in(self.ast.allocator), + MethodDefinitionKind::Set => { + if method.accessibility.is_some_and(TSAccessibility::is_private) { + elements.push(self.transform_private_modifier_method(method)); + continue; + } + let params = &method.value.params; + if params.items.is_empty() { + self.create_formal_parameters( + self.ast.binding_pattern_kind_binding_identifier(SPAN, "value"), + ) + } else { + let mut params = params.clone_in(self.ast.allocator); + if let Some(param) = params.items.first_mut() { + if let Some(annotation) = + method.key.static_name().and_then(|name| { + setter_getter_annotations + .get(&name) + .map(|a| a.clone_in(self.ast.allocator)) + }) + { + param.pattern.type_annotation = Some(annotation); + } + } + params + } + } MethodDefinitionKind::Constructor => { let params = self.transform_formal_parameters(&function.params); elements.splice( @@ -428,8 +442,16 @@ impl<'a> IsolatedDeclarations<'a> { rt } MethodDefinitionKind::Get => { - let rt = method.value.return_type.clone_in(self.ast.allocator); - if method.value.return_type.is_none() { + let rt = method.value.return_type.clone_in(self.ast.allocator).or_else( + || { + method + .key + .static_name() + .and_then(|name| setter_getter_annotations.get(&name)) + .map(|a| a.clone_in(self.ast.allocator)) + }, + ); + if rt.is_none() { self.error(accessor_must_have_explicit_return_type( method.key.span(), )); diff --git a/crates/oxc_isolated_declarations/src/declaration.rs b/crates/oxc_isolated_declarations/src/declaration.rs index b7d5b2b90f0e9..e0c29c9d141ff 100644 --- a/crates/oxc_isolated_declarations/src/declaration.rs +++ b/crates/oxc_isolated_declarations/src/declaration.rs @@ -195,7 +195,7 @@ impl<'a> IsolatedDeclarations<'a> { pub fn transform_declaration( &mut self, - decl: &mut Declaration<'a>, + decl: &Declaration<'a>, check_binding: bool, ) -> Option> { match decl { diff --git a/crates/oxc_isolated_declarations/src/lib.rs b/crates/oxc_isolated_declarations/src/lib.rs index 9c80f7b562b8f..27e3a076ec198 100644 --- a/crates/oxc_isolated_declarations/src/lib.rs +++ b/crates/oxc_isolated_declarations/src/lib.rs @@ -149,71 +149,60 @@ impl<'a> IsolatedDeclarations<'a> { ) -> oxc_allocator::Vec<'a, Statement<'a>> { self.report_error_for_expando_function(stmts); - let filtered_stmts = stmts.iter().filter_map(|stmt| { - if stmt.is_declaration() { - Some(stmt.clone_in(self.ast.allocator)) - } else { - None - } - }); - let filtered_stmts = Self::remove_function_overloads_implementation(filtered_stmts); + let mut stmts = + self.ast.vec_from_iter(stmts.iter().filter(|stmt| { + stmt.is_declaration() && !self.has_internal_annotation(stmt.span()) + })); - let mut stmts = self.ast.vec_from_iter(filtered_stmts); - for stmt in stmts.iter_mut() { - if let Some(decl) = stmt.as_declaration_mut() { - if self.has_internal_annotation(decl.span()) { - continue; - } - if let Some(new_decl) = self.transform_declaration(decl, false) { - *decl = new_decl; - } - } - } + Self::remove_function_overloads_implementation(&mut stmts); - stmts + self.ast.vec_from_iter(stmts.iter().map(|stmt| { + if let Some(new_decl) = self.transform_declaration(stmt.to_declaration(), false) { + Statement::from(new_decl) + } else { + stmt.clone_in(self.ast.allocator) + } + })) } + #[allow(clippy::missing_panics_doc)] pub fn transform_statements_on_demand( &mut self, stmts: &oxc_allocator::Vec<'a, Statement<'a>>, ) -> oxc_allocator::Vec<'a, Statement<'a>> { self.report_error_for_expando_function(stmts); + let mut stmts = self.ast.vec_from_iter(stmts.iter().filter(|stmt| { + (stmt.is_declaration() || stmt.is_module_declaration()) + && !self.has_internal_annotation(stmt.span()) + })); + Self::remove_function_overloads_implementation(&mut stmts); + // https://github.com/microsoft/TypeScript/pull/58912 let mut need_empty_export_marker = true; - let mut new_stmts = self.ast.vec(); - // Use span to identify transformed nodes let mut transformed_spans: FxHashSet = FxHashSet::default(); - - let filtered_stmts = stmts.iter().filter_map(|stmt| { - if stmt.is_declaration() || stmt.is_module_declaration() { - Some(stmt.clone_in(self.ast.allocator)) - } else { - None - } - }); - - let filtered_stmts = Self::remove_function_overloads_implementation(filtered_stmts); + let mut transformed_stmts: FxHashMap> = FxHashMap::default(); + let mut transformed_variable_declarator: FxHashMap> = + FxHashMap::default(); + let mut export_default_var = None; // 1. Collect all declarations, module declarations // 2. Transform export declarations // 3. Collect all bindings / reference from module declarations // 4. Collect transformed indexes - for mut stmt in filtered_stmts { + for stmt in &stmts { match stmt { match_declaration!(Statement) => { - if let Statement::TSModuleDeclaration(ref mut decl) = stmt { - if self.has_internal_annotation(decl.span) { - continue; - } + if let Statement::TSModuleDeclaration(decl) = stmt { // `declare global { ... }` or `declare module "foo" { ... }` // We need to emit it anyway let is_global = decl.kind.is_global(); if is_global || decl.id.is_string_literal() { transformed_spans.insert(decl.span); + let mut decl = decl.clone_in(self.ast.allocator); // Remove export keyword from all statements in `declare module "xxx" { ... }` if !is_global { if let Some(body) = @@ -224,15 +213,17 @@ impl<'a> IsolatedDeclarations<'a> { } // We need to visit the module declaration to collect all references - self.scope.visit_ts_module_declaration(decl); + self.scope.visit_ts_module_declaration(decl.as_ref()); + + transformed_stmts.insert( + decl.span, + Statement::from(Declaration::TSModuleDeclaration(decl)), + ); } } - if !self.has_internal_annotation(stmt.span()) { - new_stmts.push(stmt); - } } match_module_declaration!(Statement) => { - match stmt.to_module_declaration_mut() { + match stmt.to_module_declaration() { ModuleDeclaration::ExportDefaultDeclaration(decl) => { if self.has_internal_annotation(decl.span) { continue; @@ -242,15 +233,19 @@ impl<'a> IsolatedDeclarations<'a> { self.transform_export_default_declaration(decl) { if let Some(var_decl) = var_decl { - transformed_spans.insert(var_decl.span); self.scope.visit_variable_declaration(&var_decl); - new_stmts.push(Statement::VariableDeclaration( + export_default_var = Some(Statement::VariableDeclaration( self.ast.alloc(var_decl), )); } self.scope.visit_export_default_declaration(&new_decl); - *decl = self.ast.alloc(new_decl); + transformed_stmts.insert( + decl.span, + Statement::from(ModuleDeclaration::ExportDefaultDeclaration( + self.ast.alloc(new_decl), + )), + ); } need_empty_export_marker = false; @@ -258,41 +253,43 @@ impl<'a> IsolatedDeclarations<'a> { } ModuleDeclaration::ExportNamedDeclaration(decl) => { - if self.has_internal_annotation(decl.span) { - continue; - } transformed_spans.insert(decl.span); if let Some(new_decl) = self.transform_export_named_declaration(decl) { - *decl = self.ast.alloc(new_decl); + self.scope.visit_export_named_declaration(&new_decl); + transformed_stmts.insert( + decl.span, + Statement::from(ModuleDeclaration::ExportNamedDeclaration( + self.ast.alloc(new_decl), + )), + ); } else if decl.declaration.is_none() { need_empty_export_marker = false; + self.scope.visit_export_named_declaration(decl); } - self.scope.visit_export_named_declaration(decl); } ModuleDeclaration::ImportDeclaration(_) => { // We must transform this in the end, because we need to know all references } module_declaration => { - if self.has_internal_annotation(module_declaration.span()) { - continue; - } transformed_spans.insert(module_declaration.span()); self.scope.visit_module_declaration(module_declaration); } } - - new_stmts.push(stmt); } _ => {} } } - let last_transformed_len = transformed_spans.len(); + let last_transformed_len = transformed_spans.len() + transformed_variable_declarator.len(); // 5. Transform statements until no more transformation can be done loop { - let cur_transformed_len = transformed_spans.len(); - for stmt in new_stmts.iter_mut() { - let Some(decl) = stmt.as_declaration_mut() else { continue }; + let cur_transformed_len = + transformed_spans.len() + transformed_variable_declarator.len(); + for stmt in &stmts { + if transformed_spans.contains(&stmt.span()) { + continue; + } + let Some(decl) = stmt.as_declaration() else { continue }; // Skip transformed declarations if transformed_spans.contains(&decl.span()) { @@ -301,7 +298,7 @@ impl<'a> IsolatedDeclarations<'a> { if let Declaration::VariableDeclaration(declaration) = decl { let mut all_declarator_has_transformed = true; - for declarator in declaration.declarations.iter_mut() { + for declarator in &declaration.declarations { if transformed_spans.contains(&declarator.span) { continue; } @@ -310,25 +307,40 @@ impl<'a> IsolatedDeclarations<'a> { self.transform_variable_declarator(declarator, true) { self.scope.visit_variable_declarator(&new_declarator); - transformed_spans.insert(new_declarator.span()); - *declarator = new_declarator; + transformed_variable_declarator.insert(declarator.span, new_declarator); } else { all_declarator_has_transformed = false; } } if all_declarator_has_transformed { - declaration.declare = self.is_declare(); + let declarations = self.ast.vec_from_iter( + declaration.declarations.iter().map(|declarator| { + transformed_variable_declarator.remove(&declarator.span).unwrap() + }), + ); + let decl = self.ast.variable_declaration( + declaration.span, + declaration.kind, + declarations, + self.is_declare(), + ); + transformed_stmts.insert( + declaration.span, + Statement::from(self.ast.declaration_from_variable(decl)), + ); transformed_spans.insert(declaration.span); } } else if let Some(new_decl) = self.transform_declaration(decl, true) { self.scope.visit_declaration(&new_decl); transformed_spans.insert(new_decl.span()); - *decl = new_decl; + transformed_stmts.insert(new_decl.span(), Statement::from(new_decl)); } } // Use transformed_spans to weather we need to continue - if cur_transformed_len == transformed_spans.len() { + if cur_transformed_len + == transformed_spans.len() + transformed_variable_declarator.len() + { break; } } @@ -339,35 +351,51 @@ impl<'a> IsolatedDeclarations<'a> { } // 6. Transform variable/using declarations, import statements, remove unused imports - new_stmts.retain_mut(|stmt| { + let mut new_stm = + self.ast.vec_with_capacity(stmts.len() + usize::from(export_default_var.is_some())); + stmts.iter().for_each(|stmt| { if transformed_spans.contains(&stmt.span()) { - return true; + let new_stmt = transformed_stmts + .remove(&stmt.span()) + .unwrap_or_else(|| stmt.clone_in(self.ast.allocator)); + if matches!(new_stmt, Statement::ExportDefaultDeclaration(_)) { + if let Some(export_default_var) = export_default_var.take() { + new_stm.push(export_default_var); + } + } + new_stm.push(new_stmt); + return; } match stmt { Statement::ImportDeclaration(decl) => { // We must transform this in the end, because we need to know all references if decl.specifiers.is_none() { - true + new_stm.push(stmt.clone_in(self.ast.allocator)); } else if let Some(new_decl) = self.transform_import_declaration(decl) { - *decl = new_decl; - true - } else { - false + new_stm.push(Statement::from( + self.ast.module_declaration_from_import_declaration(new_decl), + )); } } - Statement::VariableDeclaration(ref mut decl) => { - // If here just one declaration, that means this variable declaration doesn't referenced by any other - if decl.declarations.len() == 1 { - false - } else { + Statement::VariableDeclaration(decl) => { + if decl.declarations.len() > 1 { // Remove unreferenced declarations - decl.declarations - .retain(|declarator| transformed_spans.contains(&declarator.span)); - decl.declare = self.is_declare(); - true + let declarations = self.ast.vec_from_iter( + decl.declarations.iter().filter_map(|declarator| { + transformed_variable_declarator.remove(&declarator.span) + }), + ); + new_stm.push(Statement::from(self.ast.declaration_from_variable( + self.ast.variable_declaration( + decl.span, + decl.kind, + declarations, + self.is_declare(), + ), + ))); } } - _ => false, + _ => {} } }); @@ -376,27 +404,23 @@ impl<'a> IsolatedDeclarations<'a> { let kind = ImportOrExportKind::Value; let empty_export = self.ast.alloc_export_named_declaration(SPAN, None, specifiers, None, kind, NONE); - new_stmts - .push(Statement::from(ModuleDeclaration::ExportNamedDeclaration(empty_export))); + new_stm.push(Statement::from(ModuleDeclaration::ExportNamedDeclaration(empty_export))); } else if self.scope.is_ts_module_block() { // If we are in a module block and we don't need to add `export {}`, in that case we need to remove `export` keyword from all ExportNamedDeclaration // - self.strip_export_keyword(&mut new_stmts); + self.strip_export_keyword(&mut new_stm); } - new_stmts + new_stm } - pub fn remove_function_overloads_implementation( - stmts: T, - ) -> impl Iterator> - where - T: Iterator>, - { + pub fn remove_function_overloads_implementation( + stmts: &mut oxc_allocator::Vec<'a, &Statement<'a>>, + ) { let mut last_function_name: Option> = None; let mut is_export_default_function_overloads = false; - stmts.filter_map(move |stmt| match stmt { + stmts.retain(move |stmt| match stmt { Statement::FunctionDeclaration(ref func) => { let name = &func .id @@ -410,12 +434,12 @@ impl<'a> IsolatedDeclarations<'a> { if func.body.is_some() { if last_function_name.as_ref().is_some_and(|last_name| last_name == name) { - return None; + return false; } } else { last_function_name = Some(name.clone()); } - Some(stmt) + true } Statement::ExportNamedDeclaration(ref decl) => { if let Some(Declaration::FunctionDeclaration(ref func)) = decl.declaration { @@ -430,14 +454,14 @@ impl<'a> IsolatedDeclarations<'a> { .name; if func.body.is_some() { if last_function_name.as_ref().is_some_and(|last_name| last_name == name) { - return None; + return false; } } else { last_function_name = Some(name.clone()); } - Some(stmt) + true } else { - Some(stmt) + true } } Statement::ExportDefaultDeclaration(ref decl) => { @@ -446,17 +470,17 @@ impl<'a> IsolatedDeclarations<'a> { { if is_export_default_function_overloads && func.body.is_some() { is_export_default_function_overloads = false; - return None; + return false; } is_export_default_function_overloads = true; - Some(stmt) + true } else { is_export_default_function_overloads = false; - Some(stmt) + true } } - _ => Some(stmt), - }) + _ => true, + }); } fn get_assignable_properties_for_namespaces( diff --git a/crates/oxc_isolated_declarations/src/module.rs b/crates/oxc_isolated_declarations/src/module.rs index dbe12767bc3b2..b8247021abbb2 100644 --- a/crates/oxc_isolated_declarations/src/module.rs +++ b/crates/oxc_isolated_declarations/src/module.rs @@ -9,9 +9,9 @@ use crate::{diagnostics::default_export_inferred, IsolatedDeclarations}; impl<'a> IsolatedDeclarations<'a> { pub fn transform_export_named_declaration( &mut self, - prev_decl: &mut ExportNamedDeclaration<'a>, + prev_decl: &ExportNamedDeclaration<'a>, ) -> Option> { - let decl = self.transform_declaration(prev_decl.declaration.as_mut()?, false)?; + let decl = self.transform_declaration(prev_decl.declaration.as_ref()?, false)?; Some(self.ast.export_named_declaration( prev_decl.span, @@ -35,9 +35,9 @@ impl<'a> IsolatedDeclarations<'a> { pub fn transform_export_default_declaration( &mut self, - decl: &mut ExportDefaultDeclaration<'a>, + decl: &ExportDefaultDeclaration<'a>, ) -> Option<(Option>, ExportDefaultDeclaration<'a>)> { - let declaration = match &mut decl.declaration { + let declaration = match &decl.declaration { ExportDefaultDeclarationKind::FunctionDeclaration(decl) => self .transform_function(decl, Some(false)) .map(|d| (None, ExportDefaultDeclarationKind::FunctionDeclaration(d))), diff --git a/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap b/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap index 87b1754cf65ba..0cac005c367e8 100644 --- a/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap +++ b/crates/oxc_isolated_declarations/tests/snapshots/set-get-accessor.snap @@ -7,7 +7,7 @@ input_file: crates/oxc_isolated_declarations/tests/fixtures/set-get-accessor.ts declare class Cls { get a(): number; - set a(value: number); + set a(value); get b(): string; set b(v: string); private get c();