Skip to content

Commit

Permalink
Roll back parameter default tree shaking (rollup#4520)
Browse files Browse the repository at this point in the history
* Add more regression tests

* Remove call parameter deoptimization

* Remove includeWithoutParameterDefaults

* Remove default parameter tree-shaking logic

* Clean up deoptimizations

* Clean up deoptimizations
  • Loading branch information
lukastaegert authored and pos777 committed Jun 18, 2022
1 parent 9caeb31 commit 090d1c0
Show file tree
Hide file tree
Showing 46 changed files with 112 additions and 533 deletions.
4 changes: 0 additions & 4 deletions src/ast/nodes/ArrayExpression.ts
Expand Up @@ -85,11 +85,7 @@ export default class ArrayExpression extends NodeBase {
if (element) {
if (hasSpread || element instanceof SpreadElement) {
hasSpread = true;
// This also deoptimizes parameter defaults
element.deoptimizePath(UNKNOWN_PATH);
} else {
// We do not track parameter defaults in arrays
element.deoptimizeCallParameters();
}
}
}
Expand Down
14 changes: 12 additions & 2 deletions src/ast/nodes/ArrowFunctionExpression.ts
@@ -1,12 +1,13 @@
import { type CallOptions } from '../CallOptions';
import { type HasEffectsContext } from '../ExecutionContext';
import { type HasEffectsContext, InclusionContext } from '../ExecutionContext';
import ReturnValueScope from '../scopes/ReturnValueScope';
import type Scope from '../scopes/Scope';
import { type ObjectPath } from '../utils/PathTracker';
import BlockStatement from './BlockStatement';
import Identifier from './Identifier';
import * as NodeType from './NodeType';
import FunctionBase from './shared/FunctionBase';
import { type ExpressionNode } from './shared/Node';
import { type ExpressionNode, IncludeChildren } from './shared/Node';
import { ObjectEntity } from './shared/ObjectEntity';
import { OBJECT_PROTOTYPE } from './shared/ObjectPrototype';
import type { PatternNode } from './shared/Pattern';
Expand Down Expand Up @@ -48,6 +49,15 @@ export default class ArrowFunctionExpression extends FunctionBase {
return false;
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
super.include(context, includeChildrenRecursively);
for (const param of this.params) {
if (!(param instanceof Identifier)) {
param.include(context, includeChildrenRecursively);
}
}
}

protected getObjectEntity(): ObjectEntity {
if (this.objectEntity !== null) {
return this.objectEntity;
Expand Down
4 changes: 1 addition & 3 deletions src/ast/nodes/AssignmentPattern.ts
Expand Up @@ -35,10 +35,8 @@ export default class AssignmentPattern extends NodeBase implements PatternNode {
return path.length > 0 || this.left.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context);
}

// Note that FunctionBase may directly include .left and .right without
// including the pattern itself. This is how default parameter tree-shaking
// works at the moment.
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
if (!this.deoptimized) this.applyDeoptimizations();
this.included = true;
this.left.include(context, includeChildrenRecursively);
this.right.include(context, includeChildrenRecursively);
Expand Down
2 changes: 1 addition & 1 deletion src/ast/nodes/CallExpression.ts
Expand Up @@ -95,7 +95,7 @@ export default class CallExpression extends CallExpressionBase implements Deopti
}
} else {
this.included = true;
this.callee.include(context, false, { includeWithoutParameterDefaults: true });
this.callee.include(context, false);
}
this.callee.includeCallArguments(context, this.arguments);
const returnExpression = this.getReturnExpression();
Expand Down
7 changes: 1 addition & 6 deletions src/ast/nodes/FunctionDeclaration.ts
@@ -1,17 +1,12 @@
import { InclusionContext } from '../ExecutionContext';
import type ChildScope from '../scopes/ChildScope';
import Identifier, { type IdentifierWithVariable } from './Identifier';
import type * as NodeType from './NodeType';
import FunctionNode from './shared/FunctionNode';
import type { GenericEsTreeNode, IncludeChildren } from './shared/Node';
import type { GenericEsTreeNode } from './shared/Node';

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

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
super.include(context, includeChildrenRecursively, { includeWithoutParameterDefaults: true });
}

