Skip to content

Commit 96cf84e

Browse files
lukastaegertlukastaegert
authored
Roll back parameter default tree shaking (#4520)
* Add more regression tests * Remove call parameter deoptimization * Remove includeWithoutParameterDefaults * Remove default parameter tree-shaking logic * Clean up deoptimizations * Clean up deoptimizations
1 parent 696ebd3 commit 96cf84e

File tree

46 files changed

+112
-533
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

46 files changed

+112
-533
lines changed

‎src/ast/nodes/ArrayExpression.ts

-4
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,7 @@ export default class ArrayExpression extends NodeBase {
8585
if (element) {
8686
if (hasSpread || element instanceof SpreadElement) {
8787
hasSpread = true;
88-
// This also deoptimizes parameter defaults
8988
element.deoptimizePath(UNKNOWN_PATH);
90-
} else {
91-
// We do not track parameter defaults in arrays
92-
element.deoptimizeCallParameters();
9389
}
9490
}
9591
}

‎src/ast/nodes/ArrowFunctionExpression.ts

+12-2
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import { type CallOptions } from '../CallOptions';
2-
import { type HasEffectsContext } from '../ExecutionContext';
2+
import { type HasEffectsContext, InclusionContext } from '../ExecutionContext';
33
import ReturnValueScope from '../scopes/ReturnValueScope';
44
import type Scope from '../scopes/Scope';
55
import { type ObjectPath } from '../utils/PathTracker';
66
import BlockStatement from './BlockStatement';
7+
import Identifier from './Identifier';
78
import * as NodeType from './NodeType';
89
import FunctionBase from './shared/FunctionBase';
9-
import { type ExpressionNode } from './shared/Node';
10+
import { type ExpressionNode, IncludeChildren } from './shared/Node';
1011
import { ObjectEntity } from './shared/ObjectEntity';
1112
import { OBJECT_PROTOTYPE } from './shared/ObjectPrototype';
1213
import type { PatternNode } from './shared/Pattern';
@@ -48,6 +49,15 @@ export default class ArrowFunctionExpression extends FunctionBase {
4849
return false;
4950
}
5051

52+
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
53+
super.include(context, includeChildrenRecursively);
54+
for (const param of this.params) {
55+
if (!(param instanceof Identifier)) {
56+
param.include(context, includeChildrenRecursively);
57+
}
58+
}
59+
}
60+
5161
protected getObjectEntity(): ObjectEntity {
5262
if (this.objectEntity !== null) {
5363
return this.objectEntity;

‎src/ast/nodes/AssignmentPattern.ts

+1-3
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,8 @@ export default class AssignmentPattern extends NodeBase implements PatternNode {
3535
return path.length > 0 || this.left.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context);
3636
}
3737

38-
// Note that FunctionBase may directly include .left and .right without
39-
// including the pattern itself. This is how default parameter tree-shaking
40-
// works at the moment.
4138
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
39+
if (!this.deoptimized) this.applyDeoptimizations();
4240
this.included = true;
4341
this.left.include(context, includeChildrenRecursively);
4442
this.right.include(context, includeChildrenRecursively);

‎src/ast/nodes/CallExpression.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ export default class CallExpression extends CallExpressionBase implements Deopti
9595
}
9696
} else {
9797
this.included = true;
98-
this.callee.include(context, false, { includeWithoutParameterDefaults: true });
98+
this.callee.include(context, false);
9999
}
100100
this.callee.includeCallArguments(context, this.arguments);
101101
const returnExpression = this.getReturnExpression();

‎src/ast/nodes/FunctionDeclaration.ts

+1-6
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,12 @@
1-
import { InclusionContext } from '../ExecutionContext';
21
import type ChildScope from '../scopes/ChildScope';
32
import Identifier, { type IdentifierWithVariable } from './Identifier';
43
import type * as NodeType from './NodeType';
54
import FunctionNode from './shared/FunctionNode';
6-
import type { GenericEsTreeNode, IncludeChildren } from './shared/Node';
5+
import type { GenericEsTreeNode } from './shared/Node';
76

