refactor(minifier): Implement PR review feedback for parameter inlining

Address code review comments from lukesandberg (PR #11156, review #3378066428):

1. Replace FxHashSet<usize> with sorted Vec<usize> for inlined_params
   - Simpler data structure since indices are already populated in sorted order
   - More efficient iteration during argument removal

2. Optimize implicit undefined handling with Option<Option<Box<Expr>>>
   - Avoids allocating Expr::Ident for undefined on each iteration
   - Only allocates when actually adding to params_to_inline
   - Clearer semantics: None = implicit undefined, Some(Some(expr)) = explicit value

3. Remove Option wrapper from Vec<Box<Expr>> in usage analyzer
   - Analyzer only inserts Some values, never None
   - Simplifies type signature: Vec<Vec<Box<Expr>>> instead of Vec<Vec<Option<Box<Expr>>>>

4. Simplify to always use 'let' instead of tracking reassignment
   - Preserves parameter semantics (always reassignable)
   - Reduces complexity by avoiding reassignment analysis
   - May enable better constant folding in other passes

5. Document NaN handling in expr_eq
   - Added doc comments explaining why custom equality is needed
   - NaN is treated as equal to NaN for parameter inlining
   - Context-aware identifier comparison for unresolved "undefined"

6. Simplify identifier comparison in expr_eq
   - Use naive comparison since is_safe_constant_for_param_inline already validates
   - Avoids repeating validation logic

Related: #10931

Author: github-actions[bot]

crates/swc_ecma_minifier/src/compress/optimize/mod.rs

@@ -244,8 +244,9 @@ struct Optimizer<'a> {
     functions: Box<FxHashMap<Id, FnMetadata>>,
 
     /// Tracks which parameter indices have been inlined for each function.
-    /// Maps function ID to a set of parameter indices that were removed.
-    inlined_params: Box<FxHashMap<Id, FxHashSet<usize>>>,
+    /// Maps function ID to a sorted vector of parameter indices that were
+    /// removed.
+    inlined_params: Box<FxHashMap<Id, Vec<usize>>>,
 }
 
 #[derive(Default)]

crates/swc_ecma_minifier/src/compress/optimize/params.rs

@@ -1,4 +1,3 @@
-use rustc_hash::FxHashSet;
 use swc_common::DUMMY_SP;
 use swc_ecma_ast::*;
 
@@ -104,59 +103,48 @@ impl Optimizer<'_> {
         }
 
         // Analyze each parameter
