swc-project / swc

Rust-based platform for the Web
https://swc.rs
Apache License 2.0
31.29k stars 1.23k forks source link

fix(es/compat): Add missing visit children for `destructuring` #9658

Closed stormslowly closed 1 month ago

stormslowly commented 1 month ago

Description:

add missing visit_mut_children_with in deconstructing transform

BREAKING CHANGE: NO

Related issue (if exists):

ref: https://github.com/swc-project/swc/discussions/9647

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: ebab9b3d070700e27e0f51e29e5fd46dffe91497

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

codspeed-hq[bot] commented 1 month ago

CodSpeed Performance Report

Merging #9658 will not alter performance

Comparing stormslowly:fix/deconstructing_skip_visiting_children (ebab9b3) with main (32116a0)

Summary

✅ 194 untouched benchmarks

magic-akari commented 1 month ago

I believe the fix is correct, but I can't reproduce the issue in the swc playground.

I suspect our transformer has redundant elsewhere.

stormslowly commented 1 month ago

in my opinion, in swc playground run the for_of transform then destructing , so it can't be reproduced there.

@magic-akari you can try comment out the visit children call, then run the new added case, panic will appear

kdy1 commented 1 month ago

If so, the order of transform is wrong. The compatibility transforms are expected to run in the correct order, and the correct order is.. well it's not documented but esxxxx functions encode the correct orders

magic-akari commented 1 month ago

If so, the order of transform is wrong.

https://github.com/swc-project/swc/blob/9c355207d229411801d277e445bd8b1c643e5eb9/crates/swc_ecma_compat_es2015/src/for_of.rs#L274-L283

In the for_of pass, pat has moved from the for-head to the for-body, which leads to the related visitors in the destructing always being skipped. With the current sequence, we consistently fail to find this bug. (This issue would be revealed in an environment that supports for...of but not destructuring, via SWC env target rather than es version target)

Ideally, these two transformations should be independent, allowing us to retain destructuring (the pat structure) when downgrading from for...of. Similarly, we should be able to downgrade destructuring while keeping for...of intact.

I have not yet confirmed whether the current situation matches the description above.