-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(es/minifier): Allow inlining functions with side-effect-free default parameters #11516
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?
Changes from 7 commits
989c95d
eab14af
00faf7d
5862276
d4a751a
dac5669
5b763c8
88f0f73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,12 @@ use swc_common::{util::take::Take, EqIgnoreSpan, Mark}; | |
| use swc_ecma_ast::*; | ||
| use swc_ecma_usage_analyzer::alias::{collect_infects_from, AliasConfig}; | ||
| use swc_ecma_utils::{ | ||
| class_has_side_effect, collect_decls, contains_this_expr, find_pat_ids, ExprExt, Remapper, | ||
| class_has_side_effect, collect_decls, contains_ident_ref, contains_this_expr, find_pat_ids, | ||
| ExprExt, Remapper, | ||
| }; | ||
| use swc_ecma_visit::VisitMutWith; | ||
|
|
||
| use super::Optimizer; | ||
| use super::{iife::has_conflicting_var_decl, Optimizer}; | ||
| use crate::{ | ||
| compress::{ | ||
| optimize::{util::is_valid_for_lhs, BitCtx}, | ||
|
|
@@ -746,12 +747,105 @@ impl Optimizer<'_> { | |
| usage, | ||
| ) | ||
| { | ||
| if f.function | ||
| // Collect parameter names for checking conflicting var | ||
| // declarations | ||
| let param_names: FxHashSet<_> = f | ||
| .function | ||
| .params | ||
| .iter() | ||
| .any(|param| matches!(param.pat, Pat::Rest(..) | Pat::Assign(..))) | ||
| { | ||
| return; | ||
| .filter_map(|p| match &p.pat { | ||
| Pat::Ident(id) => Some(id.sym.clone()), | ||
| Pat::Assign(a) => { | ||
| if let Pat::Ident(id) = &*a.left { | ||
| Some(id.sym.clone()) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Check if the function body contains any var declarations that | ||
| // conflict with parameter names | ||
| let has_conflicting_vars = has_conflicting_var_decl(body, ¶m_names); | ||
|
|
||
| // Check for rest parameters (can't inline) and default | ||
| // parameters with side effects or that reference other | ||
| // parameters (unsafe to inline) | ||
| for (param_idx, param) in f.function.params.iter().enumerate() { | ||
| match ¶m.pat { | ||
| Pat::Rest(..) => return, | ||
| Pat::Assign(assign) => { | ||
| // Skip inlining if the default value has side effects | ||
| if assign.right.may_have_side_effects(self.ctx.expr_ctx) { | ||
| return; | ||
| } | ||
|
|
||
| // Skip inlining if there's a conflicting var declaration | ||
| // in the function 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 let Pat::Ident(param_id) = &*assign.left { | ||
| if has_conflicting_vars | ||
| && param_names.contains(¶m_id.sym) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Skip inlining if the parameter is reassigned. | ||
| // E.g., `function(a = 2) { for (var [a] of [[1]]); }` - | ||
| // `a` is reassigned through destructuring, so | ||
| // inlining the default value would be incorrect. | ||
| if let Pat::Ident(param_id) = &*assign.left { | ||
| if let Some(param_usage) = | ||
| self.data.vars.get(¶m_id.to_id()) | ||
| { | ||
| if param_usage | ||
| .flags | ||
| .contains(VarUsageInfoFlags::REASSIGNED) | ||
| { | ||
| return; | ||
| } | ||
| } else { | ||
| // No usage data - be conservative | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Skip inlining if the default value references any | ||
| // earlier parameter. E.g., `function(a, b = a) {...}` - | ||
| // `b = a` references `a`, which makes inlining unsafe. | ||
| 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; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+821
to
+845
|
||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| report_change!( | ||
| "inline: Decided to inline function `{}{:?}` as it's very simple", | ||
|
|
||
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
has_conflicting_vars && param_names.contains(¶m_id.sym)has a logical issue. The functionhas_conflicting_var_declreturns true if ANY var declaration in the body conflicts with ANY parameter name. The subsequent checkparam_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 refactoringhas_conflicting_var_declto returnFxHashSet<Atom>containing only the parameter names that have conflicts, then checkconflicting_names.contains(¶m_id.sym).