Extract hoisted variables from dead branches (#3752)

* Extract hoisted variables from dead branches

* Improve coverage

src/ast/nodes/Identifier.ts

@@ -59,7 +59,8 @@ export default class Identifier extends NodeBase implements PatternNode {
 				variable = this.scope.addDeclaration(this, this.context, init, true);
 				break;
 			case 'function':
-				variable = this.scope.addDeclaration(this, this.context, init, 'function');
+				// in strict mode, functions are only hoisted within a scope but not across block scopes
+				variable = this.scope.addDeclaration(this, this.context, init, false);
 				break;
 			case 'let':
 			case 'const':

src/ast/nodes/IfStatement.ts

@@ -3,11 +3,19 @@ import { RenderOptions } from '../../utils/renderHelpers';
 import { removeAnnotations } from '../../utils/treeshakeNode';
 import { DeoptimizableEntity } from '../DeoptimizableEntity';
 import { BROKEN_FLOW_NONE, HasEffectsContext, InclusionContext } from '../ExecutionContext';
+import TrackingScope from '../scopes/TrackingScope';
 import { EMPTY_PATH, SHARED_RECURSION_TRACKER } from '../utils/PathTracker';
 import { LiteralValueOrUnknown, UnknownValue } from '../values';
 import BlockStatement from './BlockStatement';
+import Identifier from './Identifier';
 import * as NodeType from './NodeType';
-import { ExpressionNode, IncludeChildren, StatementBase, StatementNode } from './shared/Node';
+import {
+	ExpressionNode,
+	GenericEsTreeNode,
+	IncludeChildren,
+	StatementBase,
+	StatementNode
+} from './shared/Node';
 
 const unset = Symbol('unset');
 
@@ -17,6 +25,8 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 	test!: ExpressionNode;
 	type!: NodeType.tIfStatement;
 
+	private alternateScope?: TrackingScope;
+	private consequentScope!: TrackingScope;
 	private testValue: LiteralValueOrUnknown | typeof unset = unset;
 
 	deoptimizeCache() {
@@ -58,42 +68,62 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 		}
 	}
 
+	parseNode(esTreeNode: GenericEsTreeNode) {
+		this.consequentScope = new TrackingScope(this.scope);
+		this.consequent = new this.context.nodeConstructors[esTreeNode.consequent.type](
+			esTreeNode.consequent,
+			this,
+			this.consequentScope
+		);
+		if (esTreeNode.alternate) {
+			this.alternateScope = new TrackingScope(this.scope);
+			this.alternate = new this.context.nodeConstructors[esTreeNode.alternate.type](
+				esTreeNode.alternate,
+				this,
+				this.alternateScope
+			);
+		}
+		super.parseNode(esTreeNode);
+	}
+
 	render(code: MagicString, options: RenderOptions) {
 		// Note that unknown test values are always included
 		const testValue = this.getTestValue();
-		if (
-			!this.test.included &&
-			(testValue ? this.alternate === null || !this.alternate.included : !this.consequent.included)
-		) {
-			const singleRetainedBranch = (testValue ? this.consequent : this.alternate)!;
-			code.remove(this.start, singleRetainedBranch.start);
-			code.remove(singleRetainedBranch.end, this.end);
+		const hoistedDeclarations: Identifier[] = [];
+		const includesIfElse = this.test.included;
+		const noTreeshake = !this.context.options.treeshake;
+		if (includesIfElse) {
+			this.test.render(code, options);
+		} else {
 			removeAnnotations(this, code);
-			singleRetainedBranch.render(code, options);
+			code.remove(this.start, this.consequent.start);
+		}
+		if (this.consequent.included && (noTreeshake || testValue === UnknownValue || testValue)) {
+			this.consequent.render(code, options);
 		} else {
-			if (this.test.included) {
-				this.test.render(code, options);
-			} else {
-				code.overwrite(this.test.start, this.test.end, testValue ? 'true' : 'false');
-			}
-			if (this.consequent.included) {
-				this.consequent.render(code, options);
-			} else {
-				code.overwrite(this.consequent.start, this.consequent.end, ';');
-			}
-			if (this.alternate !== null) {
-				if (this.alternate.included) {
-					if (code.original.charCodeAt(this.alternate.start - 1) === 101 /* e */) {
+			code.overwrite(this.consequent.start, this.consequent.end, includesIfElse ? ';' : '');
+			hoistedDeclarations.push(...this.consequentScope.hoistedDeclarations);
+		}
+		if (this.alternate) {
+			if (this.alternate.included && (noTreeshake || testValue === UnknownValue || !testValue)) {
+				if (includesIfElse) {
+					if (code.original.charCodeAt(this.alternate.start - 1) === 101) {
 						code.prependLeft(this.alternate.start, ' ');
 					}
-					this.alternate.render(code, options);
-				} else if (this.shouldKeepAlternateBranch()) {
-					code.overwrite(this.alternate.start, this.alternate.end, ';');
 				} else {
-					code.remove(this.consequent.end, this.alternate.end);
+					code.remove(this.consequent.end, this.alternate.start);
+				}
+				this.alternate.render(code, options);
+			} else {
+				if (includesIfElse && this.shouldKeepAlternateBranch()) {
+					code.overwrite(this.alternate.start, this.end, ';');
+				} else {
+					code.remove(this.consequent.end, this.end);
 				}
+				hoistedDeclarations.push(...this.alternateScope!.hoistedDeclarations);
 			}
 		}
+		renderHoistedDeclarations(hoistedDeclarations, this.start, code);
 	}
 
 	private getTestValue(): LiteralValueOrUnknown {
@@ -160,3 +190,16 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 		return false;
 	}
 }
+
+function renderHoistedDeclarations(
+	hoistedDeclarations: Identifier[],
+	prependPosition: number,
+	code: MagicString
+) {
+	const hoistedVars = [
+		...new Set(hoistedDeclarations.map(identifier => identifier.variable!.getName()))
+	].join(', ');
+	if (hoistedVars) {
+		code.prependRight(prependPosition, `var ${hoistedVars}; `);
+	}
+}

src/ast/nodes/VariableDeclarator.ts

@@ -1,5 +1,3 @@
-import MagicString from 'magic-string';
-import { RenderOptions } from '../../utils/renderHelpers';
 import { ObjectPath } from '../utils/PathTracker';
 import { UNDEFINED_EXPRESSION } from '../values';
 import * as NodeType from './NodeType';
@@ -18,14 +16,4 @@ export default class VariableDeclarator extends NodeBase {
 	deoptimizePath(path: ObjectPath) {
 		this.id.deoptimizePath(path);
 	}
-
-	render(code: MagicString, options: RenderOptions) {
-		// This can happen for hoisted variables in dead branches
-		if (this.init !== null && !this.init.included) {
-			code.remove(this.id.end, this.end);
-			this.id.render(code, options);
-		} else {
-			super.render(code, options);
-		}
-	}
 }

src/ast/scopes/BlockScope.ts

@@ -9,16 +9,11 @@ export default class BlockScope extends ChildScope {
 	addDeclaration(
 		identifier: Identifier,
 		context: AstContext,
-		init: ExpressionEntity | null = null,
-		isHoisted: boolean | 'function'
+		init: ExpressionEntity | null,
+		isHoisted: boolean
 	): LocalVariable {
 		if (isHoisted) {
-			return this.parent.addDeclaration(
-				identifier,
-				context,
-				isHoisted === 'function' ? init : UNKNOWN_EXPRESSION,
-				isHoisted
-			);
+			return this.parent.addDeclaration(identifier, context, UNKNOWN_EXPRESSION, isHoisted);
 		} else {
 			return super.addDeclaration(identifier, context, init, false);
 		}

src/ast/scopes/CatchScope.ts

@@ -9,7 +9,7 @@ export default class CatchScope extends ParameterScope {
 		identifier: Identifier,
 		context: AstContext,
 		init: ExpressionEntity | null,
-		isHoisted: boolean | 'function'
+		isHoisted: boolean
 	): LocalVariable {
 		if (isHoisted) {
 			return this.parent.addDeclaration(identifier, context, init, isHoisted);

src/ast/scopes/Scope.ts

@@ -13,8 +13,8 @@ export default class Scope {
 	addDeclaration(
 		identifier: Identifier,
 		context: AstContext,
-		init: ExpressionEntity | null = null,
-		_isHoisted: boolean | 'function'
+		init: ExpressionEntity | null,
+		_isHoisted: boolean
 	) {
 		const name = identifier.name;
 		let variable = this.variables.get(name) as LocalVariable;

src/ast/scopes/TrackingScope.ts

@@ -0,0 +1,19 @@
+import { AstContext } from '../../Module';
+import Identifier from '../nodes/Identifier';
+import { ExpressionEntity } from '../nodes/shared/Expression';
+import LocalVariable from '../variables/LocalVariable';
+import BlockScope from './BlockScope';
+
+export default class TrackingScope extends BlockScope {
+	public hoistedDeclarations: Identifier[] = [];
+
+	addDeclaration(
+		identifier: Identifier,
+		context: AstContext,
+		init: ExpressionEntity | null,
+		isHoisted: boolean
+	): LocalVariable {
+		this.hoistedDeclarations.push(identifier);
+		return this.parent.addDeclaration(identifier, context, init, isHoisted);
+	}
+}

test/form/samples/hoisted-unused-conditional/_expected.js

@@ -1,5 +1,3 @@
-if (false) {
-	var deadVariable, otherDeadVariable;
-}
+var deadVariable, otherDeadVariable; 
 
 console.log(deadVariable, otherDeadVariable);

test/form/samples/hoisted-variable-if-stmt/_expected.js

@@ -1,6 +1,4 @@
-if (false) {
-    var foo;
-}
+var foo; 
 
 if (foo) {
     console.log("nope");

test/form/samples/hoisted-vars-in-dead-branches/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'renders hoisted variables in dead branches'
+};

test/form/samples/hoisted-vars-in-dead-branches/_expected.js

@@ -0,0 +1,6 @@
+var a, b; {
+	console.log(a, b);
+}
+
+var c, d; if (((() => console.log('effect'))(), true)) ;
+console.log(c, d);

test/form/samples/hoisted-vars-in-dead-branches/main.js

@@ -0,0 +1,12 @@
+if (false) {
+	for (var a; unknownGlobal && true; unknownGlobal && true) var b;
+} else {
+	console.log(a, b);
+}
+
+if (((() => console.log('effect'))(), true)) {
+} else {
+	var c = 1;
+	for (var c; unknownGlobal && true; unknownGlobal && true) var d;
+}
+console.log(c, d);

test/form/samples/no-treeshake/_expected/es.js → test/form/samples/no-treeshake/_expected.js

@@ -20,6 +20,11 @@ function unusedButIncluded() {
 	} else {
 		(true && 'first') || 'second';
 	}
+	if (false) {
+		'first';
+	} else {
+		'second';
+	}
 	'sequence', 'expression';
 	switch ('test') {
 		case 'test':