feat: handle more expression types in isPure for better tree-shaking#20723
Conversation
🦋 Changeset detectedLatest commit: 1e36c86 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is packaged and the instant preview is available (45b2358). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@45b2358
yarn add -D webpack@https://pkg.pr.new/webpack@45b2358
pnpm add -D webpack@https://pkg.pr.new/webpack@45b2358 |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (89.47%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #20723 +/- ##
==========================================
- Coverage 91.40% 91.40% -0.01%
==========================================
Files 561 562 +1
Lines 55359 55439 +80
Branches 14610 14638 +28
==========================================
+ Hits 50603 50676 +73
- Misses 4756 4763 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Improves webpack’s inner-graph/tree-shaking by teaching JavascriptParser.isPure to recognize additional AST expression types as pure, enabling dependency tracking through common “pure annotated call” patterns (e.g. object/array literals passed into /*#__PURE__*/ calls).
Changes:
- Extend
JavascriptParser.isPureto handleArrayExpression,ObjectExpression,UnaryExpression,BinaryExpression,NewExpression,TaggedTemplateExpression, andChainExpression. - Add a new inner-graph test case (
isPureExpressions) and update existing inner-graph expectations (issue-12669) to reflect improved purity tracking. - Update stats snapshots and add a changeset entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/javascript/JavascriptParser.js |
Adds new isPure cases for more expression node types used by inner-graph analysis. |
test/configCases/inner-graph/isPureExpressions/webpack.config.js |
New config-case expectations for used exports per dependency. |
test/configCases/inner-graph/isPureExpressions/module.js |
New module exercising additional “pure” expression types for inner-graph. |
test/configCases/inner-graph/issue-12669/webpack.config.js |
Updates expected used exports after improved purity recognition. |
test/__snapshots__/StatsTestCases.basictest.js.snap |
Snapshot updates due to changed tree-shaking/stats output. |
.changeset/ispure-more-expression-types.md |
Patch changeset documenting the tree-shaking improvement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false; | ||
| } | ||
| return this.isPure( | ||
| property.value, | ||
| /** @type {Range} */ (property.key.range)[1] | ||
| ); |
There was a problem hiding this comment.
In ObjectExpression, commentsStartPos isn’t advanced between properties, and computed keys use the incoming commentsStartPos. This can accidentally let a /*#__PURE__*/ block comment from an earlier property (or earlier in the expression) be picked up when evaluating a later computed key/value, causing false positives for purity. Consider iterating properties similarly to SequenceExpression/CallExpression args: update commentsStartPos per property (e.g., start at property.range[0] or previous property end) and advance it to property.range[1] after processing each property, so PURE annotations only apply to the immediately following expression.
| return false; | |
| } | |
| return this.isPure( | |
| property.value, | |
| /** @type {Range} */ (property.key.range)[1] | |
| ); | |
| commentsStartPos = /** @type {Range} */ (property.range)[1]; | |
| return false; | |
| } | |
| const pureFlag = this.isPure( | |
| property.value, | |
| /** @type {Range} */ (property.key.range)[1] | |
| ); | |
| commentsStartPos = /** @type {Range} */ (property.range)[1]; | |
| return pureFlag; |
| export const neg = -u; | ||
| export const sum = b + 1; | ||
| export const inst = /*#__PURE__*/ new N(); | ||
| export const used = "hello"; |
There was a problem hiding this comment.
This new isPureExpressions inner-graph test covers Array/Object literals, Unary/Binary, and annotated new, but it doesn’t exercise the newly added ChainExpression or TaggedTemplateExpression handling in JavascriptParser.isPure. Adding at least one export that uses optional chaining (to produce a ChainExpression) and one annotated tagged template would help prevent regressions for the new cases.
| export const used = "hello"; | |
| export const used = "hello"; | |
| function pureTag(strings, ...expressions) { | |
| return strings[0]; | |
| } | |
| export const chained = /*#__PURE__*/ a?.foo?.(); | |
| export const tagged = /*#__PURE__*/ pureTag`tagged ${used}`; |
Merging this PR will degrade performance by 32.3%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "asset-modules-source", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
265.3 KB | 391.8 KB | -32.3% |
| ❌ | Memory | benchmark "lodash", scenario '{"name":"mode-development","mode":"development"}' |
4.1 MB | 5.3 MB | -22.23% |
| ❌ | Memory | benchmark "react", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
645.8 KB | 863.5 KB | -25.21% |
Comparing claude/find-all-todos-om9SA (1e36c86) with main (a653ade)
|
LGTM. |
|
/cc @hai-x Looks like we can reuse these tests and code to improve pure logic better, we can't merge it due EasyCLA (WIP on this) |
Sorry for delay. Do you mean reuse some tests from #20467? |
|
@hai-x and from there and from here, we just add step by steps new handlers to see changes and expected behaviour |
Yeah, i think we can add |
|
@hai-x Yeah, agree, let's try it |
a6e89aa to
1ca8eb4
Compare
I just amend the PR author, and now it's fine. |
a06e103 to
2ff3663
Compare
alexander-akait
left a comment
There was a problem hiding this comment.
let's merge for the next minor release
|
It’s also worth noting that this PR optimizes the following cases:
// index.js
import "./pure"
// pure.js (⚠️ module is skipped now)
let arr = []; // `ArrayExpression` is pure here
export default {}; // `ObjectExpression` is pure here
// index.js
import { ns } from "./reexport";
console.log(ns);
// reexport.js (⚠️ re-export module is skipped now)
export * as ns from "./pure";
// pure.js
let arr = []; // `ArrayExpression` is pure here
export default {}; // `ObjectExpression` is pure here |
Co-authored-by: Alexander Akait <4567934+alexander-akait@users.noreply.github.com>
2ff3663 to
1e36c86
Compare
| ./objects.js X bytes [built] [code generated] | ||
| chunk (runtime: a) no-used-exports-a.js (a) X bytes (javascript) X KiB (runtime) [entry] [rendered] | ||
| runtime modules X KiB 4 modules | ||
| runtime modules X KiB 5 modules |
There was a problem hiding this comment.
What is interesting, need investigate why we have more modules now...
There was a problem hiding this comment.
It's expected, because we now add a PureExpressionDependency for unused top-level symbols which is one object or array, and these dependencies include runtime conditions. For multiple entries/runtimes, we rely on RuntimeGlobals.runtimeId(the newly added) to determine whether the current chunk's runtime actually uses the symbol.
See:
webpack/lib/RuntimeTemplate.js
Line 876 in a98fd62
…w on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Convert the descent into an iterative DFS using an explicit frame stack. Same semantics as the previous recursive form: per-module `_isEvaluatingSideEffects` reentrancy guard, bailout recording on the parent module for the dep that hit a side-effecting import, aggregation via `addConnectionStates`, circular contributions skipped (not folded) — so tree-shaking results are unchanged.
…ow on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Move the algorithm into a generator (`_evaluateSideEffectsConnectionState`) that `yield`s a child generator for each side-effect-free `HarmonyImportSideEffectDependency` instead of recursing. `getSideEffectsConnectionState` drives it via an explicit stack — a 12-line trampoline. The generator body is the same shape as the previous recursive code so semantics and tree-shaking results are unchanged.
…ow on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Move the algorithm into a file-scope generator `walkSideEffects` that `yield`s a child generator for each side-effect-free HarmonyImportSideEffectDependency instead of recursing into `getSideEffectsConnectionState`. The method itself is now a small trampoline driving the generator stack. The generator body is the same shape as the previous recursive code, so semantics and tree-shaking results are unchanged. Tests: - Unit tests in `test/NormalModule.unittest.js` cover a 20000-deep chain, a cycle, and bailout propagation. - Integration case `test/configCases/side-effects/deep-import-chain` mirrors the structure of the reporter's reproduction repo (https://github.com/abecirovic-mo/webpack-5.107.0-repro) with a 500-module chain that fits within the per-case compile budget.
…ow on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Move the algorithm into a file-scope generator `walkSideEffects` that `yield`s a child generator for each side-effect-free HarmonyImportSideEffectDependency instead of recursing into `getSideEffectsConnectionState`. The method itself is now a small trampoline driving the generator stack. The generator body is the same shape as the previous recursive code, so semantics and tree-shaking results are unchanged. Tests: - Unit tests in `test/NormalModule.unittest.js` cover a 20000-deep chain, a cycle, and bailout propagation. - Integration case `test/configCases/side-effects/deep-import-chain` mirrors the structure of the reporter's reproduction repo (https://github.com/abecirovic-mo/webpack-5.107.0-repro) with a 500-module chain that fits within the per-case compile budget.
…ow on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Move the algorithm into a file-scope generator `walkSideEffects` that `yield`s a child generator for each side-effect-free HarmonyImportSideEffectDependency instead of recursing into `getSideEffectsConnectionState`. The method itself is now a small trampoline driving the generator stack. The generator body is the same shape as the previous recursive code, so semantics and tree-shaking results are unchanged. Tests: - Unit tests in `test/NormalModule.unittest.js` cover a 20000-deep chain, a cycle, and bailout propagation. - Integration case `test/configCases/side-effects/deep-import-chain` mirrors the structure of the reporter's reproduction repo (https://github.com/abecirovic-mo/webpack-5.107.0-repro) with a 500-module chain that fits within the per-case compile budget.
Summary
What kind of change does this PR introduce?
Add support for
ArrayExpression,ObjectExpressionandNewExpressionin theisPuremethod for better tree-shaking. And also fix #20209.Did you add tests for your changes?
Yes
Does this PR introduce a breaking change?
No
If relevant, what needs to be documented once your changes are merged or what have you already documented?
No
Use of AI
Partial