feat(es/transformer): Merge async_to_generator

[kdy1]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7e752ea

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]

Code Review for PR #11358: Merge async_to_generator

Thank you for working on this refactoring! This PR aims to consolidate the async_to_generator transformation into the new swc_ecma_transformer architecture. Here's my analysis:

---

❌ Critical Issue: Incomplete Implementation

The main problem is that the actual transformation logic is missing from the new transformer:

File: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs

• This file is essentially empty (1 byte)
• It needs to contain the actual transformation implementation

Current state:

// crates/swc_ecma_transformer/src/es2017/async_to_generator.rs is empty

// crates/swc_ecma_transformer/src/es2017/mod.rs
impl VisitMutHook<TraverseCtx> for Es2017Pass {}  // No hook implementation!

Expected pattern (based on es2016/exponentiation_operator.rs and es2019/optional_catch_binding.rs):

// In async_to_generator.rs
pub fn hook() -> impl VisitMutHook<TraverseCtx> {
    AsyncToGeneratorPass { /* ... */ }
}

struct AsyncToGeneratorPass { /* ... */ }

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {
    // Implement the transformation hooks here
}

// In mod.rs
use crate::hook_utils::OptionalHook;

pub fn hook(options: Es2017Options) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook())
    } else {
        None
    })
}

---

🔴 CI Failure

The Cargo shear check is failing, likely because:

1. swc_ecma_transformer dependency was added to swc_ecma_compat_es2017/Cargo.toml
2. But the old implementation (~854 lines) was deleted without the new implementation being in place
3. This may result in unused dependencies or broken imports

---

📋 What Needs to Be Done

1. Port the transformation logic from the deleted code (~854 lines from crates/swc_ecma_compat_es2017/src/async_to_generator.rs) to the new hook-based architecture in swc_ecma_transformer

2. Key components to port:

   • FnState struct for tracking function state
   • AsyncToGenerator visitor implementation
   • make_fn_ref helper function
   • handle_await_for function for async iteration
   • could_potentially_throw helper
   • ShouldWork optimization check
   • All the complex transformation logic for:
     • Async functions → generator functions with helpers
     • Await expressions → yield expressions
     • Arrow functions
     • this/arguments/super handling
     • For-await-of loop transformations

3. Update es2017/mod.rs to properly integrate the hook (like other ES year modules)

4. Preserve configuration: Ensure ignore_function_length assumption is properly passed through

5. Test thoroughly: The old code had extensive logic for edge cases - all of this needs to work correctly

---

✅ What's Good

• The API design in crates/swc_ecma_compat_es2017/src/async_to_generator.rs looks clean
• Configuration passing through Options is well-structured
• Integration in swc_ecma_preset_env is simplified correctly

---

🎯 Recommendation

This PR cannot be merged in its current state. The transformation implementation needs to be completed first. I recommend:

1. Either finish the migration by porting all the transformation logic to the new architecture
2. Or revert this PR and complete the migration in a draft state before submission

---

📚 References for Implementation

Good examples to follow:

• crates/swc_ecma_transformer/src/es2016/exponentiation_operator.rs (complex transformation with helper functions)
• crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs (simpler hook pattern)

The original 854-line implementation that was deleted contains all the logic needed - it just needs to be adapted to the new hook-based VisitMutHook architecture instead of the old VisitMut pattern.

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	31M (31809608 bytes)

Commit: cdac0ff

[codspeed-hq]

CodSpeed Performance Report

Merging #11358 will improve performances by 2.43%

_{Comparing kdy1/port-async-to-generator (7e752ea) with main (c388e87)¹}

Summary

⚡ 2 improvements
✅ 136 untouched

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	es2017_async_to_generator	298 µs	291 µs	+2.41%
⚡	es/target/es2017	299.9 µs	292.8 µs	+2.43%

Footnotes

1. No successful run was found on main (5aa9f05) during the generation of this report, so c388e87 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[claude]

Pull Request Review: feat(es/transformer): Merge async_to_generator

Summary

This PR attempts to refactor the async_to_generator transform by merging it from swc_ecma_compat_es2017 into the new swc_ecma_transformer architecture. While the direction is correct, the implementation is incomplete and breaks existing functionality.

---

🚨 Critical Issues

1. Missing Implementation ⚠️

The most critical issue is that the actual transformation logic has been removed from crates/swc_ecma_compat_es2017/src/async_to_generator.rs (866 lines deleted), but no implementation exists in crates/swc_ecma_transformer/src/es2017/async_to_generator.rs (which is completely empty).

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs

The file contains no code, yet the old implementation was deleted. This means:

• The async_to_generator transform will not work at all
• All async/await code transformation will fail
• 28 test failures are expected (confirmed by running tests)

2. Test Failures ⚠️

Running cargo test -p swc_ecma_transforms_compat async_to_generator shows 28 test failures, including:

• async_to_generator_async
• async_to_generator_async_arrow_in_method
• async_to_generator_expression
• And 25 more execution tests

This confirms the implementation is broken.

3. Incomplete Integration ⚠️

In crates/swc_ecma_transformer/src/es2017/mod.rs, the hook pattern is set up correctly:

pub fn hook(options: Es2017Options) -> impl VisitMutHook<TraverseCtx> {
    Es2017Pass { options }
}

impl VisitMutHook<TraverseCtx> for Es2017Pass {}

However, there's no actual implementation of the visitor methods. Comparing with working transforms like es2016::exponentiation_operator, the pattern should be:

pub fn hook(options: Es2017Options) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook())
    } else {
        None
    })
}

And async_to_generator::hook() should return a proper implementation.

---

Code Quality Issues

4. Inconsistent Pattern

The ES2017 implementation doesn't follow the pattern used in other ES20XX modules:

• ES2016 uses OptionalHook to conditionally enable transforms
• ES2017 creates a pass struct but never uses the option flag

File: crates/swc_ecma_transformer/src/es2017/mod.rs:21

5. Missing Documentation

The new transformer module has no documentation explaining:

• The transformation it performs
• Examples of input/output
• Links to the spec or Babel equivalent

Compare with es2016::exponentiation_operator.rs which has excellent documentation.

---

What Needs to Be Done

To complete this refactoring:

1. Port the implementation from the deleted code to crates/swc_ecma_transformer/src/es2017/async_to_generator.rs

   • This includes all the visitor logic for transforming async/await
   • Handler for AsyncFunction, AsyncArrowFunction, AwaitExpression
   • Special handling for for-await-of loops
   • Super/this/arguments context handling

2. Implement the hook following the established pattern:

   // In async_to_generator.rs
   pub fn hook() -> impl VisitMutHook<TraverseCtx> {
       AsyncToGeneratorPass { /* ... */ }
   }
   
   impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {
       // Implement visitor methods here
   }

3. Update es2017/mod.rs to use OptionalHook pattern:

   pub fn hook(options: Es2017Options) -> impl VisitMutHook<TraverseCtx> {
       OptionalHook(if options.async_to_generator {
           Some(self::async_to_generator::hook())
       } else {
           None
       })
   }

4. Ensure all tests pass before merging

5. Add documentation following the repository's standards (per CLAUDE.md)

---

Performance Considerations

The refactoring approach is sound from a performance perspective:

• Moving to the hook-based architecture should reduce overhead
• The old implementation already had performance optimizations (like should_work checks)
• These need to be preserved in the new implementation

---

Security Concerns

No security issues identified in the refactoring approach itself. However, the missing implementation means:

• Async/await code won't be transpiled
• This could break production builds that rely on this transform
• This PR should not be merged in its current state

---

Test Coverage

The test coverage expectation is good (28 exec tests exist), but:

• ❌ All async_to_generator tests currently fail
• ❌ No new tests added for the transformer architecture
• ✅ Integration point in preset_env is correct

---

Recommendations

DO NOT MERGE this PR until:

1. ✅ The full implementation is ported to swc_ecma_transformer
2. ✅ All 28 failing tests pass
3. ✅ Documentation is added
4. ✅ The hook pattern matches other ES20XX modules

Positive aspects:

• ✅ The integration point in preset_env is correctly updated
• ✅ The Cargo.toml dependency is properly added
• ✅ The overall architecture direction is correct
• ✅ The config mapping preserves ignore_function_length option

---

References

Per repository guidelines (CLAUDE.md):

• ✅ Need to run execution tests: ./scripts/exec.sh (per crates/swc_ecma_minifier/AGENTS.md)
• ⚠️ Need to implement VisitMutHook properly (per crates/swc_ecma_transformer/AGENTS.md)
• ⚠️ Should read $repositoryRoot/crates/swc_ecma_hooks/src/ before completing implementation

---

Conclusion: This PR has the right idea but is incomplete. The 866 lines of transformation logic need to be ported to the new architecture before this can be merged.

[claude]

Pull Request Review

This PR merges the transformation from into to align with the new hook-based architecture. Overall, the refactoring is well-structured, but there are several issues that need attention.

---

Critical Issues

1. Missing ignore_function_length Support

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:82-100

The new implementation always creates placeholder parameters when use_arguments is true, but ignores the ignore_function_length assumption that was present in the original code.

Original code:

if \!self.c.ignore_function_length {
    let fn_len = function.params.iter()...

New code:

let fn_len = function.params.iter()...  // Always creates params

Fix needed: The hook needs access to the ignore_function_length assumption. You should either:

• Pass it through TraverseCtx, or
• Pass it as a parameter to the hook() function

Impact: This breaks the ignore_function_length assumption, potentially causing incorrect function length calculations.

---

2. Broken enter_class/exit_class Logic

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:218-225

fn enter_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    let in_subclass = mem::replace(&mut self.in_subclass, class.super_class.is_some());
    self.in_subclass = in_subclass;  // ⚠️ This line does nothing\!
}

fn exit_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    self.in_subclass = class.super_class.is_some();  // ⚠️ Incorrect restoration
}

Problem:

• Line 220: mem::replace saves the old value, but then immediately overwrites it with the same value
• Line 224: Restores based on current class, not the saved parent context

Original code (correct):

fn visit_mut_class(&mut self, class: &mut Class) {
    class.super_class.visit_mut_with(self);
    let in_subclass = mem::replace(&mut self.in_subclass, class.super_class.is_some());
    class.body.visit_mut_with(self);
    self.in_subclass = in_subclass;  // Restore saved value
}

Fix:

fn enter_class(&mut self, class: &mut Class, ctx: &mut TraverseCtx) {
    // Save old value to context or a stack
    ctx.old_in_subclass = self.in_subclass;
    self.in_subclass = class.super_class.is_some();
}

fn exit_class(&mut self, _class: &mut Class, ctx: &mut TraverseCtx) {
    self.in_subclass = ctx.old_in_subclass;
}

Impact: Nested classes will have incorrect in_subclass state, potentially causing bugs in constructor transformations.

---

3. Incomplete enter_getter_prop/enter_setter_prop

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:251-259

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    let fn_state = self.fn_state.take();
    self.fn_state = fn_state;  // ⚠️ Does nothing
}

fn enter_setter_prop(&mut self, _f: &mut SetterProp, _ctx: &mut TraverseCtx) {
    let fn_state = self.fn_state.take();
    self.fn_state = fn_state;  // ⚠️ Does nothing
}

Problem: These methods don't actually prevent fn_state from leaking into getter/setter bodies. The original code removed fn_state in visit_mut_getter_prop and restored it after visiting children.

Original approach:

fn visit_mut_getter_prop(&mut self, f: &mut GetterProp) {
    let fn_state = self.fn_state.take();
    f.visit_mut_children_with(self);
    self.fn_state = fn_state;
}

