refactor(es/typescript): Run typescript transform in two passes#11532
refactor(es/typescript): Run typescript transform in two passes#11532
Conversation
🦋 Changeset detectedLatest commit: b96fab7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
kdy1
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the TypeScript transform to use a two-pass architecture, significantly improving code organization and maintainability. The PR replaces the previous interleaved approach with:
Pass 1 (Semantic Analysis): A new semantic.rs module traverses the AST once to collect:
- Identifier usage information
- Type vs value declarations
- Export bindings
- Enum records and constant enum tracking
Pass 2 (Transform): The existing transform.rs now consumes the semantic information and performs all stripping and runtime transformations in a single traversal, removing the need for the old strip_import_export module.
Changes:
- Introduced
semantic.rswithanalyze_program()for semantic analysis - Removed
strip_import_export.rsmodule (replaced by semantic analysis) - Refactored
transform.rsto consume semantic info and consolidate transform logic - Updated
typescript.rsentrypoint to orchestrate the two-pass pipeline
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
crates/swc_ecma_transforms_typescript/src/semantic.rs |
New file implementing semantic analysis pass that collects usage, type/value distinctions, export bindings, and enum information |
crates/swc_ecma_transforms_typescript/src/transform.rs |
Refactored to consume semantic info, removed Visit impl, added stripping logic with semantic info, duplicated helper traits/functions from strip_type.rs |
crates/swc_ecma_transforms_typescript/src/typescript.rs |
Updated entrypoint to call semantic analysis before transform, passing semantic info through the pipeline |
crates/swc_ecma_transforms_typescript/src/lib.rs |
Module declaration changed from strip_import_export to semantic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| trait IsConcrete { | ||
| fn is_concrete(&self) -> bool; | ||
| } | ||
|
|
||
| impl IsConcrete for TsModuleDecl { | ||
| fn is_concrete(&self) -> bool { | ||
| self.body | ||
| .as_ref() | ||
| .map(|body| body.is_concrete()) | ||
| .unwrap_or_default() | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for TsNamespaceBody { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::TsModuleBlock(ts_module_block) => { | ||
| ts_module_block.body.iter().any(|item| item.is_concrete()) | ||
| } | ||
| Self::TsNamespaceDecl(ts_namespace_decl) => ts_namespace_decl.body.is_concrete(), | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for ModuleItem { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::ModuleDecl(module_decl) => module_decl.is_concrete(), | ||
| Self::Stmt(stmt) => stmt.is_concrete(), | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for ModuleDecl { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::Import(import_decl) => !import_decl.type_only, | ||
| Self::ExportDecl(export_decl) => export_decl.decl.is_concrete(), | ||
| Self::ExportNamed(named_export) => !named_export.type_only, | ||
| Self::ExportDefaultDecl(export_default_decl) => export_default_decl.decl.is_concrete(), | ||
| Self::ExportDefaultExpr(..) => true, | ||
| Self::ExportAll(export_all) => !export_all.type_only, | ||
| Self::TsImportEquals(ts_import_equals) => !ts_import_equals.is_type_only, | ||
| Self::TsExportAssignment(..) => true, | ||
| Self::TsNamespaceExport(..) => false, | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for Decl { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::TsInterface(..) | Self::TsTypeAlias(..) => false, | ||
| Self::Fn(function_decl) => function_decl.function.body.is_some(), | ||
| Self::Class(..) | Self::Var(..) | Self::Using(..) | Self::TsEnum(..) => true, | ||
| Self::TsModule(ts_module) => ts_module.is_concrete(), | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for DefaultDecl { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::Class(..) => true, | ||
| Self::Fn(function_expr) => function_expr.function.body.is_some(), | ||
| Self::TsInterfaceDecl(..) => false, | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IsConcrete for Stmt { | ||
| fn is_concrete(&self) -> bool { | ||
| match self { | ||
| Self::Empty(..) => false, | ||
| Self::Decl(decl) => decl.is_concrete(), | ||
| _ => true, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The IsConcrete trait and its implementations are duplicated from strip_type.rs. This creates maintenance burden as changes to one need to be synchronized with the other. Consider importing IsConcrete from strip_type module instead, or moving it to a shared location if both modules need it.
| trait IsDeclare { | ||
| fn is_declare(&self) -> bool; | ||
| } | ||
|
|
||
| impl IsDeclare for Decl { | ||
| fn is_declare(&self) -> bool { | ||
| match self { | ||
| Decl::Class(class_decl) => class_decl.declare, | ||
| Decl::Fn(function_decl) => function_decl.declare, | ||
| Decl::Var(var_decl) => var_decl.declare, | ||
| Decl::Using(..) => false, | ||
| Decl::TsInterface(..) | Decl::TsTypeAlias(..) => true, | ||
| Decl::TsEnum(ts_enum_decl) => ts_enum_decl.declare, | ||
| Decl::TsModule(ts_module_decl) => ts_module_decl.declare || ts_module_decl.global, | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The IsDeclare trait and its implementation are duplicated from strip_type.rs. This creates maintenance burden as changes to one need to be synchronized with the other. Consider importing IsDeclare from strip_type module instead, or moving it to a shared location if both modules need it.
| fn get_module_ident(ts_entity_name: &TsEntityName) -> &Ident { | ||
| match ts_entity_name { | ||
| TsEntityName::TsQualifiedName(ts_qualified_name) => { | ||
| get_module_ident(&ts_qualified_name.left) | ||
| } | ||
| TsEntityName::Ident(ident) => ident, | ||
| #[cfg(swc_ast_unknown)] | ||
| _ => panic!("unable to access unknown nodes"), | ||
| } | ||
| } |
There was a problem hiding this comment.
The get_module_ident helper function is duplicated in semantic.rs (line 505). This creates maintenance burden. Consider moving this function to a shared utility module to avoid duplication.
| fn should_retain_module_item(module_item: &ModuleItem, in_namespace: bool) -> bool { | ||
| match module_item { | ||
| ModuleItem::ModuleDecl(ModuleDecl::ExportDecl(export_decl)) => { | ||
| // Keep `export declare var` in namespace blocks for downstream transforms. | ||
| if in_namespace && export_decl.decl.is_var() { | ||
| return true; | ||
| } | ||
|
|
||
| should_retain_decl(&export_decl.decl) | ||
| } | ||
| ModuleItem::Stmt(stmt) => should_retain_stmt(stmt), | ||
| _ => module_item.is_concrete(), | ||
| } | ||
| } | ||
|
|
||
| fn should_retain_stmt(stmt: &Stmt) -> bool { | ||
| match stmt { | ||
| Stmt::Decl(decl) => should_retain_decl(decl), | ||
| _ => stmt.is_concrete(), | ||
| } | ||
| } | ||
|
|
||
| fn should_retain_decl(decl: &Decl) -> bool { | ||
| if decl.is_declare() { | ||
| return false; | ||
| } | ||
|
|
||
| decl.is_concrete() | ||
| } |
There was a problem hiding this comment.
The should_retain_module_item, should_retain_stmt, and should_retain_decl helper functions are duplicated from strip_type.rs with nearly identical implementations. This creates maintenance burden as changes to one need to be synchronized with the other. Consider importing these functions from strip_type module or consolidating them in a shared location.
| } | ||
| } | ||
|
|
||
| impl Visit for NamespaceUsageCollect { |
There was a problem hiding this comment.
The NamespaceUsageCollect Visit implementation is missing the noop_visit_type macro. This means it will visit TypeScript type annotations when it should skip them, potentially collecting identifiers from type positions that shouldn't be considered as value usages. Add noop_visit_type!(); as the first line in the Visit implementation for consistency with other visitors in this module.
| impl Visit for NamespaceUsageCollect { | |
| impl Visit for NamespaceUsageCollect { | |
| noop_visit_type!(); |
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
Binary Sizes
Commit: 56b2024 |
CodSpeed Performance ReportMerging this PR will improve performance by 10.49%Comparing Summary
Performance Changes
|
This comment has been minimized.
This comment has been minimized.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Default)] | ||
| struct NamespaceUsageCollect { | ||
| id_usage: FxHashSet<Id>, | ||
| import_chain: FxHashMap<Id, Id>, | ||
| } | ||
|
|
||
| impl NamespaceUsageCollect { | ||
| fn has_usage(&self, id: &Id) -> bool { | ||
| self.id_usage.contains(id) | ||
| } | ||
|
|
||
| fn analyze_import_chain(&mut self) { | ||
| if self.import_chain.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let mut new_usage = FxHashSet::default(); | ||
| for id in &self.id_usage { | ||
| let mut next = self.import_chain.remove(id); | ||
| while let Some(id) = next { | ||
| next = self.import_chain.remove(&id); | ||
| new_usage.insert(id); | ||
| } | ||
|
|
||
| if self.import_chain.is_empty() { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| self.id_usage.extend(new_usage); | ||
| } | ||
| } | ||
|
|
||
| impl Visit for NamespaceUsageCollect { | ||
| fn visit_ident(&mut self, node: &Ident) { | ||
| self.id_usage.insert(node.to_id()); | ||
| } | ||
|
|
||
| fn visit_expr(&mut self, node: &Expr) { | ||
| maybe_grow_default(|| node.visit_children_with(self)); | ||
| } | ||
|
|
||
| fn visit_binding_ident(&mut self, _: &BindingIdent) { | ||
| // skip | ||
| } | ||
|
|
||
| fn visit_fn_decl(&mut self, node: &FnDecl) { | ||
| // skip function identifier | ||
| node.function.visit_with(self); | ||
| } | ||
|
|
||
| fn visit_fn_expr(&mut self, node: &FnExpr) { | ||
| // skip function identifier | ||
| node.function.visit_with(self); | ||
| } | ||
|
|
||
| fn visit_class_decl(&mut self, node: &ClassDecl) { | ||
| // skip class identifier | ||
| node.class.visit_with(self); | ||
| } | ||
|
|
||
| fn visit_class_expr(&mut self, node: &ClassExpr) { | ||
| // skip class identifier | ||
| node.class.visit_with(self); | ||
| } | ||
|
|
||
| fn visit_import_decl(&mut self, _: &ImportDecl) { | ||
| // skip | ||
| } | ||
|
|
||
| fn visit_ts_import_equals_decl(&mut self, node: &TsImportEqualsDecl) { | ||
| if node.is_type_only { | ||
| return; | ||
| } | ||
|
|
||
| let TsModuleRef::TsEntityName(ts_entity_name) = &node.module_ref else { | ||
| return; | ||
| }; | ||
|
|
||
| let id = get_module_ident(ts_entity_name); | ||
|
|
||
| if node.is_export { | ||
| id.visit_with(self); | ||
| node.id.visit_with(self); | ||
| return; | ||
| } | ||
|
|
||
| self.import_chain.insert(node.id.to_id(), id.to_id()); | ||
| } | ||
|
|
||
| fn visit_export_named_specifier(&mut self, node: &ExportNamedSpecifier) { | ||
| if node.is_type_only { | ||
| return; | ||
| } | ||
|
|
||
| node.orig.visit_with(self); | ||
| } | ||
|
|
||
| fn visit_named_export(&mut self, node: &NamedExport) { | ||
| if node.type_only || node.src.is_some() { | ||
| return; | ||
| } | ||
|
|
||
| node.visit_children_with(self); | ||
| } | ||
|
|
||
| fn visit_jsx_element_name(&mut self, node: &JSXElementName) { | ||
| if matches!(node, JSXElementName::Ident(i) if i.sym.starts_with(|c: char| c.is_ascii_lowercase())) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| node.visit_children_with(self); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between the NamespaceUsageCollect struct (lines 1818-1932) and the SemanticAnalyzer in semantic.rs. Both implement nearly identical visit methods for tracking identifier usage (visit_ident, visit_binding_ident, visit_fn_decl, visit_fn_expr, visit_class_decl, visit_class_expr, visit_import_decl, visit_ts_import_equals_decl, visit_export_named_specifier, visit_named_export, visit_jsx_element_name) and both have the same analyze_import_chain logic. Consider extracting a shared usage collection trait or struct to reduce this duplication and improve maintainability.
| @@ -2,11 +2,12 @@ | |||
| #![allow(clippy::vec_box)] | |||
| #![allow(clippy::mutable_key_type)] | |||
|
|
|||
There was a problem hiding this comment.
The public function strip_type() has been removed from the API by removing it from the re-exports in lib.rs. This is a breaking change for any external code that was calling strip_type() directly. The changeset marks this as a "patch" version change, but removing a public API typically requires at least a "minor" version bump in semantic versioning. Consider whether this API removal is intentional and documented, or if the strip_type function should still be exposed (possibly as a thin wrapper around the new implementation).
| pub use self::typescript::strip_type; |
| } | ||
| } | ||
|
|
||
| /// ## This Module will transform all TypeScript specific synatx |
There was a problem hiding this comment.
Spelling error: "synatx" should be "syntax".
| /// ## This Module will transform all TypeScript specific synatx | |
| /// ## This Module will transform all TypeScript specific syntax |
|
@claude Fix cargo clippy. Run |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
|
|
1 similar comment
|
|
PR Review: refactor(es/typescript): Run typescript transform in two passesSummaryThis PR consolidates three separate AST passes ( ArchitectureThe two-pass design is a solid improvement:
This reduces the number of full-tree traversals and improves cache locality. The Code QualityPositives:
Minor concerns:
Potential Issues
Test CoverageThe PR description lists running:
This covers the relevant test suites. No test files are modified, which indicates this is a behavior-preserving refactor — good. CI checks are still pending at time of review. SecurityNo security concerns. This is a pure refactoring of AST transformation logic with no I/O, network access, or user-facing input handling changes. VerdictThis is a clean architectural improvement. The main actionable items are:
|
2 participants
Summary
Validation