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

Make accessing unknown globals a side-effect #3068

Merged
merged 6 commits into from Sep 8, 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
25 changes: 24 additions & 1 deletion docs/999-big-list-of-options.md
Expand Up @@ -802,7 +802,7 @@ Default: `false`
If this option is provided, bundling will not fail if bindings are imported from a file that does not define these bindings. Instead, new variables will be created for these bindings with the value `undefined`.

#### treeshake
Type: `boolean | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, propertyReadSideEffects?: boolean }`<br>
Type: `boolean | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, propertyReadSideEffects?: boolean, tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`<br>
CLI: `--treeshake`/`--no-treeshake`<br>
Default: `true`

Expand Down Expand Up @@ -938,6 +938,29 @@ test(otherFn);

```

**treeshake.unknownGlobalSideEffects**
Type: `boolean`<br>
CLI: `--treeshake.unknownGlobalSideEffects`/`--no-treeshake.unknownGlobalSideEffects`<br>
Default: `true`

Since accessing a non-existing global variable will throw an error, Rollup does by default retain any accesses to non-builtin global variables. Set this option to `false` to avoid this check. This is probably safe for most code-bases.

```js
// input
const jQuery = $;
const requestTimeout = setTimeout;
const element = angular.element;

// output with unknownGlobalSideEffects == true
const jQuery = $;
const element = angular.element;

