Fix var/const/let variable use before declaration (#4148)

* 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

* successfully run tests from #4149
without treeshake.correctVarBeforeDeclaration

* Fix pattern handling

* Handle TDZ class access

* Improve field name

* Fix TDZ caching

* Only include entry point exports after first treeshaking pass

* No longer make TDZ var access a side effect but treat as unknown value

* Remove special logic for TDZ var assignments

* Improve self-reference in declaration handling

* Deprecate treeshake.correctVarValueBeforeDeclaration

* Make deprecation non-active until next major version

* Undeprecate correctVarValueBeforeDeclaration

* Fix deoptimization regression for call expressions

* Improve coverage

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

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

@@ -1421,20 +1421,23 @@ 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 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`.
+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 (x) console.log('not executed');
-var x = true;
+if (Math.random() < 0.5) var x = true;
+if (!x) {
+  console.log('effect');
+}
 
-// output with treeshake.correctVarValueBeforeDeclaration === false
-console.log('not executed');
+// no output with treeshake.correctVarValueBeforeDeclaration === false
 
 // output with treeshake.correctVarValueBeforeDeclaration === true
-if (x) console.log('not executed');
-var x = true;
+if (Math.random() < 0.5) var x = true;
+if (!x) {
+  console.log('effect');
+}
 ```
 
 **treeshake.moduleSideEffects**<br>

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 {

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

src/ast/nodes/ArrayPattern.ts

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

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,

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 {

src/ast/nodes/Identifier.ts

@@ -15,18 +15,26 @@ 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, UNKNOWN_EXPRESSION } from './shared/Expression';
 import { ExpressionNode, NodeBase } from './shared/Node';
 import { PatternNode } from './shared/Pattern';
 
 export type IdentifierWithVariable = Identifier & { variable: Variable };
 
+const tdzVariableKinds = {
+	__proto__: null,
+	class: true,
+	const: true,
+	let: true,
+	var: true
+};
+
 export default class Identifier extends NodeBase implements PatternNode {
 	name!: string;
 	type!: NodeType.tIdentifier;
-
 	variable: Variable | null = null;
 	protected deoptimized = false;
+	private isTDZAccess: boolean | null = null;
 
 	addExportedVariables(
 		variables: Variable[],
@@ -72,6 +80,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,7 +105,7 @@ export default class Identifier extends NodeBase implements PatternNode {
 		recursionTracker: PathTracker,
 		origin: DeoptimizableEntity
 	): LiteralValueOrUnknown {
-		return this.variable!.getLiteralValueAtPath(path, recursionTracker, origin);
+		return this.getVariableRespectingTDZ().getLiteralValueAtPath(path, recursionTracker, origin);
 	}
 
 	getReturnExpressionWhenCalledAtPath(
@@ -105,7 +114,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,
@@ -115,6 +124,9 @@ export default class Identifier extends NodeBase implements PatternNode {
 
 	hasEffects(): boolean {
 		if (!this.deoptimized) this.applyDeoptimizations();
+		if (this.isPossibleTDZ() && this.variable!.kind !== 'var') {
+			return true;
+		}
 		return (
 			(this.context.options.treeshake as NormalizedTreeshakingOptions).unknownGlobalSideEffects &&
 			this.variable instanceof GlobalVariable &&
@@ -123,19 +135,31 @@ 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 ||
+			(path.length > 0
+				? this.getVariableRespectingTDZ()
+				: this.variable
+			).hasEffectsWhenAssignedAtPath(path, context)
+		);
 	}
 
 	hasEffectsWhenCalledAtPath(
 		path: ObjectPath,
 		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 {
@@ -149,7 +173,11 @@ 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 {
+		this.variable!.initReached = true;
 	}
 
 	render(
@@ -197,4 +225,32 @@ export default class Identifier extends NodeBase implements PatternNode {
 			this.start
 		);
 	}
+
+	private getVariableRespectingTDZ(): ExpressionEntity {
+		if (this.isPossibleTDZ()) {
+			return UNKNOWN_EXPRESSION;
+		}
+		return this.variable!;
+	}
+
+	private isPossibleTDZ(): boolean {
+		// return cached value to avoid issues with the next tree-shaking pass
+		if (this.isTDZAccess !== null) return this.isTDZAccess;
+
+		if (
+			!(this.variable instanceof LocalVariable) ||
+			!this.variable.kind ||
+			!(this.variable.kind in tdzVariableKinds)
+		) {
+			return (this.isTDZAccess = false);
+		}
+
+		if (!this.variable.initReached) {
+			// Either a const/let TDZ violation or
+			// var use before declaration was encountered.
+			return (this.isTDZAccess = true);
+		}
+
+		return (this.isTDZAccess = false);
+	}
 }

src/ast/nodes/ObjectPattern.ts

@@ -52,4 +52,10 @@ export default class ObjectPattern extends NodeBase implements PatternNode {
 		}
 		return false;
 	}
+
+	markDeclarationReached(): void {
+		for (const property of this.properties) {
+			property.markDeclarationReached();
+		}
+	}
 }

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

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) {

src/ast/nodes/VariableDeclarator.ts

@@ -28,17 +28,20 @@ export default class VariableDeclarator extends NodeBase {
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {
-		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 {
 		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 {

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,12 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
 		);
 	}
 
+	hasEffects(context: HasEffectsContext): boolean {
+		const initEffect = this.superClass?.hasEffects(context) || this.body.hasEffects(context);
+		this.id?.markDeclarationReached();
+		return initEffect || super.hasEffects(context);
+	}
+
 	hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean {
 		return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context);
 	}
@@ -102,10 +108,18 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity {
 		}
 	}
 
-	initialise(): void {
-		if (this.id !== null) {
-			this.id.declare('class', this);
+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
+		this.included = true;
+		this.superClass?.include(context, includeChildrenRecursively);
+		this.body.include(context, includeChildrenRecursively);
+		if (this.id) {
+			this.id.markDeclarationReached();
+			this.id.include();
 		}
+	}
+
+	initialise(): void {
+		this.id?.declare('class', this);
 		for (const method of this.body.body) {
 			if (method instanceof MethodDefinition && method.kind === 'constructor') {
 				this.classConstructor = method;

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