feat(es/transformer): Merge object_rest_spread

[kdy1]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: ba6f17e

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

[claude]

PR Review - feat(es/transformer): Merge object_rest_spread

Thank you for working on porting transforms to the new single-pass compiler architecture! However, this PR appears to be incomplete and has several critical issues that need to be addressed before merging.

---

Critical Issues

1. ❌ Missing Implementation

The core transformation logic is not implemented. The file crates/swc_ecma_transformer/src/es2018/object_rest_spread.rs is empty (1 byte), which means:

• Object rest patterns are not being transformed at all
• The old implementation (~1,225 lines) in object_rest.rs and object_spread.rs was deleted
• No equivalent logic was added to the new location

Evidence: Running tests shows the transformation doesn't happen:

// Input:
var { a, ...b } = _ref;

// Expected output:
var { a } = _ref, b = _object_without_properties(_ref, ["a"]);

// Actual output (unchanged):
var { a, ...b } = _ref;

2. ❌ Tests Fail

The test suite for es2018_object_rest_spread fails because the transformation logic is missing. Example failure:

test rest_var_basic ... FAILED
assertion left == right failed

3. ⚠️ Potential Circular Dependency Risk

Adding swc_ecma_transformer as a dependency of swc_ecma_compat_es2018 could create architectural issues:

• swc_ecma_compat_es2018/Cargo.toml now depends on swc_ecma_transformer
• Need to verify this doesn't create cycles or violate the intended dependency hierarchy
• The current usage (swc_ecma_transformer::Options::default()) suggests the wrapper is just configuring the transformer, but the transformer itself has no implementation

4. ⚠️ CI Failure - Cargo Shear

The cargo-shear check failed, likely because:

• The new dependency on swc_ecma_transformer might be considered unused/incomplete
• The dependency adds no actual functionality since the implementation is empty

---

What Needs to Be Done

1. Implement the transformation logic in crates/swc_ecma_transformer/src/es2018/object_rest_spread.rs

   • Port the logic from the deleted object_rest.rs and object_spread.rs files
   • Adapt it to use the new hook-based architecture (VisitMutHook<TraverseCtx>)
   • Reference the Es2018Pass struct in es2018/mod.rs which currently does nothing

2. Ensure tests pass

   • Run cargo test --test es2018_object_rest_spread
   • All existing tests must pass
   • Update test expectations if the new implementation produces equivalent but different output

3. Verify the architecture

   • Ensure the hook is properly connected in es2018::hook()
   • The Es2018Options::object_rest_spread flag should control whether the transform runs
   • Currently impl VisitMutHook<TraverseCtx> for Es2018Pass {} has no methods, so nothing happens

4. Add a changeset

   • This is a breaking change / significant refactor
   • Follow the bot's instructions to add a changeset

---

Architecture Notes

The intended architecture looks correct:

• ✅ swc_ecma_compat_es2018::object_rest_spread() creates an Options struct
• ✅ Calls options.into_pass() which builds the hook chain
• ✅ The hook chain should include the actual transformation
• ❌ But the transformation logic is missing from Es2018Pass

---

Performance & Code Quality

Cannot evaluate until implementation is complete, but some observations:

• ✅ The hook architecture is a good approach for the single-pass compiler
• ✅ Proper use of feature flags via Es2018Options
• ⚠️ The deleted implementation was ~1,225 lines of complex pattern matching - ensure the new implementation maintains correctness
• 📝 According to AGENTS.md, you should run cargo fmt --all before committing

---

Recommendations

Do not merge until:

1. The transformation logic is fully implemented
2. All tests pass
3. CI checks pass (especially cargo-shear)
4. A changeset is added

Consider looking at PR #11358 (async_to_generator) as a reference if it's further along, to ensure consistency in how these transforms are being ported.

---

Let me know if you need any clarification or would like me to help with specific aspects of the implementation!

🤖 Generated with Claude Code

[github-actions]

Binary Sizes

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

Commit: d651ed3

[codspeed-hq]

CodSpeed Performance Report

Merging #11357 will degrade performances by 6.31%

_{Comparing kdy1/port-object-rest-spread (ba6f17e) with main (25f3a47)}

Summary

❌ 6 regressions
✅ 132 untouched

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

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	es/full-target/es2017	586.6 µs	622.6 µs	-5.78%
❌	es/full-target/es2018	557.6 µs	593.6 µs	-6.05%
❌	es2018_object_rest_spread	291.2 µs	310.8 µs	-6.31%
❌	es/full-target/es2016	616.8 µs	635.6 µs	-2.96%
❌	es/target/es2018	324 µs	338.7 µs	-4.35%
❌	es2015_classes	390 µs	414.5 µs	-5.92%

[claude]

Pull Request Review: Merge object_rest_spread

Summary

This PR successfully migrates the ES2018 object rest/spread transformation from the legacy VisitMut pattern to the new VisitMutHook architecture. The implementation consolidates previously split functionality (object_rest.rs and object_spread.rs) into a unified module, improving maintainability and aligning with the project's architectural direction.

