From d4371e8f95af8ef79cee5c803a08800a0143ae90 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 27 May 2024 01:53:13 +0100 Subject: [PATCH] fix(transformer): use UIDs in TS namespace transforms (#3395) Use the `TraverseCtx::generate_uid` method introduced in #3395 to fix some of the TS namespace test cases. But... I honestly have no idea what I'm doing here! I am running up against a combination of 3 different areas where I know very little: 1. I am unfamiliar with `Semantic`. 2. I am unfamiliar with Oxc's conventions for writing transforms. 3. I don't "speak" Typescript! This PR should not be merged as is. I'm pretty sure it's wrong. I've done it mostly just to test out `generate_uid`. In particular: 1. Is `SymbolFlags::FunctionScopedVariable` the right flags to use in all cases here? 2. `generate_uid` creates a symbol, and registers a binding for it in the scope tree. So if there are any branches in this logic where `name` doesn't actually get used after `generate_uid` is called, then it's not right. 3. Identifiers which are added to AST should also have corresponding `ReferenceId`s created for them (but I imagine this is also missing in many other places in the transformer). On point 2: We could split up `generate_uid` into 2 functions. One function would find a valid UID name *but not register a binding for it*. If you want to actually use that name, you'd call a 2nd function to generate the binding. Would that be a better API? Could someone help me out to progress this please? (Sorry my lack of knowledge is a bit useless here. I will learn all these things in time, but just trying to be honest about where I'm at right now. I'm sure I could figure it out myself, but it would take me hours, whereas others will probably look at it and know what to do in about 5 mins.) --- crates/oxc_semantic/src/scope.rs | 4 +++ crates/oxc_transformer/src/lib.rs | 4 +-- crates/oxc_transformer/src/typescript/mod.rs | 4 +-- .../src/typescript/namespace.rs | 33 +++++++++++++++---- tasks/transform_conformance/babel.snap.md | 8 ++--- 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/crates/oxc_semantic/src/scope.rs b/crates/oxc_semantic/src/scope.rs index 66ff5135c..82024e535 100644 --- a/crates/oxc_semantic/src/scope.rs +++ b/crates/oxc_semantic/src/scope.rs @@ -151,6 +151,10 @@ impl ScopeTree { self.bindings[scope_id].insert(name, symbol_id); } + pub fn remove_binding(&mut self, scope_id: ScopeId, name: &CompactStr) { + self.bindings[scope_id].shift_remove(name); + } + pub(crate) fn add_unresolved_reference( &mut self, scope_id: ScopeId, diff --git a/crates/oxc_transformer/src/lib.rs b/crates/oxc_transformer/src/lib.rs index d0a0f3ba8..69bb39e74 100644 --- a/crates/oxc_transformer/src/lib.rs +++ b/crates/oxc_transformer/src/lib.rs @@ -92,8 +92,8 @@ impl<'a> Transformer<'a> { } impl<'a> Traverse<'a> for Transformer<'a> { - fn enter_program(&mut self, program: &mut Program<'a>, _ctx: &mut TraverseCtx<'a>) { - self.x0_typescript.transform_program(program); + fn enter_program(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + self.x0_typescript.transform_program(program, ctx); } fn exit_program(&mut self, program: &mut Program<'a>, _ctx: &mut TraverseCtx<'a>) { diff --git a/crates/oxc_transformer/src/typescript/mod.rs b/crates/oxc_transformer/src/typescript/mod.rs index 776f35ac5..4fcd307f8 100644 --- a/crates/oxc_transformer/src/typescript/mod.rs +++ b/crates/oxc_transformer/src/typescript/mod.rs @@ -68,8 +68,8 @@ impl<'a> TypeScript<'a> { // Transforms impl<'a> TypeScript<'a> { - pub fn transform_program(&self, program: &mut Program<'a>) { - self.transform_program_for_namespace(program); + pub fn transform_program(&self, program: &mut Program<'a>, ctx: &mut TraverseCtx) { + self.transform_program_for_namespace(program, ctx); } pub fn transform_program_on_exit(&self, program: &mut Program<'a>) { diff --git a/crates/oxc_transformer/src/typescript/namespace.rs b/crates/oxc_transformer/src/typescript/namespace.rs index 14cc23eb7..8d849e8c8 100644 --- a/crates/oxc_transformer/src/typescript/namespace.rs +++ b/crates/oxc_transformer/src/typescript/namespace.rs @@ -4,14 +4,22 @@ use super::{diagnostics::ambient_module_nested, TypeScript}; use oxc_allocator::{Box, Vec}; use oxc_ast::{ast::*, syntax_directed_operations::BoundNames}; -use oxc_span::{Atom, SPAN}; -use oxc_syntax::operator::{AssignmentOperator, LogicalOperator}; +use oxc_span::{Atom, CompactStr, SPAN}; +use oxc_syntax::{ + operator::{AssignmentOperator, LogicalOperator}, + symbol::SymbolFlags, +}; +use oxc_traverse::TraverseCtx; // TODO: // 1. register scope for the newly created function: impl<'a> TypeScript<'a> { // `namespace Foo { }` -> `let Foo; (function (_Foo) { })(Foo || (Foo = {}));` - pub(super) fn transform_program_for_namespace(&self, program: &mut Program<'a>) { + pub(super) fn transform_program_for_namespace( + &self, + program: &mut Program<'a>, + ctx: &mut TraverseCtx, + ) { // namespace declaration is only allowed at the top level if !has_namespace(program.body.as_slice()) { @@ -31,7 +39,7 @@ impl<'a> TypeScript<'a> { Statement::TSModuleDeclaration(decl) => { if !decl.modifiers.is_contains_declare() { if let Some(transformed_stmt) = - self.handle_nested(self.ctx.ast.copy(&decl).unbox(), None) + self.handle_nested(self.ctx.ast.copy(&decl).unbox(), None, ctx) { let name = decl.id.name(); if names.insert(name.clone()) { @@ -51,7 +59,7 @@ impl<'a> TypeScript<'a> { { if !decl.modifiers.is_contains_declare() { if let Some(transformed_stmt) = - self.handle_nested(self.ctx.ast.copy(decl), None) + self.handle_nested(self.ctx.ast.copy(decl), None, ctx) { let name = decl.id.name(); if names.insert(name.clone()) { @@ -119,12 +127,17 @@ impl<'a> TypeScript<'a> { &self, decl: TSModuleDeclaration<'a>, parent_export: Option>, + ctx: &mut TraverseCtx, ) -> Option> { let mut names: FxHashSet> = FxHashSet::default(); let real_name = decl.id.name(); - let name = self.ctx.ast.new_atom(&format!("_{}", real_name.clone())); // path.scope.generateUid(realName.name); + // TODO: This binding is created in wrong scope. + // Needs to be created in scope of function which `transform_namespace` creates below. + let name = self.ctx.ast.new_atom( + &ctx.generate_uid_in_current_scope(real_name, SymbolFlags::FunctionScopedVariable), + ); let namespace_top_level = if let Some(body) = decl.body { match body { @@ -161,7 +174,7 @@ impl<'a> TypeScript<'a> { } let module_name = decl.id.name().clone(); - if let Some(transformed) = self.handle_nested(decl.unbox(), None) { + if let Some(transformed) = self.handle_nested(decl.unbox(), None, ctx) { is_empty = false; if names.insert(module_name.clone()) { new_stmts.push(Statement::from( @@ -233,6 +246,7 @@ impl<'a> TypeScript<'a> { Some(self.ctx.ast.identifier_reference_expression( IdentifierReference::new(SPAN, name.clone()), )), + ctx, ) { is_empty = false; if names.insert(module_name.clone()) { @@ -267,6 +281,11 @@ impl<'a> TypeScript<'a> { } if is_empty { + // Delete the scope binding that `ctx.generate_uid_in_current_scope` created above, + // as no binding is actually being created + let current_scope_id = ctx.current_scope_id(); + ctx.scopes_mut().remove_binding(current_scope_id, &CompactStr::from(name.as_str())); + return None; } diff --git a/tasks/transform_conformance/babel.snap.md b/tasks/transform_conformance/babel.snap.md index 9f9096a37..b5c9200f8 100644 --- a/tasks/transform_conformance/babel.snap.md +++ b/tasks/transform_conformance/babel.snap.md @@ -1,6 +1,6 @@ commit: 4bd1b2c2 -Passed: 309/351 +Passed: 313/351 # All Passed: * babel-preset-react @@ -21,18 +21,14 @@ Passed: 309/351 * opts/optimizeConstEnums/input.ts * opts/rewriteImportExtensions/input.ts -# babel-plugin-transform-typescript (123/154) +# babel-plugin-transform-typescript (127/154) * enum/mix-references/input.ts * enum/scoped/input.ts * enum/ts5.0-const-foldable/input.ts * exports/declared-types/input.ts * imports/enum-value/input.ts * imports/type-only-export-specifier-2/input.ts -* namespace/contentious-names/input.ts * namespace/empty-removed/input.ts -* namespace/module-nested/input.ts -* namespace/module-nested-export/input.ts -* namespace/multiple/input.ts * namespace/mutable-fail/input.ts * namespace/namespace-flag/input.ts * namespace/namespace-nested-module/input.ts