From 3969ee6d0092893cca422433aca068b1b673f170 Mon Sep 17 00:00:00 2001 From: kzc Date: Thu, 24 Jun 2021 13:53:49 -0400 Subject: [PATCH 01/15] Fix var/const/let variable use before declaration * Also retains TDZ violations present in input * Enabled by default when treeshake is active * Low overhead - uses existing treeshaking simulated execution to mark declarations as reached --- src/ast/nodes/ArrayPattern.ts | 3 + src/ast/nodes/Identifier.ts | 79 ++++++++++++++++++- src/ast/nodes/ObjectPattern.ts | 3 + src/ast/nodes/VariableDeclarator.ts | 6 ++ src/ast/variables/Variable.ts | 2 + .../treeshake-preset-override/_expected.js | 3 + .../simplify-return-expression/_expected.js | 46 ++++++++++- .../simplify-return-expression/main.js | 52 +++++++++++- test/form/samples/tdz-common/_config.js | 3 + test/form/samples/tdz-common/_expected.js | 41 ++++++++++ test/form/samples/tdz-common/main.js | 53 +++++++++++++ .../preset-with-override/_expected.js | 3 + .../recommended/_expected.js | 3 + .../treeshake-presets/smallest/_expected.js | 3 + .../treeshake-presets/true/_expected.js | 3 + .../samples/use-var-before-decl/_config.js | 3 + .../samples/use-var-before-decl/main.js | 44 +++++++++++ 17 files changed, 344 insertions(+), 6 deletions(-) create mode 100644 test/form/samples/tdz-common/_config.js create mode 100644 test/form/samples/tdz-common/_expected.js create mode 100644 test/form/samples/tdz-common/main.js create mode 100644 test/function/samples/use-var-before-decl/_config.js create mode 100644 test/function/samples/use-var-before-decl/main.js diff --git a/src/ast/nodes/ArrayPattern.ts b/src/ast/nodes/ArrayPattern.ts index 5d1e440c983..f1f2c283272 100644 --- a/src/ast/nodes/ArrayPattern.ts +++ b/src/ast/nodes/ArrayPattern.ts @@ -29,6 +29,9 @@ export default class ArrayPattern extends NodeBase implements PatternNode { variables.push(...element.declare(kind, UNKNOWN_EXPRESSION)); } } + variables.forEach(v => { + v.initReached = true; + }); return variables; } diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 16aefa22754..839fdb1d5f4 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -15,13 +15,29 @@ import LocalVariable from '../variables/LocalVariable'; import Variable from '../variables/Variable'; import * as NodeType from './NodeType'; import SpreadElement from './SpreadElement'; -import { ExpressionEntity, LiteralValueOrUnknown } from './shared/Expression'; +import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './shared/Expression'; import { ExpressionNode, NodeBase } from './shared/Node'; import { PatternNode } from './shared/Pattern'; export type IdentifierWithVariable = Identifier & { variable: Variable }; +const tdzInitTypesToIgnore = { + __proto__: null, + ArrowFunctionExpression: true, + ClassExpression: true, + FunctionExpression: true, + ObjectExpression: true +}; + +const variableKinds = { + __proto__: null, + const: true, + let: true, + var: true +}; + export default class Identifier extends NodeBase implements PatternNode { + TDZ: boolean | undefined = undefined; name!: string; type!: NodeType.tIdentifier; @@ -72,6 +88,7 @@ export default class Identifier extends NodeBase implements PatternNode { /* istanbul ignore next */ throw new Error(`Internal Error: Unexpected identifier kind ${kind}.`); } + variable.kind = kind; return [(this.variable = variable)]; } @@ -96,6 +113,9 @@ export default class Identifier extends NodeBase implements PatternNode { recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { + if (this.isPossibleTDZ()) { + return UnknownValue; + } return this.variable!.getLiteralValueAtPath(path, recursionTracker, origin); } @@ -115,6 +135,9 @@ export default class Identifier extends NodeBase implements PatternNode { hasEffects(): boolean { if (!this.deoptimized) this.applyDeoptimizations(); + if (this.isPossibleTDZ()) { + return true; + } return ( (this.context.options.treeshake as NormalizedTreeshakingOptions).unknownGlobalSideEffects && this.variable instanceof GlobalVariable && @@ -188,6 +211,60 @@ export default class Identifier extends NodeBase implements PatternNode { } } + protected isPossibleTDZ(): boolean { + // return cached value if present + if (this.TDZ !== undefined) return this.TDZ; + + if ( + !(this.variable instanceof LocalVariable) || + !this.variable.kind || + !(this.variable.kind in variableKinds) + ) { + return (this.TDZ = false); + } + + if ( + this.variable.kind === 'var' && + ((this.parent.type === 'AssignmentExpression' && this === (this.parent as any).left) || + (this.parent.type === 'UpdateExpression' && this === (this.parent as any).argument) || + this.parent.type === 'SequenceExpression' || + this.parent.type === 'ExpressionStatement') + ) { + // If a `var` variable is modified or innocuous + // then pretend the init was reached in these cases + // and have rollup's treeshaking take care of it. + this.variable.initReached = true; + return (this.TDZ = false); + } + + if (!this.variable.initReached) { + // Either a const/let TDZ violation or + // var use before declaration was encountered. + // Retain this variable accesss to preserve the input behavior. + return (this.TDZ = true); + } + + let init, init_parent; + if ( + (init = (this.variable as any).init) && + init !== UNDEFINED_EXPRESSION && + (init_parent = (init as any).parent) && + init_parent.type == 'VariableDeclarator' && + init_parent.id.variable === this.variable && + !(init_parent.init.type in tdzInitTypesToIgnore) && + // code position comparisons must be in the same context + this.context === init_parent.id.context && + this.start >= init.start && + this.start < init.end + ) { + // any scope variable access within its own declaration init: + // let x = x + 1; + return (this.TDZ = true); + } + + return (this.TDZ = false); + } + private disallowImportReassignment() { return this.context.error( { diff --git a/src/ast/nodes/ObjectPattern.ts b/src/ast/nodes/ObjectPattern.ts index 06226a73c95..f40da2f2052 100644 --- a/src/ast/nodes/ObjectPattern.ts +++ b/src/ast/nodes/ObjectPattern.ts @@ -34,6 +34,9 @@ export default class ObjectPattern extends NodeBase implements PatternNode { for (const property of this.properties) { variables.push(...property.declare(kind, init)); } + variables.forEach(v => { + v.initReached = true; + }); return variables; } diff --git a/src/ast/nodes/VariableDeclarator.ts b/src/ast/nodes/VariableDeclarator.ts index b479ea65dda..23698221582 100644 --- a/src/ast/nodes/VariableDeclarator.ts +++ b/src/ast/nodes/VariableDeclarator.ts @@ -28,10 +28,16 @@ export default class VariableDeclarator extends NodeBase { } hasEffects(context: HasEffectsContext): boolean { + if (this.id instanceof Identifier && this.id.variable) { + this.id.variable.initReached = true; + } return this.id.hasEffects(context) || (this.init !== null && this.init.hasEffects(context)); } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { + if (this.id instanceof Identifier && this.id.variable) { + this.id.variable.initReached = true; + } this.included = true; if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) { this.id.include(context, includeChildrenRecursively); diff --git a/src/ast/variables/Variable.ts b/src/ast/variables/Variable.ts index 137d582595d..1ab08e8b5e6 100644 --- a/src/ast/variables/Variable.ts +++ b/src/ast/variables/Variable.ts @@ -8,10 +8,12 @@ import { ObjectPath } from '../utils/PathTracker'; export default class Variable extends ExpressionEntity { alwaysRendered = false; + initReached = false; isId = false; // both NamespaceVariable and ExternalVariable can be namespaces isNamespace?: boolean; isReassigned = false; + kind: string | null = null; module?: Module | ExternalModule; renderBaseName: string | null = null; renderName: string | null = null; diff --git a/test/cli/samples/treeshake-preset-override/_expected.js b/test/cli/samples/treeshake-preset-override/_expected.js index a4958370a2b..01cc5e431f9 100644 --- a/test/cli/samples/treeshake-preset-override/_expected.js +++ b/test/cli/samples/treeshake-preset-override/_expected.js @@ -11,3 +11,6 @@ try { } catch {} unknownGlobal; + +if (!foo) console.log('effect'); +var foo = true; diff --git a/test/form/samples/simplify-return-expression/_expected.js b/test/form/samples/simplify-return-expression/_expected.js index 67a7fcbae5a..ee5dd64f9c9 100644 --- a/test/form/samples/simplify-return-expression/_expected.js +++ b/test/form/samples/simplify-return-expression/_expected.js @@ -4,11 +4,51 @@ const test = () => { }; const foo = () => { - return A ; + // TODO: treeshake optimization regression. + return BUILD ? 'A' : 'B'; }; const bar = () => { - return A ; + // TODO: treeshake optimization regression. + return getBuild() ? 'A' : 'B'; }; -export { test }; +const getBuild = () => BUILD; + +const BUILD = true; + +(function() { + const test = () => { + console.log(foo()); + console.log(bar()); + }; + + const foo = () => { + // optimized + return 'A' ; + }; + + const bar = () => { + // optimized + return 'A' ; + }; + + test(); +})(); + +const test2 = () => { + console.log(foo2()); + console.log(bar2()); +}; + +const foo2 = () => { + // optimized + return 'A' ; +}; + +const bar2 = () => { + // optimized + return 'A' ; +}; + +export { test, test2 }; diff --git a/test/form/samples/simplify-return-expression/main.js b/test/form/samples/simplify-return-expression/main.js index 854407b5114..dfe2a147856 100644 --- a/test/form/samples/simplify-return-expression/main.js +++ b/test/form/samples/simplify-return-expression/main.js @@ -4,13 +4,61 @@ export const test = () => { }; const foo = () => { - return BUILD ? A : B; + // TODO: treeshake optimization regression. + return BUILD ? 'A' : 'B'; }; const bar = () => { - return getBuild() ? A : B; + // TODO: treeshake optimization regression. + return getBuild() ? 'A' : 'B'; }; const getBuild = () => BUILD; const BUILD = true; + +(function() { + const test = () => { + console.log(foo()); + console.log(bar()); + }; + + const foo = () => { + // optimized + return BUILD ? 'A' : 'B'; + }; + + const bar = () => { + // optimized + return getBuild() ? 'A' : 'B'; + }; + + // optimized away + const getBuild = () => BUILD; + + // optimized away + const BUILD = true; + + test(); +})(); + +// optimized away +const BUILD2 = true; + +// optimized away +const getBuild2 = () => BUILD2; + +export const test2 = () => { + console.log(foo2()); + console.log(bar2()); +}; + +const foo2 = () => { + // optimized + return BUILD2 ? 'A' : 'B'; +}; + +const bar2 = () => { + // optimized + return getBuild2() ? 'A' : 'B'; +}; diff --git a/test/form/samples/tdz-common/_config.js b/test/form/samples/tdz-common/_config.js new file mode 100644 index 00000000000..ba41751e0fd --- /dev/null +++ b/test/form/samples/tdz-common/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'preserve common TDZ violations' +}; diff --git a/test/form/samples/tdz-common/_expected.js b/test/form/samples/tdz-common/_expected.js new file mode 100644 index 00000000000..e7d4508ef49 --- /dev/null +++ b/test/form/samples/tdz-common/_expected.js @@ -0,0 +1,41 @@ +console.log(function() { + if (x) return "HELLO"; // TDZ + const x = 1; // keep + return "WORLD"; // not reached +}()); + +const C = 1 + C + 2; // TDZ +let L = L; // TDZ +var V = V; // TODO: uncommon scenario, but should be dropped +console.log("X+" ); // optimize + +console.log(Y ? "Y+" : "Y-"); // TDZ +const Y = 2; // keep + +console.log(Z ? "Z+" : "Z-"); // TDZ +const Z = 3; // keep +console.log("Z+" ); // keep + +console.log(obj.x.y ? 1 : 2); // TDZ +const obj = { // keep + x: { + y: true + } +}; +console.log(3 ); // keep + +L2; // TDZ for L2 +L3 = 20; // TDZ for L3 +let L2, L3; // keep L2, L3 +L3 = 30; // keep + +// Note that typical var/const/let use is still optimized +(function() { + console.log(A ? "A" : "!A"); + var A = 1; + console.log("A" ); + console.log("B"); + console.log("B" ); + console.log("C" ); + console.log("D" ); +})(); diff --git a/test/form/samples/tdz-common/main.js b/test/form/samples/tdz-common/main.js new file mode 100644 index 00000000000..bb85df765ce --- /dev/null +++ b/test/form/samples/tdz-common/main.js @@ -0,0 +1,53 @@ +console.log(function() { + if (x) return "HELLO"; // TDZ + const x = 1; // keep + return "WORLD"; // not reached +}()); + +const unused1 = 1; // drop +let unused2 = 2; // drop +var unused3 = 3; // drop + +const C = 1 + C + 2; // TDZ +let L = L; // TDZ +var V = V; // TODO: uncommon scenario, but should be dropped + +const X = 1; // drop +console.log(X ? "X+" : "X-"); // optimize + +console.log(Y ? "Y+" : "Y-"); // TDZ +const Y = 2; // keep + +console.log(Z ? "Z+" : "Z-"); // TDZ +const Z = 3; // keep +console.log(Z ? "Z+" : "Z-"); // keep + +console.log(obj.x.y ? 1 : 2); // TDZ +const obj = { // keep + x: { + y: true + } +}; +console.log(obj.x.y ? 3 : 4); // keep + +V2, L2; // TDZ for L2 +var V2; // drop +V3 = 10, L3 = 20; // TDZ for L3 +let L1, L2, L3, L4; // keep L2, L3 +var V3; // drop +L3 = 30; // keep +L4 = 40; // drop + +// Note that typical var/const/let use is still optimized +(function() { + console.log(A ? "A" : "!A"); + var A = 1, B = 2; + const C = 3; + let D = 4; + console.log(A ? "A" : "!A"); + if (B) console.log("B"); + else console.log("!B"); + console.log(B ? "B" : "!B"); + console.log(C ? "C" : "!C"); + console.log(D ? "D" : "!D"); +})(); diff --git a/test/form/samples/treeshake-presets/preset-with-override/_expected.js b/test/form/samples/treeshake-presets/preset-with-override/_expected.js index b731633f86b..096ef5b2c73 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/_expected.js +++ b/test/form/samples/treeshake-presets/preset-with-override/_expected.js @@ -1,3 +1,6 @@ console.log('main'); unknownGlobal; + +if (!foo) console.log('effect'); +var foo = true; diff --git a/test/form/samples/treeshake-presets/recommended/_expected.js b/test/form/samples/treeshake-presets/recommended/_expected.js index 65f08abb6b4..ee7d24d369c 100644 --- a/test/form/samples/treeshake-presets/recommended/_expected.js +++ b/test/form/samples/treeshake-presets/recommended/_expected.js @@ -11,3 +11,6 @@ console.log('main'); try { const noeffect = 1; } catch {} + +if (!foo) console.log('effect'); +var foo = true; diff --git a/test/form/samples/treeshake-presets/smallest/_expected.js b/test/form/samples/treeshake-presets/smallest/_expected.js index c0b933d7b56..e0c6fd50d49 100644 --- a/test/form/samples/treeshake-presets/smallest/_expected.js +++ b/test/form/samples/treeshake-presets/smallest/_expected.js @@ -1 +1,4 @@ console.log('main'); + +if (!foo) console.log('effect'); +var foo = true; diff --git a/test/form/samples/treeshake-presets/true/_expected.js b/test/form/samples/treeshake-presets/true/_expected.js index 3df383b997e..a859fbd4964 100644 --- a/test/form/samples/treeshake-presets/true/_expected.js +++ b/test/form/samples/treeshake-presets/true/_expected.js @@ -13,3 +13,6 @@ try { } catch {} unknownGlobal; + +if (!foo) console.log('effect'); +var foo = true; diff --git a/test/function/samples/use-var-before-decl/_config.js b/test/function/samples/use-var-before-decl/_config.js new file mode 100644 index 00000000000..c39aa3c5730 --- /dev/null +++ b/test/function/samples/use-var-before-decl/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'exercise `var` variables before their declarations' +}; diff --git a/test/function/samples/use-var-before-decl/main.js b/test/function/samples/use-var-before-decl/main.js new file mode 100644 index 00000000000..48194b34d1f --- /dev/null +++ b/test/function/samples/use-var-before-decl/main.js @@ -0,0 +1,44 @@ +var results = [], log = x => results.push(x); + +(function () { + var a = "PASS1"; + for (var b = 2; --b >= 0; ) { + (function() { + var c = function() { + return 1; + }(c && (a = "FAIL1")); + })(); + } + log(a); +})(); + +log(a ? "FAIL2" : "PASS2"); +var a = 1; + +var b = 2; +log(b ? "PASS3" : "FAIL3"); + +log(c ? "FAIL4" : "PASS4"); +var c = 3; +log(c ? "PASS5" : "FAIL5"); + +log(function() { + if (x) return "FAIL6"; + var x = 1; + return "PASS6"; +}()); + +(function () { + var first = state(); + var on = true; + var obj = { + state: state + }; + log(first) + log(obj.state()); + function state() { + return on ? "ON" : "OFF"; + } +})(); + +assert.strictEqual(results.join(" "), "PASS1 PASS2 PASS3 PASS4 PASS5 PASS6 OFF ON"); From dcb9e57d82ee84af724fecbb2ef52541bb4f67f2 Mon Sep 17 00:00:00 2001 From: kzc Date: Sat, 26 Jun 2021 13:57:26 -0400 Subject: [PATCH 02/15] successfully run tests from #4149 without treeshake.correctVarBeforeDeclaration --- src/ast/nodes/Identifier.ts | 5 ----- .../samples/correct-var-before-declaration-deopt/_config.js | 6 ++---- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 839fdb1d5f4..d06bc5a9e22 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -62,14 +62,9 @@ export default class Identifier extends NodeBase implements PatternNode { declare(kind: string, init: ExpressionEntity): LocalVariable[] { let variable: LocalVariable; - const { treeshake } = this.context.options; switch (kind) { case 'var': variable = this.scope.addDeclaration(this, this.context, init, true); - if (treeshake && treeshake.correctVarValueBeforeDeclaration) { - // Necessary to make sure the init is deoptimized. We cannot call deoptimizePath here. - this.scope.addDeclaration(this, this.context, UNDEFINED_EXPRESSION, true); - } break; case 'function': // in strict mode, functions are only hoisted within a scope but not across block scopes diff --git a/test/function/samples/correct-var-before-declaration-deopt/_config.js b/test/function/samples/correct-var-before-declaration-deopt/_config.js index 6c091ca84a8..ab14869f51c 100644 --- a/test/function/samples/correct-var-before-declaration-deopt/_config.js +++ b/test/function/samples/correct-var-before-declaration-deopt/_config.js @@ -1,6 +1,4 @@ module.exports = { - description: 'adds necessary deoptimizations when using treeshake.correctVarBeforeDeclaration', - options: { - treeshake: { correctVarValueBeforeDeclaration: true } - } + description: + 'adds necessary deoptimizations without using treeshake.correctVarValueBeforeDeclaration' }; From 3189ebc389b3341f545a0d54c9f81005fd427290 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 29 Jun 2021 07:03:45 +0200 Subject: [PATCH 03/15] Fix pattern handling --- src/ast/nodes/ArrayPattern.ts | 11 ++++++++--- src/ast/nodes/AssignmentPattern.ts | 4 ++++ src/ast/nodes/Identifier.ts | 4 ++++ src/ast/nodes/ObjectPattern.ts | 9 ++++++--- src/ast/nodes/Property.ts | 4 ++++ src/ast/nodes/RestElement.ts | 4 ++++ src/ast/nodes/VariableDeclarator.ts | 17 +++++++---------- src/ast/nodes/shared/Pattern.ts | 1 + test/form/samples/tdz-pattern-access/_config.js | 3 +++ .../samples/tdz-pattern-access/_expected.js | 8 ++++++++ test/form/samples/tdz-pattern-access/main.js | 8 ++++++++ 11 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 test/form/samples/tdz-pattern-access/_config.js create mode 100644 test/form/samples/tdz-pattern-access/_expected.js create mode 100644 test/form/samples/tdz-pattern-access/main.js diff --git a/src/ast/nodes/ArrayPattern.ts b/src/ast/nodes/ArrayPattern.ts index f1f2c283272..8b737f6480c 100644 --- a/src/ast/nodes/ArrayPattern.ts +++ b/src/ast/nodes/ArrayPattern.ts @@ -29,9 +29,6 @@ export default class ArrayPattern extends NodeBase implements PatternNode { variables.push(...element.declare(kind, UNKNOWN_EXPRESSION)); } } - variables.forEach(v => { - v.initReached = true; - }); return variables; } @@ -53,4 +50,12 @@ export default class ArrayPattern extends NodeBase implements PatternNode { } return false; } + + markDeclarationReached(): void { + for (const element of this.elements) { + if (element !== null) { + element.markDeclarationReached(); + } + } + } } diff --git a/src/ast/nodes/AssignmentPattern.ts b/src/ast/nodes/AssignmentPattern.ts index 9386e07f325..22b148f4880 100644 --- a/src/ast/nodes/AssignmentPattern.ts +++ b/src/ast/nodes/AssignmentPattern.ts @@ -35,6 +35,10 @@ export default class AssignmentPattern extends NodeBase implements PatternNode { return path.length > 0 || this.left.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context); } + markDeclarationReached(): void { + this.left.markDeclarationReached(); + } + render( code: MagicString, options: RenderOptions, diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index d06bc5a9e22..1d95a562fc5 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -170,6 +170,10 @@ export default class Identifier extends NodeBase implements PatternNode { this.variable!.includeCallArguments(context, args); } + markDeclarationReached(): void { + this.variable!.initReached = true; + } + render( code: MagicString, _options: RenderOptions, diff --git a/src/ast/nodes/ObjectPattern.ts b/src/ast/nodes/ObjectPattern.ts index f40da2f2052..57d1b1632ed 100644 --- a/src/ast/nodes/ObjectPattern.ts +++ b/src/ast/nodes/ObjectPattern.ts @@ -34,9 +34,6 @@ export default class ObjectPattern extends NodeBase implements PatternNode { for (const property of this.properties) { variables.push(...property.declare(kind, init)); } - variables.forEach(v => { - v.initReached = true; - }); return variables; } @@ -55,4 +52,10 @@ export default class ObjectPattern extends NodeBase implements PatternNode { } return false; } + + markDeclarationReached(): void { + for (const property of this.properties) { + property.markDeclarationReached(); + } + } } diff --git a/src/ast/nodes/Property.ts b/src/ast/nodes/Property.ts index 3f5649669d1..6e2021512ec 100644 --- a/src/ast/nodes/Property.ts +++ b/src/ast/nodes/Property.ts @@ -35,6 +35,10 @@ export default class Property extends MethodBase implements PatternNode { ); } + markDeclarationReached(): void { + (this.value as PatternNode).markDeclarationReached(); + } + render(code: MagicString, options: RenderOptions): void { if (!this.shorthand) { this.key.render(code, options); diff --git a/src/ast/nodes/RestElement.ts b/src/ast/nodes/RestElement.ts index de3fb0dc7b0..77653bde676 100644 --- a/src/ast/nodes/RestElement.ts +++ b/src/ast/nodes/RestElement.ts @@ -33,6 +33,10 @@ export default class RestElement extends NodeBase implements PatternNode { return path.length > 0 || this.argument.hasEffectsWhenAssignedAtPath(EMPTY_PATH, context); } + markDeclarationReached(): void { + this.argument.markDeclarationReached(); + } + protected applyDeoptimizations(): void { this.deoptimized = true; if (this.declarationInit !== null) { diff --git a/src/ast/nodes/VariableDeclarator.ts b/src/ast/nodes/VariableDeclarator.ts index 23698221582..1f3f1338703 100644 --- a/src/ast/nodes/VariableDeclarator.ts +++ b/src/ast/nodes/VariableDeclarator.ts @@ -28,23 +28,20 @@ export default class VariableDeclarator extends NodeBase { } hasEffects(context: HasEffectsContext): boolean { - if (this.id instanceof Identifier && this.id.variable) { - this.id.variable.initReached = true; - } - return this.id.hasEffects(context) || (this.init !== null && this.init.hasEffects(context)); + const initEffect = this.init !== null && this.init.hasEffects(context); + this.id.markDeclarationReached(); + return initEffect || this.id.hasEffects(context); } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { - if (this.id instanceof Identifier && this.id.variable) { - this.id.variable.initReached = true; - } this.included = true; - if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) { - this.id.include(context, includeChildrenRecursively); - } if (this.init) { this.init.include(context, includeChildrenRecursively); } + this.id.markDeclarationReached(); + if (includeChildrenRecursively || this.id.shouldBeIncluded(context)) { + this.id.include(context, includeChildrenRecursively); + } } render(code: MagicString, options: RenderOptions): void { diff --git a/src/ast/nodes/shared/Pattern.ts b/src/ast/nodes/shared/Pattern.ts index ccca00c9e01..a9e146a5dae 100644 --- a/src/ast/nodes/shared/Pattern.ts +++ b/src/ast/nodes/shared/Pattern.ts @@ -5,4 +5,5 @@ import { Node } from './Node'; export interface PatternNode extends WritableEntity, Node { declare(kind: string, init: ExpressionEntity | null): LocalVariable[]; + markDeclarationReached(): void; } diff --git a/test/form/samples/tdz-pattern-access/_config.js b/test/form/samples/tdz-pattern-access/_config.js new file mode 100644 index 00000000000..a50522b9fd9 --- /dev/null +++ b/test/form/samples/tdz-pattern-access/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'handles accessing variables declared in patterns before their declaration' +}; diff --git a/test/form/samples/tdz-pattern-access/_expected.js b/test/form/samples/tdz-pattern-access/_expected.js new file mode 100644 index 00000000000..44e35fc1981 --- /dev/null +++ b/test/form/samples/tdz-pattern-access/_expected.js @@ -0,0 +1,8 @@ +x + y; +const { x } = {}; +const [y] = []; + +if (z) console.log('unimportant'); +else console.log('retained'); + +var { z } = { z: true }; diff --git a/test/form/samples/tdz-pattern-access/main.js b/test/form/samples/tdz-pattern-access/main.js new file mode 100644 index 00000000000..847dbb3bc06 --- /dev/null +++ b/test/form/samples/tdz-pattern-access/main.js @@ -0,0 +1,8 @@ +const tdzAccess = x + y; +const { x } = {}; +const [y] = []; + +if (z) console.log('unimportant'); +else console.log('retained'); + +var { z } = { z: true }; From 6ce4ededad38f0314efe514eaa22c193fdddcb64 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 30 Jun 2021 06:34:00 +0200 Subject: [PATCH 04/15] Handle TDZ class access --- src/ast/nodes/Identifier.ts | 5 +++-- src/ast/nodes/shared/ClassNode.ts | 18 +++++++++++++----- test/form/samples/tdz-common/_expected.js | 3 +++ test/form/samples/tdz-common/main.js | 4 ++++ 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 1d95a562fc5..896127a9cfa 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -29,8 +29,9 @@ const tdzInitTypesToIgnore = { ObjectExpression: true }; -const variableKinds = { +const tdzVariableKinds = { __proto__: null, + class: true, const: true, let: true, var: true @@ -217,7 +218,7 @@ export default class Identifier extends NodeBase implements PatternNode { if ( !(this.variable instanceof LocalVariable) || !this.variable.kind || - !(this.variable.kind in variableKinds) + !(this.variable.kind in tdzVariableKinds) ) { return (this.TDZ = false); } diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index c0b05401d61..1c704f0a9c9 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -1,6 +1,6 @@ import { CallOptions } from '../../CallOptions'; import { DeoptimizableEntity } from '../../DeoptimizableEntity'; -import { HasEffectsContext } from '../../ExecutionContext'; +import { HasEffectsContext, InclusionContext } from '../../ExecutionContext'; import { NodeEvent } from '../../NodeEvents'; import ChildScope from '../../scopes/ChildScope'; import Scope from '../../scopes/Scope'; @@ -16,7 +16,7 @@ import Identifier from '../Identifier'; import Literal from '../Literal'; import MethodDefinition from '../MethodDefinition'; import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './Expression'; -import { ExpressionNode, NodeBase } from './Node'; +import { ExpressionNode, IncludeChildren, NodeBase } from './Node'; import { ObjectEntity, ObjectProperty } from './ObjectEntity'; import { ObjectMember } from './ObjectMember'; import { OBJECT_PROTOTYPE } from './ObjectPrototype'; @@ -76,6 +76,11 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { ); } + hasEffects(context: HasEffectsContext): boolean { + this.id?.markDeclarationReached(); + return super.hasEffects(context); + } + hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } @@ -102,10 +107,13 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { } } + include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { + this.id?.markDeclarationReached(); + super.include(context, includeChildrenRecursively); + } + initialise(): void { - if (this.id !== null) { - this.id.declare('class', this); - } + this.id?.declare('class', this); for (const method of this.body.body) { if (method instanceof MethodDefinition && method.kind === 'constructor') { this.classConstructor = method; diff --git a/test/form/samples/tdz-common/_expected.js b/test/form/samples/tdz-common/_expected.js index e7d4508ef49..17f3da3d7e5 100644 --- a/test/form/samples/tdz-common/_expected.js +++ b/test/form/samples/tdz-common/_expected.js @@ -29,6 +29,9 @@ L3 = 20; // TDZ for L3 let L2, L3; // keep L2, L3 L3 = 30; // keep +cls; // TDZ +class cls {} + // Note that typical var/const/let use is still optimized (function() { console.log(A ? "A" : "!A"); diff --git a/test/form/samples/tdz-common/main.js b/test/form/samples/tdz-common/main.js index bb85df765ce..2e1b7e957a4 100644 --- a/test/form/samples/tdz-common/main.js +++ b/test/form/samples/tdz-common/main.js @@ -7,6 +7,7 @@ console.log(function() { const unused1 = 1; // drop let unused2 = 2; // drop var unused3 = 3; // drop +class unused4 {} // drop const C = 1 + C + 2; // TDZ let L = L; // TDZ @@ -38,6 +39,9 @@ var V3; // drop L3 = 30; // keep L4 = 40; // drop +cls; // TDZ +class cls {} + // Note that typical var/const/let use is still optimized (function() { console.log(A ? "A" : "!A"); From 35e0f6ea909061e427d5129fad5d5f2bc5e14079 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 30 Jun 2021 06:41:14 +0200 Subject: [PATCH 05/15] Improve field name --- src/ast/nodes/Identifier.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 896127a9cfa..c6b828e541a 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -38,12 +38,11 @@ const tdzVariableKinds = { }; export default class Identifier extends NodeBase implements PatternNode { - TDZ: boolean | undefined = undefined; name!: string; type!: NodeType.tIdentifier; - variable: Variable | null = null; protected deoptimized = false; + private isTDZAccess = false; addExportedVariables( variables: Variable[], @@ -213,14 +212,14 @@ export default class Identifier extends NodeBase implements PatternNode { protected isPossibleTDZ(): boolean { // return cached value if present - if (this.TDZ !== undefined) return this.TDZ; + if (this.isTDZAccess !== undefined) return this.isTDZAccess; if ( !(this.variable instanceof LocalVariable) || !this.variable.kind || !(this.variable.kind in tdzVariableKinds) ) { - return (this.TDZ = false); + return (this.isTDZAccess = false); } if ( @@ -234,14 +233,14 @@ export default class Identifier extends NodeBase implements PatternNode { // then pretend the init was reached in these cases // and have rollup's treeshaking take care of it. this.variable.initReached = true; - return (this.TDZ = false); + return (this.isTDZAccess = false); } if (!this.variable.initReached) { // Either a const/let TDZ violation or // var use before declaration was encountered. // Retain this variable accesss to preserve the input behavior. - return (this.TDZ = true); + return (this.isTDZAccess = true); } let init, init_parent; @@ -259,10 +258,10 @@ export default class Identifier extends NodeBase implements PatternNode { ) { // any scope variable access within its own declaration init: // let x = x + 1; - return (this.TDZ = true); + return (this.isTDZAccess = true); } - return (this.TDZ = false); + return (this.isTDZAccess = false); } private disallowImportReassignment() { From b4b2c5b40037a6aef3b1778e8277f0ba51b48bfc Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Thu, 1 Jul 2021 06:26:39 +0200 Subject: [PATCH 06/15] Fix TDZ caching --- src/ast/nodes/Identifier.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index c6b828e541a..a20e8c4f3f4 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -42,7 +42,7 @@ export default class Identifier extends NodeBase implements PatternNode { type!: NodeType.tIdentifier; variable: Variable | null = null; protected deoptimized = false; - private isTDZAccess = false; + private isTDZAccess: boolean | null = null; addExportedVariables( variables: Variable[], @@ -212,7 +212,7 @@ export default class Identifier extends NodeBase implements PatternNode { protected isPossibleTDZ(): boolean { // return cached value if present - if (this.isTDZAccess !== undefined) return this.isTDZAccess; + if (this.isTDZAccess !== null) return this.isTDZAccess; if ( !(this.variable instanceof LocalVariable) || From 7cbecfb3a5e95f6e51c4c1d573c504cc5859f77b Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 2 Jul 2021 06:17:04 +0200 Subject: [PATCH 07/15] Only include entry point exports after first treeshaking pass --- src/Graph.ts | 16 +++++++++++----- src/Module.ts | 2 +- .../simplify-return-expression/_expected.js | 10 ++-------- .../samples/simplify-return-expression/main.js | 2 -- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/Graph.ts b/src/Graph.ts index a1643bb42e3..67b423bf724 100644 --- a/src/Graph.ts +++ b/src/Graph.ts @@ -183,11 +183,7 @@ export default class Graph { private includeStatements() { for (const module of [...this.entryModules, ...this.implicitEntryModules]) { - if (module.preserveSignature !== false) { - module.includeAllExports(false); - } else { - markModuleAndImpureDependenciesAsExecuted(module); - } + markModuleAndImpureDependenciesAsExecuted(module); } if (this.options.treeshake) { let treeshakingPass = 1; @@ -203,6 +199,16 @@ export default class Graph { } } } + if (treeshakingPass === 1) { + // We only include exports after the first pass to avoid issues with + // the TDZ detection logic + for (const module of [...this.entryModules, ...this.implicitEntryModules]) { + if (module.preserveSignature !== false) { + module.includeAllExports(false); + this.needsTreeshakingPass = true; + } + } + } timeEnd(`treeshaking pass ${treeshakingPass++}`, 3); } while (this.needsTreeshakingPass); } else { diff --git a/src/Module.ts b/src/Module.ts index 531872eb121..3cd879cc055 100644 --- a/src/Module.ts +++ b/src/Module.ts @@ -587,8 +587,8 @@ export default class Module { includeAllExports(includeNamespaceMembers: boolean): void { if (!this.isExecuted) { - this.graph.needsTreeshakingPass = true; markModuleAndImpureDependenciesAsExecuted(this); + this.graph.needsTreeshakingPass = true; } for (const exportName of this.getExports()) { diff --git a/test/form/samples/simplify-return-expression/_expected.js b/test/form/samples/simplify-return-expression/_expected.js index ee5dd64f9c9..e8a6b185d81 100644 --- a/test/form/samples/simplify-return-expression/_expected.js +++ b/test/form/samples/simplify-return-expression/_expected.js @@ -4,19 +4,13 @@ const test = () => { }; const foo = () => { - // TODO: treeshake optimization regression. - return BUILD ? 'A' : 'B'; + return 'A' ; }; const bar = () => { - // TODO: treeshake optimization regression. - return getBuild() ? 'A' : 'B'; + return 'A' ; }; -const getBuild = () => BUILD; - -const BUILD = true; - (function() { const test = () => { console.log(foo()); diff --git a/test/form/samples/simplify-return-expression/main.js b/test/form/samples/simplify-return-expression/main.js index dfe2a147856..796651eacad 100644 --- a/test/form/samples/simplify-return-expression/main.js +++ b/test/form/samples/simplify-return-expression/main.js @@ -4,12 +4,10 @@ export const test = () => { }; const foo = () => { - // TODO: treeshake optimization regression. return BUILD ? 'A' : 'B'; }; const bar = () => { - // TODO: treeshake optimization regression. return getBuild() ? 'A' : 'B'; }; From b4494df643053774ef811ed70824ffc21a37480e Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 2 Jul 2021 06:37:14 +0200 Subject: [PATCH 08/15] No longer make TDZ var access a side effect but treat as unknown value --- src/ast/nodes/Identifier.ts | 56 +++++++++++-------- .../recursive-multi-expressions/_expected.js | 2 + .../recursive-multi-expressions/main.js | 4 +- test/form/samples/tdz-common/_expected.js | 1 - test/form/samples/tdz-common/main.js | 2 +- 5 files changed, 39 insertions(+), 26 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index a20e8c4f3f4..323243d38bf 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -15,7 +15,7 @@ import LocalVariable from '../variables/LocalVariable'; import Variable from '../variables/Variable'; import * as NodeType from './NodeType'; import SpreadElement from './SpreadElement'; -import { ExpressionEntity, LiteralValueOrUnknown, UnknownValue } from './shared/Expression'; +import { ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './shared/Expression'; import { ExpressionNode, NodeBase } from './shared/Node'; import { PatternNode } from './shared/Pattern'; @@ -108,10 +108,7 @@ export default class Identifier extends NodeBase implements PatternNode { recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { - if (this.isPossibleTDZ()) { - return UnknownValue; - } - return this.variable!.getLiteralValueAtPath(path, recursionTracker, origin); + return this.getVariableRespectingTDZ().getLiteralValueAtPath(path, recursionTracker, origin); } getReturnExpressionWhenCalledAtPath( @@ -120,7 +117,7 @@ export default class Identifier extends NodeBase implements PatternNode { recursionTracker: PathTracker, origin: DeoptimizableEntity ): ExpressionEntity { - return this.variable!.getReturnExpressionWhenCalledAtPath( + return this.getVariableRespectingTDZ().getReturnExpressionWhenCalledAtPath( path, callOptions, recursionTracker, @@ -130,7 +127,7 @@ export default class Identifier extends NodeBase implements PatternNode { hasEffects(): boolean { if (!this.deoptimized) this.applyDeoptimizations(); - if (this.isPossibleTDZ()) { + if (this.isPossibleTDZ() && this.variable!.kind !== 'var') { return true; } return ( @@ -141,11 +138,16 @@ export default class Identifier extends NodeBase implements PatternNode { } hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.variable !== null && this.variable.hasEffectsWhenAccessedAtPath(path, context); + return ( + this.variable !== null && + this.getVariableRespectingTDZ().hasEffectsWhenAccessedAtPath(path, context) + ); } hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return !this.variable || this.variable.hasEffectsWhenAssignedAtPath(path, context); + return ( + !this.variable || this.getVariableRespectingTDZ().hasEffectsWhenAssignedAtPath(path, context) + ); } hasEffectsWhenCalledAtPath( @@ -153,7 +155,10 @@ export default class Identifier extends NodeBase implements PatternNode { callOptions: CallOptions, context: HasEffectsContext ): boolean { - return !this.variable || this.variable.hasEffectsWhenCalledAtPath(path, callOptions, context); + return ( + !this.variable || + this.getVariableRespectingTDZ().hasEffectsWhenCalledAtPath(path, callOptions, context) + ); } include(): void { @@ -167,7 +172,7 @@ export default class Identifier extends NodeBase implements PatternNode { } includeCallArguments(context: InclusionContext, args: (ExpressionNode | SpreadElement)[]): void { - this.variable!.includeCallArguments(context, args); + this.getVariableRespectingTDZ().includeCallArguments(context, args); } markDeclarationReached(): void { @@ -210,7 +215,24 @@ export default class Identifier extends NodeBase implements PatternNode { } } - protected isPossibleTDZ(): boolean { + private disallowImportReassignment() { + return this.context.error( + { + code: 'ILLEGAL_REASSIGNMENT', + message: `Illegal reassignment to import '${this.name}'` + }, + this.start + ); + } + + private getVariableRespectingTDZ(): ExpressionEntity { + if (this.isPossibleTDZ()) { + return UNKNOWN_EXPRESSION; + } + return this.variable!; + } + + private isPossibleTDZ(): boolean { // return cached value if present if (this.isTDZAccess !== null) return this.isTDZAccess; @@ -263,14 +285,4 @@ export default class Identifier extends NodeBase implements PatternNode { return (this.isTDZAccess = false); } - - private disallowImportReassignment() { - return this.context.error( - { - code: 'ILLEGAL_REASSIGNMENT', - message: `Illegal reassignment to import '${this.name}'` - }, - this.start - ); - } } diff --git a/test/form/samples/recursive-multi-expressions/_expected.js b/test/form/samples/recursive-multi-expressions/_expected.js index 7e29ee70457..0df267fb302 100644 --- a/test/form/samples/recursive-multi-expressions/_expected.js +++ b/test/form/samples/recursive-multi-expressions/_expected.js @@ -1,6 +1,7 @@ const unknown = globalThis.unknown; var logical1 = logical1 || (() => {}); +logical1(); logical1()(); logical1.x = 1; logical1().x = 1; @@ -10,6 +11,7 @@ var logical2 = logical2 || console.log; logical2(); var conditional1 = unknown ? conditional1 : () => {}; +conditional1(); conditional1()(); conditional1.x = 1; conditional1().x = 1; diff --git a/test/form/samples/recursive-multi-expressions/main.js b/test/form/samples/recursive-multi-expressions/main.js index 3b959d2d3b7..0df267fb302 100644 --- a/test/form/samples/recursive-multi-expressions/main.js +++ b/test/form/samples/recursive-multi-expressions/main.js @@ -1,7 +1,7 @@ const unknown = globalThis.unknown; var logical1 = logical1 || (() => {}); -logical1(); // removed +logical1(); logical1()(); logical1.x = 1; logical1().x = 1; @@ -11,7 +11,7 @@ var logical2 = logical2 || console.log; logical2(); var conditional1 = unknown ? conditional1 : () => {}; -conditional1(); // removed +conditional1(); conditional1()(); conditional1.x = 1; conditional1().x = 1; diff --git a/test/form/samples/tdz-common/_expected.js b/test/form/samples/tdz-common/_expected.js index 17f3da3d7e5..0f3aecb22da 100644 --- a/test/form/samples/tdz-common/_expected.js +++ b/test/form/samples/tdz-common/_expected.js @@ -6,7 +6,6 @@ console.log(function() { const C = 1 + C + 2; // TDZ let L = L; // TDZ -var V = V; // TODO: uncommon scenario, but should be dropped console.log("X+" ); // optimize console.log(Y ? "Y+" : "Y-"); // TDZ diff --git a/test/form/samples/tdz-common/main.js b/test/form/samples/tdz-common/main.js index 2e1b7e957a4..a5f29c17cf6 100644 --- a/test/form/samples/tdz-common/main.js +++ b/test/form/samples/tdz-common/main.js @@ -6,12 +6,12 @@ console.log(function() { const unused1 = 1; // drop let unused2 = 2; // drop +unused3; // drop var unused3 = 3; // drop class unused4 {} // drop const C = 1 + C + 2; // TDZ let L = L; // TDZ -var V = V; // TODO: uncommon scenario, but should be dropped const X = 1; // drop console.log(X ? "X+" : "X-"); // optimize From 22bdd56bdd85b417f5d565846515de0463b1e10d Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 2 Jul 2021 06:49:01 +0200 Subject: [PATCH 09/15] Remove special logic for TDZ var assignments --- src/ast/nodes/Identifier.ts | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 323243d38bf..0a8e4d923a0 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -146,7 +146,11 @@ export default class Identifier extends NodeBase implements PatternNode { hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { return ( - !this.variable || this.getVariableRespectingTDZ().hasEffectsWhenAssignedAtPath(path, context) + !this.variable || + (path.length > 0 + ? this.getVariableRespectingTDZ() + : this.variable + ).hasEffectsWhenAssignedAtPath(path, context) ); } @@ -244,20 +248,6 @@ export default class Identifier extends NodeBase implements PatternNode { return (this.isTDZAccess = false); } - if ( - this.variable.kind === 'var' && - ((this.parent.type === 'AssignmentExpression' && this === (this.parent as any).left) || - (this.parent.type === 'UpdateExpression' && this === (this.parent as any).argument) || - this.parent.type === 'SequenceExpression' || - this.parent.type === 'ExpressionStatement') - ) { - // If a `var` variable is modified or innocuous - // then pretend the init was reached in these cases - // and have rollup's treeshaking take care of it. - this.variable.initReached = true; - return (this.isTDZAccess = false); - } - if (!this.variable.initReached) { // Either a const/let TDZ violation or // var use before declaration was encountered. From 487e2278fb41462ff0bc8b23857c38602c4d347f Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 2 Jul 2021 07:12:14 +0200 Subject: [PATCH 10/15] Improve self-reference in declaration handling --- src/ast/nodes/Identifier.ts | 30 +------------------ src/ast/nodes/shared/ClassNode.ts | 17 ++++++++--- .../tdz-access-in-declaration/_config.js | 3 ++ .../tdz-access-in-declaration/_expected.js | 13 ++++++++ .../samples/tdz-access-in-declaration/main.js | 13 ++++++++ 5 files changed, 43 insertions(+), 33 deletions(-) create mode 100644 test/form/samples/tdz-access-in-declaration/_config.js create mode 100644 test/form/samples/tdz-access-in-declaration/_expected.js create mode 100644 test/form/samples/tdz-access-in-declaration/main.js diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 0a8e4d923a0..b540a3c9b90 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -9,7 +9,6 @@ import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { NodeEvent } from '../NodeEvents'; import FunctionScope from '../scopes/FunctionScope'; import { EMPTY_PATH, ObjectPath, PathTracker } from '../utils/PathTracker'; -import { UNDEFINED_EXPRESSION } from '../values'; import GlobalVariable from '../variables/GlobalVariable'; import LocalVariable from '../variables/LocalVariable'; import Variable from '../variables/Variable'; @@ -21,14 +20,6 @@ import { PatternNode } from './shared/Pattern'; export type IdentifierWithVariable = Identifier & { variable: Variable }; -const tdzInitTypesToIgnore = { - __proto__: null, - ArrowFunctionExpression: true, - ClassExpression: true, - FunctionExpression: true, - ObjectExpression: true -}; - const tdzVariableKinds = { __proto__: null, class: true, @@ -237,7 +228,7 @@ export default class Identifier extends NodeBase implements PatternNode { } private isPossibleTDZ(): boolean { - // return cached value if present + // return cached value to avoid issues with the next tree-shaking pass if (this.isTDZAccess !== null) return this.isTDZAccess; if ( @@ -251,25 +242,6 @@ export default class Identifier extends NodeBase implements PatternNode { if (!this.variable.initReached) { // Either a const/let TDZ violation or // var use before declaration was encountered. - // Retain this variable accesss to preserve the input behavior. - return (this.isTDZAccess = true); - } - - let init, init_parent; - if ( - (init = (this.variable as any).init) && - init !== UNDEFINED_EXPRESSION && - (init_parent = (init as any).parent) && - init_parent.type == 'VariableDeclarator' && - init_parent.id.variable === this.variable && - !(init_parent.init.type in tdzInitTypesToIgnore) && - // code position comparisons must be in the same context - this.context === init_parent.id.context && - this.start >= init.start && - this.start < init.end - ) { - // any scope variable access within its own declaration init: - // let x = x + 1; return (this.isTDZAccess = true); } diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index 1c704f0a9c9..87057ca8b98 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -77,8 +77,12 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { } hasEffects(context: HasEffectsContext): boolean { - this.id?.markDeclarationReached(); - return super.hasEffects(context); + const initEffect = this.superClass?.hasEffects(context) || this.body.hasEffects(context); + if (this.id) { + this.id.markDeclarationReached(); + if (this.id.hasEffects()) return true; + } + return initEffect || super.hasEffects(context); } hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { @@ -108,8 +112,13 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { - this.id?.markDeclarationReached(); - super.include(context, includeChildrenRecursively); + this.included = true; + this.superClass?.include(context, includeChildrenRecursively); + this.body.include(context, includeChildrenRecursively); + if (this.id) { + this.id.markDeclarationReached(); + this.id.include(); + } } initialise(): void { diff --git a/test/form/samples/tdz-access-in-declaration/_config.js b/test/form/samples/tdz-access-in-declaration/_config.js new file mode 100644 index 00000000000..12ef3f7a53d --- /dev/null +++ b/test/form/samples/tdz-access-in-declaration/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'detect accessing TDZ variables within the declaration' +}; diff --git a/test/form/samples/tdz-access-in-declaration/_expected.js b/test/form/samples/tdz-access-in-declaration/_expected.js new file mode 100644 index 00000000000..dfb1cc54fcb --- /dev/null +++ b/test/form/samples/tdz-access-in-declaration/_expected.js @@ -0,0 +1,13 @@ +const a = a; // keep + +const b = getB(); // keep +function getB() { + return b; +} + +function getC() { + return c; +} +const c = getC(); // keep + +class d extends d {} // keep diff --git a/test/form/samples/tdz-access-in-declaration/main.js b/test/form/samples/tdz-access-in-declaration/main.js new file mode 100644 index 00000000000..dfb1cc54fcb --- /dev/null +++ b/test/form/samples/tdz-access-in-declaration/main.js @@ -0,0 +1,13 @@ +const a = a; // keep + +const b = getB(); // keep +function getB() { + return b; +} + +function getC() { + return c; +} +const c = getC(); // keep + +class d extends d {} // keep From 1bca0a5f479e539ea53ec6a40ef0356a3b763213 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 2 Jul 2021 17:17:35 +0200 Subject: [PATCH 11/15] Deprecate treeshake.correctVarValueBeforeDeclaration --- docs/999-big-list-of-options.md | 34 ++++++------------- src/ast/scopes/BlockScope.ts | 3 +- src/rollup/types.d.ts | 2 +- src/utils/options/normalizeInputOptions.ts | 9 ++++- src/utils/options/options.ts | 3 -- .../treeshake-preset-override/_expected.js | 3 -- .../samples/treeshake-preset-override/main.js | 3 -- .../preset-with-override/_config.js | 1 - .../preset-with-override/_expected.js | 3 -- .../preset-with-override/main.js | 3 -- .../treeshake-presets/recommended/_config.js | 1 - .../recommended/_expected.js | 3 -- .../treeshake-presets/recommended/main.js | 3 -- .../treeshake-presets/safest/_config.js | 1 - .../treeshake-presets/safest/_expected.js | 3 -- .../samples/treeshake-presets/safest/main.js | 3 -- .../treeshake-presets/smallest/_config.js | 1 - .../treeshake-presets/smallest/_expected.js | 3 -- .../treeshake-presets/smallest/main.js | 3 -- .../samples/treeshake-presets/true/_config.js | 1 - .../treeshake-presets/true/_expected.js | 3 -- .../samples/treeshake-presets/true/main.js | 3 -- .../_config.js | 3 +- .../_config.js | 11 ++++++ .../main.js | 11 ++++++ 25 files changed, 44 insertions(+), 73 deletions(-) create mode 100644 test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js create mode 100644 test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js diff --git a/docs/999-big-list-of-options.md b/docs/999-big-list-of-options.md index 9db70067512..d77b8b197d2 100755 --- a/docs/999-big-list-of-options.md +++ b/docs/999-big-list-of-options.md @@ -1379,7 +1379,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 | "smallest" | "safest" | "recommended" | { annotations?: boolean, correctVarValueBeforeDeclaration?: boolean, moduleSideEffects?: ModuleSideEffectsOption, preset?: "smallest" | "safest" | "recommended", propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`
+Type: `boolean | "smallest" | "safest" | "recommended" | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, preset?: "smallest" | "safest" | "recommended", propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`
CLI: `--treeshake`/`--no-treeshake`
Default: `true` @@ -1389,8 +1389,8 @@ Whether to apply tree-shaking and to fine-tune the tree-shaking process. Setting * getters with side effects will only be retained if the return value is used (`treeshake.propertyReadSideEffects: false`) * code from imported modules will only be retained if at least one exported value is used (`treeshake.moduleSideEffects: false`) * you should not bundle polyfills that rely on detecting broken builtins (`treeshake.tryCatchDeoptimization: false`) - * some semantic issues may be swallowed (`treeshake.unknownGlobalSideEffects: false`, `treeshake.correctVarValueBeforeDeclaration: false`) -* `"recommended"` should work well for most usage patterns. Some semantic issues may be swallowed, though (`treeshake.unknownGlobalSideEffects: false`, `treeshake.correctVarValueBeforeDeclaration: false`) + * some semantic issues may be swallowed (`treeshake.unknownGlobalSideEffects: false`) +* `"recommended"` should work well for most usage patterns. Some semantic issues may be swallowed, though (`treeshake.unknownGlobalSideEffects: false`) * `"safest"` tries to be as spec compliant as possible while still providing some basic tree-shaking capabilities. * `true` is equivalent to not specifying the option and will always choose the default value (see below). @@ -1416,27 +1416,6 @@ class Impure { /*@__PURE__*/new Impure(); ``` -**treeshake.correctVarValueBeforeDeclaration**
-Type: `boolean`
-CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`
-Default: `false` - -If a variable is assigned a value in its declaration and is never reassigned, Rollup assumes the value to be constant. This is not true if the variable is declared with `var`, however, as those variables can be accessed before their declaration where they will evaluate to `undefined`. -Choosing `true` will make sure Rollup does not make (wrong) assumptions about the value of such variables. Note though that this can have a noticeable negative impact on tree-shaking results. - -```js -// input -if (x) console.log('not executed'); -var x = true; - -// output with treeshake.correctVarValueBeforeDeclaration === false -console.log('not executed'); - -// output with treeshake.correctVarValueBeforeDeclaration === true -if (x) console.log('not executed'); -var x = true; -``` - **treeshake.moduleSideEffects**
Type: `boolean | "no-external" | string[] | (id: string, external: boolean) => boolean`
CLI: `--treeshake.moduleSideEffects`/`--no-treeshake.moduleSideEffects`/`--treeshake.moduleSideEffects no-external`
@@ -1760,6 +1739,13 @@ Default: `import` This will rename the dynamic import function to the chosen name when outputting ES bundles. This is useful for generating code that uses a dynamic import polyfill such as [this one](https://github.com/uupaa/dynamic-import-polyfill). +**treeshake.correctVarValueBeforeDeclaration**
+Type: `boolean`
+CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`
+Default: `false` + +This was temporarily an option to deoptimize the values of all `var` declarations to handle accessing a variable before it is declared. This option no longer has an effect as there is now an improved logic in place to automatically detect these situations. The option will be removed entirely in future Rollup versions. + #### treeshake.pureExternalModules _Use [`treeshake.moduleSideEffects: 'no-external'`](guide/en/#treeshake) instead._
Type: `boolean | string[] | (id: string) => boolean | null`
diff --git a/src/ast/scopes/BlockScope.ts b/src/ast/scopes/BlockScope.ts index ff790dd9566..ee694c790b5 100644 --- a/src/ast/scopes/BlockScope.ts +++ b/src/ast/scopes/BlockScope.ts @@ -14,7 +14,8 @@ export default class BlockScope extends ChildScope { ): LocalVariable { if (isHoisted) { this.parent.addDeclaration(identifier, context, init, isHoisted); - // Necessary to make sure the init is deoptimized. We cannot call deoptimizePath here. + // Necessary to make sure the init is deoptimized for conditional declarations. + // We cannot call deoptimizePath here. return this.parent.addDeclaration(identifier, context, UNDEFINED_EXPRESSION, isHoisted); } else { return super.addDeclaration(identifier, context, init, false); diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts index 0bcf91597ae..64702ee6b8e 100644 --- a/src/rollup/types.d.ts +++ b/src/rollup/types.d.ts @@ -479,6 +479,7 @@ type TreeshakingPreset = 'smallest' | 'safest' | 'recommended'; export interface TreeshakingOptions { annotations?: boolean; + /** @deprecated This option no longer has any effect */ correctVarValueBeforeDeclaration?: boolean; moduleSideEffects?: ModuleSideEffectsOption; preset?: TreeshakingPreset; @@ -491,7 +492,6 @@ export interface TreeshakingOptions { export interface NormalizedTreeshakingOptions { annotations: boolean; - correctVarValueBeforeDeclaration: boolean; moduleSideEffects: HasModuleSideEffects; propertyReadSideEffects: boolean | 'always'; tryCatchDeoptimization: boolean; diff --git a/src/utils/options/normalizeInputOptions.ts b/src/utils/options/normalizeInputOptions.ts index a1aa3ff9e64..6e416038bf5 100644 --- a/src/utils/options/normalizeInputOptions.ts +++ b/src/utils/options/normalizeInputOptions.ts @@ -259,6 +259,14 @@ const getTreeshake = ( strictDeprecations ); } + if (typeof configTreeshake.correctVarValueBeforeDeclaration !== 'undefined') { + warnDeprecationWithOptions( + `The "treeshake.correctVarValueBeforeDeclaration" option is deprecated and no longer has any effect. An improved algorithm is now in place that renders this option unnecessary.`, + true, + warn, + strictDeprecations + ); + } configWithPreset = configTreeshake; const presetName = configTreeshake.preset; if (presetName) { @@ -277,7 +285,6 @@ const getTreeshake = ( } return { annotations: configWithPreset.annotations !== false, - correctVarValueBeforeDeclaration: configWithPreset.correctVarValueBeforeDeclaration === true, moduleSideEffects: typeof configTreeshake === 'object' && configTreeshake.pureExternalModules ? getHasModuleSideEffects( diff --git a/src/utils/options/options.ts b/src/utils/options/options.ts index 8899b29476f..22cf89d2923 100644 --- a/src/utils/options/options.ts +++ b/src/utils/options/options.ts @@ -38,7 +38,6 @@ export const treeshakePresets: { } = { recommended: { annotations: true, - correctVarValueBeforeDeclaration: false, moduleSideEffects: () => true, propertyReadSideEffects: true, tryCatchDeoptimization: true, @@ -46,7 +45,6 @@ export const treeshakePresets: { }, safest: { annotations: true, - correctVarValueBeforeDeclaration: true, moduleSideEffects: () => true, propertyReadSideEffects: true, tryCatchDeoptimization: true, @@ -54,7 +52,6 @@ export const treeshakePresets: { }, smallest: { annotations: true, - correctVarValueBeforeDeclaration: false, moduleSideEffects: () => false, propertyReadSideEffects: false, tryCatchDeoptimization: false, diff --git a/test/cli/samples/treeshake-preset-override/_expected.js b/test/cli/samples/treeshake-preset-override/_expected.js index 01cc5e431f9..a4958370a2b 100644 --- a/test/cli/samples/treeshake-preset-override/_expected.js +++ b/test/cli/samples/treeshake-preset-override/_expected.js @@ -11,6 +11,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/cli/samples/treeshake-preset-override/main.js b/test/cli/samples/treeshake-preset-override/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/cli/samples/treeshake-preset-override/main.js +++ b/test/cli/samples/treeshake-preset-override/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/preset-with-override/_config.js b/test/form/samples/treeshake-presets/preset-with-override/_config.js index cf5764aed70..73bd2448ad1 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/_config.js +++ b/test/form/samples/treeshake-presets/preset-with-override/_config.js @@ -11,7 +11,6 @@ module.exports = { plugins: [ { buildStart(options) { - assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, false); assert.strictEqual(options.treeshake.tryCatchDeoptimization, false); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/preset-with-override/_expected.js b/test/form/samples/treeshake-presets/preset-with-override/_expected.js index 096ef5b2c73..b731633f86b 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/_expected.js +++ b/test/form/samples/treeshake-presets/preset-with-override/_expected.js @@ -1,6 +1,3 @@ console.log('main'); unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/preset-with-override/main.js b/test/form/samples/treeshake-presets/preset-with-override/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/main.js +++ b/test/form/samples/treeshake-presets/preset-with-override/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/recommended/_config.js b/test/form/samples/treeshake-presets/recommended/_config.js index ada92c53ef7..e943af3e2aa 100644 --- a/test/form/samples/treeshake-presets/recommended/_config.js +++ b/test/form/samples/treeshake-presets/recommended/_config.js @@ -8,7 +8,6 @@ module.exports = { plugins: [ { buildStart(options) { - assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, false); diff --git a/test/form/samples/treeshake-presets/recommended/_expected.js b/test/form/samples/treeshake-presets/recommended/_expected.js index ee7d24d369c..65f08abb6b4 100644 --- a/test/form/samples/treeshake-presets/recommended/_expected.js +++ b/test/form/samples/treeshake-presets/recommended/_expected.js @@ -11,6 +11,3 @@ console.log('main'); try { const noeffect = 1; } catch {} - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/recommended/main.js b/test/form/samples/treeshake-presets/recommended/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/form/samples/treeshake-presets/recommended/main.js +++ b/test/form/samples/treeshake-presets/recommended/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/safest/_config.js b/test/form/samples/treeshake-presets/safest/_config.js index 3e5353f5c7e..35b71dfd8a1 100644 --- a/test/form/samples/treeshake-presets/safest/_config.js +++ b/test/form/samples/treeshake-presets/safest/_config.js @@ -8,7 +8,6 @@ module.exports = { plugins: [ { buildStart(options) { - assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, true); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/safest/_expected.js b/test/form/samples/treeshake-presets/safest/_expected.js index a859fbd4964..3df383b997e 100644 --- a/test/form/samples/treeshake-presets/safest/_expected.js +++ b/test/form/samples/treeshake-presets/safest/_expected.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/safest/main.js b/test/form/samples/treeshake-presets/safest/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/form/samples/treeshake-presets/safest/main.js +++ b/test/form/samples/treeshake-presets/safest/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/smallest/_config.js b/test/form/samples/treeshake-presets/smallest/_config.js index 39e1162c861..ac49dae5a46 100644 --- a/test/form/samples/treeshake-presets/smallest/_config.js +++ b/test/form/samples/treeshake-presets/smallest/_config.js @@ -8,7 +8,6 @@ module.exports = { plugins: [ { buildStart(options) { - assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, false); assert.strictEqual(options.treeshake.tryCatchDeoptimization, false); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, false); diff --git a/test/form/samples/treeshake-presets/smallest/_expected.js b/test/form/samples/treeshake-presets/smallest/_expected.js index e0c6fd50d49..c0b933d7b56 100644 --- a/test/form/samples/treeshake-presets/smallest/_expected.js +++ b/test/form/samples/treeshake-presets/smallest/_expected.js @@ -1,4 +1 @@ console.log('main'); - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/smallest/main.js b/test/form/samples/treeshake-presets/smallest/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/form/samples/treeshake-presets/smallest/main.js +++ b/test/form/samples/treeshake-presets/smallest/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/true/_config.js b/test/form/samples/treeshake-presets/true/_config.js index 0445050db42..9a024352632 100644 --- a/test/form/samples/treeshake-presets/true/_config.js +++ b/test/form/samples/treeshake-presets/true/_config.js @@ -8,7 +8,6 @@ module.exports = { plugins: [ { buildStart(options) { - assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/true/_expected.js b/test/form/samples/treeshake-presets/true/_expected.js index a859fbd4964..3df383b997e 100644 --- a/test/form/samples/treeshake-presets/true/_expected.js +++ b/test/form/samples/treeshake-presets/true/_expected.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/form/samples/treeshake-presets/true/main.js b/test/form/samples/treeshake-presets/true/main.js index 46a542d6190..2ef8f761fe7 100644 --- a/test/form/samples/treeshake-presets/true/main.js +++ b/test/form/samples/treeshake-presets/true/main.js @@ -13,6 +13,3 @@ try { } catch {} unknownGlobal; - -if (!foo) console.log('effect'); -var foo = true; diff --git a/test/function/samples/correct-var-before-declaration-deopt/_config.js b/test/function/samples/correct-var-before-declaration-deopt/_config.js index ab14869f51c..c5e5b826788 100644 --- a/test/function/samples/correct-var-before-declaration-deopt/_config.js +++ b/test/function/samples/correct-var-before-declaration-deopt/_config.js @@ -1,4 +1,3 @@ module.exports = { - description: - 'adds necessary deoptimizations without using treeshake.correctVarValueBeforeDeclaration' + description: 'adds necessary deoptimizations when using var' }; diff --git a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js new file mode 100644 index 00000000000..ac09851ac24 --- /dev/null +++ b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js @@ -0,0 +1,11 @@ +module.exports = { + description: 'marks the treeshake.correctVarValueBeforeDeclaration option as deprecated', + options: { + treeshake: { correctVarValueBeforeDeclaration: true } + }, + error: { + code: 'DEPRECATED_FEATURE', + message: + 'The "treeshake.correctVarValueBeforeDeclaration" option is deprecated and no longer has any effect. An improved algorithm is now in place that renders this option unnecessary.' + } +}; diff --git a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js new file mode 100644 index 00000000000..f8a2d88d245 --- /dev/null +++ b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js @@ -0,0 +1,11 @@ +const foo = {}; + +function doIt(x) { + if (foo[x]) { + return true; + } + foo[x] = true; +} + +doIt('x'); +assert.ok(doIt('x'), 'foo was not reassigned'); From 5f1b960c84bb60578cf234fee08a9b3424bbf5e8 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sun, 4 Jul 2021 06:57:18 +0200 Subject: [PATCH 12/15] Make deprecation non-active until next major version --- docs/999-big-list-of-options.md | 2 +- src/utils/options/normalizeInputOptions.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/999-big-list-of-options.md b/docs/999-big-list-of-options.md index d77b8b197d2..2597053bf58 100755 --- a/docs/999-big-list-of-options.md +++ b/docs/999-big-list-of-options.md @@ -1744,7 +1744,7 @@ Type: `boolean`
CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`
Default: `false` -This was temporarily an option to deoptimize the values of all `var` declarations to handle accessing a variable before it is declared. This option no longer has an effect as there is now an improved logic in place to automatically detect these situations. The option will be removed entirely in future Rollup versions. +This was temporarily an option to deoptimize the values of all `var` declarations to handle accessing a variable before it is declared. This option no longer has an effect as there is now improved logic in place to automatically detect these situations. The option will be removed entirely in future Rollup versions. #### treeshake.pureExternalModules _Use [`treeshake.moduleSideEffects: 'no-external'`](guide/en/#treeshake) instead._
diff --git a/src/utils/options/normalizeInputOptions.ts b/src/utils/options/normalizeInputOptions.ts index 6e416038bf5..67b9be336a9 100644 --- a/src/utils/options/normalizeInputOptions.ts +++ b/src/utils/options/normalizeInputOptions.ts @@ -262,7 +262,7 @@ const getTreeshake = ( if (typeof configTreeshake.correctVarValueBeforeDeclaration !== 'undefined') { warnDeprecationWithOptions( `The "treeshake.correctVarValueBeforeDeclaration" option is deprecated and no longer has any effect. An improved algorithm is now in place that renders this option unnecessary.`, - true, + false, warn, strictDeprecations ); From 131365156f789386ba17f47e281ef6e097e0a97a Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 6 Jul 2021 07:14:20 +0200 Subject: [PATCH 13/15] Undeprecate correctVarValueBeforeDeclaration --- docs/999-big-list-of-options.md | 37 ++++++++++++++----- src/ast/nodes/Identifier.ts | 6 +++ src/rollup/types.d.ts | 2 +- src/utils/options/normalizeInputOptions.ts | 9 +---- src/utils/options/options.ts | 3 ++ .../samples/treeshake-preset-override/main.js | 14 +++++++ .../preset-with-override/_config.js | 1 + .../preset-with-override/main.js | 14 +++++++ .../treeshake-presets/recommended/_config.js | 1 + .../treeshake-presets/recommended/main.js | 14 +++++++ .../treeshake-presets/safest/_config.js | 1 + .../treeshake-presets/safest/_expected.js | 14 +++++++ .../samples/treeshake-presets/safest/main.js | 14 +++++++ .../treeshake-presets/smallest/_config.js | 1 + .../treeshake-presets/smallest/main.js | 14 +++++++ .../samples/treeshake-presets/true/_config.js | 1 + .../samples/treeshake-presets/true/main.js | 14 +++++++ .../_config.js | 11 ------ .../main.js | 11 ------ 19 files changed, 141 insertions(+), 41 deletions(-) delete mode 100644 test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js delete mode 100644 test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js diff --git a/docs/999-big-list-of-options.md b/docs/999-big-list-of-options.md index 2597053bf58..33ce54fa8d4 100755 --- a/docs/999-big-list-of-options.md +++ b/docs/999-big-list-of-options.md @@ -1379,7 +1379,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 | "smallest" | "safest" | "recommended" | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, preset?: "smallest" | "safest" | "recommended", propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`
+Type: `boolean | "smallest" | "safest" | "recommended" | { annotations?: boolean, correctVarValueBeforeDeclaration?: boolean, moduleSideEffects?: ModuleSideEffectsOption, preset?: "smallest" | "safest" | "recommended", propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`
CLI: `--treeshake`/`--no-treeshake`
Default: `true` @@ -1389,8 +1389,8 @@ Whether to apply tree-shaking and to fine-tune the tree-shaking process. Setting * getters with side effects will only be retained if the return value is used (`treeshake.propertyReadSideEffects: false`) * code from imported modules will only be retained if at least one exported value is used (`treeshake.moduleSideEffects: false`) * you should not bundle polyfills that rely on detecting broken builtins (`treeshake.tryCatchDeoptimization: false`) - * some semantic issues may be swallowed (`treeshake.unknownGlobalSideEffects: false`) -* `"recommended"` should work well for most usage patterns. Some semantic issues may be swallowed, though (`treeshake.unknownGlobalSideEffects: false`) + * some semantic issues may be swallowed (`treeshake.unknownGlobalSideEffects: false`, `treeshake.correctVarValueBeforeDeclaration: false`) +* `"recommended"` should work well for most usage patterns. Some semantic issues may be swallowed, though (`treeshake.unknownGlobalSideEffects: false`, `treeshake.correctVarValueBeforeDeclaration: false`) * `"safest"` tries to be as spec compliant as possible while still providing some basic tree-shaking capabilities. * `true` is equivalent to not specifying the option and will always choose the default value (see below). @@ -1416,6 +1416,30 @@ class Impure { /*@__PURE__*/new Impure(); ``` +**treeshake.correctVarValueBeforeDeclaration**
+Type: `boolean`
+CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`
+Default: `false` + +If a variable is assigned a value in its declaration and is never reassigned, Rollup sometimes assumes the value to be constant. This is not true if the variable is declared with `var`, however, as those variables can be accessed before their declaration where they will evaluate to `undefined`. +Choosing `true` will make sure Rollup does not make (wrong) assumptions about the value of such variables. Note though that this can have a noticeable negative impact on tree-shaking results. + +```js +// input +if (Math.random() < 0.5) var x = true; +if (!x) { + console.log('effect'); +} + +// no output with treeshake.correctVarValueBeforeDeclaration === false + +// output with treeshake.correctVarValueBeforeDeclaration === true +if (Math.random() < 0.5) var x = true; +if (!x) { + console.log('effect'); +} +``` + **treeshake.moduleSideEffects**
Type: `boolean | "no-external" | string[] | (id: string, external: boolean) => boolean`
CLI: `--treeshake.moduleSideEffects`/`--no-treeshake.moduleSideEffects`/`--treeshake.moduleSideEffects no-external`
@@ -1739,13 +1763,6 @@ Default: `import` This will rename the dynamic import function to the chosen name when outputting ES bundles. This is useful for generating code that uses a dynamic import polyfill such as [this one](https://github.com/uupaa/dynamic-import-polyfill). -**treeshake.correctVarValueBeforeDeclaration**
-Type: `boolean`
-CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`
-Default: `false` - -This was temporarily an option to deoptimize the values of all `var` declarations to handle accessing a variable before it is declared. This option no longer has an effect as there is now improved logic in place to automatically detect these situations. The option will be removed entirely in future Rollup versions. - #### treeshake.pureExternalModules _Use [`treeshake.moduleSideEffects: 'no-external'`](guide/en/#treeshake) instead._
Type: `boolean | string[] | (id: string) => boolean | null`
diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index b540a3c9b90..3b4084c3972 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -9,6 +9,7 @@ import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { NodeEvent } from '../NodeEvents'; import FunctionScope from '../scopes/FunctionScope'; import { EMPTY_PATH, ObjectPath, PathTracker } from '../utils/PathTracker'; +import { UNDEFINED_EXPRESSION } from '../values'; import GlobalVariable from '../variables/GlobalVariable'; import LocalVariable from '../variables/LocalVariable'; import Variable from '../variables/Variable'; @@ -53,9 +54,14 @@ export default class Identifier extends NodeBase implements PatternNode { declare(kind: string, init: ExpressionEntity): LocalVariable[] { let variable: LocalVariable; + const { treeshake } = this.context.options; switch (kind) { case 'var': variable = this.scope.addDeclaration(this, this.context, init, true); + if (treeshake && treeshake.correctVarValueBeforeDeclaration) { + // Necessary to make sure the init is deoptimized. We cannot call deoptimizePath here. + this.scope.addDeclaration(this, this.context, UNDEFINED_EXPRESSION, true); + } break; case 'function': // in strict mode, functions are only hoisted within a scope but not across block scopes diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts index 64702ee6b8e..0bcf91597ae 100644 --- a/src/rollup/types.d.ts +++ b/src/rollup/types.d.ts @@ -479,7 +479,6 @@ type TreeshakingPreset = 'smallest' | 'safest' | 'recommended'; export interface TreeshakingOptions { annotations?: boolean; - /** @deprecated This option no longer has any effect */ correctVarValueBeforeDeclaration?: boolean; moduleSideEffects?: ModuleSideEffectsOption; preset?: TreeshakingPreset; @@ -492,6 +491,7 @@ export interface TreeshakingOptions { export interface NormalizedTreeshakingOptions { annotations: boolean; + correctVarValueBeforeDeclaration: boolean; moduleSideEffects: HasModuleSideEffects; propertyReadSideEffects: boolean | 'always'; tryCatchDeoptimization: boolean; diff --git a/src/utils/options/normalizeInputOptions.ts b/src/utils/options/normalizeInputOptions.ts index 67b9be336a9..a1aa3ff9e64 100644 --- a/src/utils/options/normalizeInputOptions.ts +++ b/src/utils/options/normalizeInputOptions.ts @@ -259,14 +259,6 @@ const getTreeshake = ( strictDeprecations ); } - if (typeof configTreeshake.correctVarValueBeforeDeclaration !== 'undefined') { - warnDeprecationWithOptions( - `The "treeshake.correctVarValueBeforeDeclaration" option is deprecated and no longer has any effect. An improved algorithm is now in place that renders this option unnecessary.`, - false, - warn, - strictDeprecations - ); - } configWithPreset = configTreeshake; const presetName = configTreeshake.preset; if (presetName) { @@ -285,6 +277,7 @@ const getTreeshake = ( } return { annotations: configWithPreset.annotations !== false, + correctVarValueBeforeDeclaration: configWithPreset.correctVarValueBeforeDeclaration === true, moduleSideEffects: typeof configTreeshake === 'object' && configTreeshake.pureExternalModules ? getHasModuleSideEffects( diff --git a/src/utils/options/options.ts b/src/utils/options/options.ts index 22cf89d2923..8899b29476f 100644 --- a/src/utils/options/options.ts +++ b/src/utils/options/options.ts @@ -38,6 +38,7 @@ export const treeshakePresets: { } = { recommended: { annotations: true, + correctVarValueBeforeDeclaration: false, moduleSideEffects: () => true, propertyReadSideEffects: true, tryCatchDeoptimization: true, @@ -45,6 +46,7 @@ export const treeshakePresets: { }, safest: { annotations: true, + correctVarValueBeforeDeclaration: true, moduleSideEffects: () => true, propertyReadSideEffects: true, tryCatchDeoptimization: true, @@ -52,6 +54,7 @@ export const treeshakePresets: { }, smallest: { annotations: true, + correctVarValueBeforeDeclaration: false, moduleSideEffects: () => false, propertyReadSideEffects: false, tryCatchDeoptimization: false, diff --git a/test/cli/samples/treeshake-preset-override/main.js b/test/cli/samples/treeshake-preset-override/main.js index 2ef8f761fe7..25388db20b9 100644 --- a/test/cli/samples/treeshake-preset-override/main.js +++ b/test/cli/samples/treeshake-preset-override/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); \ No newline at end of file diff --git a/test/form/samples/treeshake-presets/preset-with-override/_config.js b/test/form/samples/treeshake-presets/preset-with-override/_config.js index 73bd2448ad1..cf5764aed70 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/_config.js +++ b/test/form/samples/treeshake-presets/preset-with-override/_config.js @@ -11,6 +11,7 @@ module.exports = { plugins: [ { buildStart(options) { + assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, false); assert.strictEqual(options.treeshake.tryCatchDeoptimization, false); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/preset-with-override/main.js b/test/form/samples/treeshake-presets/preset-with-override/main.js index 2ef8f761fe7..d6c035738d1 100644 --- a/test/form/samples/treeshake-presets/preset-with-override/main.js +++ b/test/form/samples/treeshake-presets/preset-with-override/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/form/samples/treeshake-presets/recommended/_config.js b/test/form/samples/treeshake-presets/recommended/_config.js index e943af3e2aa..ada92c53ef7 100644 --- a/test/form/samples/treeshake-presets/recommended/_config.js +++ b/test/form/samples/treeshake-presets/recommended/_config.js @@ -8,6 +8,7 @@ module.exports = { plugins: [ { buildStart(options) { + assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, false); diff --git a/test/form/samples/treeshake-presets/recommended/main.js b/test/form/samples/treeshake-presets/recommended/main.js index 2ef8f761fe7..d6c035738d1 100644 --- a/test/form/samples/treeshake-presets/recommended/main.js +++ b/test/form/samples/treeshake-presets/recommended/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/form/samples/treeshake-presets/safest/_config.js b/test/form/samples/treeshake-presets/safest/_config.js index 35b71dfd8a1..3e5353f5c7e 100644 --- a/test/form/samples/treeshake-presets/safest/_config.js +++ b/test/form/samples/treeshake-presets/safest/_config.js @@ -8,6 +8,7 @@ module.exports = { plugins: [ { buildStart(options) { + assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, true); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/safest/_expected.js b/test/form/samples/treeshake-presets/safest/_expected.js index 3df383b997e..889d307e6f9 100644 --- a/test/form/samples/treeshake-presets/safest/_expected.js +++ b/test/form/samples/treeshake-presets/safest/_expected.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/form/samples/treeshake-presets/safest/main.js b/test/form/samples/treeshake-presets/safest/main.js index 2ef8f761fe7..d6c035738d1 100644 --- a/test/form/samples/treeshake-presets/safest/main.js +++ b/test/form/samples/treeshake-presets/safest/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/form/samples/treeshake-presets/smallest/_config.js b/test/form/samples/treeshake-presets/smallest/_config.js index ac49dae5a46..39e1162c861 100644 --- a/test/form/samples/treeshake-presets/smallest/_config.js +++ b/test/form/samples/treeshake-presets/smallest/_config.js @@ -8,6 +8,7 @@ module.exports = { plugins: [ { buildStart(options) { + assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, false); assert.strictEqual(options.treeshake.tryCatchDeoptimization, false); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, false); diff --git a/test/form/samples/treeshake-presets/smallest/main.js b/test/form/samples/treeshake-presets/smallest/main.js index 2ef8f761fe7..d6c035738d1 100644 --- a/test/form/samples/treeshake-presets/smallest/main.js +++ b/test/form/samples/treeshake-presets/smallest/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/form/samples/treeshake-presets/true/_config.js b/test/form/samples/treeshake-presets/true/_config.js index 9a024352632..0445050db42 100644 --- a/test/form/samples/treeshake-presets/true/_config.js +++ b/test/form/samples/treeshake-presets/true/_config.js @@ -8,6 +8,7 @@ module.exports = { plugins: [ { buildStart(options) { + assert.strictEqual(options.treeshake.correctVarValueBeforeDeclaration, false); assert.strictEqual(options.treeshake.propertyReadSideEffects, true); assert.strictEqual(options.treeshake.tryCatchDeoptimization, true); assert.strictEqual(options.treeshake.unknownGlobalSideEffects, true); diff --git a/test/form/samples/treeshake-presets/true/main.js b/test/form/samples/treeshake-presets/true/main.js index 2ef8f761fe7..d6c035738d1 100644 --- a/test/form/samples/treeshake-presets/true/main.js +++ b/test/form/samples/treeshake-presets/true/main.js @@ -13,3 +13,17 @@ try { } catch {} unknownGlobal; + +let flag = true; + +function test() { + if (flag) var x = true; + if (x) { + return; + } + console.log('effect'); +} + +test(); +flag = false; +test(); diff --git a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js deleted file mode 100644 index ac09851ac24..00000000000 --- a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/_config.js +++ /dev/null @@ -1,11 +0,0 @@ -module.exports = { - description: 'marks the treeshake.correctVarValueBeforeDeclaration option as deprecated', - options: { - treeshake: { correctVarValueBeforeDeclaration: true } - }, - error: { - code: 'DEPRECATED_FEATURE', - message: - 'The "treeshake.correctVarValueBeforeDeclaration" option is deprecated and no longer has any effect. An improved algorithm is now in place that renders this option unnecessary.' - } -}; diff --git a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js b/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js deleted file mode 100644 index f8a2d88d245..00000000000 --- a/test/function/samples/deprecations/treeshake-correctVarValueBeforeDeclaration/main.js +++ /dev/null @@ -1,11 +0,0 @@ -const foo = {}; - -function doIt(x) { - if (foo[x]) { - return true; - } - foo[x] = true; -} - -doIt('x'); -assert.ok(doIt('x'), 'foo was not reassigned'); From e09fcd32da385a7a9819fb1e090edf45e71e2e7b Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 6 Jul 2021 07:24:16 +0200 Subject: [PATCH 14/15] Fix deoptimization regression for call expressions --- src/ast/nodes/CallExpression.ts | 27 ++++++++++--------- .../_config.js | 3 +++ .../_expected.js | 3 +++ .../main.js | 9 +++++++ 4 files changed, 30 insertions(+), 12 deletions(-) create mode 100644 test/form/samples/return-value-access-in-conditional/_config.js create mode 100644 test/form/samples/return-value-access-in-conditional/_expected.js create mode 100644 test/form/samples/return-value-access-in-conditional/main.js diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts index e7df7509e6b..25875e511b2 100644 --- a/src/ast/nodes/CallExpression.ts +++ b/src/ast/nodes/CallExpression.ts @@ -176,19 +176,22 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt } hasEffects(context: HasEffectsContext): boolean { - if (!this.deoptimized) this.applyDeoptimizations(); - for (const argument of this.arguments) { - if (argument.hasEffects(context)) return true; + try { + for (const argument of this.arguments) { + if (argument.hasEffects(context)) return true; + } + if ( + (this.context.options.treeshake as NormalizedTreeshakingOptions).annotations && + this.annotations + ) + return false; + return ( + this.callee.hasEffects(context) || + this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions, context) + ); + } finally { + if (!this.deoptimized) this.applyDeoptimizations(); } - if ( - (this.context.options.treeshake as NormalizedTreeshakingOptions).annotations && - this.annotations - ) - return false; - return ( - this.callee.hasEffects(context) || - this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions, context) - ); } hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { diff --git a/test/form/samples/return-value-access-in-conditional/_config.js b/test/form/samples/return-value-access-in-conditional/_config.js new file mode 100644 index 00000000000..e6a08be106d --- /dev/null +++ b/test/form/samples/return-value-access-in-conditional/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'handles accessing funciton return values in deoptimized conditionals' +}; diff --git a/test/form/samples/return-value-access-in-conditional/_expected.js b/test/form/samples/return-value-access-in-conditional/_expected.js new file mode 100644 index 00000000000..87854dbd3de --- /dev/null +++ b/test/form/samples/return-value-access-in-conditional/_expected.js @@ -0,0 +1,3 @@ +console.log(true); + +console.log('retained'); diff --git a/test/form/samples/return-value-access-in-conditional/main.js b/test/form/samples/return-value-access-in-conditional/main.js new file mode 100644 index 00000000000..97b25c9ef67 --- /dev/null +++ b/test/form/samples/return-value-access-in-conditional/main.js @@ -0,0 +1,9 @@ +function foo() { + const result = false; + return result; +} + +console.log(foo() || true); + +if (foo() || true) console.log('retained'); +else console.log('removed'); From 8069a36ee276278edfa58939bdeafcdb56b0fa41 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 6 Jul 2021 08:28:28 +0200 Subject: [PATCH 15/15] Improve coverage --- src/ast/nodes/shared/ClassNode.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index 87057ca8b98..b7d0cc4e31c 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -78,10 +78,7 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { hasEffects(context: HasEffectsContext): boolean { const initEffect = this.superClass?.hasEffects(context) || this.body.hasEffects(context); - if (this.id) { - this.id.markDeclarationReached(); - if (this.id.hasEffects()) return true; - } + this.id?.markDeclarationReached(); return initEffect || super.hasEffects(context); }