With hooks, you need both enter and exit:

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    if let Some(prev) = self.fn_state.take() {
        self.fn_state_stack.push(prev);
    }
}

fn exit_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    self.fn_state = self.fn_state_stack.pop();
}

Impact: Async functions inside getter/setter properties may not transform correctly.

---

Code Quality Issues

4. Incorrect exit_arrow_expr State Restoration

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:150-159

The logic for restoring parent fn_state is convoluted:

let parent_fn_state = self.fn_state_stack.pop();

if let Some(out_fn_state) = &mut parent_fn_state.as_ref() {
    let mut updated = out_fn_state.clone();
    updated.use_this |= fn_state.use_this;
    updated.use_arguments |= fn_state.use_arguments;
    updated.use_super |= fn_state.use_super;
    self.fn_state = Some(updated);
}

Issues:

• &mut parent_fn_state.as_ref() is incorrect - as_ref() returns an immutable reference, so the &mut is misleading
• Unnecessarily clones the entire state

Better approach:

self.fn_state = parent_fn_state.map(|mut parent| {
    parent.use_this |= fn_state.use_this;
    parent.use_arguments |= fn_state.use_arguments;
    parent.use_super |= fn_state.use_super;
    parent
});

---

5. Missing Documentation

The VisitMutHook implementation lacks documentation explaining:

• Why we need a stack for fn_state
• How nested functions are handled
• The relationship between enter/exit hooks

---

Architecture Concerns

6. TraverseCtx Usage

The hook receives TraverseCtx but doesn't use it effectively. Consider:

• Storing in_subclass stack in ctx
• Passing ignore_function_length through ctx
• This would make the hook more stateless and composable

7. Adherence to AGENTS.md

✅ Good: The transformation correctly implements VisitMutHook<TraverseCtx> as required
✅ Good: Does not implement VisitMut directly (as mandated)

---

Performance Considerations

The implementation looks performant overall:

• ✅ Proper use of should_work for early exits
• ✅ Efficient AST manipulation using Take trait
• ⚠️ Unnecessary cloning in exit_arrow_expr (see issue Transformers for ecmascript #4)

---

Security Concerns

No security issues identified. The transformation properly handles:

• Identifier hygiene via unresolved_ctxt
• Error propagation in async functions

---

Test Coverage

Missing: The PR doesn't show any test additions or modifications. Given this is a significant refactoring:

• ✅ Execution tests should be run (./scripts/exec.sh per AGENTS.md)
• ✅ Fixture tests should be updated if needed (./scripts/test.sh)
• ⚠️ Consider adding tests for nested async functions and edge cases

---

Recommendations

1. Fix critical bugs in issues ecmascript parser #1, Clean up #2, and EcmaScript lexer is inefficient #3 before merging
2. Add access to ignore_function_length via hook parameters or TraverseCtx
3. Refactor in_subclass handling to use a stack or TraverseCtx
4. Implement proper getter/setter isolation with exit hooks
5. Run execution tests as per AGENTS.md to verify correctness
6. Add comments explaining the fn_state_stack mechanism
7. Consider using TraverseCtx more effectively for stateful operations

---

Summary

This is a well-intentioned refactoring that moves in the right direction architecturally, but it contains several correctness bugs that need to be addressed. The transformation from VisitMut to VisitMutHook pattern introduces complexity in state management that wasn't fully accounted for.

Recommendation: Request changes to fix issues #1, #2, and #3 before merging.

[claude]

Code Review for PR #11358: Merge async_to_generator into swc_ecma_transformer

Summary

This PR migrates the async_to_generator transform from the compat crate into the unified swc_ecma_transformer architecture using the VisitMutHook pattern. Overall, the approach is sound and follows the architectural direction outlined in the AGENTS.md instructions.

Critical Issues

1. Bug in enter_class implementation (async_to_generator.rs:215-218)

fn enter_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    let in_subclass = mem::replace(&mut self.in_subclass, class.super_class.is_some());
    self.in_subclass = in_subclass;  // ⚠️ This line negates the previous operation!
}

Issue: The enter_class method saves the old in_subclass value, updates it, but then immediately restores it. This makes the entire operation a no-op.

Fix: Remove the redundant assignment:

fn enter_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    // Just save the old value for restoration in exit_class
    self.in_subclass = class.super_class.is_some();
}

Or if you need to preserve the old value, use a stack pattern similar to fn_state_stack.

2. Bug in enter_getter_prop and enter_setter_prop (async_to_generator.rs:244-252)

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    let fn_state = self.fn_state.take();
    self.fn_state = fn_state;  // ⚠️ No-op!
}

Issue: These methods take the fn_state and immediately restore it, achieving nothing.

Expected behavior: These should clear fn_state on enter (to prevent async tracking inside getter/setter bodies) and restore it on exit. You need corresponding exit_* hooks.

Fix:

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    if let Some(prev) = self.fn_state.take() {
        self.fn_state_stack.push(prev);
    }
}

fn exit_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    self.fn_state = self.fn_state_stack.pop();
}

3. Missing ignore_function_length configuration (async_to_generator.rs:82-93)

let fn_len = function
    .params
    .iter()
    .filter(|p| !matches!(p.pat, Pat::Assign(..) | Pat::Rest(..)))
    .count();

Issue: The old implementation respected c.ignore_function_length config (line 81 in the diff), but the new hook always generates placeholder parameters without checking the assumption. The configuration is passed via options.assumptions.ignore_function_length but never read in the hook.

Fix: Pass ignore_function_length to the hook:

pub fn hook(unresolved_mark: Mark, ignore_function_length: bool) -> impl VisitMutHook<TraverseCtx> {
    AsyncToGeneratorPass {
        ignore_function_length,
        // ... rest
    }
}

And update the logic at line 82:

let params = if fn_state.use_arguments {
    let mut params = vec![];

    if !self.ignore_function_length {
        let fn_len = function
            .params
            .iter()
            .filter(|p| !matches!(p.pat, Pat::Assign(..) | Pat::Rest(..)))
            .count();
        for i in 0..fn_len {
            params.push(Param {
                pat: private_ident!(format!("_{}", i)).into(),
                span: DUMMY_SP,
                decorators: vec![],
            });
        }
    }

    mem::replace(&mut function.params, params)
} else {
    vec![]
};

Moderate Issues

4. Inconsistent state management pattern

The code uses two different patterns for state management:

• For functions/arrows: Uses fn_state + fn_state_stack (enter pushes, exit pops)
• For classes: Uses only in_subclass with enter/exit pairs

Recommendation: Be consistent. Either use a stack pattern everywhere or document why different patterns are used.

5. Missing implementation in hook_utils.rs

The hook_utils.rs file is missing implementations for getter_prop and setter_prop hooks:

optional_method!(enter_getter_prop, exit_getter_prop, GetterProp);
optional_method!(enter_setter_prop, exit_setter_prop, SetterProp);

These are present (lines 64-68), which is good, but verify they're being invoked correctly.

Minor Issues

6. Documentation

The module-level documentation was removed from the old file. Consider adding a comment explaining the hook-based architecture for future maintainers.

7. Code duplication in preset_env

In swc_ecma_preset_env/src/lib.rs, you're setting options directly instead of using the Config struct:

options.assumptions.ignore_function_length = loose || assumptions.ignore_function_length;

This is fine, but make sure this is consistent with how other transforms are configured.

Performance Considerations

✅ Good: The new hook-based implementation maintains the performance optimization of only visiting nodes when async functions are present (through the hook pattern).

✅ Good: The code preserves the "could_potentially_throw" optimization that avoids wrapping arguments when not needed.

Testing

⚠️ Important: Make sure to test:

1. Nested async functions with this/arguments/super usage
2. Async arrow functions inside regular functions
3. Async functions in class constructors with super classes
4. The ignore_function_length configuration option
5. Async generators with yield*

Summary

Blocking Issues (must fix):

1. Fix enter_class no-op bug
2. Fix enter_getter_prop/enter_setter_prop and add exit hooks
3. Wire up ignore_function_length configuration

Recommendations:

1. Consider adding more comprehensive tests for nested function scenarios
2. Document the hook-based architecture
3. Verify all CI tests pass, especially execution tests mentioned in crates/swc_ecma_minifier/AGENTS.md

The architectural direction is correct, but these bugs need to be addressed before merging.

[claude]

Pull Request Review: feat(es/transformer): Merge async_to_generator

Overview

This PR migrates the async_to_generator transformation from swc_ecma_compat_es2017 into the new swc_ecma_transformer hook-based architecture. The migration is substantial (991 additions, 875 deletions across 7 files) and represents a significant architectural improvement. However, there are several critical bugs that must be addressed.

---

🚨 Critical Issues

1. Broken enter_class Implementation

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:240-242

fn enter_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    self.in_subclass = class.super_class.is_some();
}

Problem: The exit_class hook (line 244-246) sets in_subclass back to class.super_class.is_some(), which is incorrect. It should restore the previous value from before entering the class.

Impact: Nested classes will have incorrect subclass detection, potentially breaking constructor transformations that rely on super() handling.

Fix: Use a stack pattern similar to fn_state_stack:

struct AsyncToGeneratorPass {
    in_subclass_stack: Vec<bool>,  // Add this field
    // ... existing fields
}

fn enter_class(&mut self, class: &mut Class, _ctx: &mut TraverseCtx) {
    self.in_subclass_stack.push(self.in_subclass);
    self.in_subclass = class.super_class.is_some();
}

fn exit_class(&mut self, _class: &mut Class, _ctx: &mut TraverseCtx) {
    self.in_subclass = self.in_subclass_stack.pop().unwrap_or(false);
}

---

2. Non-functional enter_getter_prop and enter_setter_prop

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:299-307

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    let fn_state = self.fn_state.take();
    self.fn_state = fn_state;  // ⚠️ This achieves nothing\!
}

Problem: Takes fn_state and immediately puts it back, resulting in a no-op. The original implementation cleared fn_state for the entire visit of getter/setter bodies.

Impact: Async detection may leak into getter/setter property bodies where it shouldn't apply.

Fix: These hooks are incomplete. You need to add exit_getter_prop and exit_setter_prop to OptionalHook in hook_utils.rs, then implement them properly:

fn enter_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    if let Some(prev) = self.fn_state.take() {
        self.fn_state_stack.push(prev);
    }
}

fn exit_getter_prop(&mut self, _f: &mut GetterProp, _ctx: &mut TraverseCtx) {
    self.fn_state = self.fn_state_stack.pop();
}

---

3. Missing ignore_function_length Support

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:88-99

let fn_len = function
    .params
    .iter()
    .filter(|p| \!matches\!(p.pat, Pat::Assign(..) | Pat::Rest(..)))
    .count();
for i in 0..fn_len {
    params.push(Param {
        pat: private_ident\!(format\!("_{}", i)).into(),
        span: DUMMY_SP,
        decorators: vec\![],
    });
}

Problem: The old implementation checked \!self.c.ignore_function_length before creating placeholder parameters. The new implementation always creates them, ignoring the configuration.

Impact: Breaks the ignore_function_length assumption, potentially causing incorrect function.length values in transpiled code.

Fix: Pass the configuration through and check it:

pub fn hook(unresolved_mark: Mark, ignore_function_length: bool) -> impl VisitMutHook<TraverseCtx> {
    AsyncToGeneratorPass {
        ignore_function_length,
        // ... rest
    }
}

// In exit_function:
let params = if fn_state.use_arguments {
    let mut params = vec\![];

    if \!self.ignore_function_length {  // Add this check
        let fn_len = function.params.iter()...
        // ...
    }
    
    mem::replace(&mut function.params, params)
} else {
    vec\![]
};

