Skip to content

Commit 8eb4eb6

Browse files
aryanraj45aryanraj45
authored
fix(javascript): scope a parameter list apart from its body in the minifier (#22439)
* fix(javascript): scope a parameter list apart from its body in the minifier A name a parameter default or catch pattern reads resolved to what the body or block declares, as terser scopes it, so the mangler renamed the read onto the inner binding and the compressor dropped the outer one as unused. Such a read now skips the body's bindings. Moves 42 test262 cases off the printer's list; minifying a 1 MB bundle is within noise. * fix(javascript): clear parameter ranges once an analysis resolves An analysis of a copied function counts its reads from zero but met the ranges an earlier analysis left on the scopes around it, so a read in the body could skip a binding there: P(a=q+q+q+q+q) copying h printed "out" for "in". Each analysis now clears its ranges after resolving. Adds the kinds the sweep reached: computed keys, writes, typeof, rest, constructors, setters, generators, const and class.
1 parent 5214180 commit 8eb4eb6

5 files changed

Lines changed: 250 additions & 17 deletions

File tree

‎.changeset/103-printer-parameter-scope.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"webpack": patch
3+
---
4+
5+
Keep a name a parameter list or catch parameter reads from resolving to what the function body or catch block declares, which the minifier renamed or dropped as unused.

‎lib/javascript/ScopeAnalyzer.js‎

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,6 +1228,10 @@ class MinifierScope extends Scope {
12281228
this.usesWith = false;
12291229
/** @type {MinifierVariable | null} a function expression's own name */
12301230
this.functionName = null;
1231+
/** @type {number} the first name its parameter list reads, as the analysis counts them, or -1 */
1232+
this.parameterStart = -1;
1233+
/** @type {number} the name after the last its parameter list reads, or -1 */
1234+
this.parameterEnd = -1;
12311235
}
12321236

12331237
/**
@@ -1666,6 +1670,7 @@ const createCompressorScope = (type, node, upper, isVarScope) =>
16661670
* @property {boolean=} ie8 whether catch parameters belong to their function
16671671
* @property {boolean=} safari10 whether a loop's scope reads its parent's names, and catch parameters belong to their function
16681672
* @property {EXPECTED_ANY=} keep_fnames which function names survive, collecting the declarators naming one
1673+
* @property {boolean=} parameterScopes whether a name read in a parameter list or a catch parameter skips what the body declares, as the spec scopes it
16691674
*/
16701675

16711676
/**
@@ -1699,28 +1704,52 @@ const isExport = (node) =>
16991704
node.type === "ExportDefaultDeclaration" ||
17001705
node.type === "ExportAllDeclaration";
17011706

1707+
/**
1708+
* @param {MinifierScope} scope the scope declaring it
1709+
* @param {MinifierVariable} variable a definition
1710+
* @param {number} read the read's place in the analysis's count
1711+
* @returns {boolean} whether the read sits in the scope's parameter list and the definition in its body, which the list cannot see
1712+
*/
1713+
const isHiddenFromParameters = (scope, variable, read) =>
1714+
read < scope.parameterEnd &&
1715+
read >= scope.parameterStart &&
1716+
variable.kind !== DECLARES_FUNARG &&
1717+
variable.kind !== DECLARES_LAMBDA &&
1718+
variable.kind !== DECLARES_CATCH;
1719+
17021720
/**
17031721
* @param {MinifierScope} from the scope a name is read in
17041722
* @param {string} name the name
1723+
* @param {number=} read the read's place in the analysis's count, under `parameterScopes`
17051724
* @returns {MinifierVariable | undefined} what it resolves to from there
17061725
*/
1707-
const findMinifierVariable = (from, name) => {
1726+
const findMinifierVariable = (from, name, read = -1) => {
17081727
for (let scope = /** @type {Scope | null} */ (from); scope !== null;) {
17091728
// `getBinding`, inlined: the interpreter pays a call per scope climbed
17101729
const index = scope._index;
1730+
/** @type {MinifierVariable | undefined} */
1731+
let variable;
17111732
if (index !== undefined) {
1712-
const variable = index.get(name);
1713-
if (variable !== undefined) {
1714-
return /** @type {MinifierVariable} */ (variable);
1715-
}
1733+
variable = /** @type {MinifierVariable | undefined} */ (index.get(name));
17161734
} else {
17171735
const variables = scope.variables;
17181736
for (let i = 0; i < variables.length; i++) {
17191737
if (variables[i].name === name) {
1720-
return /** @type {MinifierVariable} */ (variables[i]);
1738+
variable = /** @type {MinifierVariable} */ (variables[i]);
1739+
break;
17211740
}
17221741
}
17231742
}
1743+
if (
1744+
variable !== undefined &&
1745+
!isHiddenFromParameters(
1746+
/** @type {MinifierScope} */ (scope),
1747+
variable,
1748+
read
1749+
)
1750+
) {
1751+
return variable;
1752+
}
17241753
scope = scope.upper;
17251754
}
17261755
return undefined;
@@ -1763,6 +1792,12 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
17631792
this.legacyCatch = Boolean(options.ie8 || options.safari10);
17641793
/** @type {boolean} */
17651794
this.safari10 = Boolean(options.safari10);
1795+
/** @type {boolean} */
1796+
this.parameterScopes = Boolean(options.parameterScopes);
1797+
// Only this analysis's reads are counted against a range, so each is
1798+
// cleared once they resolve, before an analysis of a copy counts anew.
1799+
/** @type {MinifierScope[]} the scopes holding a range of parameter reads */
1800+
this.parameterRanges = [];
17661801
/** @type {MinifierScope} */
17671802
this.toplevel = new MinifierScope("global", program, null, true);
17681803
this.scope = this.toplevel;
@@ -2129,9 +2164,15 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
21292164
lambda.functionName = /** @type {MinifierVariable} */ (id.definition);
21302165
}
21312166
const { params } = node;
2167+
const ranged = this.parameterScopes && params.length !== 0;
2168+
if (ranged) {
2169+
lambda.parameterStart = this.referenceNodes.length;
2170+
this.parameterRanges.push(lambda);
2171+
}
21322172
for (let i = 0; i < params.length; i++) {
21332173
this._visitBinding(params[i], DECLARES_FUNARG);
21342174
}
2175+
if (ranged) lambda.parameterEnd = this.referenceNodes.length;
21352176
if (this.release !== undefined) this.release(node.body);
21362177
this._visitList(node.body.body);
21372178
this.scope = outer;
@@ -2391,7 +2432,14 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
23912432
case "CatchClause": {
23922433
stack.push(current);
23932434
const outer = this._enterBlock("catch", current);
2435+
const catchScope = /** @type {MinifierScope} */ (this.scope);
2436+
const ranged = this.parameterScopes && Boolean(current.param);
2437+
if (ranged) {
2438+
catchScope.parameterStart = this.referenceNodes.length;
2439+
this.parameterRanges.push(catchScope);
2440+
}
23942441
if (current.param) this._visitBinding(current.param, DECLARES_CATCH);
2442+
if (ranged) catchScope.parameterEnd = this.referenceNodes.length;
23952443
if (this.release !== undefined) this.release(current.body);
23962444
this._visitList(current.body.body);
23972445
this.scope = outer;
@@ -2569,6 +2617,19 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
25692617
}
25702618
}
25712619

2620+
/**
2621+
* Drops the ranges of parameter reads this analysis counted.
2622+
* @returns {void}
2623+
*/
2624+
_clearParameterRanges() {
2625+
const { parameterRanges } = this;
2626+
for (let i = 0; i < parameterRanges.length; i++) {
2627+
parameterRanges[i].parameterStart = -1;
2628+
parameterRanges[i].parameterEnd = -1;
2629+
}
2630+
parameterRanges.length = 0;
2631+
}
2632+
25722633
/**
25732634
* Resolves each name read, finds `eval`, and encloses what a catch
25742635
* parameter redefines, then moves the catch parameters old engines scope
@@ -2604,7 +2665,7 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
26042665
let variable =
26052666
flags & REFERENCE_FROM_MODULE
26062667
? undefined
2607-
: findMinifierVariable(from, name);
2668+
: findMinifierVariable(from, name, i);
26082669
if (variable === undefined) {
26092670
variable = globals.get(name);
26102671
if (variable === undefined) {
@@ -2624,6 +2685,8 @@ class MinifierScopeAnalyzer extends ScopeAnalyzer {
26242685
}
26252686
}
26262687

2688+
this._clearParameterRanges();
2689+
26272690
// Passes 3 and 4: work around old engines' catch and loop scopes.
26282691
const { catchNames, catchScopes } = this;
26292692
for (let i = 0; i < catchNames.length; i++) {
@@ -2738,7 +2801,12 @@ class CompressorScopeAnalyzer extends MinifierScopeAnalyzer {
27382801
constructor(root, options, raise, dialect, parentScope, toplevel) {
27392802
super(
27402803
/** @type {PrintProgram} */ (/** @type {unknown} */ (root)),
2741-
{ ie8: options.ie8, safari10: options.safari10, module: options.module },
2804+
{
2805+
ie8: options.ie8,
2806+
safari10: options.safari10,
2807+
module: options.module,
2808+
parameterScopes: options.parameterScopes
2809+
},
27422810
raise,
27432811
undefined
27442812
);
@@ -3098,7 +3166,7 @@ class CompressorScopeAnalyzer extends MinifierScopeAnalyzer {
30983166
/** @type {unknown} */ (
30993167
flags & REFERENCE_FROM_MODULE
31003168
? undefined
3101-
: findMinifierVariable(from, name)
3169+
: findMinifierVariable(from, name, i)
31023170
)
31033171
);
31043172
if (definition === undefined) {
@@ -3135,6 +3203,8 @@ class CompressorScopeAnalyzer extends MinifierScopeAnalyzer {
31353203
}
31363204
}
31373205

