Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deoptimize try-catch less radically #2918

Merged
merged 3 commits into from Jun 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 14 additions & 24 deletions docs/999-big-list-of-options.md
Expand Up @@ -855,25 +855,12 @@ Type: `boolean`<br>
CLI: `--treeshake.tryCatchDeoptimization`/`--no-treeshake.tryCatchDeoptimization`<br>
Default: `true`

By default, Rollup assumes that many builtin globals of the runtime behave according to the latest specs when tree-shaking and do not throw unexpected errors. In order to support e.g. feature detection workflows that rely on those errors being thrown, Rollup will by default deactivate tree-shaking inside try-statements. Furthermore, it will also deactivate tree-shaking inside functions that are called directly from a try-statement if Rollup can resolve the function. Set `treeshake.tryCatchDeoptimization` to `false` if you do not need this feature and want to have tree-shaking inside try-statements as well as inside functions called from those statements.
By default, Rollup assumes that many builtin globals of the runtime behave according to the latest specs when tree-shaking and do not throw unexpected errors. In order to support e.g. feature detection workflows that rely on those errors being thrown, Rollup will by default deactivate tree-shaking inside try-statements. If a function parameter is called from within a try-statement, this parameter will be deoptimized as well. Set `treeshake.tryCatchDeoptimization` to `false` if you do not need this feature and want to have tree-shaking inside try-statements.

```js
function directlyCalled1() {
// as this function is directly called from a try-statement, it will be
// retained unmodified for tryCatchDeoptimization: true including staements
// that would usually be removed
Object.create(null);
notDirectlyCalled();
}

function directlyCalled2() {
Object.create(null);
notDirectlyCalled();
}

function notDirectlyCalled() {
// even if this function is retained, this will be removed as the function is
// never directly called from a try-statement
function otherFn() {
// even though this function is called from a try-statement, the next line
// will be removed as side-effect-free
Object.create(null);
}

Expand All @@ -883,20 +870,23 @@ function test(callback) {
// inside try-statements for tryCatchDeoptimization: true
Object.create(null);

// directly resolvable calls will also be deoptimized
directlyCalled1();
// calls to other function are retained as well but the body of this
// function may again be subject to tree-shaking
otherFn();

// if a parameter is called, then all arguments passed to that function
// parameter will be deoptimized
callback();

// all calls will be retained but only calls of the form
// "identifier(someArguments)" will also deoptimize the target
(notDirectlyCalled && notDirectlyCalled)();
} catch {}
}

test(directlyCalled2);
test(() => {
// will be ratained
Object.create(null)
});

// call will be retained but again, otherFn is not deoptimized
test(otherFn);

```

