Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix additional let/var init bugs #4177

Merged
merged 4 commits into from Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 20 additions & 13 deletions docs/999-big-list-of-options.md
Expand Up @@ -1428,23 +1428,30 @@ Type: `boolean`<br>
CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`<br>
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.
If a variable is assigned a value in its declaration and is never reassigned, Rollup can in some adge case scenarios assume the value to be constant, see below. 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`.
lukastaegert marked this conversation as resolved.
Show resolved Hide resolved
Choosing `true` will make sure Rollup does not make any 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');
}
// everything will be tree-shaken unless treeshake.correctVarValueBeforeDeclaration === true
let logBeforeDeclaration = false;

// no output with treeshake.correctVarValueBeforeDeclaration === false
function logIfEnabled() {
if (logBeforeDeclaration) {
log();
}

// output with treeshake.correctVarValueBeforeDeclaration === true
if (Math.random() < 0.5) var x = true;
if (!x) {
console.log('effect');
}
var value = true;

function log() {
if (!value) {
console.log('should be retained, value is undefined');
}
}
};

logIfEnabled(); // could be removed
logBeforeDeclaration = true;
logIfEnabled(); // needs to be retained as it displays a log
```

**treeshake.moduleSideEffects**<br>
Expand Down
21 changes: 21 additions & 0 deletions src/ast/nodes/Identifier.ts
Expand Up @@ -245,6 +245,19 @@ export default class Identifier extends NodeBase implements PatternNode {
return (this.isTDZAccess = false);
}

let decl_id;
if (
this.variable.declarations &&
this.variable.declarations.length === 1 &&
(decl_id = this.variable.declarations[0] as any) &&
this.start < decl_id.start &&
closestParentFunctionOrProgram(this) === closestParentFunctionOrProgram(decl_id)
) {
// a variable accessed before its declaration
// in the same function or at top level of module
return (this.isTDZAccess = true);
}

if (!this.variable.initReached) {
// Either a const/let TDZ violation or
// var use before declaration was encountered.
Expand All @@ -254,3 +267,11 @@ export default class Identifier extends NodeBase implements PatternNode {
return (this.isTDZAccess = false);
}
}

function closestParentFunctionOrProgram(node: any): any {
while (node && !/^Program|Function/.test(node.type)) {
node = node.parent;
}
// one of: ArrowFunctionExpression, FunctionDeclaration, FunctionExpression or Program
return node;
}
2 changes: 1 addition & 1 deletion src/ast/scopes/TrackingScope.ts
Expand Up @@ -14,6 +14,6 @@ export default class TrackingScope extends BlockScope {
isHoisted: boolean
): LocalVariable {
this.hoistedDeclarations.push(identifier);
return this.parent.addDeclaration(identifier, context, init, isHoisted);
return super.addDeclaration(identifier, context, init, isHoisted);
}
}
14 changes: 14 additions & 0 deletions test/cli/samples/treeshake-preset-override/_expected.js
Expand Up @@ -11,3 +11,17 @@ try {
} catch {}

unknownGlobal;

let flag = true;

function test() {
if (flag) var x = true;
if (x) {
return;
}
console.log('effect');
}

test();
flag = false;
test();
13 changes: 13 additions & 0 deletions test/form/samples/tdz-common/_expected.js
Expand Up @@ -41,3 +41,16 @@ class cls {}
console.log("C" );
console.log("D" );
})();

(function let_tdz() {
let flag = false;
function foo() {
if (flag) {
value; // TDZ
}
let value;
}
foo();
flag = true;
foo();
})();
23 changes: 23 additions & 0 deletions test/form/samples/tdz-common/main.js
Expand Up @@ -55,3 +55,26 @@ class cls {}
console.log(C ? "C" : "!C");
console.log(D ? "D" : "!D");
})();

(function let_tdz() {
let flag = false;
function foo() {
if (flag) {
value; // TDZ
}
let value;
}
foo();
flag = true;
foo();
})();

// should be dropped
(function() {
function foo() {
const access = () => value;
let value;
access();
};
foo();
})();
@@ -1,3 +1,17 @@
console.log('main');

unknownGlobal;

let flag = true;

function test() {
if (flag) var x = true;
if (x) {
return;
}
console.log('effect');
}

test();
flag = false;
test();
14 changes: 14 additions & 0 deletions test/form/samples/treeshake-presets/recommended/_expected.js
Expand Up @@ -11,3 +11,17 @@ console.log('main');
try {
const noeffect = 1;
} catch {}

let flag = true;

function test() {
if (flag) var x = true;
if (x) {
return;
}
console.log('effect');
}

test();
flag = false;
test();
14 changes: 14 additions & 0 deletions test/form/samples/treeshake-presets/smallest/_expected.js
@@ -1 +1,15 @@
console.log('main');

let flag = true;

function test() {
if (flag) var x = true;
if (x) {
return;
}
console.log('effect');
}

test();
flag = false;
test();
14 changes: 14 additions & 0 deletions test/form/samples/treeshake-presets/true/_expected.js
Expand Up @@ -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();
19 changes: 18 additions & 1 deletion test/function/samples/use-var-before-decl/main.js
Expand Up @@ -41,4 +41,21 @@ log(function() {
}
})();

assert.strictEqual(results.join(" "), "PASS1 PASS2 PASS3 PASS4 PASS5 PASS6 OFF ON");
(function () {
var flag = false;

function foo() {
if (flag) {
if (!value) log(flag + "1");
}
var value = true;
log(flag + "2");
}

foo();

flag = true;
foo();
})();

assert.strictEqual(results.join(" "), "PASS1 PASS2 PASS3 PASS4 PASS5 PASS6 OFF ON false2 true1 true2");