3206+
this._clearParameterRanges();
3207+
31383208
// Passes 3 and 4: work around old engines' catch and loop scopes.
31393209
const { catchNames, catchScopes } = this;
31403210
for (let i = 0; i < catchNames.length; i++) {

‎lib/javascript/syntax-printer.js‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4175,9 +4175,13 @@ const installScope = (modules) => {
41754175
if (!isToplevelNode(toplevel)) {
41764176
throw new Error("Invalid toplevel scope");
41774177
}
4178+
const { corrections } = modules;
41784179
analyzeCompressorScope(
41794180
self,
4180-
options,
4181+
// The correct phase scopes a parameter list apart from its body.
4182+
corrections !== undefined && corrections.enabled
4183+
? { ...options, parameterScopes: true }
4184+
: options,
41814185
raise,
41824186
dialect,
41834187
parentScope || null,
@@ -30107,13 +30111,18 @@ const installEstreeMangle = (modules) => {
3010730111
* @param {boolean=} releases whether nobody keeps the tree, so each node read drops what only the compressor read
3010830112
* @returns {MinifierScopeAnalysis} the scopes
3010930113
*/
30110-
const analyzeManglingScopes = (program, options, releases) =>
30111-
analyzeMinifierScope(
30114+
const analyzeManglingScopes = (program, options, releases) => {
30115+
const { corrections } = modules;
30116+
return analyzeMinifierScope(
3011230117
program,
30113-
options,
30118+
// The correct phase scopes a parameter list apart from its body.
30119+
corrections !== undefined && corrections.enabled
30120+
? { ...options, parameterScopes: true }
30121+
: options,
3011430122
raise,
3011530123
releases === true ? releaseCompressorFields : undefined
3011630124
);
30125+
};
3011730126
modules.analyzeManglingScopes = analyzeManglingScopes;
3011830127
modules.isUnmangleable = isUnmangleable;
3011930128

‎test/specCases/test262.spectest.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1409,7 +1409,7 @@ const MINIFIED_FAILURES = [
14091409
{
14101410
reason:
14111411
"terser writes a program that reads a binding outside its scope",
1412-
minifiers: ["terser", "printer"],
1412+
minifiers: ["terser"],
14131413
tests: [
14141414
"expressions/arrow-function/scope-paramsbody-var-open.js (sloppy)",
14151415
"expressions/arrow-function/scope-paramsbody-var-open.js (strict)",

0 commit comments

Comments
 (0)