🦋 Changeset detected

Latest commit: f614d18

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

Code Review

Summary

This PR refactors the swc_ecma_compat_es2018 crate to use the merged transformer pattern instead of composing individual transforms. This is a performance improvement that consolidates regexp and object rest/spread transformations into a single unified pass through swc_ecma_transformer::Options.

✅ Positive Changes

1. Performance Improvement: The merged transformer approach reduces the number of AST traversals from multiple passes to a single unified pass, which aligns with the repository's priority on performance (per AGENTS.md).

2. Dependency Reduction: Removes the swc_ecma_compat_common dependency, reducing the dependency graph and potentially improving compile times.

3. Consistent Pattern: This aligns with the modern transformer architecture using VisitMutHook pattern (as seen in swc_ecma_transformer/src/lib.rs:34-76).

4. Code Simplification: The new implementation in crates/swc_ecma_compat_es2018/src/lib.rs:11-28 is cleaner and more maintainable.

🔍 Observations & Suggestions

1. Configuration Mapping Correctness (lib.rs:14-26):

   • The regexp options mapping looks correct (dot_all, lookbehind_assertion, named_capturing_groups, unicode_property)
   • The object_rest_spread options mapping correctly translates Config fields to Options.assumptions
   • ✅ All original regexp features from the old code are preserved

2. Missing Features Check:
   The old code configured these regexp features:

   • dot_all_regex: true ✅ (line 15)
   • has_indices: false ⚠️ Not set (defaults to false)
   • lookbehind_assertion: true ✅ (line 16)
   • named_capturing_groups_regex: true ✅ (line 17)
   • sticky_regex: false ⚠️ Not set (defaults to false)
   • unicode_property_regex: true ✅ (line 18)
   • unicode_regex: false ⚠️ Not set (defaults to false)
   • unicode_sets_regex: false ⚠️ Not set (defaults to false)

   The unset fields are all explicitly set to false in the old code and rely on defaults in the new code. This is correct as RegExpOptions::default() sets all fields to false (per swc_ecma_transformer/src/regexp.rs:11).

3. Behavioral Equivalence:
   The object_rest_spread behavior is preserved correctly:

   • Old: Individual pass with assumptions
   • New: Unified pass with the same assumptions
   • Both should produce identical output ✅

📋 Testing Recommendations

Since this is a refactoring that changes the internal architecture:

1. Execution Tests: Per crates/swc_ecma_minifier/AGENTS.md, execution tests should be run. For this crate, ensure:

   • Object rest/spread transformations produce correct output
   • Regexp transformations (dotAll, lookbehind, named groups, unicode property) work correctly

2. Fixture Tests: Run fixture tests to ensure output matches expectations

3. Integration Tests: Test the es2018() function is called correctly from higher-level APIs

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Performance improvement as intended (fewer AST traversals)
• ✅ No unsafe code introduced

📝 Code Quality