-        let mut params_to_inline: Vec<(usize, Box<Expr>, bool)> = Vec::new();
+        let mut params_to_inline: Vec<(usize, Box<Expr>)> = Vec::new();
 
         for (param_idx, param) in f.params.iter().enumerate() {
             // Only handle simple identifier parameters
-            let param_id = match &param.pat {
+            let _param_id = match &param.pat {
                 Pat::Ident(ident) => ident.id.to_id(),
                 _ => continue, // Skip destructuring, rest params, etc.
             };
 
-            // Check if parameter is reassigned within the function
-            // If so, we'll use 'let' instead of 'const'
-            let is_reassigned = if let Some(param_usage) = self.data.vars.get(&param_id) {
-                param_usage.flags.contains(VarUsageInfoFlags::REASSIGNED)
-            } else {
-                false
-            };
-
             // Collect argument values for this parameter across all call sites
-            let mut common_value: Option<Box<Expr>> = None;
+            // Use Option<Option<Box<Expr>>> to track:
+            // - None = no common value yet established
+            // - Some(None) = all call sites have implicit undefined
+            // - Some(Some(expr)) = all call sites have the same explicit expression
+            let mut common_value: Option<Option<Box<Expr>>> = None;
             let mut inlinable = true;
 
             for call_site in call_sites {
-                let arg_value = if param_idx < call_site.len() {
-                    call_site[param_idx].clone()
+                // Get argument at this position, or None if implicit undefined
+                let arg_value: Option<Box<Expr>> = if param_idx < call_site.len() {
+                    Some(call_site[param_idx].clone())
                 } else {
-                    // Implicit undefined - use unresolved context
-                    Some(Box::new(Expr::Ident(Ident::new(
-                        "undefined".into(),
-                        DUMMY_SP,
-                        self.ctx.expr_ctx.unresolved_ctxt,
-                    ))))
+                    None // Implicit undefined
                 };
 
-                let arg_value = match arg_value {
-                    Some(v) => v,
-                    None => {
+                // Check if this is a safe, constant value to inline
+                if let Some(ref expr) = arg_value {
+                    if !self.is_safe_constant_for_param_inline(expr) {
                         inlinable = false;
                         break;
                     }
-                };
-
-                // Check if this is a safe, constant value to inline
-                if !self.is_safe_constant_for_param_inline(&arg_value) {
-                    inlinable = false;
-                    break;
                 }
+                // Implicit undefined (None) is always safe to inline
 
                 match &common_value {
                     None => {
+                        // First call site, establish the common value
                         common_value = Some(arg_value);
                     }
                     Some(existing) => {
-                        if !self.expr_eq(existing, &arg_value) {
+                        // Check if this call site has the same value
+                        if !self.arg_eq(existing, &arg_value) {
                             inlinable = false;
                             break;
                         }
@@ -166,8 +154,17 @@ impl Optimizer<'_> {
 
             // If all call sites pass the same constant value, mark for inlining
             if inlinable {
-                if let Some(value) = common_value {
-                    params_to_inline.push((param_idx, value, is_reassigned));
+                if let Some(Some(value)) = common_value {
+                    // Explicit value passed at all call sites
+                    params_to_inline.push((param_idx, value));
+                } else if let Some(None) = common_value {
+                    // All call sites have implicit undefined
+                    let undefined_expr = Box::new(Expr::Ident(Ident::new(
+                        "undefined".into(),
+                        DUMMY_SP,
+                        self.ctx.expr_ctx.unresolved_ctxt,
+                    )));
+                    params_to_inline.push((param_idx, undefined_expr));
                 }
             }
         }
@@ -182,7 +179,7 @@ impl Optimizer<'_> {
     fn apply_param_inlining(
         &mut self,
         f: &mut Function,
-        params_to_inline: &[(usize, Box<Expr>, bool)],
+        params_to_inline: &[(usize, Box<Expr>)],
         fn_id: &Id,
     ) {
         // Sort in reverse order to remove from the end first
@@ -192,25 +189,20 @@ impl Optimizer<'_> {
         // Collect variable declarations to add at the beginning of function body
         let mut var_decls = Vec::new();
 
-        // Track which parameter indices are being inlined
-        let mut inlined_indices = FxHashSet::default();
+        // Track which parameter indices are being inlined (will be sorted)
+        let mut inlined_indices = Vec::new();
 
-        for (param_idx, value, is_reassigned) in &sorted_params {
-            inlined_indices.insert(*param_idx);
+        for (param_idx, value) in &sorted_params {
+            inlined_indices.push(*param_idx);
 
             if let Some(param) = f.params.get(*param_idx) {
                 if let Pat::Ident(ident) = &param.pat {
-                    // Create variable declaration: const/let paramName = value;
-                    // Use 'let' if the parameter is reassigned, 'const' otherwise
-                    let var_kind = if *is_reassigned {
-                        VarDeclKind::Let
-                    } else {
-                        VarDeclKind::Const
-                    };
+                    // Create variable declaration: let paramName = value;
+                    // Always use 'let' to preserve parameter semantics (reassignable)
                     var_decls.push(Stmt::Decl(Decl::Var(Box::new(VarDecl {
                         span: DUMMY_SP,
                         ctxt: Default::default(),
-                        kind: var_kind,
+                        kind: VarDeclKind::Let,
                         declare: false,
                         decls: vec![VarDeclarator {
                             span: DUMMY_SP,
@@ -224,7 +216,7 @@ impl Optimizer<'_> {
         }
 
         // Remove parameters (in reverse order)
-        for (param_idx, _, _) in &sorted_params {
+        for (param_idx, _) in &sorted_params {
             f.params.remove(*param_idx);
         }
 
@@ -236,7 +228,8 @@ impl Optimizer<'_> {
         }
 
         // Store the inlined parameter indices for later use when visiting call
-        // sites
+        // sites. Reverse sort to get descending order for efficient removal.
+        inlined_indices.sort_by(|a, b| b.cmp(a));
         self.inlined_params.insert(fn_id.clone(), inlined_indices);
 
         self.changed = true;
@@ -283,14 +276,31 @@ impl Optimizer<'_> {
         }
     }
 
+    /// Compare two optional argument values for equality.
+    /// None represents implicit undefined.
+    fn arg_eq(&self, a: &Option<Box<Expr>>, b: &Option<Box<Expr>>) -> bool {
+        match (a, b) {
+            (None, None) => true, // Both implicit undefined
+            (Some(a_expr), Some(b_expr)) => self.expr_eq(a_expr, b_expr),
+            _ => false, // One is implicit undefined, the other is not
+        }
+    }
+
     /// Compare two expressions for equality (structural equality for simple
     /// cases).
+    ///
+    /// We cannot use the derived `Eq` trait because:
+    /// 1. NaN handling: We treat NaN as equal to NaN for parameter inlining
+    ///    purposes, whereas standard floating point comparison has NaN != NaN.
+    /// 2. Context-aware identifier comparison: We only consider unresolved
+    ///    "undefined" identifiers as safe to inline, checking the syntax
+    ///    context.
     fn expr_eq(&self, a: &Expr, b: &Expr) -> bool {
         match (a, b) {
             (Expr::Lit(Lit::Null(_)), Expr::Lit(Lit::Null(_))) => true,
             (Expr::Lit(Lit::Bool(a)), Expr::Lit(Lit::Bool(b))) => a.value == b.value,
             (Expr::Lit(Lit::Num(a)), Expr::Lit(Lit::Num(b))) => {
-                // Handle NaN specially
+                // Handle NaN specially: treat NaN as equal to NaN for parameter inlining
                 if a.value.is_nan() && b.value.is_nan() {
                     true
                 } else {
@@ -299,15 +309,9 @@ impl Optimizer<'_> {
             }
             (Expr::Lit(Lit::Str(a)), Expr::Lit(Lit::Str(b))) => a.value == b.value,
             (Expr::Lit(Lit::BigInt(a)), Expr::Lit(Lit::BigInt(b))) => a.value == b.value,
-            // Compare identifiers:
-            // Only allow unresolved "undefined" identifiers
-            (Expr::Ident(a), Expr::Ident(b)) => {
-                let a_is_unresolved = a.ctxt == self.ctx.expr_ctx.unresolved_ctxt;
-                let b_is_unresolved = b.ctxt == self.ctx.expr_ctx.unresolved_ctxt;
-
-                // Both must be unresolved "undefined"
-                a_is_unresolved && b_is_unresolved && a.sym == "undefined" && b.sym == "undefined"
-            }
+            // Compare identifiers: we can compare naively since is_safe_constant_for_param_inline
+            // already validated that only unresolved "undefined" identifiers reach here
+            (Expr::Ident(a), Expr::Ident(b)) => a.sym == b.sym && a.ctxt == b.ctxt,
             (
                 Expr::Unary(UnaryExpr {
                     op: op_a,
@@ -348,11 +352,8 @@ impl Optimizer<'_> {
         };
 
         // Remove arguments at the inlined parameter indices
-        // We need to iterate in reverse order to avoid index shifting issues
-        let mut indices_to_remove: Vec<usize> = inlined_indices.iter().copied().collect();
-        indices_to_remove.sort_by(|a, b| b.cmp(a)); // Sort in descending order
-
-        for idx in indices_to_remove {
+        // The indices are already sorted in descending order from apply_param_inlining
+        for &idx in inlined_indices {
             if idx < call.args.len() {
                 call.args.remove(idx);
                 self.changed = true;

crates/swc_ecma_minifier/src/program_data.rs

@@ -135,9 +135,11 @@ pub(crate) struct VarUsageInfo {
     pub(crate) accessed_props: FxHashMap<Wtf8Atom, u32>,
     pub(crate) accessed_props: FxHashMap<Atom, u32>,
 
-    /// Tracks call sites for functions. Maps parameter index to list of
-    /// argument expressions. Used for parameter inlining optimization.
-    pub(crate) call_site_args: Option<Vec<Vec<Option<Box<Expr>>>>>,
+    /// Tracks call sites for functions. Each inner Vec contains the arguments
+    /// passed at that call site. Used for parameter inlining optimization.
+    /// Arguments beyond the call site's actual argument count are implicitly
+    /// undefined.
+    pub(crate) call_site_args: Option<Vec<Vec<Box<Expr>>>>,
 }
 
 impl Default for VarUsageInfo {
@@ -566,7 +568,7 @@ impl Storage for ProgramData {
         self.vars.get(&id).map(|v| v.as_ref())
     }
 
-    fn record_call_site_args(&mut self, callee_id: Id, args: Vec<Option<Box<Expr>>>) {
+    fn record_call_site_args(&mut self, callee_id: Id, args: Vec<Box<Expr>>) {
         let var = self.vars.entry(callee_id).or_default();
 
         // Initialize the call_site_args if it doesn't exist

crates/swc_ecma_usage_analyzer/src/analyzer/mod.rs

@@ -445,7 +445,7 @@ where
                         call_args.clear();
                         break;
                     }
-                    call_args.push(Some(arg.expr.clone()));
+                    call_args.push(arg.expr.clone());
                 }
 
                 // Only record if we have a valid argument list (no spread)

crates/swc_ecma_usage_analyzer/src/analyzer/storage.rs

@@ -45,7 +45,7 @@ pub trait Storage: Sized + Default {
 
     /// Records arguments passed to a function at a call site.
     /// Used for parameter inlining optimization.
-    fn record_call_site_args(&mut self, callee_id: Id, args: Vec<Option<Box<Expr>>>);
+    fn record_call_site_args(&mut self, callee_id: Id, args: Vec<Box<Expr>>);
 }
 
 pub trait ScopeDataLike: Sized + Default + Clone {