Also update the call site in es2017/mod.rs.

---

📋 Code Quality Issues

4. Awkward State Management in exit_arrow_expr

Location: crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:158-164

if let Some(out_fn_state) = &parent_fn_state {
    let mut updated = out_fn_state.clone();  // Unnecessary clone
    updated.use_this |= fn_state.use_this;
    updated.use_arguments |= fn_state.use_arguments;
    updated.use_super |= fn_state.use_super;
    self.fn_state = Some(updated);
}

Suggestion: Use map for cleaner code:

self.fn_state = parent_fn_state.map(|mut parent| {
    parent.use_this |= fn_state.use_this;
    parent.use_arguments |= fn_state.use_arguments;
    parent.use_super |= fn_state.use_super;
    parent
});

5. Missing Hook Methods in hook_utils.rs

Location: crates/swc_ecma_transformer/src/hook_utils.rs:64-68

The new getter/setter exit hooks are properly added to OptionalHook, which is good. However, verify that all hooks needed by async_to_generator are present in the trait.

---

✅ Positive Aspects

1. Correct Architecture: Follows the mandated pattern from AGENTS.md - implements VisitMutHook instead of VisitMut
2. Good Integration: The preset_env integration is clean and properly configured
3. Preserves Optimizations: Maintains the could_potentially_throw performance optimization
4. Proper State Management: The fn_state_stack pattern correctly handles nested functions
5. Complete Logic Port: The transformation logic (await→yield, for-await-of, etc.) appears correctly ported

---

🔍 Testing Concerns

According to crates/swc_ecma_minifier/AGENTS.md:

• You should run execution tests: ./scripts/exec.sh
• For fixture tests: ./scripts/test.sh

Recommendation: Before merging, ensure:

1. All existing async_to_generator tests pass
2. Test nested async functions with various this/arguments/super combinations
3. Test the ignore_function_length configuration
4. Test async functions in subclass constructors

---

⚡ Performance Analysis

CodSpeed reports a 2.81% performance regression for the es2017_async_to_generator benchmark.

Analysis:

• The hook-based architecture adds some overhead through the hook dispatch mechanism
• The transformation logic itself appears unchanged, so this is likely architectural overhead
• The regression is within acceptable bounds for an architectural improvement

Recommendations:

1. Profile to identify if specific hot paths can be optimized
2. Consider if the cleaner architecture justifies the minor performance cost
3. Ensure the should_work pattern is still being used effectively (may need to be added to the hook system)

---

🎯 Recommendations

Must Fix Before Merge:

1. ✅ Fix enter_class/exit_class to use proper state restoration
2. ✅ Implement exit_getter_prop and exit_setter_prop
3. ✅ Wire up ignore_function_length configuration

Should Fix:

4. Refactor exit_arrow_expr to avoid unnecessary clone
5. Run all test suites and verify they pass
6. Add tests for edge cases if not already covered

Nice to Have:

7. Add module-level documentation explaining the hook architecture
8. Profile and optimize if the 2.81% regression is concerning

---

📚 Adherence to Repository Guidelines

Per CLAUDE.md:

• ✅ Code follows performant patterns
• ✅ Documentation would benefit from expansion
• ✅ No unstable rustc features used
• ⚠️ Need to verify tests pass (execution tests per minifier AGENTS.md)
• ⚠️ Should run cargo fmt --all before committing

Per crates/swc_ecma_transformer/AGENTS.md:

• ✅ Correctly implements VisitMutHook\<TraverseCtx\>
• ✅ Does not implement VisitMut directly

---

Summary

This is a well-architected refactoring that moves the codebase in the right direction. The transformation logic has been successfully ported, but three critical bugs need to be fixed before merging:

1. Class nesting state management
2. Getter/setter property isolation
3. Configuration option support

Once these are addressed and tests pass, this will be a solid improvement to the codebase.

Verdict: 🔴 Request Changes - Fix the three critical issues, then this will be ready to merge.

[claude]

PR Review: feat(es/transformer): Merge async_to_generator

Summary

This PR refactors the async_to_generator transform by migrating it from swc_ecma_compat_es2017 to the new swc_ecma_transformer architecture using the VisitMutHook pattern. This is part of the broader effort to consolidate transforms into the new hook-based system.

Architecture Review ✅

Positive:

• Correctly follows the architectural pattern specified in crates/swc_ecma_transformer/AGENTS.md
• The transform now implements VisitMutHook<TraverseCtx> instead of VisitMut (as required)
• Properly integrates into the transform_hook builder chain in lib.rs:47-51
• The old implementation in swc_ecma_compat_es2017 now acts as a thin wrapper that delegates to the new transformer

Concerns:

• The dependency direction introduces a circular relationship: swc_ecma_compat_es2017 now depends on swc_ecma_transformer. This may be why "Cargo shear" is failing in CI. Typically, the transformer layer should be independent and higher-level crates should depend on it, not lower-level compat crates.

Code Quality & Correctness ✅

Positive:

• The core transformation logic has been preserved accurately (lines 411-450, 452-479, 482-870)
• Proper use of state stacks (fn_state_stack, in_subclass_stack) to handle nested functions
• The fix for issue Wrong "this" in async arrow function params #4208 is included (line 19 in output: .call(_this) instead of ()), which correctly handles this binding in constructors

Potential Issues:

1. Constructor Context Handling (Lines 188-195, 273-306) ⚠️

The new code introduces special handling for this in constructor context with async arrows:

// In constructor context with `this`, replace `this` with `_this`
if fn_state.in_constructor && fn_state.use_this {
    if self.this_var.is_none() {
        self.this_var = Some(private_ident!("_this"));
    }
    let this_var = self.this_var.clone().unwrap();
    replace_this_in_block_stmt_or_expr(&mut arrow_expr.body, &this_var);
}

