refactor(es/typescript): Convert TypeScript transforms to VisitMutHook pattern

[kdy1]

Summary

• Converted StripType and StripImportExport to implement VisitMutHook<()> instead of VisitMut
• Updated TypeScript transform to use CompositeHook and VisitMutWithHook for composition
• Made strip_type and strip_import_export modules public for hook usage
• Fixed namespace handling in StripImportExport to properly track depth and process nested modules

Technical Details

This refactoring aligns with the architectural guidelines in crates/swc_ecma_transformer/AGENTS.md:

• Transform types implement VisitMutHook<TraverseCtx> instead of VisitMut
• Hooks are composed using CompositeHook and executed via VisitMutWithHook
• Main Transformer orchestrates hook execution

Key changes:

• StripType: Converted all visit_mut_* methods to enter_* and exit_* hook callbacks
• StripImportExport: Properly handles namespace depth tracking and processes module items before children
• Integration: Uses CompositeHook to chain strip_import_export::hook() and strip_type::hook()

Test plan

• Run existing TypeScript transform tests
• Verify namespace and module import/export handling
• Check that type stripping works correctly
• Ensure no behavioral changes from the refactoring

🤖 Generated with Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: cb17976

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

[claude]

Pull Request Review

I've reviewed PR #11432 which refactors the TypeScript transforms to use the VisitMutHook pattern. Overall, this is a well-executed architectural refactoring that aligns with the project's guidelines. Here's my detailed feedback:

✅ Strengths

1. Architectural Alignment: The refactoring correctly follows the guidelines in crates/swc_ecma_transformer/AGENTS.md:

   • Transform subtypes now implement VisitMutHook<TraverseCtx> instead of VisitMut
   • Uses CompositeHook for composition
   • Uses VisitMutWithHook for execution

2. Improved Namespace Handling: The namespace depth tracking in StripImportExport is a significant improvement:

   • Properly tracks depth with enter_ts_module_block and exit_ts_module_block
   • Correctly processes nested modules recursively
   • Separates top-level and nested module processing logic

3. Code Organization: The refactoring makes the code more composable and maintainable:

   • Clear separation of concerns between StripType and StripImportExport
   • Hook composition is explicit and understandable

⚠️ Issues Found

1. Visibility Issue (Compiler Warning)

pub fn hook() -> StripType {
    StripType::default()
}

pub(crate) struct StripType {
    in_namespace: bool,
}

Location: crates/swc_ecma_transforms_typescript/src/strip_type.rs:5

Issue: The function hook() is public but returns a pub(crate) type, causing a private_interfaces warning.

Fix: Either make StripType public or make the hook() function pub(crate):

pub(crate) fn hook() -> StripType {
    StripType::default()
}

2. Potential Logic Issue in Namespace Depth Tracking

Location: crates/swc_ecma_transforms_typescript/src/strip_import_export.rs:337-343

The namespace_depth tracking has a potential issue: the depth is incremented in enter_ts_module_block, but if enter_module_items is called for the top-level module before any TsModuleBlock is visited, the depth will be 0. However, looking at the code more carefully, I see that process_top_level_module_items manually processes nested modules first (lines 348-355), which should work correctly.

Recommendation: Add a comment explaining why manual processing is needed before the visitor traverses children, to make the logic clearer for future maintainers.

3. Missing Stack Growth Protection

Location: crates/swc_ecma_transforms_typescript/src/strip_type.rs

The old code had maybe_grow_default for stack protection:

fn visit_mut_expr(&mut self, n: &mut Expr) {
    // ...
    maybe_grow_default(|| n.visit_mut_children_with(self));
}

Current code (line 137):

fn enter_expr(&mut self, n: &mut Expr, _ctx: &mut ()) {
    // Handles TsAs, TsSatisfies, TsNonNull, TsConstAssertion
    while let Expr::TsAs(...) | ... {
        *n = *expr.take();
    }
    // No explicit child traversal - handled by hook framework
}

Analysis: The hook framework should handle child traversal automatically, but if the while loop itself can be deeply nested in the AST structure, there might be a stack overflow risk. This is likely fine since the loop only unwraps assertion wrappers, but worth monitoring.

Recommendation: Consider whether stack protection is needed for the unwrapping loop, especially for pathological cases with many nested assertions.

📝 Code Quality Observations

1. Good: The refactoring maintains all the original functionality while improving structure
2. Good: Comments are helpful (e.g., lines 323-324 in strip_import_export.rs)
3. Good: The separation of process_top_level_module_items and process_nested_module_items is clearer than the original StripTsImportEquals visitor

🧪 Test Coverage

The PR description includes a test plan but doesn't indicate tests have been run yet. According to project guidelines in AGENTS.md:

• ✅ You should run tests after making changes
• ✅ Write unit tests for your code
• ✅ Run cargo fmt --all before committing

Recommendation:

# Run TypeScript transform tests
cargo test --package swc_ecma_transforms_typescript

# Ensure code is formatted
cargo fmt --all

🚀 Performance Considerations

The refactoring should have minimal performance impact:

• Hook composition adds a small indirection cost but maintains the same traversal pattern
• The namespace processing logic is reorganized but performs the same operations
• No additional allocations or unnecessary cloning detected

🔒 Security Concerns

No security issues identified. The changes are architectural and don't introduce new security risks.

📋 Recommendations Summary

1. Must Fix: Change visibility of hook() function to pub(crate) to resolve compiler warning
2. Should Add: Comment explaining manual nested module processing logic
3. Should Verify: Run full test suite to ensure no behavioral changes
4. Optional: Consider stack protection for deeply nested type assertions

Conclusion

This is a solid refactoring that improves code architecture and maintainability. The main issue is the visibility warning which should be fixed before merging. Once that's addressed and tests pass, this PR should be good to merge.

Overall Rating: ⭐⭐⭐⭐ (4/5) - Well-executed refactoring with one minor issue to fix

---

🤖 Review generated by Claude Code

[Copilot]

Pull request overview

This PR refactors the TypeScript transforms to use the VisitMutHook pattern instead of the traditional VisitMut pattern, aligning with the architectural guidelines in AGENTS.md. The refactoring converts StripType and StripImportExport to implement VisitMutHook<()> and uses CompositeHook for composition.

Key changes:

• Converted visitor methods from visit_mut_* to enter_*/exit_* hook callbacks
• Introduced hook factory functions for creating visitor instances
• Made modules public to support external hook usage

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/swc_ecma_transforms_typescript/src/typescript.rs	Updated to use CompositeHook for composing strip_import_export and strip_type hooks with VisitMutWithHook
crates/swc_ecma_transforms_typescript/src/strip_type.rs	Converted from VisitMut to VisitMutHook<()> with enter_*/exit_* methods; removed two specialized visitor methods
crates/swc_ecma_transforms_typescript/src/strip_import_export.rs	Converted to VisitMutHook<()> with namespace depth tracking and separated processing logic into helper methods
crates/swc_ecma_transforms_typescript/src/lib.rs	Made strip_type and strip_import_export modules public; removed wildcard re-export
crates/swc_ecma_transforms_typescript/Cargo.toml	Added swc_ecma_hooks dependency
Cargo.lock	Reflected the new dependency in the lock file

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The namespace tracking logic doesn't properly handle nested namespaces. When entering a nested TsModuleBlock, in_namespace is unconditionally set to true, but when exiting, it's unconditionally set to false. This means that if we have nested namespaces, exiting the inner namespace will incorrectly mark the outer namespace as not being in a namespace.

The original code used mem::replace to save and restore the previous state. The hook-based pattern should track depth instead (similar to how StripImportExport does with namespace_depth), or save/restore the previous value of in_namespace.

[claude]

Pull Request Review: Refactor TypeScript transforms to VisitMutHook pattern

Summary

This PR successfully refactors StripType and StripImportExport to use the VisitMutHook pattern, aligning with the architectural guidelines in crates/swc_ecma_transformer/AGENTS.md. The refactoring is well-structured and maintains backward compatibility while improving code organization.

✅ Strengths

1. Architectural Alignment

