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/fixer): Wrap in expr in for-in head #9209

Merged
merged 10 commits into from
Jul 13, 2024

Conversation

magic-akari
Copy link
Member

@magic-akari magic-akari commented Jul 11, 2024

Related issue (if exists):

@magic-akari magic-akari requested a review from a team as a code owner July 11, 2024 14:10
@magic-akari magic-akari marked this pull request as draft July 11, 2024 14:44
@kdy1 kdy1 added this to the Planned milestone Jul 11, 2024
@kdy1 kdy1 self-assigned this Jul 11, 2024
Copy link

codspeed-hq bot commented Jul 11, 2024

CodSpeed Performance Report

Merging #9209 will not alter performance

Comparing magic-akari:fix/issue-9200 (a50f036) with main (69719c2)

Summary

✅ 173 untouched benchmarks

@@ -0,0 +1,72 @@
for(var a = (b in c) in {});
for(var a = 1 || (b in c) in {});
for(var a = 1 + (2 || (b in c)) in {});
Copy link
Member Author

Choose a reason for hiding this comment

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

We have some false positives. It is no longer necessary to add parentheses around the in expression that is already in parentheses.

We need to check the in expressions from the outside in. But currently, the wrap calls are from the inside out, which cannot be completed in a single pass.

Do you have any thoughts? @kdy1

Copy link
Member

Choose a reason for hiding this comment

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

Can we proceed with some false positives for now and refactor them outside? If you prefer, rewriting the fixer to work outside in is also fine.

Copy link
Member Author

@magic-akari magic-akari Jul 13, 2024

Choose a reason for hiding this comment

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

I plan to leave a TODO for now, as this is a very rare edge case and has no negative impact.

@magic-akari magic-akari marked this pull request as ready for review July 13, 2024 09:55
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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


swc-bump:

  • swc_ecma_transforms_base
  • swc_core

@kdy1 kdy1 enabled auto-merge (squash) July 13, 2024 10:33
@kdy1 kdy1 changed the title fix(es/base): Wrap in expr in for-in head fix(es/fixer): Wrap in expr in for-in head Jul 13, 2024
@kdy1 kdy1 merged commit 5cd837f into swc-project:main Jul 13, 2024
151 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.7.0 Jul 17, 2024
@magic-akari magic-akari deleted the fix/issue-9200 branch August 7, 2024 06:13
@swc-project swc-project locked as resolved and limited conversation to collaborators Sep 6, 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.

Missing parenthesis in initializer of for loop
2 participants