Concern: This adds manual this replacement logic (lines 872-981) which duplicates some functionality that FnEnvHoister already provides. The old code used FnEnvHoister::to_stmt_in_subclass() (removed lines 255-260 from the old implementation). This new approach may be correct but is more complex and harder to maintain.

2. Missing #[cfg(swc_ast_unknown)] Handling

The old code had this block that was removed:

#[cfg(swc_ast_unknown)]
_ => panic!("unable to access unknown nodes"),

While this was in a match for BlockStmtOrExpr, the new code doesn't handle this case. This might be acceptable if the cfg is never used, but it's worth noting.

3. State Management Complexity

The use of multiple stacks (fn_state_stack, in_subclass_stack, this_var) increases complexity. The exit handlers must carefully restore state:

• Lines 66-68, 130-131: Function exit restores fn_state
• Lines 161-172: Arrow exit needs to merge state with parent
• Lines 304-305: Constructor exit restores state

A bug in any of these could lead to incorrect transformations in deeply nested code.

Performance ✅

Positive:

• No significant performance regressions expected
• The hook-based architecture is designed for composability without sacrificing performance
• Same early-exit optimizations are preserved (e.g., checking is_async before processing)

Test Coverage ⚠️

Concern:

• The PR shows one test output change (crates/swc/tests/fixture/issues-4xxx/4208/1/output/index.ts)
• No new test files are added
• The substantial refactoring warrants comprehensive test coverage to ensure correctness across edge cases:
  • Nested async functions
  • Async arrows in constructors (the new code path)
  • Super usage in async functions
  • Arguments usage
  • Deeply nested scenarios

Recommendation: The existing test suite should be run (appears to be pending in CI), and any execution tests mentioned in crates/swc_ecma_minifier/AGENTS.md should pass.

Security ⚠️

Minor Concern:
The manual this replacement helpers (lines 872-981) traverse the AST but don't handle all expression types. For example:

• Line 970: _ => {} in replace_this_in_expr means some expression types are silently ignored

This could potentially leave this unreplaced in certain expressions, leading to runtime errors. The code should either:

1. Be exhaustive and handle all cases, or
2. Have a comment explaining why certain cases don't need handling

Code Style ✅

• Follows Rust conventions
• Proper use of tracing::instrument for debugging
• Good documentation comments
• Consistent with existing codebase patterns

Recommendations

Critical:

1. Investigate the dependency issue: The circular dependency between swc_ecma_compat_es2017 and swc_ecma_transformer needs to be resolved. Consider:
   • Moving the public API to a different location
   • Deprecating the old wrapper and having users migrate to the new API directly
   • Using a different dependency structure

High Priority:

2. Verify constructor handling: Ensure the new replace_this_in_* approach correctly handles all edge cases that FnEnvHoister::to_stmt_in_subclass() handled
3. Make replace_this_in_expr exhaustive: Either handle all expression types or document why some are safely ignored

Medium Priority:

4. Add integration tests: Specifically test async arrows in constructors with this usage
5. Wait for CI: The "Cargo shear" failure and pending tests need to complete

Verdict

This is a well-executed refactoring that successfully migrates the transform to the new architecture. The main concerns are:

1. Dependency structure (blocking - needs resolution)
2. Constructor this handling (needs verification)
3. Test coverage (needs CI confirmation)

Once the dependency issue is resolved and CI passes, this PR should be good to merge. The code quality is high and the transformation logic appears sound.

Suggested Action: Wait for CI to complete and address the "Cargo shear" failure before merging.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​swc/​counter@​0.1.3
	jest@​29.7.0 ⏵ 25.5.4	+1		+1
	@​typescript-eslint/​parser@​6.21.0
	@​ast-grep/​napi@​0.3.3
	@​typescript-eslint/​eslint-plugin@​6.21.0
	@​napi-rs/​cli@​3.2.0
	@​actions/​core@​1.10.1
	@​octokit/​rest@​18.12.0
	@​antfu/​ni@​0.21.12

View full report

[claude]

Pull Request Review: Merge async_to_generator into swc_ecma_transformer

I've reviewed this PR which migrates the async_to_generator transformation from the old VisitMut-based architecture to the new VisitMutHook system. Overall, this is a well-executed refactoring with solid implementation. Here are my findings:

---

✅ Strengths

1. Correct Architecture Migration

   • Properly implements VisitMutHook<TraverseCtx> as required by AGENTS.md
   • Correctly uses enter/exit hook pattern instead of VisitMut
   • Clean separation of concerns with the hook builder pattern

2. State Management

   • Excellent use of stack-based state management (fn_state_stack, in_subclass_stack)
   • Properly handles nested functions and arrow expressions
   • Constructor context tracking (in_constructor flag) is well-implemented

