Fix additional let/var init bugs (#4177)

* Fix additional variable state change bugs
including blockless if statement `var` declarations
and let/var use before declaration within same function

* Replace negative example

* Improve wording

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>

docs/999-big-list-of-options.md

@@ -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.
+In some edge cases if a variable is accessed before its declaration assignment and is not reassigned, then Rollup may incorrectly assume that variable is constant throughout the program, as in the example 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`.
+Choosing `true` will make sure Rollup does not make any assumptions about the value of variables declared with `var`. 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>

src/ast/nodes/Identifier.ts

@@ -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.
@@ -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;
+}

src/ast/scopes/TrackingScope.ts

@@ -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);
 	}
 }

test/cli/samples/treeshake-preset-override/_expected.js

@@ -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();

test/form/samples/tdz-common/_expected.js

@@ -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();
+})();

test/form/samples/tdz-common/main.js

@@ -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();
+})();

test/form/samples/treeshake-presets/preset-with-override/_expected.js

@@ -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();

test/form/samples/treeshake-presets/recommended/_expected.js

@@ -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();

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();

test/form/samples/treeshake-presets/true/_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();

test/function/samples/use-var-before-decl/main.js

@@ -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");