perf(es/compat): Merge async_to_generator

[kdy1]

Description:

BREAKING CHANGE:

Related issue (if exists):

[changeset-bot]

⚠️ No Changeset found

Latest commit: 1a78f6f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[kdy1]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• perf(es/compat): Merge optional_catch_binding #11313
• perf(es/compat): Merge object_rest_spread #11312
• perf(es/compat): Merge async_to_generator #11311 👈 (View in Graphite)
• perf(es/compat): Merge exponentation_operator #11310
• perf(es/compat): Merge export_namespace_from to Transformer #11309
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Code Review - PR #11311: perf(es/compat): Merge async_to_generator

Overview

This PR refactors the async_to_generator transform by moving the implementation from swc_ecma_compat_es2017 to the new swc_ecma_transformer crate. This appears to be part of a larger effort to consolidate transformations into a unified system using hooks.

---

Issues Found

🐛 Critical: Typo in Parameter Name

Location: crates/swc_ecma_transformer/src/es2017/mod.rs:18,23

There's a typo: asseumptions should be assumptions

// Line 18 - should be 'assumptions'
pub(crate) fn hook(
    unresolved_ctxt: SyntaxContext,
    options: Es2017Options,
    asseumptions: Assumptions,  // ❌ Typo here
) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook(
            unresolved_ctxt,
            asseumptions.ignore_function_length,  // ❌ And here
        ))
    } else {
        None
    })
}

Fix: Rename parameter to assumptions

---

⚠️ Missing Implementation

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

The AsyncToGeneratorPass struct implements VisitMutHook but has an empty implementation:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

Issue: This PR removes ~850 lines of the actual async-to-generator transformation logic from swc_ecma_compat_es2017/src/async_to_generator.rs but doesn't implement it in the new location. The hook is essentially a no-op.

Expected: The actual transformation logic should be implemented in the VisitMutHook trait methods, or there should be a comment explaining where the implementation will come from.

---

Code Quality & Architecture

✅ Good:

1. Proper dependency management - Added swc_ecma_transformer as a dependency in swc_ecma_compat_es2017/Cargo.toml
2. Backward compatibility - The public API in swc_ecma_compat_es2017 is maintained as a thin wrapper
3. Configuration structure - Properly passes ignore_function_length through the options chain
4. Follows project patterns - Aligns with the new transformer architecture

⚠️ Concerns:

1. Breaking the transformation - Removing 850+ lines of logic without replacement means async functions won't be transformed anymore
2. No tests in new location - The transformation has extensive test coverage in swc_ecma_transforms_compat, but none in the new swc_ecma_transformer crate
3. Empty PR description - No motivation or explanation for the change
4. Incomplete refactoring - This appears to be only part of a larger change

---

Performance Considerations

According to CLAUDE.md rule #1: "Write performant code. Always prefer performance over other things."

• ✅ The hook-based architecture should allow for more efficient traversal
• ⚠️ However, without the actual implementation, performance is N/A
• 📝 The PR title mentions "perf" but there's no actual performance improvement without the logic

---

Test Coverage

Per CLAUDE.md rule #5: "Write unit tests for your code"

• ✅ Existing tests in crates/swc_ecma_transforms_compat/tests/es2017_async_to_generator.rs still exist (2100+ lines)
• ❌ No new tests in swc_ecma_transformer
• ⚠️ Tests will likely fail since the transformation is now a no-op

Recommendation: Run cargo test --package swc_ecma_transforms_compat --test es2017_async_to_generator to verify

---

Security Concerns

No security issues identified. The refactoring doesn't introduce new attack vectors.

---

Recommendations

1. Fix the typo immediately (asseumptions → assumptions)
2. Implement the transformation logic - Either:
   • Port the 850 lines of transformation code to the new AsyncToGeneratorPass
   • Or explain in comments why it's empty and where the actual implementation is
3. Add documentation explaining:
   • Why this refactoring is being done
   • What the migration path is
   • Whether this is part of a larger PR series
4. Verify tests pass before merging
5. Follow CLAUDE.md rule Travis and codecov #7 - Add documentation for the new code structure

---

CI Status

The Cargo shear check is failing. This might be due to:

• Unused dependencies
• The incomplete implementation
• The typo preventing compilation in some contexts

---

Summary

This PR appears to be incomplete. While the architectural direction is sound (moving to a hook-based system), the actual transformation logic has been removed without being replaced. This will break async function transformations.

Recommendation: Do not merge until the transformation implementation is completed. The typo should be fixed, and the full transformation logic should be ported to the new location.

---

Review generated by Claude Code. Please verify all findings.

[codspeed-hq]