3. Complex Edge Cases Handled

   • Handles this in async arrows within constructors (issues assert_this_initialized check fired before super() #8452, this initialised are before super() when with async function #9432)
   • Properly deals with super references using FnEnvHoister
   • Correctly transforms async generators with wrapAsyncGenerator
   • For-await-of loop transformation is comprehensive

4. Code Quality

   • Good use of Rust patterns (Option, mem::replace, take())
   • Appropriate tracing instrumentation
   • Clear comments explaining complex logic

---

⚠️ Issues & Concerns

1. Incomplete this Replacement (Critical)

The replace_this_in_* functions at lines 876-984 are incomplete. They only handle a subset of expression and statement types:

Missing Expression Types:

• Expr::Object - objects with method shorthand using this
• Expr::Array - arrays with spread/element expressions
• Expr::Cond - ternary expressions
• Expr::Tpl - template literals with expressions
• Expr::TaggedTpl - tagged templates
• Expr::New - constructor arguments
• Expr::Update - ++this.x
• Expr::OptChain - optional chaining
• Expr::JSXElement - JSX expressions

Missing Statement Types:

• Stmt::For/ForIn/ForOf - loop init/test/update/body
• Stmt::While/DoWhile - loop condition/body
• Stmt::Switch - switch discriminant and cases
• Stmt::Try - try/catch/finally blocks
• Stmt::Throw - throw expressions
• Stmt::Labeled - labeled statements

Recommendation: Use VisitMut trait for complete traversal, or switch to using FnEnvHoister exclusively for this transformation.

2. Unused Helper Function

Line 981-983: replace_this_in_expr_or_spread is defined but never called. Should be removed or used.

3. Performance Consideration

The hook now traverses ALL code, not just async functions. The old code used should_work checks to skip non-async code. While the commit message (7604f75) states this was removed for performance, this needs validation:

• Does the hook system efficiently skip irrelevant nodes?
• Have you benchmarked this change?

4. Missing Function Body Safety Check

Lines 189-195: When fn_state.in_constructor && fn_state.use_this, the code directly mutates arrow_expr.body without checking if it's already been transformed or if it contains nested functions that define their own this context.

5. Pattern Consistency

Lines 167-173: Creating a clone of parent_fn_state to update it feels awkward. Consider using a mutable reference pattern instead:

if let Some(out_fn_state) = &mut self.fn_state {
    out_fn_state.use_this |= fn_state.use_this;
    // ...
}

6. Test Coverage Concerns

The PR mentions "146 out of 150 tests passing" but:

• What are the 4 failing tests?
• Are they regressions or known limitations?
• Should they be tracked as TODO or issues?

---

🔍 Security & Correctness

1. No Security Issues - The transformation is AST-to-AST with no external input or injection risks
2. Memory Safety - Proper use of Rust ownership and borrowing
3. Logic Correctness - Core transformation logic appears sound, aside from the incomplete this replacement

---

📋 Recommendations

High Priority:

1. Fix the incomplete replace_this_in_* implementation or document limitations
2. Remove unused replace_this_in_expr_or_spread function
3. Clarify test failures - are there regressions?

Medium Priority:
4. Add inline documentation explaining the in_constructor flow (it's complex!)
5. Consider adding a test specifically for nested object methods with this
6. Benchmark performance vs old implementation

Low Priority:
7. Simplify the parent state update pattern (line 167-173)

---

📝 Code Convention Adherence

✅ Follows AGENTS.md requirements:

• Implements VisitMutHook correctly
• Subtypes execute hook methods properly
• No direct VisitMut implementation for transforms

✅ Follows CLAUDE.md requirements:

• Performant code patterns
• English documentation
• No nightly features
• Appropriate use of Cow<str> in string handling

---

🎯 Overall Assessment

This is a solid architectural migration with good handling of complex edge cases. The main concern is the incomplete this replacement which could lead to incorrect transformations for code patterns not covered by the current test suite.

Verdict: Approve with required changes to the replace_this_in_* functions before merge.

---

Let me know if you'd like me to provide a more detailed implementation for the missing expression/statement types!

[Copilot]

Pull request overview

This PR refactors the async_to_generator transform by merging it into the unified transformer architecture. The transform is moved from a standalone visitor-based implementation to a hook-based system that integrates with the new swc_ecma_transformer infrastructure.

Key Changes

• Reimplemented async_to_generator as a VisitMutHook instead of a VisitMut visitor
• Integrated the transform into the unified options and hook chain system
• Added support for tracking constructor context to properly handle this in async arrows within class constructors
• Migrated from the old compat API to the new transformer API

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/swc_ecma_transformer/src/lib.rs	Updated es2017::hook() call to pass additional parameters (unresolved_ctxt and ignore_function_length)
crates/swc_ecma_transformer/src/hook_utils.rs	Added optional hook methods for function, arrow expression, class, constructor, getter/setter properties, and super
crates/swc_ecma_transformer/src/es2017/mod.rs	Refactored to conditionally return async_to_generator hook based on options instead of empty pass
crates/swc_ecma_transformer/src/es2017/async_to_generator.rs	New implementation with hook-based visitor, enhanced constructor context tracking, and this replacement logic
crates/swc_ecma_preset_env/src/lib.rs	Simplified to set options.env.es2017.async_to_generator = true instead of directly instantiating the transform
crates/swc_ecma_compat_es2017/src/async_to_generator.rs	Converted to thin wrapper that delegates to swc_ecma_transformer::Options
crates/swc_ecma_compat_es2017/Cargo.toml	Updated dependencies to include swc_ecma_transformer and removed swc_ecma_visit and swc_trace_macro
crates/swc/tests/fixture/issues-4xxx/4208/1/output/index.ts	Test output shows improved this binding with .call(_this) for proper context preservation
Cargo.lock	Reflects the dependency changes in swc_ecma_compat_es2017

---

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

[Copilot]

This function replace_this_in_expr_or_spread is defined but never used. Consider removing it or using it where appropriate.

Suggested change

	fn replace_this_in_expr_or_spread(expr: &mut ExprOrSpread, this_var: &Ident) {
	replace_this_in_expr(&mut expr.expr, this_var);
	}

[claude]

Pull Request Review: Merge async_to_generator into Transformer

Overview

This PR refactors the async_to_generator transform by moving it from swc_ecma_compat_es2017 into the new swc_ecma_transformer architecture using the VisitMutHook pattern. This is part of a larger architectural improvement to consolidate transforms.

---

✅ Strengths

1. Architectural Improvement: Successfully migrates to the hook-based architecture, which is the recommended pattern per the project's AGENTS.md instructions.

2. Code Preservation: The core transformation logic (854 lines) has been carefully preserved and moved intact, maintaining behavioral compatibility.

3. Improved State Management: The new implementation uses explicit stacks (fn_state_stack, in_subclass_stack) for managing nested function contexts, which is more robust than the previous approach.

4. Constructor Context Handling: Added proper in_constructor flag and this_var tracking to handle async arrows in constructor contexts, which appears to fix the test case in issues-4xxx/4208/1/output/index.ts where .call(_this) is now correctly generated.

5. Clean Integration: The swc_ecma_compat_es2017 now acts as a thin wrapper that delegates to the transformer, maintaining backward compatibility.

---

🔍 Code Quality Issues

1. Incomplete replace_this_in_expr Implementation (High Priority)

Lines 928-975 in async_to_generator.rs implement manual AST traversal to replace this expressions, but the implementation is incomplete:

fn replace_this_in_expr(expr: &mut Expr, this_var: &Ident) {
    match expr {
        Expr::This(..) => {
            *expr = Expr::Ident(this_var.clone());
        }
        // ... handles ~15 expression types
        _ => {}  // Silently ignores other types!
    }
}

Problems:

• Missing cases: Expr::Array, Expr::Object, Expr::Cond, Expr::New, Expr::Tpl, Expr::TaggedTpl, Expr::Update, Expr::OptChain, etc.
• If this appears in these unhandled expression types within constructor async arrows, it won't be replaced, causing incorrect behavior.
• The replace_this_in_stmt function at lines 897-926 also has incomplete statement type coverage.

Recommendation: Either:

• Use VisitMut trait for traversal instead of manual matching (preferred for correctness)
• Add comprehensive match arms for all expression/statement variants
• Add unit tests specifically for this replacement logic

2. Missing Error Handling in Stack Operations

Lines 131, 162, 257, 307, 318, 328:

self.fn_state = self.fn_state_stack.pop();  // Returns Option but treated as infallible

While the logic appears correct, there's an inconsistency:

• Line 257: self.in_subclass_stack.pop().unwrap_or(false) - handles None case
• Other lines: Direct assignment without handling None

Recommendation: Add debug assertions or comments explaining why the stack is guaranteed to have values, or use unwrap_or_default() consistently.

3. Unused Function Parameter (Minor)

Line 981-982:

fn replace_this_in_expr_or_spread(expr: &mut ExprOrSpread, this_var: &Ident) {
    replace_this_in_expr(&mut expr.expr, this_var);
}

This function is defined but never called in the codebase (checked via grep). Should either be removed or the spread case should be handled.

---

🐛 Potential Bugs

1. Logic Change in make_fn_ref (Medium Priority)

Line 441-447 introduces a new condition:

} else if fn_state.use_this && !fn_state.in_constructor {
    // fn.call(this)
    // Note: in constructor context, we don't use .call(this) because
    // FnEnvHoister has already replaced `this` with `_this`

Issue: The comment says "FnEnvHoister has already replaced this with _this", but looking at the code:

• Lines 189-195: replace_this_in_block_stmt_or_expr is called manually, NOT FnEnvHoister
• Lines 72-80: FnEnvHoister is only used when fn_state.use_super is true

Recommendation:

• Verify this logic is correct - should the condition be !fn_state.in_constructor or !fn_state.use_super?
• Update the comment to accurately describe what's happening
• Add integration tests for constructor + async arrow combinations

2. Output Format Change in Test

The test output change in crates/swc/tests/fixture/issues-4xxx/4208/1/output/index.ts:

-        })();
+        }).call(_this);

