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 1 commit
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
16 changes: 0 additions & 16 deletions docs/999-big-list-of-options.md
Expand Up @@ -1431,22 +1431,6 @@ 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**<br>
Type: `boolean | "no-external" | string[] | (id: string, external: boolean) => boolean`<br>
CLI: `--treeshake.moduleSideEffects`/`--no-treeshake.moduleSideEffects`/`--treeshake.moduleSideEffects no-external`<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");