fix(es/minifier): Inline before merge if#11526
fix(es/minifier): Inline before merge if#11526Austaras wants to merge 1 commit intoswc-project:mainfrom
Conversation
|
|
Let's see how performance changes. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #11517 where the SWC minifier incorrectly merged conditional statements, causing semantic changes in the output code. The issue occurred when two if statements had identical consequent blocks but different conditions, and the consequent blocks contained local variables with different values (e.g., different string literals).
Changes:
- Added inline pass before merging if statements to ensure variables marked for inlining are actually inlined
- Modified comparison logic to use
SyntaxContext::within_ignored_ctxt()when comparing if statement consequents, allowing safe merging after inlining - Updated test outputs to reflect improved minification (multiple test files show better optimization where consecutive if statements with identical bodies are now correctly merged)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/swc_ecma_minifier/src/compress/optimize/conditionals.rs | Adds inline pass before merge and wraps comparison in SyntaxContext::within_ignored_ctxt() to fix the bug |
| crates/swc_ecma_minifier/tests/libs-size.snapshot.md | Updates size benchmarks showing minor improvements in echarts and typescript |
| crates/swc_ecma_minifier/tests/fixture/next/wrap-contracts/output.js | Shows improved minification where two if statements with same body are merged |
| crates/swc_ecma_minifier/tests/fixture/next/react-pdf-renderer/output.js | Shows improved minification with merged if statements |
| crates/swc_ecma_minifier/tests/fixture/next/feedback-util-promisify/chunks/pages/_app-72ad41192608e93a/output.js | Shows improved minification with merged if statements |
| crates/swc_ecma_minifier/tests/fixture/next/33265/static/chunks/pages/index-cb36c1bf7f830e3c/output.js | Shows improved minification with merged if statements |
| crates/swc_ecma_minifier/tests/fixture/next/33265/static/chunks/d6e1aeb5-38a8d7ae57119c23/output.js | Shows improved minification with multiple merged if statements |
| crates/swc_ecma_minifier/tests/benches-full/echarts.js | Shows improved minification with merged if statements and nested conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Binary Sizes
Commit: 17bd804 |
CodSpeed Performance ReportMerging this PR will degrade performance by 2.04%Comparing Summary
Performance Changes
|
|
Seems fine. |
|
Nothing happened. claude stops working? |
|
Ah. The problem is that this is a PR from a fork. Hmm... I'll invoke claude locally |
|
We can rebase this PR once #11530 is merged, which adds lots of execution tests. |
|
I've added 113 execution tests to verify this fix in #11530. The tests cover the original issue reproduction and many variations including different variable types, statement types, data structures, function types, and real-world patterns like Redux reducers, i18n, HTTP status handling, etc. |
|
That's excessively a lot of tests. |
Description:
The original issue is more subtle than claude found. It happens because minifier take var to ready for inline, but not actually doing the inline, so leave the to be merged statement into a bad middle state. Besides we should merge two if ignore span because variable defined in two if cons would have different syntax context, but if their value and usage is the same, we can still merge those two if.
BREAKING CHANGE:
Related issue (if exists):
Fixes #11517