This appears to be a behavioral change (likely a fix), but:

• No explanation in the PR description about why this change is correct
• Could affect existing codebases relying on the old behavior

Recommendation: Document whether this is a bugfix or intentional behavioral change in the changeset.

---

⚡ Performance Considerations

1. Manual AST Traversal: The replace_this_* functions (lines 876-983) do manual recursion through the AST. Consider:

   • This could be expensive for deeply nested expressions
   • Using VisitMut might be more efficient as it's optimized by the compiler

2. Cloning in Stacks: State is frequently cloned when pushing to stacks:

   • Line 168: let mut updated = out_fn_state.clone();
   • Consider if this cloning is necessary for correctness

---

🔒 Security Concerns

No security issues identified. The transform doesn't introduce injection vulnerabilities.

---

🧪 Test Coverage

Good:

• Fixture test output shows the transform is working
• Integration with existing test suite

Concerns:

1. No new unit tests for the hook implementation
2. No tests specifically for the replace_this_* helper functions
3. No tests for edge cases like:
   • Deeply nested async functions
   • All expression types in constructor async arrows
   • Generator + async combinations

Recommendation: Add tests in crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:

#[cfg(test)]
mod tests {
    // Test replace_this_in_expr with all expression variants
    // Test nested function state management
    // Test constructor + async arrow edge cases
}

---

📝 Best Practices & Style

1. Documentation: Functions replace_this_in_* lack doc comments explaining when/why they're needed
2. Naming: fn_state_stack and in_subclass_stack - clear names ✓
3. Performance Comments: Line 86-89 has a good comment explaining the complexity trade-off ✓

---

✅ Adherence to Project Guidelines

Per CLAUDE.md and AGENTS.md:

• ✅ Correctly implements VisitMutHook (not VisitMut directly)
• ✅ Uses TraverseCtx parameter
• ✅ Placed in appropriate module structure
• ❌ Missing unit tests (guideline: "Write unit tests for your code")
• ⚠️ Should run cargo fmt --all before committing (assumed done but not verified)

---

🎯 Summary & Recommendations

Must Fix Before Merge:

1. Complete the replace_this_in_expr and replace_this_in_stmt implementations to handle all AST node types
2. Verify/fix the logic in make_fn_ref regarding in_constructor vs use_super
3. Document the behavioral change in test output

Should Fix:

1. Add unit tests for helper functions
2. Handle stack pop operations more defensively
3. Remove unused replace_this_in_expr_or_spread function
4. Add doc comments to helper functions

Nice to Have:

1. Consider using VisitMut for replace_this operations
2. Benchmark performance vs old implementation
3. Add more edge case tests

Overall Assessment: The refactoring is well-structured and follows the project's architectural direction. However, the incomplete AST traversal in replace_this_in_expr is a significant correctness issue that could cause subtle bugs. Once the must-fix items are addressed, this will be a solid improvement to the codebase.