Code Quality ✅

Strengths:

• Excellent adherence to architectural patterns: The implementation correctly follows the VisitMutHook<TraverseCtx> pattern as specified in the repository's AGENTS.md guidelines
• Well-structured code: Clear separation of concerns with distinct visitors (RestVisitor, PatternRestVisitor), output contexts (DeclOutput, ExprOutput), and the core lowering algorithm (RestLowerer)
• Comprehensive coverage: Handles all necessary AST node types (functions, arrows, constructors, catch clauses, for-loops, var declarations, assignments, module exports)
• Good documentation: Inline comments reference esbuild implementation links, providing context for the algorithm design

Minor observations:

• The code follows Rust best practices with appropriate use of traits, pattern matching, and memory management
• Helper functions like is_pure_expr, is_lit_str, and collect_idents_from_pat are well-isolated and testable

Performance Considerations ⚡

Positives:

• Fast-path optimization: Uses should_work with specialized visitors to avoid unnecessary processing
• Efficient memory handling: Uses mem::take() and drain() to avoid clones where possible
• Smart initialization: Pre-allocates vectors with appropriate capacity hints
• Optimization for computed keys: The code at lines 996-1029 includes a clever optimization for impure computed keys, avoiding unnecessary array creation when there's only one impure key

Potential Issues & Concerns 🔍

1. ExprOutput::exprs type change (Minor)

Location: crates/swc_ecma_transformer/src/es2018/object_rest_spread.rs:897

In the new implementation, ExprOutput::exprs is Vec<Expr> but later gets converted to Vec<Box<Expr>> (line 171). The old implementation used Vec<Box<Expr>> directly. While this works, it adds an extra allocation step. Consider whether storing Vec<Box<Expr>> directly would be more efficient.

// Current (line 171):
let exprs = exprs.into_iter().map(Box::new).collect();

2. Module export transformation logic (Moderate)

Location: crates/swc_ecma_transformer/src/es2018/object_rest_spread.rs:395-397

The transformation detection logic assumes that if decls.len() > 1, transformation occurred. However, this might not always be accurate since the original declaration could have multiple declarators before transformation. Consider tracking whether transformation actually occurred via a flag or by comparing against the original length.

let transformation_occurred = var_decl.decls.len() > 1 || !self.vars.is_empty();

3. Error handling in ExprOutput::assign (Minor)

Location: crates/swc_ecma_transformer/src/es2018/object_rest_spread.rs:915-927

The assign method silently drops patterns that can't convert to AssignTarget. While this might be intentional, it could hide bugs. Consider logging or asserting in debug mode.

if let Ok(target) = pat.try_into() {
    self.exprs.push(/* ... */);
}
// Silently drops if conversion fails

4. CI Failure (Critical) ⚠️

The "Cargo shear" check is currently failing. This needs to be resolved before merging. It's likely a dependency issue related to the new swc_ecma_transformer dependency added to swc_ecma_compat_es2018/Cargo.toml.

Test Coverage ✅

The existing test suite in crates/swc_ecma_transforms_compat/tests/es2018_object_rest_spread.rs provides good coverage:

• Basic rest/spread patterns
• Nested patterns
• Function parameters
• Arrow functions
• Assignment expressions
• Edge cases (issues Unexpected named exports, related to spread operator #162, Object rest spread doesnt work with function arguments #181, Spread arguments lost, undeclared local referenced #227, Abort found #233, Rest spread in constructor causes abort #239)

Recommendation: Since this is a refactoring with architectural changes, verify that all existing tests still pass and consider adding a few tests specifically for the edge cases mentioned above.

Security Concerns 🔒

No security issues identified. The transformation correctly handles:

• Expression evaluation order (RHS before LHS)
• Side effects in computed properties
• Proper temporary variable generation with unique identifiers

Architecture & Best Practices 🏗️

Excellent adherence to repository conventions:

• ✅ Follows the VisitMutHook pattern requirement from crates/swc_ecma_transformer/AGENTS.md
• ✅ Does not implement VisitMut on the transform (ES20xx types)
• ✅ Uses helper functions appropriately
• ✅ Maintains performance-first approach

Recommendations

1. Fix CI: Resolve the Cargo shear failure before merging
2. Consider optimization: Review the ExprOutput::exprs type choice (line 897)
3. Strengthen export logic: Add more robust transformation detection for module exports (line 395-397)
4. Add assertions: Consider debug assertions for error cases like the pattern conversion failure
5. Run execution tests: Per crates/swc_ecma_minifier/AGENTS.md, run ./scripts/exec.sh to verify execution tests pass

Conclusion

This is a high-quality refactoring that successfully modernizes the object rest/spread transformation to use the new architecture. The code is well-structured, performant, and maintains backward compatibility. The main blocking issue is the CI failure, which should be straightforward to resolve. Once the CI passes, this PR will be ready to merge.

Overall Assessment: ✅ Approve (pending CI fix)

---

Generated by Claude Code Review