Fix var/const/let variable use before declaration

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 }`<br>
+Type: `boolean | "smallest" | "safest" | "recommended" | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, preset?: "smallest" | "safest" | "recommended", propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`<br>
 CLI: `--treeshake`/`--no-treeshake`<br>
 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**<br>
-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`.
-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**<br>
 Type: `boolean | "no-external" | string[] | (id: string, external: boolean) => boolean`<br>
 CLI: `--treeshake.moduleSideEffects`/`--no-treeshake.moduleSideEffects`/`--treeshake.moduleSideEffects no-external`<br>
@@ -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**<br>
+Type: `boolean`<br>
+CLI: `--treeshake.correctVarValueBeforeDeclaration`/`--no-treeshake.correctVarValueBeforeDeclaration`<br>
+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._<br>
 Type: `boolean | string[] | (id: string) => boolean | null`<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/Identifier.ts

@@ -9,24 +9,31 @@ 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';
 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[],
@@ -46,14 +53,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
@@ -72,6 +74,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 +99,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 +108,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 +118,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 +129,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 +167,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 +219,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 {