CodSpeed Performance Report

Merging #11311 will degrade performances by 6.34%

_{Comparing kdy1/async-to-generator (2757573) with kdy1/exponentation-operator (1252b09)¹}

Summary

⚡ 3 improvements
❌ 1 regression
✅ 134 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	es/full-target/es2016	588.7 µs	562.9 µs	+4.59%
⚡	es2016_exponentiation	291.9 µs	275.8 µs	+5.84%
❌	es2020_nullish_coalescing	291 µs	310.7 µs	-6.34%
⚡	es/target/es2016	291.7 µs	276.9 µs	+5.35%

Footnotes

1. No successful run was found on kdy1/exponentation-operator (28a97ff) during the generation of this report, so 94f2133 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[claude]

Code Review

Critical Issues

1. Typo in Parameter Name (High Priority)

Location:

There's a typo in the parameter name asseumptions which should be assumptions:

pub(crate) fn hook(
    unresolved_ctxt: SyntaxContext,
    options: Es2017Options,
    asseumptions: Assumptions,  // ❌ Typo: should be 'assumptions'
) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook(
            unresolved_ctxt,
            asseumptions.ignore_function_length,  // ❌ Used again here
        ))

This would cause a compilation error. Please fix the spelling to assumptions.

---

2. Missing Implementation (Blocker)

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

The AsyncToGeneratorPass struct implements VisitMutHook<TraverseCtx> but has an empty implementation:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

This is a critical issue. The entire 800+ lines of async-to-generator transformation logic were deleted from swc_ecma_compat_es2017/src/async_to_generator.rs but no actual implementation exists in the new location. The transformation will not work at all.

Action Required: The actual visitor implementation must be added. You need to:

1. Implement the hooks that transform async functions to generators
2. Handle await expressions, for await loops, etc.
3. Port the logic from the deleted code or ensure it exists elsewhere

---

Architecture & Design Concerns

3. Incomplete Migration to New Architecture

This PR appears to be part of a larger refactoring to merge transforms into a unified swc_ecma_transformer architecture using hooks. However, the migration is incomplete:

• The old implementation (800+ lines) was completely removed
• Only a stub was created in the new location
• No actual transformation logic exists

Suggestion: Consider whether this should be:

• A draft PR until implementation is complete
• Split into multiple PRs (setup infrastructure first, then migrate logic)
• Completed before merging to avoid breaking async-to-generator functionality

---

4. Circular Dependency Concern

Location: crates/swc_ecma_compat_es2017/Cargo.toml

Added dependency: swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

This creates a dependency from swc_ecma_compat_es2017 → swc_ecma_transformer. Please verify:

• Does swc_ecma_transformer depend on any compat crates (directly or transitively)?
• Is this the intended architecture going forward?
• Should swc_ecma_compat_es2017 be deprecated in favor of the new transformer?

---

Code Quality & Best Practices

5. Missing Documentation

The new async_to_generator.rs file lacks:

• Module-level documentation explaining the transformation
• Function documentation for hook()
• Comments explaining the hook-based architecture

Per CLAUDE.md guideline #7: "Write documentation for your code."

---

6. Test Coverage Concern

The PR removes substantial logic but doesn't appear to add or modify tests. Given the complexity of async-to-generator transformation:

• Are existing tests still passing?
• Do tests cover the new hook-based implementation?
• Should integration tests be added to ensure the migration doesn't break functionality?

Per CLAUDE.md guideline #5: "Write unit tests for your code."

---

Performance Considerations

7. Performance Impact Assessment Needed

The PR title indicates this is a performance optimization ("perf(es/compat): Merge async_to_generator"). However:

• The actual optimization logic is not implemented yet
• Performance benefits are unclear from the stub implementation
• No benchmark results provided

Suggestion: Once implemented, measure performance impact with realistic workloads.

---

Minor Issues

8. PR Description is Empty

The PR description has placeholder text but no actual content:

• No explanation of the motivation
• No description of the changes
• No related issue linked

Please fill this out to help reviewers understand:

• Why this change is being made
• What problem it solves
• How it improves performance
• Whether this is part of a larger refactoring

---

Summary

This PR cannot be merged in its current state due to:

1. ✅ Compilation error (typo in parameter name)
2. ✅ Missing implementation (async-to-generator logic not ported)

Once these are fixed, please also address:

• Documentation
• Test coverage
• PR description
• Performance benchmarks

---

Status: ❌ Changes Requested

The concept of migrating to a hook-based architecture looks promising for the stated performance goal, but the implementation needs to be completed before this can be reviewed for merge.

[claude]

Updated Code Review - PR #11311

I've reviewed the previous comments and the PR more thoroughly. Here's an updated assessment:

Critical Issue - Typo

Location: crates/swc_ecma_transformer/src/es2017/mod.rs:18,23

There's a typo that will prevent compilation:

// Line 18 - parameter name has typo
pub(crate) fn hook(
    unresolved_ctxt: SyntaxContext,
    options: Es2017Options,
    asseumptions: Assumptions,  // ❌ Should be 'assumptions'
) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook(
            unresolved_ctxt,
            asseumptions.ignore_function_length,  // ❌ Should be 'assumptions'
        ))
    } else {
        None
    })
}

Fix Required: Rename asseumptions to assumptions on lines 18 and 23.

Architecture Understanding

After deeper review, I now understand this is part of a larger refactoring to move transformations to a hook-based system. The PR:

1. Sets up the infrastructure in swc_ecma_transformer
2. Integrates it with swc_ecma_preset_env via options.into_pass() (line 331)
3. Maintains backward compatibility through the wrapper in swc_ecma_compat_es2017

Missing Implementation

The AsyncToGeneratorPass struct at crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:21 has an empty implementation:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

Questions:

• Is the actual implementation in another PR in this stack (perf(es/compat): Merge object_rest_spread #11312 or perf(es/compat): Merge optional_catch_binding #11313)?
• Or is this intentionally using a different mechanism (generated code via proc macros)?
• This appears to be part of a Graphite stack, so maybe the implementation comes later?

Testing Status

The CI shows a test failure. This is likely due to the typo preventing compilation. Once fixed, please verify:

cargo test --package swc_ecma_transforms_compat --test es2017_async_to_generator

Adherence to CLAUDE.md

Per the project guidelines:

✅ Performance (Rule #1): Hook-based architecture should enable better performance
⚠️ Comments (Rule #2): Could use more documentation explaining the migration
✅ No unstable features (Rule #3): Clean
❌ Unit tests (Rule #5): Missing tests for new code
❌ Documentation (Rule #7): Missing docs explaining the new structure
⚠️ Formatting (Rule #8): Can't run cargo fmt due to typo

Recommendations

1. Fix the typo immediately - This is blocking compilation
2. Clarify implementation status - Add a comment explaining if this is part of a multi-PR stack
3. Add documentation - Explain the hook-based architecture for future maintainers
4. Run formatting - After fixing typo: cargo fmt --all

Stack Context

This appears to be part of a Graphite stack with PRs #11309-#11313. Consider:

• Does perf(es/compat): Merge object_rest_spread #11312 or perf(es/compat): Merge optional_catch_binding #11313 contain the actual hook implementation?
• Should the implementation be completed before merging this PR?
• Or is this intentionally scaffolding for later PRs?

---

Verdict: Do not merge until the typo is fixed. Consider whether the empty implementation is intentional given the stack context.

cc: @kdy1

[github-actions]

Binary Sizes

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

Commit: 4a84d10

[claude]

Pull Request Review - PR #11311: perf(es/compat): Merge async_to_generator

Summary

This PR refactors the async_to_generator transform by moving the core implementation from swc_ecma_compat_es2017 to a new unified swc_ecma_transformer crate. The goal appears to be performance optimization through better organization and potentially reusable transformation infrastructure.

---

🐛 Critical Issues (Must Fix)

1. Version Mismatch in Cargo.toml ⚠️

Location: crates/swc_ecma_compat_es2017/Cargo.toml:22

The dependency specifies swc_ecma_transformer = "0.1.0", but the actual version in crates/swc_ecma_transformer/Cargo.toml:10 is "1.0.0".

Error:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

Fix: Update the dependency version to match:

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

2. Typo in Parameter Name 📝

Location: crates/swc_ecma_transformer/src/es2017/mod.rs:18

asseumptions: Assumptions,  // ❌ Should be "assumptions"

This typo appears twice in the file (lines 18 and 23). This will cause compilation errors.

Fix:

-    asseumptions: Assumptions,
+    assumptions: Assumptions,

---

🔍 Code Quality & Architecture

Positive Aspects ✅

1. Good abstraction: Moving to a hook-based architecture with VisitMutHook is a cleaner design
2. Performance focus: The PR aligns with the project's emphasis on performance (CLAUDE.md rule ecmascript parser #1)
3. Maintains backward compatibility: The public API in swc_ecma_compat_es2017 remains intact
4. Code reduction: Removes 854 lines while maintaining functionality

Concerns ⚠️

1. Empty Implementation

   impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

   The AsyncToGeneratorPass struct has an empty VisitMutHook implementation. Where is the actual transformation logic? This appears incomplete.

2. Missing Core Logic
   The original file had ~850 lines of transformation logic including:

   • handle_await_for() function
   • Async generator delegate handling
   • Super/this/arguments hoisting
   • Complex state management

   None of this appears in the new async_to_generator.rs file. Is this logic:

   • Moved elsewhere in the transformer?
   • Generated by the hook framework?
   • Accidentally deleted?

3. No Test Coverage Changes
   The PR doesn't show any test modifications. How do we verify:

   • The transformation still works correctly?
   • Performance actually improved?
   • Edge cases are handled (super in subclasses, arguments usage, etc.)?

---

📊 Performance Considerations

Good:

• Reduced code duplication potential
• Hook-based architecture may enable better optimization passes

Unclear:

• No benchmarks or performance metrics provided
• The PR title claims "perf" but doesn't demonstrate improvement
• Empty hook implementation raises questions about actual execution path

---

🔒 Security Concerns

No security issues identified. The transformation logic doesn't handle user input directly, and the changes are structural rather than algorithmic.

---

📋 Missing Information

1. BREAKING CHANGE section is empty - Is this actually a breaking change?
2. No related issue linked - What motivated this refactor?
3. No migration guide - If breaking, how should users migrate?
4. Incomplete diff - The full implementation of the new transformer logic isn't visible

---

✅ Adherence to CLAUDE.md

• ✅ Comments in English
• ❓ No unstable features visible
• ❌ Missing unit tests (Rule Handle early errors in ecmascript parser #5: "Write unit tests for your code")
• ❓ Documentation unclear (Rule Travis and codecov #7)
• ⚠️ Need to run cargo fmt --all after fixing typos (Rule Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

---

🎯 Recommendations

Before merging:

1. Fix the version mismatch - Update Cargo.toml to use version 1.0.0
2. Fix the typo - Correct "asseumptions" → "assumptions"
3. Complete the implementation - The AsyncToGeneratorPass needs actual transformation logic
4. Add tests - Verify the transformation still works (required by CLAUDE.md)
5. Provide performance data - Justify the "perf" claim with benchmarks
6. Document the architecture change - Explain where the transformation logic now lives
7. Fill in PR description - Add motivation, related issues, and breaking change details

Questions for the author:

1. Where is the actual transformation logic now implemented?
2. Is the hook framework generating the visitor methods automatically?
3. What performance improvements does this PR provide?
4. Are there any API breaking changes users should be aware of?

---

Overall Assessment

Status: ⚠️ Needs Work

While the architectural direction seems sound, the PR has critical bugs and appears incomplete. The empty hook implementation and missing transformation logic need clarification before this can be merged.

Confidence Level: High on identified issues, Medium on overall architecture (due to incomplete visibility into the new implementation)

[claude]

Code Review for PR #11311: perf(es/compat): Merge async_to_generator

Thank you for working on this performance improvement! This PR attempts to merge the async_to_generator transform into the new single-pass compiler architecture. However, I've identified several critical issues that need to be addressed before this can be merged.

---

🔴 Critical Issues

1. Typo in parameter name (crates/swc_ecma_transformer/src/es2017/mod.rs:18,23)

asseumptions: Assumptions,  // Should be: assumptions

This typo will cause compilation errors. It appears in both the function signature and the function call.

2. Wrong version number in Cargo.toml (crates/swc_ecma_compat_es2017/Cargo.toml:22)

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

The actual version in swc_ecma_transformer/Cargo.toml is 1.0.0, not 0.1.0. This causes cargo dependency resolution to fail:

error: failed to select a version for the requirement \`swc_ecma_transformer = "^0.1.0"\`
candidate versions found which didn't match: 1.0.0

3. Missing implementation (crates/swc_ecma_transformer/src/es2017/async_to_generator.rs)

The new async_to_generator implementation is just a 21-line stub with an empty VisitMutHook implementation:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

The original implementation was ~850 lines with complex logic for:

• Handling await expressions
• Converting async functions to generators
• Managing this, arguments, and super contexts
• Handling for await...of loops
• Function hoisting for subclasses

All of this functionality has been deleted but not reimplemented. This means the transform will do nothing, breaking all async function transformations.

---

📋 Additional Concerns

4. Test Coverage

While there are existing tests in crates/swc_ecma_transforms_compat/tests/es2017_async_to_generator.rs, they still reference the old implementation path. These tests need to be:

• Updated to use the new transformer architecture
• Verified to ensure they still pass with the new implementation
• Run before merging to catch the missing implementation

5. Breaking Change Documentation

The PR template includes a "BREAKING CHANGE" section but it's empty. Since this refactors a core transformation:

• Document whether this is a breaking change
• Update migration docs if needed
• Clarify the motivation (performance improvement) more explicitly

6. Performance Claims

The PR title claims this is a performance improvement, but:

• No benchmarks are provided
• The new "single-pass" architecture is mentioned but not explained
• It's unclear what performance gains are expected

---

🔧 Required Fixes

1. Fix the typo: asseumptions → assumptions
2. Fix the version: 0.1.0 → 1.0.0
3. Implement the actual transformation logic in AsyncToGeneratorPass
4. Run all tests before requesting review
5. Add benchmarks to validate the performance improvements
6. Document the change properly in the PR description

---

📝 Code Quality Notes (per CLAUDE.md)

From the repository's style guide:

• ✅ Comments would be in English (when implementation is complete)
• ⚠️ No unit tests were added for the new implementation
• ⚠️ cargo fmt --all appears to have been run, but code doesn't compile
• ❌ The existing tests were not verified to still work

---

💡 Recommendation

This PR is not ready for merge. The implementation is incomplete (just a stub) and contains compilation errors. I recommend:

1. Complete the actual implementation by porting the logic from the old async_to_generator.rs to the new hook-based architecture
2. Fix the two compilation errors (typo and version)
3. Verify all existing tests pass
4. Add benchmarks showing the performance improvement
5. Request review again once the implementation is complete

If you need help with the migration to the new architecture or have questions about the hook system, please let me know!

---

Summary: While the refactoring goal is good, the current implementation is incomplete and won't work. The PR needs significant additional work before it can be merged.

[claude]

Pull Request Review

Thank you for working on this performance optimization to merge the async_to_generator transform! I've reviewed the changes and found several issues that need to be addressed:

---

🐛 Critical Issues

1. Incorrect dependency version (crates/swc_ecma_compat_es2017/Cargo.toml:22)

-swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }
+swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

Issue: The version is set to 0.1.0, but swc_ecma_transformer/Cargo.toml:10 shows the actual version is 1.0.0. This causes the build to fail with:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

---

2. Typo: "asseumptions" instead of "assumptions" (crates/swc_ecma_transformer/src/es2017/mod.rs:18, 23)

-    asseumptions: Assumptions,
+    assumptions: Assumptions,

-            asseumptions.ignore_function_length,
+            assumptions.ignore_function_length,

Impact: This typo will cause compilation errors and needs to be fixed.

---

⚠️ Implementation Concerns

3. Empty implementation of VisitMutHook (crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:21)

The AsyncToGeneratorPass struct implements VisitMutHook<TraverseCtx> but has an empty implementation:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

Issue: This means the actual transformation logic is missing! The old implementation in swc_ecma_compat_es2017/src/async_to_generator.rs had ~850 lines of transformation logic that:

• Converted async functions to generators wrapped in helpers
• Handled await expressions → yield expressions
• Managed this, arguments, and super references
• Transformed for await...of loops
• Applied various optimizations

What needs to happen: The transformation logic needs to be migrated from the old implementation to the new hook-based implementation. Simply creating an empty hook defeats the purpose of the transform.

---

📝 Missing Context

4. No description or related issue

The PR body is mostly template boilerplate. For a refactoring of this magnitude (deleting 854 lines, restructuring the implementation), it would be helpful to have:

• Explanation of why this merge is being done (performance benefits?)
• Context on how the new hook-based architecture works
• Information about where the actual transformation logic went (is it being implemented elsewhere?)
• Benchmark results showing the performance improvement
• Migration plan or related issue reference

---

✅ Positive Aspects

1. Good architectural direction: Moving to a hook-based architecture with swc_ecma_transformer seems like a good unification effort
2. Proper integration in preset-env: The integration in swc_ecma_preset_env/src/lib.rs:202-204 correctly sets the option
3. Code organization: The structure with Es2017Options and the optional hook pattern looks clean

---

🔧 Recommendations

1. Fix the version mismatch in Cargo.toml (0.1.0 → 1.0.0)
2. Fix the typo (asseumptions → assumptions)
3. Complete the implementation - The actual async-to-generator transformation logic needs to be ported to the new hook system
4. Add tests - According to CLAUDE.md:5, unit tests should be written. Ensure the existing test cases still pass
5. Update the PR description with context about this refactoring
6. Run cargo fmt --all as per CLAUDE.md:8

---

🎯 Blocking Issues

The following must be addressed before this PR can be merged:

• ❌ Version mismatch (build failure)
• ❌ Typo causing compilation error
• ❌ Missing transformation logic (functionality regression)

Once these are fixed, please re-run the tests to ensure no regressions were introduced.

[claude]

Pull Request Review: PR #11311 - Merge async_to_generator

Summary

This PR attempts to refactor the async_to_generator transformation by moving it into a new unified transformer architecture. However, there are several critical issues that prevent this PR from working correctly.

🔴 Critical Issues

1. Empty Implementation (Blocking)

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

The AsyncToGeneratorPass struct has an empty implementation of VisitMutHook<TraverseCtx>:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

Problem: The original 854 lines of transformation logic were deleted from crates/swc_ecma_compat_es2017/src/async_to_generator.rs, but the new implementation in swc_ecma_transformer doesn't actually contain any transformation logic. This means:

• Async functions will not be transformed to generators
• The feature is completely broken
• All existing tests that rely on this transformation will fail

Required Action: The actual transformation logic needs to be implemented in the new location, or the logic needs to be reused from the original implementation.

---

2. Version Mismatch in Cargo.toml (Blocking)

Location: crates/swc_ecma_compat_es2017/Cargo.toml:22

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

Problem: The actual version of swc_ecma_transformer is 1.0.0 (as specified in its Cargo.toml), not 0.1.0. This causes a compilation error:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

Fix: Change the version to match:

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

---

3. Typo in Parameter Name (Blocking)

Location: crates/swc_ecma_transformer/src/es2017/mod.rs:18,23

pub(crate) fn hook(
    unresolved_ctxt: SyntaxContext,
    options: Es2017Options,
    asseumptions: Assumptions,  // ❌ Typo: "asseumptions"
) -> impl VisitMutHook<TraverseCtx> {
    OptionalHook(if options.async_to_generator {
        Some(self::async_to_generator::hook(
            unresolved_ctxt,
            asseumptions.ignore_function_length,  // ❌ Typo here too
        ))

Fix: Correct spelling to assumptions

---

⚠️ Design Concerns

4. Incomplete Migration Pattern

The PR creates a wrapper in swc_ecma_compat_es2017 that delegates to the new transformer, but:

• The new transformer has no actual implementation
• This creates unnecessary indirection without benefits
• It's unclear if this is a work-in-progress or the intended final state

Questions:

• Is the actual transformation logic intended to be implemented later?
• Should the old implementation be preserved temporarily during migration?
• What is the migration strategy for ensuring no regressions?

---

5. Missing Test Coverage

Problem: No tests were found in crates/swc_ecma_compat_es2017 for the async_to_generator functionality. The project instructions (CLAUDE.md:5) state: "Write unit tests for your code."

Recommendation:

• Add tests before removing the original implementation
• Ensure tests cover edge cases like:
  • Async functions with this usage
  • Async functions with arguments usage
  • Async functions with super usage
  • Async arrow functions
  • Async generator functions
  • for await...of loops

---

📝 Style & Convention Issues

6. Documentation Missing

According to CLAUDE.md:7: "Write documentation for your code."

The new AsyncToGeneratorPass struct and hook function lack documentation explaining:

• Purpose and behavior
• Parameters
• Return values
• Usage examples

---

✅ Positive Aspects

1. Good PR Structure: The commits are logical and the refactoring direction makes sense
2. Code Formatting: The code follows Rust formatting standards
3. Architecture Direction: Moving towards a unified transformer architecture is a good long-term goal

---

🎯 Required Actions Before Merge

1. ✅ Fix version mismatch in Cargo.toml (0.1.0 → 1.0.0)
2. ✅ Fix typo asseumptions → assumptions
3. ✅ Implement actual transformation logic in AsyncToGeneratorPass
4. ✅ Add comprehensive test coverage
5. ✅ Add documentation for new types and functions
6. ✅ Verify CI passes (currently failing multiple checks)

---

🔍 Performance Considerations

Per CLAUDE.md:1: "Write performant code. Always prefer performance over other things."

Question: How does the new hook-based architecture compare to the old visitor pattern in terms of performance? Consider:

• Allocation overhead from the chaining pattern
• Virtual dispatch costs
• Impact on compilation time

Recommendation: Run benchmarks to ensure no performance regression, especially for large codebases with many async functions.

---

Summary

This PR is not ready for merge due to critical blocking issues:

• Empty implementation (no transformation logic)
• Compilation errors (version mismatch)
• Syntax errors (typo)

The architectural direction is sound, but the implementation is incomplete. Please address the critical issues and add proper test coverage before this can be reviewed for merge.

---

Review Status: ❌ Request Changes

Estimated Effort to Fix: Medium - requires implementing the transformation logic and fixing compilation errors

Priority: High - this breaks core ES2017 compatibility features

[claude]

Code Review

Overview

This PR merges the transform into the new crate as part of the ongoing single-pass compiler refactoring. The change removes ~850 lines of code from and replaces it with a stub that delegates to the new transformer infrastructure.

Critical Issues

❌ 1. Empty Implementation

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

The AsyncToGeneratorPass struct implements VisitMutHook<TraverseCtx> but has no methods defined. This means the transformation logic is completely missing:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

This is a critical bug - the async/await transformation will not work at all with this implementation. The entire logic that was removed (handling of AwaitExpr, YieldExpr, for-await-of loops, etc.) needs to be ported to the new hook-based system.

❌ 2. Typo in Variable Name

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

asseumptions: Assumptions,  // Should be "assumptions"

This typo exists in both the function signature and the usage at line 23.

Code Quality Issues

⚠️ 3. Missing Test Coverage

• No tests were added to verify the new implementation works
• The removed implementation in swc_ecma_compat_es2017 had comprehensive logic for:
  • Converting async functions to generator functions wrapped in helpers
  • Handling await expressions as yield expressions
  • Managing this, arguments, and super references
  • Transforming for-await-of loops
  • Handling async generators

Given that the implementation is empty, all existing tests using this transform would fail if they ran.

⚠️ 4. Incomplete Migration

The PR description is empty with no explanation of:

• Whether this is work-in-progress or meant to be complete
• What the migration strategy is
• Whether the old implementation should be kept as reference

Performance Considerations

✅ The intent of this refactoring aligns with the CLAUDE.md directive (#1: "Write performant code") - the single-pass compiler should improve performance. However, the current implementation doesn't actually do anything, so there's no performance to evaluate.

Security Concerns

✅ No security issues identified (though the transform doesn't work at all currently).

Recommendations

1. DO NOT MERGE until the actual transformation logic is implemented in AsyncToGeneratorPass
2. Fix the typo: asseumptions → assumptions
3. Port all the transformation logic from the deleted code:
   • visit_mut_function - transform async functions
   • visit_mut_arrow_expr - transform async arrow functions
   • visit_mut_expr - handle await/yield expressions
   • visit_mut_stmt - handle for-await-of loops
   • Helper functions like make_fn_ref, handle_await_for, etc.
4. Add tests to verify the migrated implementation works correctly
5. Run cargo fmt --all before committing (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)
6. Add a proper PR description explaining the migration strategy

CI Status

The PR currently has failing checks (Cargo clippy, Cargo shear) which may be related to these issues.

---

Verdict: This appears to be an incomplete work-in-progress that was accidentally submitted. The transformation logic needs to be fully implemented before this can be merged.

[claude]

Code Review - PR #11311: perf(es/compat): Merge async_to_generator

Summary

This PR attempts to merge the async_to_generator transformation into the new single-pass compiler infrastructure (swc_ecma_transformer). However, there are critical issues that prevent this PR from functioning correctly.

---

Critical Issues

1. Missing Implementation 🚨

The most critical issue is that the actual transformation logic has been removed but not replaced:

• Removed: 836 lines of implementation from crates/swc_ecma_compat_es2017/src/async_to_generator.rs
• Added: Empty stub in crates/swc_ecma_transformer/src/es2017/async_to_generator.rs:21

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

This is an empty implementation - all the transformation logic that converts async functions to generators is missing. The PR removes the working implementation but doesn't port it to the new system.

Impact: This would break all async-to-generator transformations, causing tests to fail and breaking user code that relies on this transformation.

---

2. Typo in Parameter Name

crates/swc_ecma_transformer/src/es2017/mod.rs:18

asseumptions: Assumptions,  // Should be "assumptions"

This typo appears twice (lines 18 and 23) and would cause compilation errors.

---

Code Quality Issues

3. Incomplete Refactoring

The PR description says "Merge async_to_generator" suggesting a performance optimization by consolidating transforms into the single-pass compiler. However:

• The old implementation is deleted
• The new location has no implementation
• No migration of the actual transformation logic has occurred

This looks like an incomplete work-in-progress rather than a complete refactoring.

---

4. Circular Dependency Risk

crates/swc_ecma_compat_es2017/Cargo.toml:22 now depends on swc_ecma_transformer:

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

While async_to_generator.rs:30 calls options.into_pass(), this creates a wrapper that delegates to the single-pass compiler. However, this feels like an architectural inconsistency - the compat crates shouldn't need to depend on the transformer crate just to be empty wrappers.

---

Missing Implementation Components

The removed implementation (854 lines) included:

1. Core transformation logic for async functions and arrow functions
2. State tracking for this, arguments, and super usage
3. Generator wrapping with helper calls to _async_to_generator and _wrap_async_generator
4. Await expression conversion to yield expressions
5. For-await-of loop handling with complex try-catch-finally logic
6. Function parameter analysis to determine if wrapping is needed
7. Performance optimizations to skip unnecessary transformations

None of this logic has been ported to the new location.

---

Test Coverage Concerns

The PR has no test changes, but given that the implementation is completely missing:

• All existing tests for async_to_generator would fail
• The CI shows "Cargo clippy" and "Cargo shear" as failed, likely due to the typo or unused code

---

Performance Considerations

While the PR title suggests this is a performance optimization ("perf(es/compat): Merge..."), no implementation exists to evaluate performance. The single-pass compiler approach is theoretically beneficial, but only if the logic is actually implemented.

---

Recommendations

1. Do not merge this PR in its current state - it would break production functionality
2. Implement the actual transformation logic in crates/swc_ecma_transformer/src/es2017/async_to_generator.rs by porting the 800+ lines of removed code
3. Fix the typo: asseumptions → assumptions
4. Add migration tests to ensure the new implementation produces identical output to the old one
5. Consider the architectural pattern: If moving to single-pass, ensure all similar transforms follow the same pattern

---

Alignment with CLAUDE.md

Per the project's guidelines:

• ✅ Performance focus: The intent is good (single-pass optimization)
• ❌ Missing implementation: Cannot evaluate performance without actual code
• ❌ Tests: Would fail due to missing implementation (violates guideline Internal documentations #6: "When instructed to fix tests, do not remove or modify existing tests")
• ❌ Documentation: The removed documentation and comments should be preserved in the new location

---

Conclusion

This PR appears to be an incomplete work-in-progress that cannot be merged. The transformation logic needs to be fully ported to the new location before this can be reviewed for correctness and performance benefits.

Recommendation: Request Changes - implementation required before this can be merged.

[claude]

Code Review

Overview

This PR merges the async_to_generator transform into the new swc_ecma_transformer crate as part of the ongoing single-pass compiler refactoring. The change removes ~850 lines of code from swc_ecma_compat_es2017 and replaces it with a stub that delegates to the new transformer infrastructure.

Critical Issues

❌ 1. Empty Implementation

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

The AsyncToGeneratorPass struct implements VisitMutHook but has no methods defined. This means the transformation logic is completely missing:

impl VisitMutHook<TraverseCtx> for AsyncToGeneratorPass {}

This is a critical bug - the async/await transformation will not work at all with this implementation. The entire logic that was removed (handling of AwaitExpr, YieldExpr, for-await-of loops, etc.) needs to be ported to the new hook-based system.

❌ 2. Typo in Variable Name

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

asseumptions: Assumptions,  // Should be "assumptions"

This typo exists in both the function signature and the usage at line 23.

Code Quality Issues

⚠️ 3. Missing Test Coverage

• No tests were added to verify the new implementation works
• The removed implementation in swc_ecma_compat_es2017 had comprehensive logic for:
  • Converting async functions to generator functions wrapped in helpers
  • Handling await expressions as yield expressions
  • Managing this, arguments, and super references
  • Transforming for-await-of loops
  • Handling async generators

Given that the implementation is empty, all existing tests using this transform would fail if they ran.

⚠️ 4. Incomplete Migration

The PR description is empty with no explanation of:

• Whether this is work-in-progress or meant to be complete
• What the migration strategy is
• Whether the old implementation should be kept as reference

Performance Considerations

✅ The intent of this refactoring aligns with the CLAUDE.md directive (#1: "Write performant code") - the single-pass compiler should improve performance. However, the current implementation doesn't actually do anything, so there's no performance to evaluate.

Security Concerns

✅ No security issues identified (though the transform doesn't work at all currently).

Recommendations

1. DO NOT MERGE until the actual transformation logic is implemented in AsyncToGeneratorPass
2. Fix the typo: asseumptions → assumptions
3. Port all the transformation logic from the deleted code:
   • visit_mut_function - transform async functions
   • visit_mut_arrow_expr - transform async arrow functions
   • visit_mut_expr - handle await/yield expressions
   • visit_mut_stmt - handle for-await-of loops
   • Helper functions like make_fn_ref, handle_await_for, etc.
4. Add tests to verify the migrated implementation works correctly
5. Run cargo fmt --all before committing (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)
6. Add a proper PR description explaining the migration strategy

CI Status

The PR currently has failing checks (Cargo clippy, Cargo shear) which may be related to these issues.

---

Verdict: This appears to be an incomplete work-in-progress that was accidentally submitted. The transformation logic needs to be fully implemented before this can be merged.