87
export default class FunctionDeclaration extends FunctionNode {
98
declare type: NodeType.tFunctionDeclaration;
109

11-
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
12-
super.include(context, includeChildrenRecursively, { includeWithoutParameterDefaults: true });
13-
}
14-
1510
initialise(): void {
1611
super.initialise();
1712
if (this.id !== null) {

‎src/ast/nodes/Identifier.ts

-4
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,6 @@ export default class Identifier extends NodeBase implements PatternNode {
8686
return [(this.variable = variable)];
8787
}
8888

89-
deoptimizeCallParameters() {
90-
this.variable!.deoptimizeCallParameters();
91-
}
92-
9389
deoptimizePath(path: ObjectPath): void {
9490
if (path.length === 0 && !this.scope.contains(this.name)) {
9591
this.disallowImportReassignment();

‎src/ast/nodes/ObjectExpression.ts

+1-8
Original file line numberDiff line numberDiff line change
@@ -102,14 +102,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
102102
}
103103
}
104104

105-
protected applyDeoptimizations() {
106-
this.deoptimized = true;
107-
for (const property of this.properties) {
108-
if (property instanceof Property) {
109-
property.value.deoptimizeCallParameters();
110-
}
111-
}
112-
}
105+
protected applyDeoptimizations() {}
113106

114107
private getObjectEntity(): ObjectEntity {
115108
if (this.objectEntity !== null) {

‎src/ast/nodes/VariableDeclarator.ts

+1-3
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,7 @@ export default class VariableDeclarator extends NodeBase {
3535

3636
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
3737
this.included = true;
38-
this.init?.include(context, includeChildrenRecursively, {
39-
includeWithoutParameterDefaults: true
40-
});
38+
this.init?.include(context, includeChildrenRecursively);
4139
this.id.markDeclarationReached();
4240
if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) {
4341
this.id.include(context, includeChildrenRecursively);

‎src/ast/nodes/YieldExpression.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export default class YieldExpression extends NodeBase {
1111

1212
hasEffects(context: HasEffectsContext): boolean {
1313
if (!this.deoptimized) this.applyDeoptimizations();
14-
return !context.ignore.returnYield || !!this.argument?.hasEffects(context);
14+
return !(context.ignore.returnYield && !this.argument?.hasEffects(context));
1515
}
1616

1717
render(code: MagicString, options: RenderOptions): void {

‎src/ast/nodes/shared/ClassNode.ts

-10
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import type ClassBody from '../ClassBody';
1616
import Identifier from '../Identifier';
1717
import type Literal from '../Literal';
1818
import MethodDefinition from '../MethodDefinition';
19-
import PropertyDefinition from '../PropertyDefinition';
2019
import { type ExpressionEntity, type LiteralValueOrUnknown } from './Expression';
2120
import { type ExpressionNode, type IncludeChildren, NodeBase } from './Node';
2221
import { ObjectEntity, type ObjectProperty } from './ObjectEntity';
@@ -40,12 +39,6 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
4039

4140
deoptimizePath(path: ObjectPath): void {
4241
this.getObjectEntity().deoptimizePath(path);
43-
if (path.length === 1 && path[0] === UnknownKey) {
44-
// A reassignment of UNKNOWN_PATH is considered equivalent to having lost track
45-
// which means the constructor needs to be reassigned
46-
this.classConstructor?.deoptimizePath(UNKNOWN_PATH);
47-
this.superClass?.deoptimizePath(UNKNOWN_PATH);
48-
}
4942
}
5043

5144
deoptimizeThisOnEventAtPath(
@@ -150,11 +143,8 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
150143
) {
151144
// Calls to methods are not tracked, ensure that the return value is deoptimized
152145
definition.deoptimizePath(UNKNOWN_PATH);
153-
} else if (definition instanceof PropertyDefinition) {
154-
definition.value?.deoptimizeCallParameters();
155146
}
156147
}
157-
this.superClass?.deoptimizeCallParameters();
158148
this.context.requestTreeshakingPass();
159149
}
160150

‎src/ast/nodes/shared/Expression.ts

-3
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,11 @@ export interface InclusionOptions {
1818
* Include the id of a declarator even if unused to ensure it is a valid statement.
1919
*/
2020
asSingleStatement?: boolean;
21-
includeWithoutParameterDefaults?: boolean;
2221
}
2322

2423
export class ExpressionEntity implements WritableEntity {
2524
included = false;
2625

27-
deoptimizeCallParameters(): void {}
28-
2926
deoptimizePath(_path: ObjectPath): void {}
3027

3128
deoptimizeThisOnEventAtPath(

‎src/ast/nodes/shared/FunctionBase.ts

+8-101
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import type { NormalizedTreeshakingOptions } from '../../../rollup/types';
2-
import { BLANK } from '../../../utils/blank';
32
import { type CallOptions, NO_ARGS } from '../../CallOptions';
43
import { DeoptimizableEntity } from '../../DeoptimizableEntity';
54
import {
@@ -9,27 +8,12 @@ import {
98
} from '../../ExecutionContext';
109
import { NodeEvent } from '../../NodeEvents';
1110
import ReturnValueScope from '../../scopes/ReturnValueScope';
12-
import {
13-
EMPTY_PATH,
14-
type ObjectPath,
15-
PathTracker,
16-
SHARED_RECURSION_TRACKER,
17-
UNKNOWN_PATH,
18-
UnknownKey
19-
} from '../../utils/PathTracker';
20-
import LocalVariable from '../../variables/LocalVariable';
21-
import AssignmentPattern from '../AssignmentPattern';
11+
import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker';
2212
import BlockStatement from '../BlockStatement';
2313
import * as NodeType from '../NodeType';
2414
import RestElement from '../RestElement';
2515
import type SpreadElement from '../SpreadElement';
26-
import {
27-
type ExpressionEntity,
28-
InclusionOptions,
29-
LiteralValueOrUnknown,
30-
UNKNOWN_EXPRESSION,
31-
UnknownValue
32-
} from './Expression';
16+
import { type ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './Expression';
3317
import {
3418
type ExpressionNode,
3519
type GenericEsTreeNode,
@@ -39,31 +23,20 @@ import {
3923
import { ObjectEntity } from './ObjectEntity';
4024
import type { PatternNode } from './Pattern';
4125

42-
export default abstract class FunctionBase extends NodeBase implements DeoptimizableEntity {
26+
export default abstract class FunctionBase extends NodeBase {
4327
declare async: boolean;
4428
declare body: BlockStatement | ExpressionNode;
4529
declare params: readonly PatternNode[];
4630
declare preventChildBlockScope: true;
4731
declare scope: ReturnValueScope;
4832
protected objectEntity: ObjectEntity | null = null;
4933
private deoptimizedReturn = false;
50-
private forceIncludeParameters = false;
51-
private declare parameterVariables: LocalVariable[][];
52-
53-
deoptimizeCache() {
54-
this.forceIncludeParameters = true;
55-
}
56-
57-
deoptimizeCallParameters() {
58-
this.forceIncludeParameters = true;
59-
}
6034

6135
deoptimizePath(path: ObjectPath): void {
6236
this.getObjectEntity().deoptimizePath(path);
6337
if (path.length === 1 && path[0] === UnknownKey) {
6438
// A reassignment of UNKNOWN_PATH is considered equivalent to having lost track
6539
// which means the return expression needs to be reassigned
66-
this.forceIncludeParameters = true;
6740
this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH);
6841
}
6942
}
@@ -150,89 +123,31 @@ export default abstract class FunctionBase extends NodeBase implements Deoptimiz
150123
return true;
151124
}
152125
}
153-
for (let position = 0; position < this.params.length; position++) {
154-
const parameter = this.params[position];
155-
if (parameter instanceof AssignmentPattern) {
156-
if (parameter.left.hasEffects(context)) {
157-
return true;
158-
}
159-
const argumentValue = callOptions.args[position]?.getLiteralValueAtPath(
160-
EMPTY_PATH,
161-
SHARED_RECURSION_TRACKER,
162-
this
163-
);
164-
if (
165-
(argumentValue === undefined || argumentValue === UnknownValue) &&
166-
parameter.right.hasEffects(context)
167-
) {
168-
return true;
169-
}
170-
} else if (parameter.hasEffects(context)) {
171-
return true;
172-
}
126+
for (const param of this.params) {
127+
if (param.hasEffects(context)) return true;
173128
}
174129
return false;
175130
}
176131

177-
include(
178-
context: InclusionContext,
179-
includeChildrenRecursively: IncludeChildren,
180-
{ includeWithoutParameterDefaults }: InclusionOptions = BLANK
181-
): void {
132+
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
182133
if (!this.deoptimized) this.applyDeoptimizations();
183134
this.included = true;
184135
const { brokenFlow } = context;
185136
context.brokenFlow = BROKEN_FLOW_NONE;
186137
this.body.include(context, includeChildrenRecursively);
187138
context.brokenFlow = brokenFlow;
188-
if (
189-
!includeWithoutParameterDefaults ||
190-
includeChildrenRecursively ||
191-
this.forceIncludeParameters
192-
) {
193-
for (const param of this.params) {
194-
param.include(context, includeChildrenRecursively);
195-
}
196-
}
197139
}
198140

199141
includeCallArguments(
200142
context: InclusionContext,
201143
args: readonly (ExpressionEntity | SpreadElement)[]
202144
): void {
203-
for (let position = 0; position < this.params.length; position++) {
204-
const parameter = this.params[position];
205-
if (parameter instanceof AssignmentPattern) {
206-
if (parameter.left.shouldBeIncluded(context)) {
207-
parameter.left.include(context, false);
208-
}
209-
const argumentValue = args[position]?.getLiteralValueAtPath(
210-
EMPTY_PATH,
211-
SHARED_RECURSION_TRACKER,
212-
this
213-
);
214-
// If argumentValue === UnknownTruthyValue, then we do not need to
215-
// include the default
216-
if (
217-
(argumentValue === undefined || argumentValue === UnknownValue) &&
218-
(this.parameterVariables[position].some(variable => variable.included) ||
219-
parameter.right.shouldBeIncluded(context))
220-
) {
221-
parameter.right.include(context, false);
222-
}
223-
} else if (parameter.shouldBeIncluded(context)) {
224-
parameter.include(context, false);
225-
}
226-
}
227145
this.scope.includeCallArguments(context, args);
228146
}
229147

230148
initialise(): void {
231-
this.parameterVariables = this.params.map(param =>
232-
param.declare('parameter', UNKNOWN_EXPRESSION)
233-
);
234149
this.scope.addParameterVariables(
235-
this.parameterVariables,
150+
this.params.map(param => param.declare('parameter', UNKNOWN_EXPRESSION)),
236151
this.params[this.params.length - 1] instanceof RestElement
237152
);
238153
if (this.body instanceof BlockStatement) {
@@ -249,15 +164,7 @@ export default abstract class FunctionBase extends NodeBase implements Deoptimiz
249164
super.parseNode(esTreeNode);
250165
}
251166

252-
protected applyDeoptimizations() {
253-
// We currently do not track deoptimizations of default values, deoptimize them
254-
// just as we deoptimize call arguments
255-
for (const param of this.params) {
256-
if (param instanceof AssignmentPattern) {
257-
param.right.deoptimizePath(UNKNOWN_PATH);
258-
}
259-
}
260-
}
167+
protected applyDeoptimizations() {}
261168

262169
protected abstract getObjectEntity(): ObjectEntity;
263170
}

‎src/ast/nodes/shared/FunctionNode.ts

+10-12
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
1-
import { BLANK } from '../../../utils/blank';
21
import { type CallOptions } from '../../CallOptions';
32
import { type HasEffectsContext, type InclusionContext } from '../../ExecutionContext';
43
import { EVENT_CALLED, type NodeEvent } from '../../NodeEvents';
54
import FunctionScope from '../../scopes/FunctionScope';
65
import { type ObjectPath, PathTracker } from '../../utils/PathTracker';
76
import BlockStatement from '../BlockStatement';
8-
import { type IdentifierWithVariable } from '../Identifier';
9-
import { type ExpressionEntity, InclusionOptions, UNKNOWN_EXPRESSION } from './Expression';
7+
import Identifier, { type IdentifierWithVariable } from '../Identifier';
8+
import { type ExpressionEntity, UNKNOWN_EXPRESSION } from './Expression';
109
import FunctionBase from './FunctionBase';
1110
import { type IncludeChildren } from './Node';
1211
import { ObjectEntity } from './ObjectEntity';
@@ -74,16 +73,15 @@ export default class FunctionNode extends FunctionBase {
7473
return false;
7574
}
7675

77-
include(
78-
context: InclusionContext,
79-
includeChildrenRecursively: IncludeChildren,
80-
{ includeWithoutParameterDefaults }: InclusionOptions = BLANK
81-
): void {
76+
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
77+
super.include(context, includeChildrenRecursively);
8278
this.id?.include();
83-
super.include(context, includeChildrenRecursively, {
84-
includeWithoutParameterDefaults:
85-
includeWithoutParameterDefaults && !this.scope.argumentsVariable.included
86-
});
79+
const hasArguments = this.scope.argumentsVariable.included;
80+
for (const param of this.params) {
81+
if (!(param instanceof Identifier) || hasArguments) {
82+
param.include(context, includeChildrenRecursively);
83+
}
84+
}
8785
}
8886

8987
initialise(): void {

0 commit comments

Comments
 (0)