• Correctly implements VisitMutHook<()> instead of VisitMut, following the documented pattern
• Properly uses CompositeHook to chain transformations
• Makes subtypes composable and easier to test in isolation

2. Code Quality

• Clean separation of concerns between StripType and StripImportExport
• Good use of enter_* and exit_* hooks at appropriate times
• Maintains existing behavior while improving structure

3. Namespace Handling Improvements

The namespace depth tracking is a notable improvement:

fn enter_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.namespace_depth += 1;
}

fn exit_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.namespace_depth -= 1;
}

This is more robust than the previous mem::replace approach.

⚠️ Issues & Concerns

1. Critical: Missing Child Visitation in StripType ⚠️

In strip_type.rs, several hooks that previously called visit_mut_children_with no longer do so implicitly. The VisitMutWithHook wrapper handles this, BUT there's a potential issue with hooks that filter collections before children are visited:

Location: crates/swc_ecma_transforms_typescript/src/strip_type.rs:86

fn enter_class_members(&mut self, n: &mut Vec<ClassMember>, _ctx: &mut ()) {
    n.retain(|member| match member {
        ClassMember::TsIndexSignature(..) => false,
        // ... filtering logic
    });
}

Issue: The enter_class_members hook filters items but is called BEFORE children are visited. The previous implementation called n.visit_mut_children_with(self) AFTER filtering. Now the VisitMutWithHook will visit children after enter_* returns, but the filtering happens in enter_*, not exit_*.

Impact: Likely benign since we're removing type-only members that don't need child processing, but this is a subtle ordering change that should be verified.

Recommendation: Consider whether filtering should happen in exit_class_members instead, or add a comment explaining why filtering in enter_* is correct here.

2. Missing maybe_grow_default for Stack Safety

Location: crates/swc_ecma_transforms_typescript/src/strip_type.rs:134

The old code had:

fn visit_mut_expr(&mut self, n: &mut Expr) {
    // ... unwrap logic
    maybe_grow_default(|| n.visit_mut_children_with(self));
}

The new code removes maybe_grow_default:

fn enter_expr(&mut self, n: &mut Expr, _ctx: &mut ()) {
    // ... unwrap logic (same)
}

Issue: maybe_grow_default was protecting against stack overflow on deeply nested expressions. This is now handled by VisitMutWithHook, but the unwrapping loop in enter_expr itself could still overflow on pathological ASTs with deeply chained type assertions.

Recommendation: Consider re-adding maybe_grow_default around the unwrapping loop:

fn enter_expr(&mut self, n: &mut Expr, _ctx: &mut ()) {
    maybe_grow_default(|| {
        while let Expr::TsAs(..) | Expr::TsNonNull(..) | ... = n {
            *n = *expr.take();
        }
    });
}

3. Unused Import

Location: crates/swc_ecma_transforms_typescript/src/strip_import_export.rs:4

use swc_ecma_utils::stack_size::maybe_grow_default;

This import is no longer used after the refactoring and should be removed.

4. Documentation: Context Type Mismatch

Location: crates/swc_ecma_transformer/AGENTS.md:5

The guidelines state:

Subtypes must implement VisitMutHook<TraverseCtx>.

However, this PR implements VisitMutHook<()>, using () as the context type instead of TraverseCtx.

Impact: This is actually fine - the guidelines are general, and () is appropriate when no context is needed. However, this discrepancy might confuse future contributors.

Recommendation: Either:

• Update the guidelines to mention that () can be used when no context is needed, OR
• Add a comment in the code explaining why () is used instead of TraverseCtx

5. Public API Change

Location: crates/swc_ecma_transforms_typescript/src/lib.rs:5-7

-pub use self::{strip_type::*, typescript::*};
+pub use self::typescript::*;
 mod config;
 mod macros;
-mod strip_import_export;
-mod strip_type;
+pub mod strip_import_export;
+pub mod strip_type;

Issue: The modules are now public, but their types (StripType, StripImportExport) are no longer re-exported at the crate root. This is a breaking change - users who previously used swc_ecma_transforms_typescript::StripType would need to change to swc_ecma_transforms_typescript::strip_type::StripType.

Recommendation: Either:

• Restore the pub use with explicit types: pub use self::strip_type::StripType;, OR
• Document this as a breaking change in the PR description

🎯 Testing

✅ Passed Tests

• All namespace tests pass (5/5)
• 189/190 strip tests pass
• The architecture correctly separates concerns

❓ Test Failure

One test failed (issue_960_2), but this appears to be a test infrastructure issue (missing mocha), not a logic problem with the refactoring.

Recommendation: Ensure the test environment has all required dependencies, or skip this test in CI if it's environment-specific.

🔒 Security

No security concerns identified. The refactoring maintains the same security properties as the original code.

⚡ Performance

Positive: The hook-based architecture is slightly more efficient than the previous visitor pattern because:

• CompositeHook composes statically without virtual dispatch
• Enter/exit hooks are inlined
• No unnecessary tree traversals

Neutral: The context passing (&mut ()) has zero cost due to optimization.

📝 Minor Suggestions

1. Code Formatting

Please run cargo fmt --all before committing (per AGENTS.md guidelines).

2. Commit Message

The generated commit message is clear and follows conventions. Good work!

3. Consider Adding Tests

While existing tests pass, consider adding specific tests for:

• Deeply nested namespaces (to verify namespace_depth tracking)
• Complex type assertion chains (to verify the unwrapping logic)
• Mixed scenarios with both type stripping and import/export stripping

🎓 Overall Assessment

Rating: 8/10 - This is a solid refactoring that successfully modernizes the codebase architecture. The issues identified are mostly minor and can be addressed with small follow-up changes.

Action Items:

1. ⚠️ High Priority: Remove unused maybe_grow_default import from strip_import_export.rs
2. Medium Priority: Consider re-adding maybe_grow_default to the enter_expr unwrapping loop
3. Medium Priority: Clarify the public API change (re-export types or document as breaking change)
4. Low Priority: Update documentation to explain () context usage
5. Low Priority: Add comment explaining why enter_class_members filters before children are visited

Great work on aligning with the architectural patterns! 🚀

---

🤖 Review generated with Claude Code

[claude]

Code Review: TypeScript Transforms Hook Refactoring

Summary

This PR successfully refactors the TypeScript transforms to follow the architectural guidelines in crates/swc_ecma_transformer/AGENTS.md. The changes convert StripType and StripImportExport from implementing VisitMut directly to using the VisitMutHook<()> pattern.

✅ Strengths

1. Architectural Compliance: The refactoring correctly follows the documented guidelines:

   • Subtypes now implement VisitMutHook<TraverseCtx> instead of VisitMut
   • The main TypeScript transformer orchestrates hook execution via CompositeHook and VisitMutWithHook
   • This improves composability and modularity

2. Improved Namespace Handling: The StripImportExport implementation now properly tracks namespace depth:

   • namespace_depth field prevents incorrect processing of nested module items
   • enter_ts_module_block and exit_ts_module_block hooks track depth correctly
   • Top-level vs nested namespace processing is properly separated

3. Cleaner Hook API: Converting from visit_mut_* to enter_*/exit_* hooks:

   • Makes the visitor lifecycle more explicit
   • Follows a more standard visitor pattern
   • Children visiting is handled by the framework, reducing boilerplate

4. Proper Hook Composition: The use of CompositeHook in typescript.rs ensures both hooks run in the correct order (enter methods in order, exit methods in reverse).

⚠️ Issues & Concerns

1. Critical: Namespace Depth Tracking Bug

fn enter_module_items(&mut self, n: &mut Vec<ModuleItem>, _ctx: &mut ()) {
    // ...
    if self.namespace_depth == 0 {
        self.process_top_level_module_items(n);
    }
}

Problem: The namespace_depth will be 0 for ALL module_items because enter_ts_module_block increments the depth AFTER module_items are processed. The hook traversal order is:

1. enter_module → visits children
2. enter_module_items (depth=0) ← processed BEFORE entering the TsModuleBlock
3. enter_ts_module_block (depth=1)

Impact: The depth check won't work as intended. Nested namespace module items may be processed when they shouldn't be.