// output with unknownGlobalSideEffects == false
const element = angular.element;
```

In the example, the last line is always retained as accessing the `element` property could also throw an error if `angular` is e.g. `null`. To avoid this check, set `treeshake.propertyReadSideEffects` to `false` as well.

### Experimental options

These options reflect new features that have not yet been fully finalized. Availability, behaviour and usage may therefore be subject to change between minor versions.
Expand Down
7 changes: 5 additions & 2 deletions src/Graph.ts
Expand Up @@ -116,13 +116,16 @@ export default class Graph {
(options.treeshake as TreeshakingOptions).propertyReadSideEffects !== false,
pureExternalModules: (options.treeshake as TreeshakingOptions).pureExternalModules,
tryCatchDeoptimization:
(options.treeshake as TreeshakingOptions).tryCatchDeoptimization !== false
(options.treeshake as TreeshakingOptions).tryCatchDeoptimization !== false,
unknownGlobalSideEffects:
(options.treeshake as TreeshakingOptions).unknownGlobalSideEffects !== false
}
: {
annotations: true,
moduleSideEffects: true,
propertyReadSideEffects: true,
tryCatchDeoptimization: true
tryCatchDeoptimization: true,
unknownGlobalSideEffects: true
};
if (typeof this.treeshakingOptions.pureExternalModules !== 'undefined') {
this.warnDeprecation(
Expand Down
3 changes: 3 additions & 0 deletions src/Module.ts
Expand Up @@ -110,6 +110,7 @@ export interface AstContext {
traceVariable: (name: string) => Variable | null;
treeshake: boolean;
tryCatchDeoptimization: boolean;
unknownGlobalSideEffects: boolean;
usesTopLevelAwait: boolean;
warn: (warning: RollupWarning, pos: number) => void;
warnDeprecation: (deprecation: string | RollupWarning, activeDeprecation: boolean) => void;
Expand Down Expand Up @@ -606,6 +607,8 @@ export default class Module {
treeshake: !!this.graph.treeshakingOptions,
tryCatchDeoptimization: (!this.graph.treeshakingOptions ||
this.graph.treeshakingOptions.tryCatchDeoptimization) as boolean,
unknownGlobalSideEffects: (!this.graph.treeshakingOptions ||
this.graph.treeshakingOptions.unknownGlobalSideEffects) as boolean,
usesTopLevelAwait: false,
warn: this.warn.bind(this),
warnDeprecation: this.graph.warnDeprecation.bind(this.graph)
Expand Down
45 changes: 23 additions & 22 deletions src/ast/nodes/Identifier.ts
Expand Up @@ -7,7 +7,8 @@ import { DeoptimizableEntity } from '../DeoptimizableEntity';
import { ExecutionPathOptions } from '../ExecutionPathOptions';
import FunctionScope from '../scopes/FunctionScope';
import { ImmutableEntityPathTracker } from '../utils/ImmutableEntityPathTracker';
import { LiteralValueOrUnknown, ObjectPath, UNKNOWN_EXPRESSION, UNKNOWN_VALUE } from '../values';
import { EMPTY_PATH, LiteralValueOrUnknown, ObjectPath } from '../values';
import GlobalVariable from '../variables/GlobalVariable';
import LocalVariable from '../variables/LocalVariable';
import Variable from '../variables/Variable';
import * as NodeType from './NodeType';
Expand Down Expand Up @@ -62,24 +63,20 @@ export default class Identifier extends NodeBase implements PatternNode {
case 'parameter':
variable = (this.scope as FunctionScope).addParameterDeclaration(this);
break;
/* istanbul ignore next */
default:
throw new Error(`Unexpected identifier kind ${kind}.`);
/* istanbul ignore next */
throw new Error(`Internal Error: Unexpected identifier kind ${kind}.`);
}
return [(this.variable = variable)];
}

deoptimizePath(path: ObjectPath) {
if (!this.bound) this.bind();
if (this.variable !== null) {
if (
path.length === 0 &&
this.name in this.context.importDescriptions &&
!this.scope.contains(this.name)
) {
this.disallowImportReassignment();
}
this.variable.deoptimizePath(path);
if (path.length === 0 && !this.scope.contains(this.name)) {
this.disallowImportReassignment();
}
(this.variable as Variable).deoptimizePath(path);
}

getLiteralValueAtPath(
Expand All @@ -88,10 +85,7 @@ export default class Identifier extends NodeBase implements PatternNode {
origin: DeoptimizableEntity
): LiteralValueOrUnknown {
if (!this.bound) this.bind();
if (this.variable !== null) {
return this.variable.getLiteralValueAtPath(path, recursionTracker, origin);
}
return UNKNOWN_VALUE;
return (this.variable as Variable).getLiteralValueAtPath(path, recursionTracker, origin);
}

getReturnExpressionWhenCalledAtPath(
Expand All @@ -100,10 +94,19 @@ export default class Identifier extends NodeBase implements PatternNode {
origin: DeoptimizableEntity
) {
if (!this.bound) this.bind();
if (this.variable !== null) {
return this.variable.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin);
}
return UNKNOWN_EXPRESSION;
return (this.variable as Variable).getReturnExpressionWhenCalledAtPath(
path,
recursionTracker,
origin
);
}

hasEffects(): boolean {
return (
this.context.unknownGlobalSideEffects &&
this.variable instanceof GlobalVariable &&
this.variable.hasEffectsWhenAccessedAtPath(EMPTY_PATH)
);
}

hasEffectsWhenAccessedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {
Expand Down Expand Up @@ -132,9 +135,7 @@ export default class Identifier extends NodeBase implements PatternNode {
}

includeCallArguments(args: (ExpressionNode | SpreadElement)[]): void {
if (this.variable) {
this.variable.includeCallArguments(args);
}
(this.variable as Variable).includeCallArguments(args);
}

render(
Expand Down
4 changes: 2 additions & 2 deletions src/ast/nodes/shared/FunctionNode.ts
Expand Up @@ -41,8 +41,8 @@ export default class FunctionNode extends NodeBase {
return path.length === 0 ? this.scope.getReturnExpression() : UNKNOWN_EXPRESSION;
}

hasEffects(options: ExecutionPathOptions) {
return this.id !== null && this.id.hasEffects(options);
hasEffects() {
return this.id !== null && this.id.hasEffects();
}

hasEffectsWhenAccessedAtPath(path: ObjectPath) {
Expand Down