initialise(): void {
super.initialise();
if (this.id !== null) {
Expand Down
4 changes: 0 additions & 4 deletions src/ast/nodes/Identifier.ts
Expand Up @@ -86,10 +86,6 @@ export default class Identifier extends NodeBase implements PatternNode {
return [(this.variable = variable)];
}

deoptimizeCallParameters() {
this.variable!.deoptimizeCallParameters();
}

deoptimizePath(path: ObjectPath): void {
if (path.length === 0 && !this.scope.contains(this.name)) {
this.disallowImportReassignment();
Expand Down
9 changes: 1 addition & 8 deletions src/ast/nodes/ObjectExpression.ts
Expand Up @@ -102,14 +102,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
}
}

protected applyDeoptimizations() {
this.deoptimized = true;
for (const property of this.properties) {
if (property instanceof Property) {
property.value.deoptimizeCallParameters();
}
}
}
protected applyDeoptimizations() {}

private getObjectEntity(): ObjectEntity {
if (this.objectEntity !== null) {
Expand Down
4 changes: 1 addition & 3 deletions src/ast/nodes/VariableDeclarator.ts
Expand Up @@ -35,9 +35,7 @@ export default class VariableDeclarator extends NodeBase {

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
this.included = true;
this.init?.include(context, includeChildrenRecursively, {
includeWithoutParameterDefaults: true
});
this.init?.include(context, includeChildrenRecursively);
this.id.markDeclarationReached();
if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) {
this.id.include(context, includeChildrenRecursively);
Expand Down
2 changes: 1 addition & 1 deletion src/ast/nodes/YieldExpression.ts
Expand Up @@ -11,7 +11,7 @@ export default class YieldExpression extends NodeBase {

hasEffects(context: HasEffectsContext): boolean {
if (!this.deoptimized) this.applyDeoptimizations();
return !context.ignore.returnYield || !!this.argument?.hasEffects(context);
return !(context.ignore.returnYield && !this.argument?.hasEffects(context));
}

render(code: MagicString, options: RenderOptions): void {
Expand Down
10 changes: 0 additions & 10 deletions src/ast/nodes/shared/ClassNode.ts
Expand Up @@ -16,7 +16,6 @@ import type ClassBody from '../ClassBody';
import Identifier from '../Identifier';
import type Literal from '../Literal';
import MethodDefinition from '../MethodDefinition';
import PropertyDefinition from '../PropertyDefinition';
import { type ExpressionEntity, type LiteralValueOrUnknown } from './Expression';
import { type ExpressionNode, type IncludeChildren, NodeBase } from './Node';
import { ObjectEntity, type ObjectProperty } from './ObjectEntity';
Expand All @@ -40,12 +39,6 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {

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

deoptimizeThisOnEventAtPath(
Expand Down Expand Up @@ -150,11 +143,8 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
) {
// Calls to methods are not tracked, ensure that the return value is deoptimized
definition.deoptimizePath(UNKNOWN_PATH);
} else if (definition instanceof PropertyDefinition) {
definition.value?.deoptimizeCallParameters();
}
}
this.superClass?.deoptimizeCallParameters();
this.context.requestTreeshakingPass();
}

Expand Down
3 changes: 0 additions & 3 deletions src/ast/nodes/shared/Expression.ts
Expand Up @@ -18,14 +18,11 @@ export interface InclusionOptions {
* Include the id of a declarator even if unused to ensure it is a valid statement.
*/
asSingleStatement?: boolean;
includeWithoutParameterDefaults?: boolean;
}

export class ExpressionEntity implements WritableEntity {
included = false;

deoptimizeCallParameters(): void {}

deoptimizePath(_path: ObjectPath): void {}

deoptimizeThisOnEventAtPath(
Expand Down
109 changes: 8 additions & 101 deletions src/ast/nodes/shared/FunctionBase.ts
@@ -1,5 +1,4 @@
import type { NormalizedTreeshakingOptions } from '../../../rollup/types';
import { BLANK } from '../../../utils/blank';
import { type CallOptions, NO_ARGS } from '../../CallOptions';
import { DeoptimizableEntity } from '../../DeoptimizableEntity';
import {
Expand All @@ -9,27 +8,12 @@ import {
} from '../../ExecutionContext';
import { NodeEvent } from '../../NodeEvents';
import ReturnValueScope from '../../scopes/ReturnValueScope';
import {
EMPTY_PATH,
type ObjectPath,
PathTracker,
SHARED_RECURSION_TRACKER,
UNKNOWN_PATH,
UnknownKey
} from '../../utils/PathTracker';
import LocalVariable from '../../variables/LocalVariable';
import AssignmentPattern from '../AssignmentPattern';
import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker';
import BlockStatement from '../BlockStatement';
import * as NodeType from '../NodeType';
import RestElement from '../RestElement';
import type SpreadElement from '../SpreadElement';
import {
type ExpressionEntity,
InclusionOptions,
LiteralValueOrUnknown,
UNKNOWN_EXPRESSION,
UnknownValue
} from './Expression';
import { type ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './Expression';
import {
type ExpressionNode,
type GenericEsTreeNode,
Expand All @@ -39,31 +23,20 @@ import {
import { ObjectEntity } from './ObjectEntity';
import type { PatternNode } from './Pattern';

export default abstract class FunctionBase extends NodeBase implements DeoptimizableEntity {
export default abstract class FunctionBase extends NodeBase {
declare async: boolean;
declare body: BlockStatement | ExpressionNode;
declare params: readonly PatternNode[];
declare preventChildBlockScope: true;
declare scope: ReturnValueScope;
protected objectEntity: ObjectEntity | null = null;
private deoptimizedReturn = false;
private forceIncludeParameters = false;
private declare parameterVariables: LocalVariable[][];

deoptimizeCache() {
this.forceIncludeParameters = true;
}

deoptimizeCallParameters() {
this.forceIncludeParameters = true;
}

deoptimizePath(path: ObjectPath): void {
this.getObjectEntity().deoptimizePath(path);
if (path.length === 1 && path[0] === UnknownKey) {
// A reassignment of UNKNOWN_PATH is considered equivalent to having lost track
// which means the return expression needs to be reassigned
this.forceIncludeParameters = true;
this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH);
}
}
Expand Down Expand Up @@ -150,89 +123,31 @@ export default abstract class FunctionBase extends NodeBase implements Deoptimiz
return true;
}
}
for (let position = 0; position < this.params.length; position++) {
const parameter = this.params[position];
if (parameter instanceof AssignmentPattern) {
if (parameter.left.hasEffects(context)) {
return true;
}
const argumentValue = callOptions.args[position]?.getLiteralValueAtPath(
EMPTY_PATH,
SHARED_RECURSION_TRACKER,
this
);
if (
(argumentValue === undefined || argumentValue === UnknownValue) &&
parameter.right.hasEffects(context)
) {
return true;
}
} else if (parameter.hasEffects(context)) {
return true;
}
for (const param of this.params) {
if (param.hasEffects(context)) return true;
}
return false;
}

include(
context: InclusionContext,
includeChildrenRecursively: IncludeChildren,
{ includeWithoutParameterDefaults }: InclusionOptions = BLANK
): void {
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
if (!this.deoptimized) this.applyDeoptimizations();
this.included = true;
const { brokenFlow } = context;
context.brokenFlow = BROKEN_FLOW_NONE;
this.body.include(context, includeChildrenRecursively);
context.brokenFlow = brokenFlow;
if (
!includeWithoutParameterDefaults ||
includeChildrenRecursively ||
this.forceIncludeParameters
) {
for (const param of this.params) {
param.include(context, includeChildrenRecursively);
}
}
}

includeCallArguments(
context: InclusionContext,
args: readonly (ExpressionEntity | SpreadElement)[]
): void {
for (let position = 0; position < this.params.length; position++) {
const parameter = this.params[position];
if (parameter instanceof AssignmentPattern) {
if (parameter.left.shouldBeIncluded(context)) {
parameter.left.include(context, false);
}
const argumentValue = args[position]?.getLiteralValueAtPath(
EMPTY_PATH,
SHARED_RECURSION_TRACKER,
this
);
// If argumentValue === UnknownTruthyValue, then we do not need to
// include the default
if (
(argumentValue === undefined || argumentValue === UnknownValue) &&
(this.parameterVariables[position].some(variable => variable.included) ||
parameter.right.shouldBeIncluded(context))
) {
parameter.right.include(context, false);
}
} else if (parameter.shouldBeIncluded(context)) {
parameter.include(context, false);
}
}
this.scope.includeCallArguments(context, args);
}

initialise(): void {
this.parameterVariables = this.params.map(param =>
param.declare('parameter', UNKNOWN_EXPRESSION)
);
this.scope.addParameterVariables(
this.parameterVariables,
this.params.map(param => param.declare('parameter', UNKNOWN_EXPRESSION)),
this.params[this.params.length - 1] instanceof RestElement
);
if (this.body instanceof BlockStatement) {
Expand All @@ -249,15 +164,7 @@ export default abstract class FunctionBase extends NodeBase implements Deoptimiz
super.parseNode(esTreeNode);
}

protected applyDeoptimizations() {
// We currently do not track deoptimizations of default values, deoptimize them
// just as we deoptimize call arguments
for (const param of this.params) {
if (param instanceof AssignmentPattern) {
param.right.deoptimizePath(UNKNOWN_PATH);
}
}
}
protected applyDeoptimizations() {}

protected abstract getObjectEntity(): ObjectEntity;
}
Expand Down
22 changes: 10 additions & 12 deletions src/ast/nodes/shared/FunctionNode.ts
@@ -1,12 +1,11 @@
import { BLANK } from '../../../utils/blank';
import { type CallOptions } from '../../CallOptions';
import { type HasEffectsContext, type InclusionContext } from '../../ExecutionContext';
import { EVENT_CALLED, type NodeEvent } from '../../NodeEvents';
import FunctionScope from '../../scopes/FunctionScope';
import { type ObjectPath, PathTracker } from '../../utils/PathTracker';
import BlockStatement from '../BlockStatement';
import { type IdentifierWithVariable } from '../Identifier';
import { type ExpressionEntity, InclusionOptions, UNKNOWN_EXPRESSION } from './Expression';
import Identifier, { type IdentifierWithVariable } from '../Identifier';
import { type ExpressionEntity, UNKNOWN_EXPRESSION } from './Expression';
import FunctionBase from './FunctionBase';
import { type IncludeChildren } from './Node';
import { ObjectEntity } from './ObjectEntity';
Expand Down Expand Up @@ -74,16 +73,15 @@ export default class FunctionNode extends FunctionBase {
return false;
}

include(
context: InclusionContext,
includeChildrenRecursively: IncludeChildren,
{ includeWithoutParameterDefaults }: InclusionOptions = BLANK
): void {
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
super.include(context, includeChildrenRecursively);
this.id?.include();
super.include(context, includeChildrenRecursively, {
includeWithoutParameterDefaults:
includeWithoutParameterDefaults && !this.scope.argumentsVariable.included
});
const hasArguments = this.scope.argumentsVariable.included;
for (const param of this.params) {
if (!(param instanceof Identifier) || hasArguments) {
param.include(context, includeChildrenRecursively);
}
}
}

initialise(): void {
Expand Down

0 comments on commit 090d1c0

Please sign in to comment.