-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(es/minifier): More strict check if cannot add ident when invoking IIFE #11399
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: a99de98 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the ECMAScript minifier where IIFE (Immediately Invoked Function Expression) inlining was incorrectly applied in contexts where new identifiers cannot be added, particularly in class field initializers. The fix addresses issue #10931.
Key Changes:
- Refactored IIFE inlining to use
ExactSizeIteratorinstead of passing parameter length separately - Added stricter checks in
can_extract_paramto verify that arguments are simple literals when identifiers cannot be added - Removed the
DontInvokeIifeflag mechanism in favor of the more comprehensivemay_add_ident()check
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/swc_ecma_minifier/src/compress/optimize/iife.rs |
Refactored IIFE inlining functions to accept ExactSizeIterator parameters and added new validation logic to check argument types when identifiers cannot be added |
crates/swc_ecma_minifier/src/compress/optimize/mod.rs |
Commented out the unused DontInvokeIife flag and its usage in class visitor |
crates/swc_ecma_minifier/tests/fixture/issues/11368/input.js |
Added test case demonstrating the bug with class field initialization |
crates/swc_ecma_minifier/tests/fixture/issues/11368/output.js |
Expected output showing the fix prevents incorrect inlining |
| Multiple test output files | Updated expected outputs to reflect the fix preventing over-aggressive IIFE inlining |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| || !matches!( | ||
| &*arg.expr, | ||
| Expr::Lit( | ||
| Lit::Num(..) | Lit::Str(..) | Lit::Bool(..) | Lit::BigInt(..) |
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The literal type check should include Lit::Null(..) to be consistent with other parts of the codebase. In other optimization functions like inline.rs and sequences.rs, null literals are treated as safe, side-effect-free values that can be inlined without creating variables. Omitting null from this check may unnecessarily prevent valid IIFE inlining optimizations when null is passed as an argument.
| Lit::Num(..) | Lit::Str(..) | Lit::Bool(..) | Lit::BigInt(..) | |
| Lit::Num(..) | |
| | Lit::Str(..) | |
| | Lit::Bool(..) | |
| | Lit::BigInt(..) | |
| | Lit::Null(..) |
| .ctx | ||
| .clone() | ||
| .with(BitCtx::DontInvokeIife, true) | ||
| // .with(BitCtx::DontInvokeIife, true) |
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code should be removed rather than left in place. This code is no longer needed after the refactoring, as evidenced by the removal of the DontInvokeIife flag usage throughout the codebase. Leaving commented code reduces readability and can cause confusion for future maintainers.
| // .with(BitCtx::DontInvokeIife, true) |
| // if self.ctx.bit_ctx.contains(BitCtx::DontInvokeIife) { | ||
| // log_abort!("iife: Inline is prevented"); | ||
| // return false; | ||
| // } | ||
|
|
Copilot
AI
Dec 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code should be removed rather than left in place. This check is no longer needed after the refactoring, which now relies on the more comprehensive may_add_ident() check. Leaving commented code reduces readability and can cause confusion for future maintainers.
| // if self.ctx.bit_ctx.contains(BitCtx::DontInvokeIife) { | |
| // log_abort!("iife: Inline is prevented"); | |
| // return false; | |
| // } |
Binary Sizes
Commit: c85e8ad |
CodSpeed Performance ReportMerging #11399 will not alter performanceComparing Summary
|
crates/swc_ecma_minifier/tests/terser/compress/collapse_vars/issue_2187_3/output.js
Show resolved
Hide resolved
kdy1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn can_inline_fn_arg(usage: &VarUsageInfo, arg: &Expr) -> bool { | ||
| if usage.ref_count > 1 | ||
| || usage.assign_count > 0 | ||
| || usage.flags.contains(VarUsageInfoFlags::REASSIGNED) | ||
| || usage.flags.contains(VarUsageInfoFlags::INLINE_PREVENTED) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| match arg { | ||
| Expr::Lit(Lit::Num(..) | Lit::Str(..) | Lit::Bool(..) | Lit::BigInt(..)) => true, | ||
| Expr::Fn(..) if usage.can_inline_fn_once() => true, | ||
| _ => false, | ||
| } | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition usage.property_mutation_count == 0 check was removed from the inlining logic in inline_fn_param_stmt. Previously, the code checked this condition before inlining, but now can_inline_fn_arg doesn't verify property mutations. This could potentially lead to incorrect inlining when the parameter has property mutations. Consider adding this check to the can_inline_fn_arg function to maintain the same safety guarantees.
|
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
… IIFE
Description:
BREAKING CHANGE:
Related issue (if exists):
Closes #11368