• ✅ Follows the repository's Rust conventions
• ✅ Uses appropriate types (Cow<str> pattern not applicable here)
• ⚠️ Minor: Consider adding a doc comment to the es2018() function explaining the transformation scope:

  /// Creates a transformer for ES2018 compatibility.
  ///
  /// Transforms:
  /// - Object rest/spread properties
  /// - RegExp features (dotAll flag, lookbehind assertions, named capture groups, unicode property escapes)
  pub fn es2018(config: Config) -> impl Pass {

✨ Conclusion

This is a well-executed refactoring that achieves its performance goals while maintaining correctness. The code quality is good and follows repository conventions. The main recommendation is to ensure thorough testing to verify behavioral equivalence.

Recommendation: ✅ Approve (pending CI green)

CodSpeed Performance Report

Merging #11366 will improve performances by 7.25%

_{Comparing kdy1/use-port (f614d18) with main (57a8731)}

Summary

⚡ 4 improvements
✅ 134 untouched

Benchmarks breakdown

Code Review for PR #11366

Summary

This PR refactors the es2018() function in swc_ecma_compat_es2018 to use the merged transformer pattern by directly configuring swc_ecma_transformer::Options instead of composing individual passes. This is a performance optimization that reduces overhead from multiple pass compositions.

Positive Aspects ✅

1. Performance improvement: Using a single merged transformer instead of composing multiple passes (regexp + object_rest_spread) should reduce overhead and improve compilation performance
2. Dependency reduction: Removes the swc_ecma_compat_common dependency, simplifying the dependency graph
3. Consistency: This aligns with the pattern used in object_rest_spread.rs:8-16 which already uses this approach
4. Correct configuration mapping: The regexp and object_rest_spread configurations are correctly mapped to the new Options structure

Issues and Concerns ⚠️

1. Typo in Changeset (Minor)

Location: .changeset/six-mangos-kiss.md:2

swc_coere: patch

Should be swc_core not swc_coere.

2. Code Style - Unnecessary Block Scoping

Location: crates/swc_ecma_compat_es2018/src/lib.rs:14-19 and 21-26

The extra block scopes are unnecessary and don't follow typical Rust conventions. Consider removing them:

pub fn es2018(config: Config) -> impl Pass {
    let mut options = swc_ecma_transformer::Options::default();

    // Configure regexp transforms
    options.env.regexp.dot_all_regex = true;
    options.env.regexp.lookbehind_assertion = true;
    options.env.regexp.named_capturing_groups_regex = true;
    options.env.regexp.unicode_property_regex = true;

    // Configure object rest spread
    options.env.es2018.object_rest_spread = true;
    options.assumptions.object_rest_no_symbols = config.object_rest_spread.no_symbol;
    options.assumptions.set_spread_properties = config.object_rest_spread.set_property;
    options.assumptions.pure_getters = config.object_rest_spread.pure_getters;

    options.into_pass()
}

3. Missing Documentation Comments

According to the CLAUDE.md guidelines: "Write documentation for your code." The function should have a doc comment explaining its purpose, similar to the one in object_rest_spread.rs:7.

Suggestion:

/// ES2018 compatibility transformer
/// 
/// Transforms ES2018 features including:
/// - Object rest/spread properties
/// - RegExp features (dotAll, lookbehind assertions, named capture groups, Unicode property escapes)
pub fn es2018(config: Config) -> impl Pass {

4. Test Coverage Verification Needed

The CLAUDE.md guidelines state: "Write unit tests for your code." While this is a refactoring that should maintain existing behavior:

• Are there integration tests that verify the output remains identical?
• Have you verified that all ES2018 features still work correctly with this change?
• Please confirm that existing test suites pass

Verification Checklist 📋

Before merging, please ensure:

• cargo fmt --all has been run (per CLAUDE.md)
• All existing tests pass
• The changeset typo is fixed
• Consider adding a doc comment for better code documentation

Performance Validation 🚀

Since this is a performance optimization:

• Have you measured the performance improvement?
• Consider adding benchmark results to the PR description to demonstrate the benefit

Overall Assessment

This is a solid refactoring that improves performance by reducing pass composition overhead. The logic is correct and the configuration mapping is accurate. The main issues are minor style and documentation concerns that should be addressed before merging.

Recommendation: Approve with minor changes requested (fix typo, improve code style, add documentation).

Pull Request Review: perf(es/compat): Use merged transformer

Summary

This PR refactors swc_ecma_compat_es2018 to use the new merged transformer architecture from swc_ecma_transformer instead of composing individual passes. This is a performance optimization that reduces the number of AST traversals.

✅ Positive Aspects

1. Architecture Improvement: The change aligns with the new unified transformer pattern, which should reduce AST traversal overhead by merging multiple passes into a single traversal.

2. Dependency Cleanup: Removed the swc_ecma_compat_common dependency, which simplifies the dependency graph.

3. Consistent Configuration Mapping: The configuration mapping from the old API to the new Options structure is clear and preserves all configuration options:

   • config.object_rest_spread.no_symbol → options.assumptions.object_rest_no_symbols
   • config.object_rest_spread.set_property → options.assumptions.set_spread_properties
   • config.object_rest_spread.pure_getters → options.assumptions.pure_getters

4. Build Success: The package compiles successfully without errors.

🔍 Code Quality Observations

1. Unnecessary Block Scoping (Minor):

   {
       options.env.regexp.dot_all_regex = true;
       // ...
   }
   
   {
       options.env.es2018.object_rest_spread = true;
       // ...
   }

   The block scopes around configuration assignments are unnecessary and add no value. Consider removing them for cleaner code:

   options.env.regexp.dot_all_regex = true;
   options.env.regexp.lookbehind_assertion = true;
   options.env.regexp.named_capturing_groups_regex = true;
   options.env.regexp.unicode_property_regex = true;
   
   options.env.es2018.object_rest_spread = true;
   options.assumptions.object_rest_no_symbols = config.object_rest_spread.no_symbol;
   options.assumptions.set_spread_properties = config.object_rest_spread.set_property;
   options.assumptions.pure_getters = config.object_rest_spread.pure_getters;

2. Duplicate Implementation: The object_rest_spread.rs file in this crate now duplicates similar logic from lib.rs. Both files:

   • Create swc_ecma_transformer::Options
   • Set similar configuration
   • Call options.into_pass()

   However, object_rest_spread.rs omits the RegExp transformations. This could lead to confusion about which function to use.

⚠️ Potential Issues

1. Test Coverage: The PR shows 0 tests in the swc_ecma_compat_es2018 crate:

   running 0 tests
   test result: ok. 0 passed; 0 failed; 0 ignored
   

   This is concerning for a refactoring that changes the implementation approach. While integration tests may exist elsewhere, it would be beneficial to:

   • Verify that existing integration tests in swc_ecma_transformer or swc_ecma_preset_env cover this functionality
   • Consider adding smoke tests to ensure the configuration mapping works correctly

2. RegExp Configuration Mismatch: In the new es2018() function, you're setting these RegExp options:

   options.env.regexp.dot_all_regex = true;
   options.env.regexp.lookbehind_assertion = true;
   options.env.regexp.named_capturing_groups_regex = true;
   options.env.regexp.unicode_property_regex = true;

   However, the old implementation set different values:

   regexp::Config {
       dot_all_regex: true,
       has_indices: false,
       lookbehind_assertion: true,
       named_capturing_groups_regex: true,
       sticky_regex: false,
       unicode_property_regex: true,
       unicode_regex: false,
       unicode_sets_regex: false,
   }

   The old code explicitly set some options to false, while the new code only sets the true options and relies on defaults for the rest. This is likely fine (since defaults are false), but it's worth verifying that RegExpOptions::default() indeed has all these as false.

3. Documentation Gap: The changeset message is minimal ("perf(es/compat): Use merged transformer"). Consider adding more context about:

   • Why this change improves performance
   • What behavioral changes (if any) users should expect
   • Whether this is a breaking change

🎯 Recommendations

1. Remove Unnecessary Block Scopes: Clean up the unnecessary {} blocks in lib.rs:14-26 for better readability.

2. Verify Test Coverage:

   • Run the execution tests mentioned in crates/swc_ecma_minifier/AGENTS.md
   • Verify integration tests pass in dependent crates
   • Consider adding basic configuration tests to swc_ecma_compat_es2018

3. Document RegExp Defaults: Add a comment confirming that the RegExp defaults match the old explicit false values, or verify in /home/runner/work/swc/swc/crates/swc_ecma_transformer/src/regexp.rs:9-28 that RegExpOptions::default() sets the right values.

4. Consider API Consolidation: The duplication between lib.rs::es2018() and object_rest_spread.rs::object_rest_spread() could be reduced. Perhaps es2018() could call object_rest_spread() with appropriate configuration, or vice versa.

🔐 Security & Performance

• Security: ✅ No security concerns identified. The change is internal refactoring with no new attack surface.
• Performance: ✅ This is explicitly a performance optimization to reduce AST traversals, which aligns with the PR's stated goal.

📝 Final Verdict

Approval Status: ⚠️ Approve with minor recommendations

The refactoring is sound and achieves its performance goal. The concerns are minor and mostly around code cleanliness and test verification. The implementation correctly maps the old API to the new transformer architecture.

Before Merging: Verify that integration tests pass in dependent crates (especially swc_ecma_preset_env and swc_ecma_minifier).

🤖 Generated with Claude Code

Binary Sizes

Commit: 3193273