Suggested Fix: Track depth differently, perhaps using a separate method or checking the parent node type directly.

2. Removed Public API

-pub use self::{strip_type::*, typescript::*};
+pub use self::typescript::*;

Problem: This removes strip_type from the public API, which is marked as a major breaking change in the changeset. However, the changeset description doesn't clearly document what users should migrate to.

Question: Is this intentional? If so, should the changeset include migration guidance?

3. Performance Consideration in strip_type.rs

The original code used maybe_grow_default when processing expressions:

// Old
fn visit_mut_expr(&mut self, n: &mut Expr) {
    // ...
    maybe_grow_default(|| n.visit_mut_children_with(self));
}

// New
fn enter_expr(&mut self, n: &mut Expr, _ctx: &mut ()) {
    // ... no stack growth check
}

Problem: The stack growth protection was removed. Expression trees can be deeply nested and cause stack overflows.

Question: Does VisitMutWithHook handle stack growth internally, or should this protection be re-added?

4. Module Processing Logic Complexity

The new namespace handling in strip_import_export.rs introduces three methods:

• process_top_level_module_items
• process_nested_module_items
• process_all_nested_modules

Concern: The logic is somewhat complex. The comment "Process module items BEFORE children are visited" suggests a specific ordering requirement, but hooks naturally process in a specific order anyway.

Question: Could this logic be simplified? The recursive processing in process_all_nested_modules seems to manually replicate what the visitor framework would do automatically.

🔍 Minor Issues

1. Inconsistent Hook Timing:

   • Some hooks clear fields in exit_* (e.g., exit_array_pat)
   • Others clear in enter_* (e.g., enter_opt_ts_type)
   • Recommendation: Document why different timing is used

2. Unused VisitMut Methods Removed:

   • visit_mut_script was removed from StripImportExport
   • Question: Are Scripts still handled correctly, or was this path never used?

3. Context Type: Both hooks use () as the context type

   • Question: Are there plans to add context in the future? If so, using a named type might make migration easier.

✅ Testing

• Tests pass (189 passed; 1 failed due to unrelated mocha runtime issue)
• The failed test issue_960_2 appears to be an infrastructure issue, not related to these changes

📋 Recommendations

Before Merge:

1. ✅ Fix or clarify the namespace depth tracking logic
2. ✅ Add comment explaining stack growth handling for expressions
3. ✅ Update changeset with migration guidance if API removal is intentional

Nice to Have:

1. Add inline comments explaining the module processing order requirements
2. Consider simplifying the nested namespace processing logic
3. Add unit tests specifically for nested namespace import/export handling

📊 Overall Assessment

Quality: Good architectural refactoring with proper hook composition
Risk: Medium - namespace depth tracking needs verification, stack growth protection removed
Recommendation: Request clarification on the identified issues before merging

The refactoring successfully achieves its goal of converting to the hook pattern, but the namespace handling logic needs review to ensure it works as intended.

---

Review generated by Claude Code

[claude]

Pull Request Review

Summary

This PR refactors TypeScript transforms to use the VisitMutHook pattern, aligning with the architectural guidelines in crates/swc_ecma_transformer/AGENTS.md. The changes convert StripType and StripImportExport from VisitMut implementations to VisitMutHook<()> implementations and compose them using CompositeHook.

---

✅ Strengths

1. Architectural Alignment: The refactoring properly follows the documented architectural pattern where transform types implement VisitMutHook<TraverseCtx> instead of VisitMut, as specified in crates/swc_ecma_transformer/AGENTS.md.

2. Hook Composition: The use of CompositeHook to chain strip_import_export::hook() and strip_type::hook() is elegant and reduces the traversal overhead from two sequential visit_mut_with calls to a single pass.

3. Namespace Handling: The namespace depth tracking in StripImportExport (lines 294, 336-342 in strip_import_export.rs) is a smart solution to distinguish top-level from nested namespaces.

4. Code Cleanliness: Removal of the nested StripTsImportEquals visitor and integration of its logic directly into StripImportExport simplifies the codebase.

5. Test Coverage: 189/190 tests pass, indicating the refactoring maintains existing behavior.

---

⚠️ Issues & Concerns

1. Failing Test (Critical)

• Issue: The issue_960_2 test is failing with "failed to find mocha from path"
• Location: Test suite execution
• Analysis: This appears to be a test infrastructure issue rather than a code issue, but needs investigation to confirm the transform logic is correct
• Recommendation: Verify this is purely an environment issue and not masking a real behavioral change

2. Public API Changes (Major)

• Issue: The PR changes module visibility but removes public exports
• Location: crates/swc_ecma_transforms_typescript/src/lib.rs:5
• Code: Changed from pub use self::{strip_type::*, typescript::*}; to pub use self::typescript::*;
• Impact: This is marked as a major version bump in the changeset, which is appropriate. However, any code depending on directly importing StripType or related types will break
• Recommendation: ✅ Correct - major version bump is appropriate. Consider documenting the migration path for users.

3. Verbatim Module Syntax Early Return

• Issue: Early returns when verbatim_module_syntax is true could skip important hook initialization
• Location: strip_import_export.rs:292-295, 330-332
• Code:

