Try to make logical expression deoptimization more robust (rollup#4519)

* Try to make logical expression deoptimization more robust

* Share argument rendering logic between call and new expression

Solves an issue where trying to render a non-included argument crashes

src/ast/nodes/CallExpression.ts

@@ -1,11 +1,8 @@
 import type MagicString from 'magic-string';
 import type { NormalizedTreeshakingOptions } from '../../rollup/types';
 import { BLANK } from '../../utils/blank';
-import {
-	findFirstOccurrenceOutsideComment,
-	type NodeRenderOptions,
-	type RenderOptions
-} from '../../utils/renderHelpers';
+import { renderCallArguments } from '../../utils/renderCallArguments';
+import { type NodeRenderOptions, type RenderOptions } from '../../utils/renderHelpers';
 import type { DeoptimizableEntity } from '../DeoptimizableEntity';
 import type { HasEffectsContext, InclusionContext } from '../ExecutionContext';
 import { EVENT_CALLED } from '../NodeEvents';
@@ -116,36 +113,7 @@ export default class CallExpression extends CallExpressionBase implements Deopti
 			isCalleeOfRenderedParent: true,
 			renderedSurroundingElement
 		});
-		if (this.arguments.length > 0) {
-			if (this.arguments[this.arguments.length - 1].included) {
-				for (const arg of this.arguments) {
-					arg.render(code, options);
-				}
-			} else {
-				let lastIncludedIndex = this.arguments.length - 2;
-				while (lastIncludedIndex >= 0 && !this.arguments[lastIncludedIndex].included) {
-					lastIncludedIndex--;
-				}
-				if (lastIncludedIndex >= 0) {
-					for (let index = 0; index <= lastIncludedIndex; index++) {
-						this.arguments[index].render(code, options);
-					}
-					code.remove(
-						findFirstOccurrenceOutsideComment(
-							code.original,
-							',',
-							this.arguments[lastIncludedIndex].end
-						),
-						this.end - 1
-					);
-				} else {
-					code.remove(
-						findFirstOccurrenceOutsideComment(code.original, '(', this.callee.end) + 1,
-						this.end - 1
-					);
-				}
-			}
-		}
+		renderCallArguments(code, options, this);
 	}
 
 	protected applyDeoptimizations(): void {

src/ast/nodes/LogicalExpression.ts

@@ -42,13 +42,16 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
 	private usedBranch: ExpressionNode | null = null;
 
 	deoptimizeCache(): void {
-		if (this.usedBranch !== null) {
+		if (this.usedBranch) {
 			const unusedBranch = this.usedBranch === this.left ? this.right : this.left;
 			this.usedBranch = null;
 			unusedBranch.deoptimizePath(UNKNOWN_PATH);
 			for (const expression of this.expressionsToBeDeoptimized) {
 				expression.deoptimizeCache();
 			}
+			// Request another pass because we need to ensure "include" runs again if
+			// it is rendered
+			this.context.requestTreeshakingPass();
 		}
 	}
 

src/ast/nodes/NewExpression.ts

@@ -1,4 +1,7 @@
+import MagicString from 'magic-string';
 import type { NormalizedTreeshakingOptions } from '../../rollup/types';
+import { renderCallArguments } from '../../utils/renderCallArguments';
+import { RenderOptions } from '../../utils/renderHelpers';
 import type { CallOptions } from '../CallOptions';
 import type { HasEffectsContext } from '../ExecutionContext';
 import { InclusionContext } from '../ExecutionContext';
@@ -54,6 +57,11 @@ export default class NewExpression extends NodeBase {
 		};
 	}
 
+	render(code: MagicString, options: RenderOptions) {
+		this.callee.render(code, options);
+		renderCallArguments(code, options, this);
+	}
+
 	protected applyDeoptimizations(): void {
 		this.deoptimized = true;
 		for (const argument of this.arguments) {

src/utils/renderCallArguments.ts

@@ -0,0 +1,41 @@
+import MagicString from 'magic-string';
+import CallExpression from '../ast/nodes/CallExpression';
+import NewExpression from '../ast/nodes/NewExpression';
+import { findFirstOccurrenceOutsideComment, RenderOptions } from './renderHelpers';
+
+export function renderCallArguments(
+	code: MagicString,
+	options: RenderOptions,
+	node: CallExpression | NewExpression
+): void {
+	if (node.arguments.length > 0) {
+		if (node.arguments[node.arguments.length - 1].included) {
+			for (const arg of node.arguments) {
+				arg.render(code, options);
+			}
+		} else {
+			let lastIncludedIndex = node.arguments.length - 2;
+			while (lastIncludedIndex >= 0 && !node.arguments[lastIncludedIndex].included) {
+				lastIncludedIndex--;
+			}
+			if (lastIncludedIndex >= 0) {
+				for (let index = 0; index <= lastIncludedIndex; index++) {
+					node.arguments[index].render(code, options);
+				}
+				code.remove(
+					findFirstOccurrenceOutsideComment(
+						code.original,
+						',',
+						node.arguments[lastIncludedIndex].end
+					),
+					node.end - 1
+				);
+			} else {
+				code.remove(
+					findFirstOccurrenceOutsideComment(code.original, '(', node.callee.end) + 1,
+					node.end - 1
+				);
+			}
+		}
+	}
+}

test/form/samples/side-effect-es5-classes/_expected.js

@@ -22,7 +22,7 @@ var Arrow = ( x ) => {
 console.log( 'before' );
 new Bar(5);
 new Baz(5);
-new Qux(5);
+new Qux();
 Corge(5);
 new Arrow(5);
 

test/function/samples/unused-constructor-argument-fallback/_config.js

@@ -0,0 +1,8 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'handles unused logical expressions as constructor arguments (#4517)',
+	exports({ create }) {
+		assert.strictEqual(create().foo, 'foo');
+	}
+};

test/function/samples/unused-constructor-argument-fallback/main.js

@@ -0,0 +1,7 @@
+function Note() {
+	this.foo = 'foo';
+}
+
+export function create(data) {
+	return new Note(data || {});
+}