Repository: Rich-Harris/buble (js)

Issue:
Array spread breaks ternaries due to not parenthesising them
[**Minimal test case:**](https://buble.surge.sh/#%5B...a%20%3F%20b%20%3A%20c%2C%20d%5D)

```js
[...a ? b : c, d]
```

**Result:**

```js
a ? b : c.concat( [d])
```

**Expected:**

```js
(a ? b : c).concat( [d])
```

These two are *wildly* different!

The **workaround** is manual parenthesisation:

```js
[...(a ? b : c), d]
```

Given that you can only spread iterables in array spread, I think ternaries are the only realistic situation where this will matter. (Well, I guess strings could wreck it too—`...a + b`—but #166 shows spreading strings is broken anyway.)

Pull request:
Parenthesize a first spread element when necessary
In most cases this is unnecessary, but for some expression types it is necessary, notably in ternaries; for `a ? b : c.concat(…)` is very different from `(a ? b : c).concat(…)`.

I’m not clear why the insertion of the opening parenthesis had to happen in src/program/types/CallExpression.js rather than src/utils/spread.js when start === element.start; but if I did it inside spread(), if I used appendLeft or prependLeft it’d end up like `.apply((Math, a).concat(…))` instead of `.apply(Math, (a).concat(…))`, and if I used appendRight or prependRight, nothing happened.

Fixes #177.

Patch:
diff --git a/src/program/types/CallExpression.js b/src/program/types/CallExpression.js
index 1e0eee4..b757697 100644
--- a/src/program/types/CallExpression.js
+++ b/src/program/types/CallExpression.js
@@ -1,5 +1,5 @@
 import Node from '../Node.js';
-import spread, { isArguments, inlineSpreads } from '../../utils/spread.js';
+import spread, { isArguments, inlineSpreads, needsParentheses } from '../../utils/spread.js';
 import removeTrailingComma from '../../utils/removeTrailingComma.js';
 
 export default class CallExpression extends Node {
@@ -77,7 +77,11 @@ export default class CallExpression extends Node {
 					_super.noCall = true; // bit hacky...
 
 					if (this.arguments.length > 1) {
-						if (firstArgument.type !== 'SpreadElement') {
+						if (firstArgument.type === 'SpreadElement') {
+							if (needsParentheses(firstArgument.argument)) {
+								code.prependRight(firstArgument.start, `( `);
+							}
+						} else {
 							code.prependRight(firstArgument.start, `[ `);
 						}
 
@@ -90,7 +94,11 @@ export default class CallExpression extends Node {
 					code.prependRight(firstArgument.start, `${context}, `);
 				} else {
 					if (firstArgument.type === 'SpreadElement') {
-						code.appendLeft(firstArgument.start, `${context}, `);
+						if (needsParentheses(firstArgument.argument)) {
+							code.appendLeft(firstArgument.start, `${context}, ( `);
+						} else {
+							code.appendLeft(firstArgument.start, `${context}, `);
+						}
 					} else {
 						code.appendLeft(firstArgument.start, `${context}, [ `);
 					}
diff --git a/src/utils/spread.js b/src/utils/spread.js
index bfca294..1c13043 100644
--- a/src/utils/spread.js
+++ b/src/utils/spread.js
@@ -1,3 +1,5 @@
+import CompileError from './CompileError.js';
+
 export function isArguments(node) {
 	return node.type === 'Identifier' && node.name === 'arguments';
 }
@@ -55,6 +57,28 @@ export function inlineSpreads(
 	}
 }
 
+// Returns false if it’s safe to simply append a method call to the node,
+// e.g. `a` → `a.concat()`.
+//
+// Returns true if it may not be and so parentheses should be employed,
+// e.g. `a ? b : c` → `a ? b : c.concat()` would be wrong.
+//
+// This test may be overcautious; if desired it can be refined over time.
+export function needsParentheses(node) {
+	switch (node.type) {
+		// Currently whitelisted are all relevant ES5 node types ('Literal' and
+		// 'ObjectExpression' are skipped as irrelevant for array/call spread.)
+		case 'ArrayExpression':
+		case 'CallExpression':
+		case 'Identifier':
+		case 'ParenthesizedExpression':
+		case 'ThisExpression':
+			return false;
+		default:
+			return true;
+	}
+}
+
 export default function spread(
 	code,
 	elements,
@@ -100,8 +124,28 @@ export default function spread(
 	const previousElement = elements[firstSpreadIndex - 1];
 
 	if (!previousElement) {
-		code.remove(start, element.start);
-		code.overwrite(element.end, elements[1].start, '.concat( ');
+		// We may need to parenthesize it to handle ternaries like [...a ? b : c].
+		let addClosingParen;
+		if (start !== element.start) {
+			if ((addClosingParen = needsParentheses(element.argument))) {
+				code.overwrite(start, element.start, '( ');
+			} else {
+				code.remove(start, element.start);
+			}
+		} else if (element.parent.type === 'CallExpression') {
+			// CallExpression inserts `( ` itself, we add the ).
+			// (Yeah, CallExpression did the needsParentheses call already,
+			// but we don’t have its result handy, so do it again. It’s cheap.)
+			addClosingParen = needsParentheses(element.argument);
+		} else {
+			// Should be unreachable, but doing this is more robust.
+			throw new CompileError(
+				'Unsupported spread construct, please raise an issue at https://github.com/Rich-Harris/buble/issues',
+				element
+			);
+		}
+		code.overwrite(element.end, elements[1].start,
+			addClosingParen ? ' ).concat( ' : '.concat( ');
 	} else {
 		code.overwrite(previousElement.end, element.start, ' ].concat( ');
 	}


Test patch:
diff --git a/test/samples/spread-operator.js b/test/samples/spread-operator.js
index 3adb3d8..5748184 100644
--- a/test/samples/spread-operator.js
+++ b/test/samples/spread-operator.js
@@ -882,4 +882,12 @@ module.exports = [
 			new f(w, x, y, z);
 		`
 	},
+
+	{
+		description: 'transpiles a first spread element comprising a ternary operator',
+
+		input: '[...a ? b : c, d]',
+
+		output: '( a ? b : c ).concat( [d])',
+	}
 ];