Expand Down
18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
Expand Up @@ -76,7 +76,10 @@
"buble": "^0.19.7",
"chokidar": "^2.1.6",
"console-group": "^0.3.3",
"core-js": "^3.1.3",
"date-time": "^3.1.0",
"es5-shim": "^4.5.13",
"es6-shim": "^0.35.5",
"eslint": "^5.16.0",
"eslint-plugin-import": "^2.17.3",
"execa": "^1.0.0",
Expand Down
6 changes: 3 additions & 3 deletions src/ast/nodes/CallExpression.ts
Expand Up @@ -23,7 +23,7 @@ import {
import Identifier from './Identifier';
import * as NodeType from './NodeType';
import { ExpressionEntity } from './shared/Expression';
import { ExpressionNode, INCLUDE_VARIABLES, IncludeChildren, NodeBase } from './shared/Node';
import { ExpressionNode, INCLUDE_PARAMETERS, IncludeChildren, NodeBase } from './shared/Node';
import SpreadElement from './SpreadElement';

export default class CallExpression extends NodeBase implements DeoptimizableEntity {
Expand Down Expand Up @@ -204,11 +204,11 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
if (includeChildrenRecursively) {
super.include(includeChildrenRecursively);
if (
includeChildrenRecursively === INCLUDE_VARIABLES &&
includeChildrenRecursively === INCLUDE_PARAMETERS &&
this.callee instanceof Identifier &&
this.callee.variable
) {
this.callee.variable.includeInitRecursively();
this.callee.variable.markCalledFromTryStatement();
}
} else {
this.included = true;
Expand Down
4 changes: 2 additions & 2 deletions src/ast/nodes/TryStatement.ts
Expand Up @@ -2,7 +2,7 @@ import { ExecutionPathOptions } from '../ExecutionPathOptions';
import BlockStatement from './BlockStatement';
import CatchClause from './CatchClause';
import * as NodeType from './NodeType';
import { INCLUDE_VARIABLES, IncludeChildren, StatementBase } from './shared/Node';
import { INCLUDE_PARAMETERS, IncludeChildren, StatementBase } from './shared/Node';

export default class TryStatement extends StatementBase {
block!: BlockStatement;
Expand All @@ -25,7 +25,7 @@ export default class TryStatement extends StatementBase {
this.included = true;
this.directlyIncluded = true;
this.block.include(
this.context.tryCatchDeoptimization ? INCLUDE_VARIABLES : includeChildrenRecursively
this.context.tryCatchDeoptimization ? INCLUDE_PARAMETERS : includeChildrenRecursively
);
}
if (this.handler !== null) {
Expand Down
4 changes: 2 additions & 2 deletions src/ast/nodes/shared/Node.ts
Expand Up @@ -20,8 +20,8 @@ export interface GenericEsTreeNode {
[key: string]: any;
}

export const INCLUDE_VARIABLES: 'variables' = 'variables';
export type IncludeChildren = boolean | typeof INCLUDE_VARIABLES;
export const INCLUDE_PARAMETERS: 'variables' = 'variables';
export type IncludeChildren = boolean | typeof INCLUDE_PARAMETERS;

export interface Node extends Entity {
annotations?: CommentDescription[];
Expand Down
13 changes: 5 additions & 8 deletions src/ast/scopes/ParameterScope.ts
Expand Up @@ -42,30 +42,27 @@ export default class ParameterScope extends ChildScope {
}

includeCallArguments(args: (ExpressionNode | SpreadElement)[]): void {
let hasInitBeenForceIncluded = false;
let calledFromTryStatement = false;
let argIncluded = false;
const restParam = this.hasRest && this.parameters[this.parameters.length - 1];
for (let index = args.length - 1; index >= 0; index--) {
const paramVars = this.parameters[index] || restParam;
const arg = args[index];
if (paramVars) {
hasInitBeenForceIncluded = false;
calledFromTryStatement = false;
for (const variable of paramVars) {
if (variable.included) {
argIncluded = true;
}
if (variable.hasInitBeenForceIncluded) {
hasInitBeenForceIncluded = true;
if (variable.calledFromTryStatement) {
calledFromTryStatement = true;
}
}
} else if (!argIncluded && arg.shouldBeIncluded()) {
argIncluded = true;
}
if (argIncluded) {
arg.include(hasInitBeenForceIncluded);
if (hasInitBeenForceIncluded && arg instanceof Identifier && arg.variable) {
arg.variable.includeInitRecursively();
}
arg.include(calledFromTryStatement);
}
}
}
Expand Down
11 changes: 3 additions & 8 deletions src/ast/variables/LocalVariable.ts
Expand Up @@ -25,8 +25,8 @@ const MAX_PATH_DEPTH = 7;

export default class LocalVariable extends Variable {
additionalInitializers: ExpressionEntity[] | null = null;
calledFromTryStatement = false;
declarations: (Identifier | ExportDefaultDeclaration)[];
hasInitBeenForceIncluded = false;
init: ExpressionEntity | null;
module: Module;

Expand Down Expand Up @@ -203,12 +203,7 @@ export default class LocalVariable extends Variable {
}
}

includeInitRecursively() {
if (!this.hasInitBeenForceIncluded) {
this.hasInitBeenForceIncluded = true;
if (this.init && this.init !== UNKNOWN_EXPRESSION) {
this.init.include(true);
}
}
markCalledFromTryStatement() {
this.calledFromTryStatement = true;
}
}
2 changes: 1 addition & 1 deletion src/ast/variables/Variable.ts
Expand Up @@ -91,7 +91,7 @@ export default class Variable implements ExpressionEntity {
}
}

includeInitRecursively() {}
markCalledFromTryStatement() {}

setRenderNames(baseName: string | null, name: string | null) {
this.renderBaseName = baseName;
Expand Down
8 changes: 8 additions & 0 deletions test/form/samples/supports-core-js/_config.js
@@ -0,0 +1,8 @@
module.exports = {
description: 'supports core-js',
options: {
// check against tree-shake: false when updating the polyfill
treeshake: true,
plugins: [require('rollup-plugin-node-resolve')(), require('rollup-plugin-commonjs')()]
}
};