feat(es/minifier): Remove unused args for IIFE#11536
feat(es/minifier): Remove unused args for IIFE#11536Austaras wants to merge 1 commit intoswc-project:mainfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the SWC ECMAScript minifier’s IIFE argument optimization to remove unused call arguments (addressing the first part of #11089), and updates large golden outputs accordingly.
Changes:
- Update
ignore_unused_args_of_iifeto drop/neutralize unused IIFE call arguments, including removing trailing arguments when safe. - Add an
eval-presence guard (contains_eval) to avoid unsafe transformations. - Refresh expected outputs for large fixtures/bench outputs to reflect the new minification result.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/swc_ecma_minifier/src/compress/optimize/iife.rs |
Implements removal of unused/trailing IIFE call arguments and adds eval guard. |
crates/swc_ecma_minifier/tests/fixture/next/react-pdf-renderer/output.js |
Updates expected minified output to reflect dropped unused IIFE args. |
crates/swc_ecma_minifier/tests/benches-full/victory.js |
Updates expected benchmark output with removed unused IIFE call argument. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if e.args.len() > params.len() { | ||
| for i in (params.len()..e.args.len()).rev() { | ||
| if let Some(arg) = e.args.get_mut(i) { | ||
| let new = self.ignore_return_value(&mut arg.expr); | ||
|
|
||
| if let Some(new) = new { | ||
| arg.expr = Box::new(new); | ||
| } else { | ||
| e.args.remove(i); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This change introduces new behavior (dropping trailing IIFE call arguments) with several tricky edge cases (e.g. spread args, rest params, side-effect-only rewrites). Please add a focused regression fixture/test that covers the reported pattern from #11089 and at least one spread/rest scenario, so future optimizer changes don't reintroduce incorrect argument dropping.
| if e.args.len() > params.len() { | ||
| for i in (params.len()..e.args.len()).rev() { | ||
| if let Some(arg) = e.args.get_mut(i) { | ||
| let new = self.ignore_return_value(&mut arg.expr); | ||
|
|
There was a problem hiding this comment.
The new trailing-argument removal loop ignores arg.spread. For a call like (function(){})(...xs) or (function(a){})(x, ...xs), removing a spread argument is not semantics-preserving because spreading triggers iterator evaluation (can throw / have side effects) even if arg.expr itself looks side-effect-free. Add a guard to bail out when any argument has spread, or at least skip/abort removal when arg.spread.is_some() inside this loop.
| if e.args.len() > params.len() { | ||
| for i in (params.len()..e.args.len()).rev() { | ||
| if let Some(arg) = e.args.get_mut(i) { |
There was a problem hiding this comment.
params.len() is not a safe cutoff for removing “extra” args when the callee has a rest parameter (e.g. (function(a,...rest){use(rest)})(1,2,3)). Args beyond params.len() are still part of rest, so removing them changes observable behavior. Consider detecting Pat::Rest in params and skipping this optimization in that case (or only removing args starting at the rest index when you can prove the rest binding is unused).
Binary Sizes
Commit: 157a9a1 |
CodSpeed Performance ReportMerging this PR will degrade performance by 2.42%Comparing Summary
Performance Changes
|
1 participant
Description:
BREAKING CHANGE:
Related issue (if exists):
First part of #11089