Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

stormslowly
Copy link
Contributor

Description:

add missing visit_mut_children_with in deconstructing transform

BREAKING CHANGE:
NO

Related issue (if exists):

ref: #9647

@stormslowly stormslowly requested a review from a team as a code owner October 17, 2024 15:58
Copy link

changeset-bot bot commented Oct 17, 2024

🦋 Changeset detected

Latest commit: ebab9b3

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

@stormslowly stormslowly marked this pull request as draft October 17, 2024 17:33
Copy link

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #9658 will not alter performance

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

Summary

✅ 194 untouched benchmarks

@stormslowly stormslowly marked this pull request as ready for review October 18, 2024 02:02
@magic-akari
Copy link
Member

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
Copy link
Contributor Author

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 kdy1 self-assigned this Oct 18, 2024
@kdy1 kdy1 modified the milestone: Planned Oct 18, 2024
@kdy1
Copy link
Member

kdy1 commented Oct 18, 2024

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
Copy link
Member

magic-akari commented Oct 18, 2024

If so, the order of transform is wrong.

ForHead::Pat(pat) => prepend_stmt(
&mut body.stmts,
AssignExpr {
span: DUMMY_SP,
left: pat.try_into().unwrap(),
op: op!("="),
right: step.clone().make_member(quote_ident!("value")).into(),
}
.into_stmt(),
),

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.

magic-akari
magic-akari previously approved these changes Oct 18, 2024
Copy link
Member

@magic-akari magic-akari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this fix is in the right direction, let's merge this fix first.

@kdy1 kdy1 added this to the Planned milestone Oct 18, 2024
@kdy1 kdy1 requested a review from a team as a code owner October 18, 2024 16:27
@kdy1 kdy1 changed the title fix: 🐛 add missing visit children in for stmt fix(es/compat): Add missing visit children for destructuring Oct 18, 2024
@kdy1 kdy1 merged commit 32116a0 into swc-project:main Oct 18, 2024
23 of 24 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.7.37 Oct 21, 2024
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants