Skip to content

Conversation

@Austaras
Copy link
Member

@Austaras Austaras commented Jan 7, 2026

Description:

BREAKING CHANGE:

Related issue (if exists):

Copilot AI review requested due to automatic review settings January 7, 2026 14:36
@Austaras Austaras requested a review from a team as a code owner January 7, 2026 14:36
@changeset-bot
Copy link

changeset-bot bot commented Jan 7, 2026

⚠️ No Changeset found

Latest commit: e17ddae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

param.id.ctxt
);
vars.insert(param.to_id(), arg.clone());
vars.insert(param.take().to_id(), arg.take());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the near future we should merge this with var inline.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 7, 2026

CodSpeed Performance Report

Merging this PR will improve performance by 2.2%

Comparing Austaras:main (e17ddae) with main (dd007c6)

Summary

⚡ 1 improved benchmark
✅ 184 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
es/minifier/libs/typescript 3.9 s 3.9 s +2.2%

@kdy1 kdy1 added this to the Planned milestone Jan 7, 2026
Copy link
Contributor

Copilot AI left a 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 optimizes the IIFE (Immediately Invoked Function Expression) inlining process in the minifier by removing inlined arguments and parameters instead of cloning them. This improves both performance and code size by properly removing unused parameters and arguments after inlining their values into the function body.

Key changes:

  • Replace .clone() with .take() for arguments and parameters during IIFE inlining
  • Properly clean up invalid arguments and parameters after inlining
  • Add comprehensive pattern validation to detect and remove dummy identifiers and invalid patterns

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/swc_ecma_minifier/src/compress/optimize/iife.rs Core implementation: Changed from cloning to taking arguments/parameters when inlining IIFEs, added improved cleanup logic for invalid patterns
crates/swc_ecma_minifier/tests/terser/compress/reduce_vars/defun_inline_1/output.js Test output: IIFE parameters and arguments properly removed after inlining
crates/swc_ecma_minifier/tests/terser/compress/reduce_vars/defun_inline_2/output.js Test output: IIFE parameters and arguments properly removed after inlining
crates/swc_ecma_minifier/tests/terser/compress/hoist_props/name_collision_3/output.js Test output: Unused IIFE parameters removed
crates/swc_ecma_minifier/tests/terser/compress/hoist_props/name_collision_2/output.js Test output: Unused IIFE parameters removed
crates/swc_ecma_minifier/tests/terser/compress/hoist_props/direct_access_2/output.js Test output: Unused IIFE parameters removed
crates/swc_ecma_minifier/tests/terser/compress/functions/issue_2657/output.js Test output: Unused IIFE parameters removed
crates/swc_ecma_minifier/tests/terser/compress/evaluate/issue_2916_1/output.js Test output: First parameter of IIFE removed after inlining
crates/swc_ecma_minifier/tests/projects/output/react-dom-17.0.2.js Test output: Multiple IIFE parameters removed throughout large project file
crates/swc_ecma_minifier/tests/projects/output/jquery.mobile-1.4.2.js Test output: IIFE parameters removed in production code
crates/swc_ecma_minifier/tests/fixture/next/wrap-contracts/output.js Test output: IIFE parameters removed in Next.js fixture
crates/swc_ecma_minifier/tests/fixture/next/syncfusion/933-e9f9a6bf671b96fc/output.js Test output: IIFE parameters removed in Syncfusion fixture
crates/swc_ecma_minifier/tests/fixture/next/react-pdf-renderer/output.js Test output: IIFE parameters removed in React PDF renderer
crates/swc_ecma_minifier/tests/fixture/next/47005/output.js Test output: IIFE parameters and unnecessary variable declarations removed
crates/swc_ecma_minifier/tests/fixture/next/33265/static/chunks/pages/index-cb36c1bf7f830e3c/output.js Test output: IIFE parameters removed
crates/swc_ecma_minifier/tests/fixture/issues/react-countup/1/output.js Test output: IIFE parameters removed in destructuring helpers
crates/swc_ecma_minifier/tests/fixture/issues/quagga2/1.4.2/1/output.js Test output: IIFE parameters removed
crates/swc_ecma_minifier/tests/fixture/issues/firebase-firestore/1/output.js Test output: IIFE parameters removed
crates/swc_ecma_minifier/tests/fixture/issues/firebase-core/1/output.js Test output: IIFE parameters removed in destructuring helpers
crates/swc_ecma_minifier/tests/fixture/issues/emotion/react/1/output.js Test output: IIFE parameters removed in destructuring helpers
crates/swc_ecma_minifier/tests/benches-full/victory.js Test output: IIFE parameters removed in benchmark file
crates/swc_ecma_minifier/tests/benches-full/echarts.js Test output: Multiple IIFE parameters removed throughout large benchmark file
crates/swc/tests/tsc-references/jsDeclarationsClasses.2.minified.js Test output: IIFE with null superclass has parameter removed
crates/swc/tests/tsc-references/emitDefaultParametersFunctionExpressionES6.2.minified.js Test output: Entire unused IIFE removed
crates/swc/tests/tsc-references/callNonGenericFunctionWithTypeArguments.2.minified.js Test output: Variable declarations updated due to IIFE inlining
crates/swc/tests/tsc-references/callGenericFunctionWithZeroTypeArguments.2.minified.js Test output: Variable declarations updated due to IIFE inlining
crates/swc/tests/fixture/issues-4xxx/4953/output/index.js Test output: IIFE parentheses changed from () to !function pattern

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28447816 bytes)

Commit: 4b0515b

@kdy1
Copy link
Member

kdy1 commented Jan 7, 2026

@claude Review this PR,

@claude
Copy link
Contributor

claude bot commented Jan 7, 2026

Claude finished @kdy1's task in 2m 42s —— View job


Code Review Complete

Summary

This PR optimizes IIFE argument inlining by replacing .clone() operations with .take() operations, which moves ownership instead of copying. This reduces allocations and improves performance. The test results show a 2.06% performance improvement on the TypeScript benchmark and smaller output sizes.

Issues Found

🔴 Critical: Span accessed after take() in rest parameter handling

Location: crates/swc_ecma_minifier/src/compress/optimize/iife.rs:264-266

The code takes param_id on line 264, then accesses param_id.span on line 266:

vars.insert(
    param_id.take().to_id(),  // Line 264: param_id is taken here
    ArrayLit {
        span: param_id.span,  // Line 266: PROBLEM - accessing span after take()
        elems: e.args.iter_mut()...

After .take(), param_id is replaced with a dummy identifier, so param_id.span will likely be DUMMY_SP instead of the original span. This is a semantic change from the original code where the span was captured before the take.

Fix: Save the span before taking:

let span = param_id.span;
vars.insert(
    param_id.take().to_id(),
    ArrayLit {
        span,  // Use saved span
        elems: ...

🟡 Minor: Inconsistent parameter handling for undefined arguments

Location: crates/swc_ecma_minifier/src/compress/optimize/iife.rs:235

When an argument is undefined (None), the code uses param.to_id() without taking the param:

} else {
    // No argument provided
    vars.insert(param.to_id(), Expr::undefined(param.span()));  // Doesn't take param
}

But when an argument IS provided and inlined, it uses param.take().to_id(). This creates an inconsistency where some params are taken and others aren't. The param should probably be taken in both cases for consistency, since it will be removed from the parameter list later anyway.

Fix: Take the param in all branches:

} else {
    let span = param.span();
    vars.insert(param.take().to_id(), Expr::undefined(span));
}

Positive Aspects

Performance improvement: The change from cloning to taking is a good optimization
Cleaner argument cleanup: The new e.args.retain(|a| !a.expr.is_invalid()) cleanup is more explicit
Better pattern validation: The new is_invalid_pat() helper properly checks for dummy identifiers and handles nested patterns
Test coverage: All 29 test output files show the expected behavior - unused IIFE parameters are properly removed
Size reduction: Test snapshots show smaller output sizes (e.g., TypeScript libs reduced by a few bytes)

Minor Observations

  • The author's comment about merging this with var inline in the future makes sense - there's definitely overlap in the logic
  • The improved error messages (e.g., "iife: used recursively" instead of "iife: [x] Recursive?") are more consistent
  • The change from immutable to mutable borrows (get()get_mut()) is correct for the new take-based approach

Breaking Change Note

The PR description mentions this is a breaking change but doesn't explain why. Based on the code changes, this appears to be an internal optimization that shouldn't break public APIs. The "breaking" aspect might refer to:

  • Changed output format (parameters removed from IIFEs)
  • Potential span changes in edge cases (like the issue noted above)

However, these are output changes not API changes. Consider clarifying in the PR description whether this is truly a breaking change or just an optimization.

Recommendations

  1. Fix the critical span issue in the rest parameter handling (line 266)
  2. Consider taking params consistently in all branches for undefined arguments
  3. Add a changeset as noted by the changeset-bot
  4. Clarify the "BREAKING CHANGE" label in the PR description - explain what breaks
  5. Consider adding a test specifically for rest parameters to ensure spans are preserved correctly

Test Evidence

The test outputs look correct - for example, in defun_inline_1/output.js:

// Before: function(b) { return 2; }(0)
// After:  function() { return 2; }()

The unused parameter b and argument 0 are both correctly removed. ✅


Overall Assessment: Good performance optimization with one critical bug that needs fixing before merge. The approach is sound, but the span handling after take() needs attention.

Copilot AI review requested due to automatic review settings January 8, 2026 02:58
Copy link
Contributor

Copilot AI left a 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 60 out of 60 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/swc/tests/tsc-references/emitDefaultParametersFunctionExpressionES6.2.minified.js:2

  • The entire IIFE call has been removed, leaving only the file header comment. This suggests that the IIFE had no side effects and was completely optimized away. However, this seems too aggressive - the original code likely had !function(num, boo = !1) {}(0); which is a function call that should at least remain as a statement, even if it does nothing. Removing it entirely changes the semantics of the code.
//// [emitDefaultParametersFunctionExpressionES6.ts]


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants