Do not tear apart declarations in loop or if bodies (#3947)

CHANGELOG.md

@@ -1,5 +1,14 @@
 # rollup changelog
 
+## 2.38.4
+*2021-02-02*
+
+### Bug Fixes
+* Do not change logic when tree-shaking declarations in if statements or loops (#3947)
+
+### Pull Requests
+* [#3947](https://github.com/rollup/rollup/pull/3947): Do not tear apart declarations in loop or if bodies (@lukastaegert)
+
 ## 2.38.3
 *2021-02-01*
 

build-plugins/conditional-fsevents-import.js

@@ -23,8 +23,8 @@ export default function conditionalFsEventsImport() {
 				return { code: magicString.toString(), map: magicString.generateMap({ hires: true }) };
 			}
 		},
-		buildEnd() {
-			if (!transformed) {
+		buildEnd(error) {
+			if (!(error || transformed)) {
 				throw new Error('Could not find "fsevents-handler.js", was the file renamed?');
 			}
 		}

src/ast/nodes/DoWhileStatement.ts

@@ -26,7 +26,7 @@ export default class DoWhileStatement extends StatementBase {
 		this.included = true;
 		this.test.include(context, includeChildrenRecursively);
 		const { brokenFlow } = context;
-		this.body.include(context, includeChildrenRecursively);
+		this.body.includeAsSingleStatement(context, includeChildrenRecursively);
 		context.brokenFlow = brokenFlow;
 	}
 }

src/ast/nodes/ForInStatement.ts

@@ -49,11 +49,11 @@ export default class ForInStatement extends StatementBase {
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
 		this.included = true;
-		this.left.includeAllDeclaredVariables(context, includeChildrenRecursively);
+		this.left.include(context, includeChildrenRecursively || true);
 		this.left.deoptimizePath(EMPTY_PATH);
 		this.right.include(context, includeChildrenRecursively);
 		const { brokenFlow } = context;
-		this.body.include(context, includeChildrenRecursively);
+		this.body.includeAsSingleStatement(context, includeChildrenRecursively);
 		context.brokenFlow = brokenFlow;
 	}
 

src/ast/nodes/ForOfStatement.ts

@@ -34,11 +34,11 @@ export default class ForOfStatement extends StatementBase {
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
 		this.included = true;
-		this.left.includeAllDeclaredVariables(context, includeChildrenRecursively);
+		this.left.include(context, includeChildrenRecursively || true);
 		this.left.deoptimizePath(EMPTY_PATH);
 		this.right.include(context, includeChildrenRecursively);
 		const { brokenFlow } = context;
-		this.body.include(context, includeChildrenRecursively);
+		this.body.includeAsSingleStatement(context, includeChildrenRecursively);
 		context.brokenFlow = brokenFlow;
 	}
 

src/ast/nodes/ForStatement.ts

@@ -40,11 +40,11 @@ export default class ForStatement extends StatementBase {
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
 		this.included = true;
-		if (this.init) this.init.includeAllDeclaredVariables(context, includeChildrenRecursively);
+		if (this.init) this.init.includeAsSingleStatement(context, includeChildrenRecursively);
 		if (this.test) this.test.include(context, includeChildrenRecursively);
 		const { brokenFlow } = context;
 		if (this.update) this.update.include(context, includeChildrenRecursively);
-		this.body.include(context, includeChildrenRecursively);
+		this.body.includeAsSingleStatement(context, includeChildrenRecursively);
 		context.brokenFlow = brokenFlow;
 	}
 

src/ast/nodes/IfStatement.ts

@@ -136,10 +136,10 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 			this.test.include(context, false);
 		}
 		if (testValue && this.consequent.shouldBeIncluded(context)) {
-			this.consequent.include(context, false);
+			this.consequent.includeAsSingleStatement(context, false);
 		}
 		if (this.alternate !== null && !testValue && this.alternate.shouldBeIncluded(context)) {
-			this.alternate.include(context, false);
+			this.alternate.includeAsSingleStatement(context, false);
 		}
 	}
 
@@ -159,12 +159,12 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 		const { brokenFlow } = context;
 		let consequentBrokenFlow = BROKEN_FLOW_NONE;
 		if (this.consequent.shouldBeIncluded(context)) {
-			this.consequent.include(context, false);
+			this.consequent.includeAsSingleStatement(context, false);
 			consequentBrokenFlow = context.brokenFlow;
 			context.brokenFlow = brokenFlow;
 		}
 		if (this.alternate !== null && this.alternate.shouldBeIncluded(context)) {
-			this.alternate.include(context, false);
+			this.alternate.includeAsSingleStatement(context, false);
 			context.brokenFlow =
 				context.brokenFlow < consequentBrokenFlow ? context.brokenFlow : consequentBrokenFlow;
 		}

src/ast/nodes/VariableDeclaration.ts

@@ -68,14 +68,13 @@ export default class VariableDeclaration extends NodeBase {
 		}
 	}
 
-	includeAllDeclaredVariables(
-		context: InclusionContext,
-		includeChildrenRecursively: IncludeChildren
-	): void {
+	includeAsSingleStatement(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
 		this.included = true;
 		for (const declarator of this.declarations) {
-			declarator.id.included = true;
-			declarator.includeAllDeclaredVariables(context, includeChildrenRecursively);
+			if (includeChildrenRecursively || declarator.shouldBeIncluded(context)) {
+				declarator.include(context, includeChildrenRecursively);
+				declarator.id.include(context, includeChildrenRecursively);
+			}
 		}
 	}
 
@@ -87,7 +86,6 @@ export default class VariableDeclaration extends NodeBase {
 
 	render(code: MagicString, options: RenderOptions, nodeRenderOptions: NodeRenderOptions = BLANK) {
 		if (
-			nodeRenderOptions.isNoStatement ||
 			areAllDeclarationsIncludedAndNotExported(this.declarations, options.exportNamesByVariable)
 		) {
 			for (const declarator of this.declarations) {
@@ -111,12 +109,15 @@ export default class VariableDeclaration extends NodeBase {
 		actualContentEnd: number,
 		renderedContentEnd: number,
 		systemPatternExports: Variable[],
-		options: RenderOptions
+		options: RenderOptions,
+		isNoStatement: boolean | undefined
 	): void {
 		if (code.original.charCodeAt(this.end - 1) === 59 /*";"*/) {
 			code.remove(this.end - 1, this.end);
 		}
-		separatorString += ';';
+		if (!isNoStatement) {
+			separatorString += ';';
+		}
 		if (lastSeparatorPos !== null) {
 			if (
 				code.original.charCodeAt(actualContentEnd - 1) === 10 /*"\n"*/ &&
@@ -148,7 +149,7 @@ export default class VariableDeclaration extends NodeBase {
 	private renderReplacedDeclarations(
 		code: MagicString,
 		options: RenderOptions,
-		{ start = this.start, end = this.end }: NodeRenderOptions
+		{ start = this.start, end = this.end, isNoStatement }: NodeRenderOptions
 	): void {
 		const separatedNodes = getCommaSeparatedNodesWithBoundaries(
 			this.declarations,
@@ -247,7 +248,8 @@ export default class VariableDeclaration extends NodeBase {
 				actualContentEnd!,
 				renderedContentEnd,
 				systemPatternExports,
-				options
+				options,
+				isNoStatement
 			);
 		} else {
 			code.remove(start, end);

src/ast/nodes/VariableDeclarator.ts

@@ -39,17 +39,6 @@ export default class VariableDeclarator extends NodeBase {
 		}
 	}
 
-	includeAllDeclaredVariables(
-		context: InclusionContext,
-		includeChildrenRecursively: IncludeChildren
-	): void {
-		this.included = true;
-		this.id.include(context, includeChildrenRecursively);
-		if (this.init) {
-			this.init.include(context, includeChildrenRecursively);
-		}
-	}
-
 	render(code: MagicString, options: RenderOptions) {
 		const renderId = this.id.included;
 		if (renderId) {

src/ast/nodes/WhileStatement.ts

@@ -26,7 +26,7 @@ export default class WhileStatement extends StatementBase {
 		this.included = true;
 		this.test.include(context, includeChildrenRecursively);
 		const { brokenFlow } = context;
-		this.body.include(context, includeChildrenRecursively);
+		this.body.includeAsSingleStatement(context, includeChildrenRecursively);
 		context.brokenFlow = brokenFlow;
 	}
 }

src/ast/nodes/shared/Node.ts

@@ -65,10 +65,9 @@ export interface Node extends Entity {
 
 	/**
 	 * Alternative version of include to override the default behaviour of
-	 * declarations to only include nodes for declarators that have an effect. Necessary
-	 * for for-loops that do not use a declared loop variable.
+	 * declarations to not include the id by default if the declarator has an effect.
 	 */
-	includeAllDeclaredVariables(
+	includeAsSingleStatement(
 		context: InclusionContext,
 		includeChildrenRecursively: IncludeChildren
 	): void;
@@ -207,10 +206,7 @@ export class NodeBase implements ExpressionNode {
 		}
 	}
 
-	includeAllDeclaredVariables(
-		context: InclusionContext,
-		includeChildrenRecursively: IncludeChildren
-	): void {
+	includeAsSingleStatement(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
 		this.include(context, includeChildrenRecursively);
 	}
 

test/function/samples/unused-do-while-loop-declaration/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'does not partially tree-shake unused declarations with side-effects in do-while loop bodies'
+};

test/function/samples/unused-do-while-loop-declaration/main.js

@@ -0,0 +1,8 @@
+let result = 3;
+var b = 3;
+
+do var b = b - 1, unused = result--, unused2 = 0;
+while (b > 0);
+
+assert.strictEqual(b, 0);
+assert.strictEqual(result, 0);

test/function/samples/unused-for-in-loop-declaration/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'does not partially tree-shake unused declarations with side-effects in for-in loop bodies'
+};

test/function/samples/unused-for-in-loop-declaration/main.js

@@ -0,0 +1,7 @@
+let result = 3;
+var b = 3;
+
+for (var x in {foo: 1, bar: 2}) var b = b - 1, unused = result--, unused2 = 0;
+
+assert.strictEqual(b, 1);
+assert.strictEqual(result, 1);

test/function/samples/unused-for-loop-body-declaration/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'does not partially tree-shake unused declarations with side-effects in for loop bodies'
+};

test/function/samples/unused-for-loop-body-declaration/main.js

@@ -0,0 +1,6 @@
+let result = 3;
+
+for (var a = 0; a < 3; a++) var b = a, unused = result--;
+
+assert.strictEqual(b, 2);
+assert.strictEqual(result, 0);

test/function/samples/unused-for-loop-declaration/main.js → test/function/samples/unused-for-loop-init-declaration/main.js

@@ -1,7 +1,7 @@
 let result;
 let reassigned;
 
-for (var a = (reassigned = 'reassigned'), b = 0; b < 2; b++) {
+for (var a = (reassigned = 'reassigned'), b = 0, unused = 3; b < 2; b++) {
 	result = b;
 }
 

test/function/samples/unused-for-of-loop-declaration/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'does not partially tree-shake unused declarations with side-effects in for-of loop bodies'
+};

test/function/samples/unused-for-of-loop-declaration/main.js

@@ -0,0 +1,7 @@
+let result = 3;
+var b = 3;
+
+for (var x of [1, 2]) var b = b - 1, unused = result--, unused2 = 0;
+
+assert.strictEqual(b, 1);
+assert.strictEqual(result, 1);

test/function/samples/unused-if-statement-declaration/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'does not partially tree-shake unused declarations with side-effects in for loops'
+};

test/function/samples/unused-if-statement-declaration/main.js

@@ -0,0 +1,12 @@
+let result;
+
+export function testStatement(condition) {
+	if (condition) var a = 1, b = result = 1, unused1 = 3;
+	else var a = 2, c = result = 2, unused2 = 3;
+	return a;
+}
+
+assert.strictEqual(testStatement(true), 1);
+assert.strictEqual(result, 1);
+assert.strictEqual(testStatement(false), 2);
+assert.strictEqual(result, 2);

test/function/samples/unused-while-loop-declaration/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'does not partially tree-shake unused declarations with side-effects in while loop bodies'
+};

test/function/samples/unused-while-loop-declaration/main.js

@@ -0,0 +1,8 @@
+let result = 3;
+var b = 3;
+
+while (b > 0)
+var b = b - 1, unused = result--, unused2 = 0;
+
+assert.strictEqual(b, 0);
+assert.strictEqual(result, 0);