Prevent ASI errors for conditional expressions (#3035)

* Prevent ASI errors for conditional expressions

* Prevent ASI errors for logical and sequence expressions, also check
for throw statements

src/ast/nodes/ConditionalExpression.ts

@@ -3,6 +3,7 @@ import { BLANK } from '../../utils/blank';
 import {
 	findFirstOccurrenceOutsideComment,
 	NodeRenderOptions,
+	removeLineBreaks,
 	RenderOptions
 } from '../../utils/renderHelpers';
 import { removeAnnotations } from '../../utils/treeshakeNode';
@@ -151,20 +152,20 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
+		{ renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK
 	) {
 		if (!this.test.included) {
 			const colonPos = findFirstOccurrenceOutsideComment(code.original, ':', this.consequent.end);
+			const inclusionStart =
+				(this.consequent.included
+					? findFirstOccurrenceOutsideComment(code.original, '?', this.test.end)
+					: colonPos) + 1;
+			if (preventASI) {
+				removeLineBreaks(code, inclusionStart, (this.usedBranch as ExpressionNode).start);
+			}
+			code.remove(this.start, inclusionStart);
 			if (this.consequent.included) {
-				const questionmarkPos = findFirstOccurrenceOutsideComment(
-					code.original,
-					'?',
-					this.test.end
-				);
-				code.remove(this.start, questionmarkPos + 1);
 				code.remove(colonPos, this.end);
-			} else {
-				code.remove(this.start, colonPos + 1);
 			}
 			removeAnnotations(this, code);
 			(this.usedBranch as ExpressionNode).render(code, options, {

src/ast/nodes/LogicalExpression.ts

@@ -3,6 +3,7 @@ import { BLANK } from '../../utils/blank';
 import {
 	findFirstOccurrenceOutsideComment,
 	NodeRenderOptions,
+	removeLineBreaks,
 	RenderOptions
 } from '../../utils/renderHelpers';
 import { removeAnnotations } from '../../utils/treeshakeNode';
@@ -155,7 +156,7 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
+		{ renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK
 	) {
 		if (!this.left.included || !this.right.included) {
 			const operatorPos = findFirstOccurrenceOutsideComment(
@@ -165,6 +166,9 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable
 			);
 			if (this.right.included) {
 				code.remove(this.start, operatorPos + 2);
+				if (preventASI) {
+					removeLineBreaks(code, operatorPos + 2, this.right.start);
+				}
 			} else {
 				code.remove(operatorPos, this.end);
 			}

src/ast/nodes/ReturnStatement.ts

@@ -22,7 +22,7 @@ export default class ReturnStatement extends StatementBase {
 
 	render(code: MagicString, options: RenderOptions) {
 		if (this.argument) {
-			this.argument.render(code, options);
+			this.argument.render(code, options, { preventASI: true });
 			if (this.argument.start === this.start + 6 /* 'return'.length */) {
 				code.prependLeft(this.start + 6, ' ');
 			}

src/ast/nodes/SequenceExpression.ts

@@ -3,6 +3,7 @@ import { BLANK } from '../../utils/blank';
 import {
 	getCommaSeparatedNodesWithBoundaries,
 	NodeRenderOptions,
+	removeLineBreaks,
 	RenderOptions
 } from '../../utils/renderHelpers';
 import { treeshakeNode } from '../../utils/treeshakeNode';
@@ -81,10 +82,9 @@ export default class SequenceExpression extends NodeBase {
 	render(
 		code: MagicString,
 		options: RenderOptions,
-		{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
+		{ renderedParentType, isCalleeOfRenderedParent, preventASI }: NodeRenderOptions = BLANK
 	) {
-		let firstStart = 0,
-			includedNodes = 0;
+		let includedNodes = 0;
 		for (const { node, start, end } of getCommaSeparatedNodesWithBoundaries(
 			this.expressions,
 			code,
@@ -96,7 +96,9 @@ export default class SequenceExpression extends NodeBase {
 				continue;
 			}
 			includedNodes++;
-			if (firstStart === 0) firstStart = start;
+			if (includedNodes === 1 && preventASI) {
+				removeLineBreaks(code, start, node.start);
+			}
 			if (node === this.expressions[this.expressions.length - 1] && includedNodes === 1) {
 				node.render(code, options, {
 					isCalleeOfRenderedParent: renderedParentType

src/ast/nodes/ThrowStatement.ts

@@ -1,3 +1,5 @@
+import MagicString from 'magic-string';
+import { RenderOptions } from '../../utils/renderHelpers';
 import { ExecutionPathOptions } from '../ExecutionPathOptions';
 import * as NodeType from './NodeType';
 import { ExpressionNode, StatementBase } from './shared/Node';
@@ -9,4 +11,8 @@ export default class ThrowStatement extends StatementBase {
 	hasEffects(_options: ExecutionPathOptions) {
 		return true;
 	}
+
+	render(code: MagicString, options: RenderOptions) {
+		this.argument.render(code, options, { preventASI: true });
+	}
 }

src/utils/renderHelpers.ts

@@ -17,6 +17,7 @@ export interface NodeRenderOptions {
 	isCalleeOfRenderedParent?: boolean;
 	isNoStatement?: boolean;
 	isShorthandProperty?: boolean;
+	preventASI?: boolean;
 	renderedParentType?: string; // also serves as a flag if the rendered parent is different from the actual parent
 	start?: number;
 }
@@ -164,3 +165,15 @@ export function getCommaSeparatedNodesWithBoundaries<N extends Node>(
 	});
 	return splitUpNodes;
 }
+
+export function removeLineBreaks(code: MagicString, start: number, end: number) {
+	let lineBreakPos = start;
+	while (true) {
+		lineBreakPos = findFirstLineBreakOutsideComment(code.original, lineBreakPos);
+		if (lineBreakPos === -1 || lineBreakPos >= end) {
+			break;
+		}
+		code.remove(lineBreakPos, lineBreakPos + 1);
+		lineBreakPos++;
+	}
+}

test/form/samples/keep-tree-shaking-comments-no-asi/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'always keep leading comments when tree-shaking and no automatic semicolons are inserted'
+};

test/form/samples/keep-tree-shaking-comments-no-asi/_expected.js

@@ -0,0 +1,14 @@
+console.log(
+	/* keep me */
+	'expected');
+
+console.log(
+	/* keep me */
+	'expected' );
+
+console.log(
+	/* keep me */
+	'expected');
+
+console.log((/* keep me */
+	'expected'));

test/form/samples/keep-tree-shaking-comments-no-asi/main.js

@@ -0,0 +1,17 @@
+console.log(false ?
+	'unexpected' :
+	/* keep me */
+	'expected');
+
+console.log(true ?
+	/* keep me */
+	'expected' :
+	'unexpected');
+
+console.log(true &&
+	/* keep me */
+	'expected');
+
+console.log((true,
+	/* keep me */
+	'expected'));

test/function/samples/prevent-tree-shaking-asi/_config.js

@@ -0,0 +1,6 @@
+module.exports = {
+	description: 'prevent automatic semicolon insertion from changing behaviour when tree-shaking',
+	options: {
+		treeshake: { tryCatchDeoptimization: false }
+	}
+};

test/function/samples/prevent-tree-shaking-asi/main.js

@@ -0,0 +1,42 @@
+function test1() {
+	return true ?
+		/* kept */
+
+		'expected' :
+		'unexpected';
+}
+assert.strictEqual(test1(), 'expected');
+
+function test2() {
+	return false ?
+		'unexpected' :
+		/* kept */
+
+		'expected';
+}
+assert.strictEqual(test2(), 'expected');
+
+function test3() {
+	return true &&
+		/* kept */
+
+		'expected';
+}
+assert.strictEqual(test3(), 'expected');
+
+function test4() {
+	return 'removed',
+		/* kept */
+
+		'expected';
+}
+assert.strictEqual(test4(), 'expected');
+
+try {
+  throw true ?
+
+    new Error('expected') :
+    null;
+} catch (err) {
+	assert.strictEqual(err.message, 'expected');
+}