fix(es/minifier): Allow inlining functions with side-effect-free default parameters#11516
fix(es/minifier): Allow inlining functions with side-effect-free default parameters#11516
Conversation
|
Binary Sizes
Commit: 306211a |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
|
@claude fix CI |
This comment has been minimized.
This comment has been minimized.
|
|
1 similar comment
|
|
|
@claude fix ci |
This comment has been minimized.
This comment has been minimized.
|
@claude Rebase to resolve merge conflicts |
This comment has been minimized.
This comment has been minimized.
… parameters
When a function has parameters with default values, the minifier was
previously unable to inline it, even if the default value had no side effects.
This change:
1. Modifies `store_decl_for_inlining` to allow functions with default
parameters to be inlined as "simple functions" when the default value
has no side effects.
2. Adds IIFE argument inlining support for `Pat::Assign` (default parameters):
- When an argument is provided at the call site, use the argument
- When no argument is provided and the default has no side effects,
use the default value
Fixes #11512
Co-Authored-By: Donny/강동윤 <[email protected]>
… params When inlining functions with default parameters, we need to check not only that the default value has no side effects, but also that it doesn't reference any earlier parameters in the function signature. For example, `function(a, b = a)` - the default `a` is side-effect-free but it references the parameter `a`, which would be incorrectly inlined. This fixes test failures: - destructuring/default_precedence - parameters/default_earlier_params - parameters/assumption_ignoreFunctionLength/default_earlier_params Co-Authored-By: Donny/강동윤 <[email protected]>
claude
bot
force-pushed
the
claude/issue-11512-20260128-0423
branch
from
505fef1 to
eab14af
Compare
|
@claude fix ci |
This comment has been minimized.
This comment has been minimized.
…arams When handling default parameters in IIFE inlining, if no usage data is found for a parameter, be conservative and skip inlining. This prevents incorrectly substituting the default value when the parameter might be reassigned inside the function body. This can happen when a function declaration is inlined as an IIFE and the syntax context no longer matches the original analysis data. Co-authored-by: Donny/강동윤 <[email protected]>
|
@claude Fix CI |
This comment has been minimized.
This comment has been minimized.
…lining
When deciding if a function with default parameters is simple enough
to inline, we need to also check if the parameter with the default
value is reassigned in the function body. If it is reassigned
(e.g., through destructuring like `for (var [a] of [[1]])`),
inlining the default value would be incorrect.
Test case:
```javascript
function foo(a = 2) {
for (var [a] of [[1]]);
expect(a).toBe(1);
}
foo();
```
Without this fix, minified output was: `expect(2).toBe(1)` - WRONG!
The parameter `a` is reassigned to `1` through the destructuring,
so the default value `2` should not be inlined.
Co-authored-by: Donny/강동윤 <[email protected]>
|
@claude Add an enormous amount of execution tests to the minifier. Think about very complex cases. |
This comment has been minimized.
This comment has been minimized.
Add ~150 new execution tests covering complex minification scenarios: - Default parameters: closure capture, earlier param references, destructuring, function calls, reassignment in body, rest combinations - Closures: loop captures with let/var, multiple captures, nested scopes, self-referencing, IIFE sequences, arguments object - Destructuring: nested objects/arrays, renaming, rest operators, computed properties, function parameters - Spread/Rest: array concatenation, object merge, function calls, generators - Classes: inheritance, super calls, static members, getters/setters, computed method names, prototype chains - Symbols and iterators: custom iterators, Symbol.species - Generators: delegation, two-way communication, infinite sequences - Proxy/Reflect: get/set/has traps - Template literals: nested expressions, tagged templates - Optional chaining and nullish coalescing: deep access, methods - Control flow: labeled break/continue, try/catch/finally nesting - Map/Set/WeakMap/WeakSet operations - Array methods: reduce, flat, flatMap, from, find - Object methods: entries/fromEntries, defineProperty - String/Number/Math/BigInt operations - Expression evaluation: short-circuit, comma operator, conditionals - Recursion: mutual recursion, closure chains - Module patterns: revealing module pattern - This binding: arrow vs regular, call/apply/bind - Hoisting edge cases: var/function, TDZ - Property descriptors: freeze/seal - Type coercion: Symbol.toPrimitive - IIFE inlining scenarios - Dead code elimination - Constant folding - typeof/instanceof edge cases - Unicode identifiers and strings - Regex: groups, named groups, replace with function - Error handling: custom errors, rethrow Co-authored-by: kdy1 <[email protected]>
|
@claude fix CI |
This comment has been minimized.
This comment has been minimized.
When inlining default parameter values, we now check if the function body
contains any `var` declarations with the same symbol name as parameters.
Due to var hoisting, such declarations effectively reassign the parameter
even though they have different SyntaxContext values (the resolver gives
parameters and for-loop var declarations different contexts).
This fixes the incorrect transformation of:
```javascript
function foo(a = 2) {
for (var [a] of [[1]]);
expect(a).toBe(1);
}
foo();
```
Which was incorrectly being transformed to `expect(2).toBe(1)`.
Co-authored-by: Donny/강동윤 <[email protected]>
|
@claude Fix CI |
This comment has been minimized.
This comment has been minimized.
…ccess This fixes a test regression where the USED_RECURSIVELY flag was being lost between optimization passes. Added a direct check for function body property access (e.g., f.length, f.name) to prevent incorrect parameter inlining. Also updated fixture outputs to match the new more aggressive (but correct) optimization behavior. Co-authored-by: Donny/강동윤 <[email protected]>
This comment has been minimized.
This comment has been minimized.
Remove ConflictingVarFinder and IdentWithPropertyAccessFinder Visitors and replace them with simple recursive functions that iterate through statements and expressions. This is a refactoring that maintains the same functionality but avoids defining new Visitor types. Co-authored-by: Donny/강동윤 <[email protected]>
PR Review: fix(es/minifier): Allow inlining functions with side-effect-free default parametersSummaryThis PR addresses issue #11512, enabling the minifier to inline functions with default parameters when those default values have no side effects. The changes are well-structured and comprehensive. Code Quality and Best Practices ✅Strengths:
Minor suggestions:
Potential Bugs or Issues
|
There was a problem hiding this comment.
Pull request overview
This pull request enables the SWC minifier to inline functions that have default parameters with side-effect-free values. Previously, any function with default parameters (including Pat::Assign patterns) would be skipped for inlining, even when the default values were simple literals without side effects.
Changes:
- Enhanced function inlining logic to support default parameters with side-effect-free values
- Added IIFE (Immediately Invoked Function Expression) argument inlining support for default parameters
- Implemented safety checks for parameter references, side effects, and var declaration conflicts
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/swc_ecma_minifier/src/compress/optimize/inline.rs | Added logic to check and allow inlining of functions with side-effect-free default parameters, including checks for parameter interdependencies and conflicting var declarations |
| crates/swc_ecma_minifier/src/compress/optimize/iife.rs | Extended IIFE optimization to handle default parameters, with comprehensive safety checks and helper functions for detecting property access and var conflicts |
| crates/swc_ecma_minifier/tests/fixture/issues/11512/* | Test cases demonstrating the fix for the reported issue |
| crates/swc_ecma_minifier/tests/fixture/issues/11512-simple/* | Simplified test case for default parameter inlining |
| crates/swc_ecma_minifier/tests/fixture/pr/11446/* | Test case for default parameters referencing earlier parameters |
| crates/swc_ecma_minifier/tests/terser/compress/evaluate/issue_2926_1/output.js | Updated expected output after optimization improvement |
| crates/swc_ecma_minifier/tests/fixture/issues/firebase-firestore/1/output.js | Updated expected output showing function inlining |
| crates/swc_ecma_minifier/tests/exec.rs | Added 3000+ lines of comprehensive execution tests covering edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let references_earlier_param = | ||
| param_idents.iter().take(idx).any(|maybe_ident| { | ||
| match maybe_ident { | ||
| Some(ident) => { | ||
| contains_ident_ref(&*assign_pat.right, ident) | ||
| } | ||
| // Complex pattern - be conservative | ||
| None => true, | ||
| } | ||
| }); |
There was a problem hiding this comment.
The logic for checking if a default parameter references earlier parameters appears correct. However, there's a subtle issue: when checking if a default value references earlier parameters in the IIFE code (lines 385-394), the code returns true if maybe_ident is None (complex pattern). This is conservative but might be overly restrictive. For simple cases where the default doesn't reference anything, this would still prevent inlining even though it's safe. Consider checking if the default value references ANY identifier before being conservative about complex patterns.
| for earlier_param in | ||
| f.function.params.iter().take(param_idx) | ||
| { | ||
| let earlier_ident = match &earlier_param.pat { | ||
| Pat::Ident(id) => Some(&id.id), | ||
| Pat::Assign(a) => { | ||
| if let Pat::Ident(id) = &*a.left { | ||
| Some(&id.id) | ||
| } else { | ||
| // Complex pattern - be conservative | ||
| return; | ||
| } | ||
| } | ||
| _ => { | ||
| // Complex pattern - be conservative | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if let Some(ident) = earlier_ident { | ||
| if contains_ident_ref(&*assign.right, ident) { | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When checking for references to earlier parameters in inline.rs (lines 821-845), the code returns early if any earlier parameter has a complex pattern (lines 830-831, 834-836). While this is conservative and safe, it might be overly restrictive. Consider: if the default value doesn't reference any identifiers at all (e.g., it's just a literal like 42), it would still skip inlining due to a complex earlier parameter. This might prevent some valid optimizations. However, given the complexity of handling all edge cases, the current conservative approach is reasonable.
| if let Pat::Ident(param_id) = &*assign.left { | ||
| if has_conflicting_vars | ||
| && param_names.contains(¶m_id.sym) | ||
| { |
There was a problem hiding this comment.
The condition has_conflicting_vars && param_names.contains(¶m_id.sym) has a logical issue. The function has_conflicting_var_decl returns true if ANY var declaration in the body conflicts with ANY parameter name. The subsequent check param_names.contains(¶m_id.sym) will always be true for a parameter (since we're iterating over parameters), making it redundant. The code should instead check if THIS specific parameter has a conflicting var declaration. Consider refactoring has_conflicting_var_decl to return FxHashSet<Atom> containing only the parameter names that have conflicts, then check conflicting_names.contains(¶m_id.sym).
| if let Pat::Ident(param_id) = &*assign.left { | |
| if has_conflicting_vars | |
| && param_names.contains(¶m_id.sym) | |
| { | |
| if let Pat::Ident(_param_id) = &*assign.left { | |
| if has_conflicting_vars { |
| // body that has the same name as this parameter. Due to var | ||
| // hoisting, such declarations effectively reassign the parameter, | ||
| // but they may have different SyntaxContext values. | ||
| if has_conflicting_vars && param_names.contains(¶m_id.sym) { |
There was a problem hiding this comment.
The condition has_conflicting_vars && param_names.contains(¶m_id.sym) has a logical issue. The function has_conflicting_var_decl returns true if ANY var declaration in the body conflicts with ANY parameter name. The subsequent check param_names.contains(¶m_id.sym) will always be true for a parameter (since we're iterating over parameters), making it redundant. The code should instead check if THIS specific parameter has a conflicting var declaration. Consider refactoring has_conflicting_var_decl to return FxHashSet<Atom> containing only the parameter names that have conflicts, then check conflicting_names.contains(¶m_id.sym).
| if has_conflicting_vars && param_names.contains(¶m_id.sym) { | |
| if has_conflicting_vars { |
| // Check if this is a member expression with the target ident as object | ||
| Expr::Member(member) => { | ||
| if let Expr::Ident(obj_ident) = &*member.obj { | ||
| if obj_ident.sym == ident.sym { |
There was a problem hiding this comment.
The property access check only compares symbol names (sym) without checking the SyntaxContext (ctxt). This could lead to false positives when different identifiers with the same name but different contexts are used. Consider using obj_ident.sym == ident.sym && obj_ident.ctxt == ident.ctxt to ensure proper identifier comparison, similar to how contains_ident_ref works in swc_ecma_utils.
| if obj_ident.sym == ident.sym { | |
| if obj_ident.sym == ident.sym && obj_ident.span.ctxt == ident.span.ctxt { |
| Expr::OptChain(opt) => match &*opt.base { | ||
| OptChainBase::Member(m) => { | ||
| if let Expr::Ident(obj_ident) = &*m.obj { | ||
| if obj_ident.sym == ident.sym { |
There was a problem hiding this comment.
The property access check only compares symbol names (sym) without checking the SyntaxContext (ctxt). This could lead to false positives when different identifiers with the same name but different contexts are used. Consider using obj_ident.sym == ident.sym && obj_ident.ctxt == ident.ctxt to ensure proper identifier comparison, similar to how contains_ident_ref works in swc_ecma_utils.
| if obj_ident.sym == ident.sym { | |
| if obj_ident.sym == ident.sym && obj_ident.span.ctxt == ident.span.ctxt { |
| if let Expr::Ident(obj_ident) = &*m.obj { | ||
| if obj_ident.sym == ident.sym { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The property access check only compares symbol names (sym) without checking the SyntaxContext (ctxt). This could lead to false positives when different identifiers with the same name but different contexts are used. Consider using obj_ident.sym == ident.sym && obj_ident.ctxt == ident.ctxt to ensure proper identifier comparison, similar to how contains_ident_ref works in swc_ecma_utils.
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
2 participants
Summary
Fixes #11512
🤖 Generated with Claude Code