fn enter_module(&mut self, n: &mut Module, _ctx: &mut ()) {
    if self.verbatim_module_syntax {
        return;  // Skips usage_info and declare_info collection
    }

• Analysis: This is likely intentional but could be error-prone if future hooks depend on this initialization
• Recommendation: Add a comment explaining why the early return is safe and what guarantees it provides

4. Context Type is Unit ()

• Observation: Both hooks use VisitMutHook<()> with unit context
• Location: strip_type.rs:18, strip_import_export.rs:309
• Analysis: This is acceptable but limits future extensibility. If cross-hook state sharing becomes necessary, this would require refactoring
• Recommendation: This is fine for now, but consider if a shared context type might be beneficial in the future

5. Missing Stack Overflow Protection

• Issue: StripType previously used maybe_grow_default for deep recursion protection in visit_mut_expr
• Location: Previously at strip_type.rs (removed in this PR)
• Old Code: maybe_grow_default(|| n.visit_mut_children_with(self));
• Impact: Deep AST traversal could potentially cause stack overflow on complex expressions
• Analysis: The hook system may handle this differently, but this deserves verification
• Recommendation: ⚠️ Verify that the hook system provides equivalent stack overflow protection, or re-add it if necessary

---

🔍 Code Quality Observations

Good Practices:

• Clear separation of concerns between top-level and nested processing
• Recursive processing of nested modules is well-structured
• Usage of namespace_depth tracking is clear and maintainable

Minor Issues:

1. Naming: The function hook() is somewhat generic. Consider create_hook() or new() for clarity
2. Documentation: The public hook functions lack doc comments explaining their purpose and parameters
3. Code Comments: The complex namespace processing logic could benefit from more inline comments explaining the traversal order

---

🔒 Security Considerations

No security issues identified. This is a refactoring that maintains existing behavior without introducing new attack surfaces.

---

⚡ Performance Considerations

Positive Impact:

• Combining two sequential visitors into a single pass should improve performance by reducing AST traversal overhead
• The hook system may allow better compiler optimizations through composition

Potential Concern:

• The CompositeHook pattern calls both hooks' enter_* methods before recursing, then calls both exit_* methods. This is correct but slightly different from two sequential passes. Ensure this doesn't affect any transformation that depends on seeing partially-processed AST state.

---

📋 Test Coverage

Current State: 189/190 tests pass (99.5%)

Recommendations:

1. Investigate and fix the issue_960_2 test failure
2. Add specific tests for the new namespace depth tracking if not already covered
3. Consider adding tests that verify the hook composition order is correct

---

🎯 Final Recommendations

1. Before Merge: Resolve or document the issue_960_2 test failure
2. Consider: Adding doc comments to public hook() functions
3. Verify: Stack overflow protection is maintained in the new architecture
4. Optional: Add inline comments to complex namespace traversal logic

---

Overall Assessment

Status: ✅ Approve with minor concerns

This is a well-executed architectural refactoring that properly aligns with the project's design guidelines. The code is clean, the hook composition is elegant, and the test coverage is excellent. The main concern is the single failing test, which needs investigation to ensure it's not masking a behavioral change.

The major version bump is appropriate given the API changes. Once the test failure is resolved, this PR significantly improves the codebase's architectural consistency.

---

Reviewed with Claude Code

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	28M (28422728 bytes)

Commit: 3c45ca3

[codspeed-hq]

CodSpeed Performance Report

Merging this PR will not alter performance

_{Comparing kdy1/merge-ts (cb17976) with main (dd007c6)}

Summary

✅ 185 untouched benchmarks

[claude]

Code Review for PR #11432

Summary

This PR refactors the TypeScript transforms to use the VisitMutHook pattern, aligning with the architectural guidelines in AGENTS.md. The overall approach is sound and follows the documented patterns well.

---

🔴 Critical Issues

1. Duplicated Code in hook_utils.rs (2,482 lines)

The most significant issue is the wholesale duplication of VisitMutWithHook from swc_ecma_hooks/src/generated.rs into crates/swc_ecma_transforms_typescript/src/hook_utils.rs.

Location: crates/swc_ecma_transforms_typescript/src/hook_utils.rs:6-2482

Problem:

• This creates 2,482 lines of duplicated, generated code that must now be maintained separately
• The comment says it's a "Fork of [VisitMutWithHook] that has [maybe_grow_default] applied to it"
• However, only ONE method has maybe_grow_default applied: visit_mut_expr (line ~687 in the original)
• All other methods are identical to the upstream implementation

Recommendation:
Instead of duplicating the entire implementation, consider one of these approaches:

1. Wrapper approach (preferred for minimal changes):

pub(crate) struct VisitMutImpl<H, C> {
    inner: swc_ecma_hooks::VisitMutWithHook<H, C>,
}

impl<H: VisitMutHook<C>, C> VisitMut for VisitMutImpl<H, C> {
    // Only override visit_mut_expr
    fn visit_mut_expr(&mut self, node: &mut Expr) {
        self.inner.hook.enter_expr(node, &mut self.inner.context);
        maybe_grow_default(|| node.visit_mut_children_with(&mut self.inner));
        self.inner.hook.exit_expr(node, &mut self.inner.context);
    }
    
    // Delegate all other methods
    fn visit_mut_program(&mut self, node: &mut Program) {
        self.inner.visit_mut_program(node)
    }
    // ... etc for other methods
}

2. Upstream fix (better long-term):

• Add maybe_grow_default to the appropriate places in the code generator for swc_ecma_hooks
• Use VisitMutWithHook directly without any wrapper

Impact: This affects maintainability significantly. Any changes to the visitor pattern would need to be applied in two places.

---

⚠️ Moderate Issues

2. Namespace Depth Tracking Could Use Documentation

Location: crates/swc_ecma_transforms_typescript/src/strip_import_export.rs:294,337-343

The namespace_depth field is well-implemented but could benefit from clearer documentation:

/// Tracks nesting depth of TypeScript namespaces/modules.
/// 0 = top-level module
/// >0 = inside nested namespace
namespace_depth: usize,

Minor: The enter/exit pattern is correct and handles nesting properly.

3. Potential Edge Case in Nested Module Processing

Location: crates/swc_ecma_transforms_typescript/src/strip_import_export.rs:492-504

The process_all_nested_modules method recursively processes nested modules, but there's potential for deep recursion with heavily nested namespaces:

fn process_all_nested_modules(&self, n: &mut Vec<ModuleItem>) {
    // Recursively process deeper nested namespaces first
    for module_item in n.iter_mut() {
        if let ModuleItem::Stmt(Stmt::Decl(Decl::TsModule(ts_module))) = module_item {
            if let Some(TsNamespaceBody::TsModuleBlock(ref mut block)) = ts_module.body {
                self.process_all_nested_modules(&mut block.body);  // Could stack overflow
            }
        }
    }
    // ...
}

Recommendation: Consider using maybe_grow_default here for deeply nested namespace scenarios, though this is likely rare in practice.

---

✅ Positive Aspects

1. Architectural Alignment: The refactoring correctly follows the pattern specified in AGENTS.md:

   • Hooks implement VisitMutHook<TraverseCtx> ✓
   • Main transformer uses CompositeHook for composition ✓
   • Proper separation of concerns between StripType and StripImportExport ✓

2. Hook Composition: The use of CompositeHook in typescript.rs:47-54 is clean and idiomatic:

let hook = CompositeHook {
    first: strip_import_export::hook(...),
    second: strip_type::hook(),
};

3. Context-Free Design: Using () as the context type is appropriate since these transforms don't need shared state.

4. Namespace Handling: The fix for namespace depth tracking (StripImportExport:337-343) properly addresses nested module processing.

5. Type Safety: The conversions maintain type safety throughout, especially in the SimpleAssignTarget handling (strip_type.rs:194-205).

---

📋 Minor/Style Issues

4. Unused #[allow] Attributes

The generated code in hook_utils.rs has many #[inline] attributes that may not provide benefits given the code size. Consider profiling before/after to ensure this doesn't negatively impact compile times.

5. Code Formatting

Location: Multiple files

Ensure cargo fmt --all has been run as specified in AGENTS.md before committing.

---

🧪 Testing Considerations

Test Coverage Recommendations:

1. Deeply nested namespaces - Test the recursive processing with 5+ levels of nesting
2. TsImportEquals in nested modules - Verify usage tracking works correctly
3. Performance regression tests - The new hook pattern should not significantly impact performance
4. Edge cases:
   • Empty modules/namespaces
   • Namespaces with only type declarations
   • Circular namespace references (if possible)

The PR indicates tests should be run but doesn't show test additions for the namespace depth tracking changes.

---

🔒 Security

No security concerns identified. The changes are architectural refactoring without new external inputs or unsafe code.

---

📊 Performance Considerations

1. Hook Composition Overhead: The CompositeHook pattern adds minimal overhead (two hook calls instead of monolithic logic). This is acceptable for the architectural benefits.

2. Stack Growth: The addition of maybe_grow_default in visit_mut_expr is good for preventing stack overflows with deeply nested expressions.

3. Consideration: The 2,482-line hook_utils.rs will impact compile times. If compile time becomes an issue, the wrapper approach mentioned above would help.

---

📝 Documentation

The PR body provides excellent documentation of the changes. Consider adding similar details to the code:

• Comment explaining why VisitMutImpl exists vs using upstream VisitMutWithHook
• Document the namespace depth tracking strategy

---

Summary

Must Address Before Merge:

• Eliminate code duplication in hook_utils.rs (Issue ecmascript parser #1)

Should Address:

• Add documentation for namespace_depth field (Issue Clean up #2)
• Consider maybe_grow_default in recursive namespace processing (Issue EcmaScript lexer is inefficient #3)

Nice to Have:

• Add specific tests for deeply nested namespaces
• Run benchmarks to ensure no performance regression
• Document the rationale for VisitMutImpl in code comments

Overall Assessment: This is a well-executed architectural refactoring that follows the project guidelines. The main blocker is the code duplication issue which significantly impacts maintainability. Once that's resolved, this PR will be in excellent shape.

---

Review generated by Claude Code 🤖

[claude]

Pull Request Review: TypeScript Transform Refactoring

Summary

This PR successfully refactors the TypeScript transforms to follow the VisitMutHook pattern as specified in crates/swc_ecma_transformer/AGENTS.md. The refactoring is well-structured and aligns with the project's architectural guidelines.

---

✅ Strengths

Architecture & Design

• Excellent adherence to architectural guidelines: The PR correctly implements the hook pattern where subtypes (StripType, StripImportExport) implement VisitMutHook<()> instead of VisitMut, exactly as required by AGENTS.md
• Clean composition: Uses CompositeHook to chain hooks together elegantly
• Good separation of concerns: Import/export stripping is separate from type stripping, making the code more maintainable

Code Quality

• Well-structured conversion: The conversion from visit_mut_* methods to enter_*/exit_* hooks is clean and systematic
• Proper state management: namespace_depth tracking in StripImportExport correctly handles nested modules
• Clear intent: The code clearly documents what each hook does and when it executes

Performance Considerations

• Stack overflow protection: The custom VisitMutImpl correctly applies maybe_grow_default to visit_mut_expr, which is crucial for deeply nested expressions
• Efficient traversal order: Processing nested modules before filtering ensures correct behavior without multiple passes

---

⚠️ Issues & Concerns

1. Code Duplication: 2,482 lines in hook_utils.rs

Severity: High - This is the biggest concern with this PR.

The entire VisitMutImpl struct (2,482 lines) is duplicated from the standard VisitMutWithHook just to add maybe_grow_default to one method (visit_mut_expr). This creates significant maintenance burden.

Recommendation:

• Consider upstreaming the maybe_grow_default addition to the generated VisitMutWithHook in swc_ecma_hooks
• Or use a wrapper pattern that delegates to VisitMutWithHook but overrides only visit_mut_expr
• If this duplication is temporary, add a clear TODO comment explaining the plan

Example wrapper pattern:

pub(crate) struct VisitMutImpl<H, C> {
    inner: VisitMutWithHook<H, C>,
}

impl<H: VisitMutHook<C>, C> VisitMut for VisitMutImpl<H, C> {
    fn visit_mut_expr(&mut self, node: &mut Expr) {
        self.inner.hook.enter_expr(node, &mut self.inner.context);
        maybe_grow_default(|| node.visit_mut_children_with(&mut self.inner));
        self.inner.hook.exit_expr(node, &mut self.inner.context);
    }
    
    // Delegate all other methods...
}

2. Namespace Depth Tracking Logic

Severity: Medium

In strip_import_export.rs:325-335, the logic for processing module items only at the top level (namespace_depth == 0) is correct, but the relationship between enter_module_items and process_all_nested_modules is subtle and could be clearer.

Recommendation:

• Add a comment explaining why nested namespaces are not processed in enter_module_items (because they're handled by process_all_nested_modules)
• Consider renaming namespace_depth to ts_module_depth for clarity

3. Empty Context Type

Severity: Low

Both hooks use VisitMutHook<()> with an empty context. This is fine for now, but if you ever need to share state between hooks, you'll need to refactor.

Recommendation:

• Consider whether a shared context type might be beneficial in the future
• Document why () is sufficient for these transforms

4. Test Coverage Verification Needed

Severity: Medium

The PR description includes an unchecked test plan. Before merging:

Recommendations:

• ✅ Run existing TypeScript transform tests
• ✅ Verify namespace and module import/export handling
• ✅ Check that type stripping works correctly
• ✅ Ensure no behavioral changes from the refactoring
• ✅ Add regression tests for the namespace depth tracking fixes mentioned in the PR description

---

🔍 Specific Code Review

strip_type.rs

Positive:

• Clean conversion of all visit_mut_* to enter_*/exit_* hooks
• Proper use of enter_* for filtering operations (e.g., enter_class_members, enter_module_items)
• Correct preservation of namespace context with in_namespace flag

Minor suggestion:

• Line 148-154: Consider extracting the namespace tracking to a helper method for reusability

strip_import_export.rs

Positive:

• Good separation between top-level and nested module processing
• Proper usage info collection and analysis
• Correct handling of TsImportEquals declarations

Concerns:

• Lines 492-504: process_all_nested_modules is recursive and could potentially hit stack limits with very deeply nested namespaces. Consider adding a depth limit or using an iterative approach.

typescript.rs

Positive:

• Clean integration of the composed hooks
• Proper use of mem::take to move ownership
• Good preservation of ESM context

hook_utils.rs

Concern:

• As mentioned above, this entire file is problematic due to duplication

---

🔒 Security

No security concerns identified. The refactoring maintains the same security properties as the original code.

---

📊 Performance

Positive:

• The hook pattern should have similar or better performance than the original VisitMut implementation
• maybe_grow_default correctly prevents stack overflow
• Single-pass processing for most operations

Neutral:

• The CompositeHook adds a small amount of overhead for double enter/exit calls, but this is negligible
• Recursive process_all_nested_modules could be a concern for pathological inputs (see above)

---

✅ Recommendations Before Merge

1. High Priority: Address the hook_utils.rs duplication issue
2. Medium Priority: Run and verify all tests in the test plan
3. Medium Priority: Add comments explaining namespace depth tracking
4. Low Priority: Consider stack depth limits for recursive namespace processing
5. Low Priority: Run cargo fmt --all to ensure formatting consistency (as per CLAUDE.md)

---

📝 Final Verdict

This is a well-executed refactoring that correctly implements the architectural pattern. The main blocker is the 2,482 lines of duplicated code in hook_utils.rs, which needs to be addressed before merging. Once that's resolved, this PR will be a great improvement to the codebase architecture.

Suggested Action: Request changes to address the code duplication, then approve after that's resolved.

---

🤖 Generated with Claude Code - Automated PR Review

[claude]

Code Review Summary

I've reviewed PR #11432 which refactors TypeScript transforms to use the VisitMutHook pattern. Overall, this is a well-executed architectural improvement that aligns with the project's design guidelines. Here's my detailed feedback:

---

✅ Strengths

Architecture & Design

• Excellent alignment with guidelines: The refactoring properly follows crates/swc_ecma_transformer/AGENTS.md, converting transforms to implement VisitMutHook<TraverseCtx> instead of VisitMut
• Clean composition: Using CompositeHook to chain strip_import_export::hook() and strip_type::hook() is elegant and maintainable
• Proper separation of concerns: The main TypeScript transformer orchestrates hook execution while subtypes handle specific transformations

Code Quality

• Consistent conversion: Both StripType and StripImportExport are converted systematically from visit_mut_* methods to enter_*/exit_* hook callbacks
• Good use of Rust patterns: Proper use of Default trait, clear naming conventions, and appropriate visibility modifiers (pub(crate))
• Stack overflow protection: The custom VisitMutImpl correctly applies maybe_grow_default for deep expression trees (line 439 in hook_utils.rs)

---

⚠️ Concerns & Suggestions

1. Large Generated File (hook_utils.rs - 2482 lines)

Issue: The hook_utils.rs file is entirely boilerplate code implementing the visitor pattern for every AST node type.

Concerns:

• Increases maintenance burden if not generated
• If hand-written, very error-prone
• Adds 2620 lines to the codebase

Questions:

• Is this code generated or hand-written?
• If generated, should the generator be committed to the repo?
• If hand-written, could it be replaced by a shared implementation from swc_ecma_hooks?

Recommendation:

// Consider if this could be upstreamed to swc_ecma_hooks as:
pub struct VisitMutWithStackGrowth<H, C> { ... }

This would benefit other transforms needing stack growth protection.

2. Namespace Depth Tracking Edge Case

Location: strip_import_export.rs:337-343

fn enter_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.namespace_depth += 1;
}

fn exit_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.namespace_depth -= 1;
}

Concern: If an error/panic occurs during traversal, the depth counter could become corrupted in subsequent transforms (though the visitor should be single-use).

Suggestion: Consider documenting that StripImportExport is single-use, or use RAII pattern if the transform can be reused.

3. Method Ordering in process_top_level_module_items

Location: strip_import_export.rs:347-355

The code processes nested modules first, then the current level:

// Process nested TsModules first
for module_item in n.iter_mut() {
    if let ModuleItem::Stmt(Stmt::Decl(Decl::TsModule(ts_module))) = module_item {
        if let Some(TsNamespaceBody::TsModuleBlock(ref mut block)) = ts_module.body {
            self.process_all_nested_modules(&mut block.body);
        }
    }
}

Question: Is this depth-first traversal necessary for correctness, or just an implementation detail? Consider adding a comment explaining why nested modules must be processed before the top level.

4. Test Coverage

Observation: The PR marks "Run existing TypeScript transform tests" as unchecked in the test plan.

Strong Recommendation:

• Run the existing test suite: cargo test -p swc_ecma_transforms_typescript
• Verify no behavioral changes from the refactoring
• Specifically test:
  • Nested namespace handling with the new depth tracking
  • Import/export stripping with various configurations
  • Edge cases like empty imports, type-only imports, etc.

---

🔒 Security & Safety

• ✅ No unsafe code introduced
• ✅ No obvious security vulnerabilities (no external input handling)
• ✅ Stack overflow protection maintained via maybe_grow_default

---

⚡ Performance Considerations

Positive:

• Hook composition via CompositeHook should have minimal overhead
• The refactoring maintains existing performance characteristics

Potential Concern:

• The new architecture involves more indirection (hook dispatch), but this is likely negligible
• Consider benchmarking TypeScript stripping performance before/after if this is a hot path

---

📋 Recommendations Summary

1. High Priority: Run the full test suite and mark it as complete in the PR
2. Medium Priority: Clarify whether hook_utils.rs is generated and document the generation process
3. Low Priority: Add comments explaining the depth-first traversal strategy
4. Consider: Upstreaming VisitMutImpl to swc_ecma_hooks for reuse

---

🎯 Final Verdict

This is a solid architectural refactoring that improves code organization and follows project conventions. The main concern is ensuring test coverage is complete. Once tests pass, this should be safe to merge.

Approval Status: ✅ Approved pending test verification

Great work on this refactoring! The hook pattern makes the code more composable and maintainable.

[claude]

Pull Request Review: TypeScript Transform Hook Pattern Refactoring

Overview

This PR refactors the TypeScript transforms to use the VisitMutHook pattern, aligning with the architectural guidelines in crates/swc_ecma_transformer/AGENTS.md. The refactoring converts StripType and StripImportExport from implementing VisitMut directly to implementing VisitMutHook<()>.

Positive Aspects

Architectural Alignment ✓

• The changes properly follow the documented pattern where subtypes implement VisitMutHook<TraverseCtx> instead of VisitMut
• Uses CompositeHook correctly to chain hooks together
• Good separation of concerns between the two transformation phases

Code Quality ✓

• Clear factory functions (hook()) for instantiating hooks
• Well-structured hook composition in typescript.rs:47-54
• Maintains existing behavior while improving architecture
• Good documentation in the PR description

Issues and Concerns

1. CRITICAL: Large Generated File Added (hook_utils.rs - 2,482 lines)

Location: crates/swc_ecma_transforms_typescript/src/hook_utils.rs

Issue: This PR adds a 2,482-line generated file hook_utils.rs that appears to be a fork of VisitMutWithHook with maybe_grow_default applied.

Problems:

• Code Duplication: This duplicates functionality that should exist in a shared location
• Maintainability: Generated code in the repository is hard to maintain and can get out of sync
• Size: Adding 2,482 lines for what is essentially a wrapper is excessive
• Proper Location: This belongs in swc_ecma_hooks or swc_ecma_visit, not in the TypeScript transforms crate

Recommendation:

// Instead of duplicating 2,482 lines, consider:
// 1. Add VisitMutImpl to swc_ecma_hooks crate with maybe_grow_default
// 2. Or create a macro/trait that applies stack growth to any VisitMut implementation
// 3. Or use the existing VisitMutWithHook and wrap specific methods that need stack growth

// Example approach:
use swc_ecma_hooks::VisitMutWithHook;
use swc_ecma_utils::stack_size::maybe_grow_default;

struct StackSafeVisitor<H, C> {
    inner: VisitMutWithHook<H, C>,
}

impl<H: VisitMutHook<C>, C> VisitMut for StackSafeVisitor<H, C> {
    fn visit_mut_expr(&mut self, node: &mut Expr) {
        maybe_grow_default(|| self.inner.visit_mut_expr(node));
    }
    // Only override methods that need stack growth
}

2. Namespace Depth Tracking Logic

Location: strip_import_export.rs:294,337-343

Issue: The ts_module_depth field is added to track nesting, but the logic could be clearer.

Current Code:

fn enter_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.ts_module_depth += 1;
}

fn exit_ts_module_block(&mut self, _node: &mut TsModuleBlock, _ctx: &mut ()) {
    self.ts_module_depth -= 1;
}

Concerns:

• What happens if there's an early return or panic between enter and exit?
• Initial value of ts_module_depth should be documented (it's 0 for top-level)
• Consider using RAII pattern for depth tracking

Recommendation:

// Add documentation
/// Depth of TypeScript module/namespace nesting. 
/// 0 = top-level, increments for each nested namespace
ts_module_depth: usize,

// Or use a guard pattern:
struct DepthGuard<'a>(&'a mut usize);
impl<'a> DepthGuard<'a> {
    fn new(depth: &'a mut usize) -> Self {
        *depth += 1;
        Self(depth)
    }
}
impl Drop for DepthGuard<'_> {
    fn drop(&mut self) {
        *self.0 -= 1;
    }
}

3. Processing Order in enter_module_items

Location: strip_import_export.rs:325-335

Issue: The comment says "Process module items BEFORE children are visited" but the depth check logic could be more explicit.

fn enter_module_items(&mut self, n: &mut Vec<ModuleItem>, _ctx: &mut ()) {
    if self.verbatim_module_syntax {
        return;
    }

    // Only process top-level
    // Nested namespaces are handled by process_all_nested_modules
    if self.ts_module_depth == 0 {  // ⚠️ Should this be strictly 0?
        self.process_top_level_module_items(n);
    }
}

Questions:

• Is ts_module_depth == 0 the correct check, or should we check if we're entering a module?
• What happens to module items at ts_module_depth > 0? They're skipped in enter_module_items but processed in process_all_nested_modules - is this intentional?

Recommendation: Add more detailed comments explaining the processing phases and why certain depths are skipped.

4. Performance Consideration: Nested Module Processing

Location: strip_import_export.rs:347-355,492-504

Issue: The code processes nested modules recursively in two passes:

fn process_top_level_module_items(&mut self, n: &mut Vec<ModuleItem>) {
    // Process nested TsModules first
    for module_item in n.iter_mut() {
        if let ModuleItem::Stmt(Stmt::Decl(Decl::TsModule(ts_module))) = module_item {
            if let Some(TsNamespaceBody::TsModuleBlock(ref mut block)) = ts_module.body {
                self.process_all_nested_modules(&mut block.body);  // Recursive
            }
        }
    }
    // Then process this level...
}

Concern:

• For deeply nested namespaces, this could have O(n²) behavior
• Multiple passes over the same nodes might impact performance

Recommendation: Profile with deeply nested namespaces. Consider a single-pass algorithm if performance becomes an issue.

5. Memory Usage: UsageCollect and DeclareCollect

Location: strip_import_export.rs:312-320

Issue: Both UsageCollect and DeclareCollect visit the entire module to build FxHashSet collections.

fn enter_module(&mut self, n: &mut Module, _ctx: &mut ()) {
    if self.verbatim_module_syntax {
        return;
    }

    n.visit_with(&mut self.usage_info);  // Full traversal
    n.visit_with(&mut self.declare_info);  // Full traversal

    self.usage_info.analyze_import_chain();
}

Concern:

• Two full AST traversals before transformation
• For large files, this could consume significant memory

Recommendation:

• Consider combining these into a single visitor if performance is critical
• Or document that this is intentional for clarity

6. Error Handling: Unwrap in strip_type.rs

Location: strip_type.rs:203

fn enter_simple_assign_target(&mut self, n: &mut SimpleAssignTarget, _ctx: &mut ()) {
    while let SimpleAssignTarget::TsAs(TsAsExpr { expr, .. })
    // ... other patterns
    {
        *n = expr.take().try_into().unwrap();  // ⚠️ Panic risk
    }
}

Issue: Uses .unwrap() which could panic. While this should always succeed if the AST is valid, it's not defensive.

Recommendation:

// Add a comment explaining why this is safe:
// SAFETY: Expr can always convert to SimpleAssignTarget in these cases
// because we're only stripping type assertion wrappers
*n = expr.take().try_into().unwrap();

// Or use expect with a message:
*n = expr.take().try_into()
    .expect("Type assertion should always contain valid SimpleAssignTarget");

Testing Recommendations

Missing from PR:

1. No test results shown - Please run existing tests and confirm they pass
2. Namespace depth tests - Add tests for deeply nested namespaces (3+ levels)
3. Edge cases:
   • Empty namespaces
   • Namespace with only type declarations
   • Mixed value/type exports in nested namespaces
4. Performance tests - Compare performance with old implementation on large files

Test Commands to Run:

# As mentioned in AGENTS.md for this crate
cd crates/swc_ecma_transforms_typescript
./scripts/test.sh

# If execution tests exist
./scripts/exec.sh

Security Considerations

✓ No security issues identified. The refactoring doesn't introduce:

• Command injection risks
• Unsafe code blocks
• External dependencies
• Data exposure

Best Practices per CLAUDE.md

✓ Followed:

• Written documentation
• Performance-conscious design
• English comments
• Using Cow<str>/&str for atoms
• Unit test structure preserved

⚠️ Needs Attention:

• "Run cargo fmt --all before committing files" - Verify this was done
• "Write unit tests for your code" - Are there tests for the new hook composition?
• "When instructed to fix tests, do not remove or modify existing tests" - Ensure all tests still pass

Overall Assessment

Architectural Quality: 8/10 - Good alignment with guidelines, clean hook composition
Code Quality: 6/10 - Main concern is the 2,482-line generated file
Performance: 7/10 - Multiple traversals may impact performance
Maintainability: 6/10 - Generated file reduces maintainability significantly

Recommendations Priority

1. CRITICAL: Remove or relocate hook_utils.rs (2,482 lines) to shared infrastructure
2. HIGH: Add test results and confirm all tests pass
3. MEDIUM: Document depth tracking logic and processing phases more clearly
4. MEDIUM: Consider combining the two visitor passes for better performance
5. LOW: Add defensive error messages to .unwrap() calls

Conclusion

This is a well-intentioned architectural refactoring that successfully adopts the hook pattern. However, the addition of a massive generated file is a significant maintainability concern that should be addressed before merging. The refactoring demonstrates good understanding of the target architecture, but the implementation approach needs refinement to avoid code duplication and technical debt.

Recommendation: Request changes, primarily to address the generated file issue.

[kdy1]

@magic-akari I'm working to merge the whole typescript pass into a single visit using VisitMutHook, and this is a first PR for it. Any concern about this PR?

[magic-akari]

@magic-akari I'm working to merge the whole typescript pass into a single visit using VisitMutHook, and this is a first PR for it. Any concern about this PR?

This isn’t specifically about this PR, but about the broader direction of this migration. For context, this split-visit design was something I originally suggested, so this is more of a retrospective concern on my side.

Over time, I’ve found that the oxc-style pattern of splitting a single visit into two parts can feel ergonomically awkward and tends to make code review harder. I’ve experimented with a few alternative approaches and suspect there may be cleaner solutions, although I haven’t landed on a concrete replacement yet.

I’m curious what your thoughts are on this design today. If better approaches emerge, would we be open to revisiting or evolving this direction?

[kdy1]

Yeap, of course. My primary concern of this migration is the performance overhead of the visitor pattern. If there's a better alternative, I'm 100% open to migrating to it.

Beside it, I agree that this kind of visitor hook system has bad impact on code review. It has far more interactions between passes compared to standalone visitors, and that's why I'm trying to do it gradually, but doing it gradually wasn't enough for code review.

[magic-akari]

My primary concern of this migration is the performance overhead of the visitor pattern.

I was initially expecting this migration to bring some performance improvements, but looking at the Codespeed results, there's no measurable change. I'm curious to understand your specific concerns about the visitor pattern overhead.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[kdy1]

I'll try merging more visitors in this PR to see if it really improves the performance.
There are too many visitors in typescript operation, and I only merged two of them

[claude]

Pull Request Review: TypeScript Transform Refactoring to VisitMutHook Pattern

Overview

This PR successfully refactors the TypeScript transforms to follow the VisitMutHook pattern as outlined in crates/swc_ecma_transformer/AGENTS.md. The refactoring is architecturally sound and aligns with the project's goals for composable transform hooks.

Code Quality & Best Practices

✅ Strengths

1. Architectural Alignment: The PR correctly follows the documented pattern where:

   • Transform subtypes (StripType, StripImportExport) implement VisitMutHook<()> instead of VisitMut
   • Hooks are composed using CompositeHook and executed via VisitMutImpl
   • The main TypeScript transformer orchestrates hook execution

2. Clean Conversion: The conversion from visit_mut_* methods to enter_* and exit_* hook callbacks is systematic and well-executed. For example, in strip_type.rs:

   • enter_* methods handle pre-order logic (e.g., stripping type annotations)
   • exit_* methods handle post-order cleanup (e.g., clearing type-only fields)

3. Proper Depth Tracking: The StripImportExport implementation correctly tracks namespace depth using ts_module_depth in lines 294, 337-343 of strip_import_export.rs, preventing incorrect processing of nested modules.

4. Code Comments: Good inline documentation explaining complex logic, especially around TypeScript proposal links (e.g., lines 126-127, 139, 165 in strip_type.rs).

⚠️ Concerns & Issues

1. Large Generated File (hook_utils.rs - 2,482 lines)

Severity: Medium

The hook_utils.rs file contains a massive generated VisitMutImpl implementation. While the comment at line 6 indicates it's a "fork of [VisitMutWithHook] that has [maybe_grow_default] applied to it," this raises questions:

• Maintainability: Having a 2,482-line generated file in the codebase could be problematic for maintenance and code review
• Alternative Approach: Could this be solved with a macro or procedural macro instead? Or by contributing the maybe_grow_default integration back to the swc_ecma_hooks crate?
• Code Duplication: The file essentially duplicates the VisitMutWithHook pattern from swc_ecma_hooks with one modification (adding maybe_grow_default on line 508)

Recommendation: Consider:

1. Contributing a feature to swc_ecma_hooks to support stack size checking
2. Using a procedural macro to generate this wrapper
3. At minimum, add comments about why this duplication is necessary and how to regenerate it

2. Namespace Processing Order (strip_import_export.rs:348-355)

Severity: Low

In process_top_level_module_items, nested TsModules are processed before filtering the top-level items. The logic appears correct but is complex:

// Process nested TsModules first
for module_item in n.iter_mut() {
    if let ModuleItem::Stmt(Stmt::Decl(Decl::TsModule(ts_module))) = module_item {
        if let Some(TsNamespaceBody::TsModuleBlock(ref mut block)) = ts_module.body {
            self.process_all_nested_modules(&mut block.body);
        }
    }
}

Suggestion: Add a comment explaining why nested modules must be processed first (likely to ensure proper import resolution from inner to outer scopes).

3. Empty Context Type (VisitMutHook<()>)

Severity: Low

Both transforms use VisitMutHook<()> with an empty context. While this works, it suggests the hooks don't share any state or context.

Question: Would these transforms benefit from a shared context type in the future? If so, consider using a named context type even if empty initially to make future extension easier.

Performance Considerations

✅ Good Practices

1. Stack Size Management: The VisitMutImpl applies maybe_grow_default to visit_mut_expr (line 508 in hook_utils.rs), which is essential for deeply nested expressions to avoid stack overflows.

2. Efficient Data Structures: Uses FxHashSet and FxHashMap (rustc's fast hash collections) for tracking IDs and usage information.

3. Single-Pass Processing: The composition of hooks via CompositeHook maintains efficient single-pass processing over the AST.

⚠️ Potential Concerns

Multiple Traversals in StripImportExport::enter_module (strip_import_export.rs:312-321):

n.visit_with(&mut self.usage_info);
n.visit_with(&mut self.declare_info);
self.usage_info.analyze_import_chain();

This performs two complete AST traversals in the enter phase. While necessary for correctness, this could impact performance on large files.

Recommendation: Document why these separate traversals are required and whether they could be combined in the future.

Security Concerns

✅ No Major Issues

The refactoring maintains the same security properties as the original code. Type stripping is a relatively safe operation that removes metadata rather than transforming executable code.

Minor Note

The #[cfg(swc_ast_unknown)] branches (e.g., line 270 in strip_type.rs) panic on unknown AST nodes. This is appropriate for development but ensure unknown nodes can't be triggered by malicious input in production.

Test Coverage

⚠️ Testing Gaps

The PR description includes a test plan checklist but all items are unchecked:

• Run existing TypeScript transform tests
• Verify namespace and module import/export handling
• Check that type stripping works correctly
• Ensure no behavioral changes from the refactoring

Critical: Before merging, you should:

1. Run the existing test suite and confirm all tests pass
2. Add tests specifically covering the namespace depth tracking changes (lines 337-343 in strip_import_export.rs)
3. Verify that nested module import handling works correctly (the process_all_nested_modules logic)

The AGENTS.md file indicates you can run tests with:

cargo test -p swc_ecma_transforms_typescript

Architecture & Design

✅ Excellent Design

1. Composability: The use of CompositeHook (typescript.rs:47-54) is a clean pattern for combining multiple transform phases:

let hook = CompositeHook {
    first: strip_import_export::hook(...),
    second: strip_type::hook(),
};

2. Separation of Concerns: Clear separation between:

   • Import/export stripping (based on usage analysis)
   • Type annotation stripping (purely syntactic)

3. Hook Factories: The hook() functions in both modules provide clean factory interfaces.

Breaking Changes

The PR is marked as major in the changeset, which is appropriate since:

• Modules are now pub(crate) instead of potentially private (lines 5, 297 in both files)
• This is an internal refactoring that could affect downstream code depending on these internals

Recommendations

High Priority

1. Complete the test plan - Run all tests and verify no behavioral changes
2. Document hook_utils.rs - Explain why this file exists, how it differs from the standard hook pattern, and how to regenerate it if needed

Medium Priority

3. Consider alternative to hook_utils.rs - Explore macro-based or upstream solutions to avoid the 2,482-line generated file
4. Add comments about traversal order - Document why nested modules are processed before top-level filtering

Low Priority

5. Consider named context type - Use a named empty struct instead of () for easier future extension
6. Add tests for new namespace logic - Specifically test the depth tracking and nested module processing

Conclusion

This is a well-executed architectural refactoring that successfully migrates the TypeScript transforms to the VisitMutHook pattern. The code quality is high, follows project conventions, and maintains the existing functionality.

The main concerns are:

1. The large generated hook_utils.rs file needs justification and documentation
2. Tests must be completed before merging
3. Some complex logic could benefit from additional comments

Recommendation: Approve with changes - Address the testing requirements and document the hook_utils.rs file before merging.

---

Review generated by Claude Code

[socket-security]

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

[Copilot]

Pull request overview

Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The removal of pub use self::{strip_type::*, ...} from the public API represents a breaking change. Previously, items from strip_type module were re-exported, but now only items from typescript module are re-exported.

This is likely intentional as part of the refactoring to make the hook-based implementation internal, but it should be clearly noted as a breaking API change. Users who were directly using strip_type() or other exported items from that module will need to update their code.

Suggested change

	pub use self::typescript::*;
	pub use self::{strip_type::*, typescript::*};

[claude]

Pull Request Review: TypeScript Transforms to VisitMutHook Pattern

Summary

This PR refactors the TypeScript transforms (StripType and StripImportExport) from implementing VisitMut directly to using the VisitMutHook pattern. This is an architectural improvement that aligns with the project's composability guidelines in crates/swc_ecma_transformer/AGENTS.md.

---

✅ Strengths

1. Architectural Alignment: The refactoring correctly follows the documented pattern where transform types implement VisitMutHook<TraverseCtx> rather than VisitMut, allowing for better composition via CompositeHook.

2. Proper Hook Composition: The use of CompositeHook to chain strip_import_export::hook() and strip_type::hook() is well-designed. The composite pattern ensures enter hooks run first→second and exit hooks run second→first (LIFO), which is correct.

3. Namespace Depth Tracking: The StripImportExport now properly tracks ts_module_depth with enter_ts_module_block and exit_ts_module_block hooks. This is a significant improvement over the previous approach.

4. Test Coverage: Tests pass successfully (189/190 - the one failure is infrastructure-related, not code-related).

5. Performance Consideration: The custom VisitMutImpl in hook_utils.rs applies maybe_grow_default to prevent stack overflow on deeply nested expressions, showing attention to performance requirements.

---

⚠️ Concerns & Issues

1. Large Generated File (hook_utils.rs - 2,482 lines)

• This file appears to be a fork of VisitMutWithHook with maybe_grow_default applied
• Question: Why is this needed as a local copy? Could this be contributed upstream to swc_ecma_hooks instead?
• Maintainability: Having a 2.5k line generated file in this crate creates maintenance burden. If the visitor interface changes, this needs regeneration.
• Recommendation: Consider either:
  • Moving this to swc_ecma_hooks as the default implementation
  • Or documenting clearly in code comments that this is a temporary fork until upstream support is added

2. Module Item Processing Order

The enter_module_items hook processes items BEFORE children are visited, but this creates an asymmetry:

fn enter_module_items(&mut self, n: &mut Vec<ModuleItem>, _ctx: &mut ()) {
    if self.verbatim_module_syntax {
        return;
    }
    
    // Only process top-level
    if self.ts_module_depth == 0 {
        self.process_top_level_module_items(n);
    }
}

Issue: The nested module processing happens in process_top_level_module_items which then recursively calls process_all_nested_modules. This manual recursion bypasses the normal visitor pattern.

Recommendation: Consider whether nested namespace processing should happen via exit hooks instead, allowing the normal visitor traversal to handle recursion.

3. Context Type

Both hooks use VisitMutHook<()> with an empty context.

Question: Is there a future plan to use context for sharing state between hooks? If not, this is fine, but if yes, consider documenting what the context will be used for.

4. API Surface

The hook() functions return concrete types rather than trait objects:

pub(crate) fn hook() -> StripType { ... }
pub(crate) fn hook(...) -> StripImportExport { ... }

This is good for performance (no dynamic dispatch), but means these hooks can't be swapped at runtime. This seems intentional and appropriate for this use case.

---

🔒 Security

No security concerns identified. The code handles TypeScript type stripping, which is a safe transformation that removes syntax.

---

🚀 Performance

• Positive: Uses maybe_grow_default to handle deep expression nesting
• Positive: Uses FxHashSet and FxHashMap (fast hash implementations)
• Neutral: Hook pattern adds minimal overhead compared to direct VisitMut - the function call overhead is negligible and will likely be inlined

---

🧪 Testing

• Coverage: Existing tests pass (189/190)
• Note: The test plan in the PR description is still marked as incomplete. Consider running those checks before merging.

---

📝 Code Quality

Positive:

• Clear separation of concerns (type stripping vs import/export stripping)
• Good use of helper functions (should_retain_module_item, etc.)
• Proper trait implementations (IsConcrete, IsDeclare)

Minor Issues:

• Some repetitive code in process_top_level_module_items for import specifier filtering (lines 366-400)
• Could potentially extract common pattern into a helper

---

🎯 Recommendations

1. Address the hook_utils.rs situation:

   • Document why this fork exists
   • Consider upstreaming to swc_ecma_hooks
   • Or add a comment explaining it's temporary

2. Complete the test plan from the PR description

3. Consider adding a comment in typescript.rs explaining the hook execution order:

   // Hooks execute in order:
   // - strip_import_export (enter) -> strip_type (enter) -> children
   // - children -> strip_type (exit) -> strip_import_export (exit)

4. Documentation: Add module-level documentation explaining the hook pattern for future contributors

---

✅ Verdict

APPROVE with minor suggestions

This is a solid architectural refactoring that improves code organization and follows project conventions. The main concern is the large generated hook_utils.rs file, which should be addressed with either upstreaming or clear documentation. The functionality is sound, tests pass, and the hook pattern is properly implemented.

The PR is ready to merge once the test plan is completed and the hook_utils.rs situation is clarified.

---

References:

• Follows guidelines in: crates/swc_ecma_transformer/AGENTS.md
• Uses patterns from: crates/swc_ecma_hooks/src/

---

🤖 Automated review by Claude Code