From 376006d700a7ef1812787e407f70f44720b6e6bb Mon Sep 17 00:00:00 2001 From: Chris Morgan Date: Thu, 10 Jan 2019 12:51:04 +1100 Subject: [PATCH] Parenthesize a first spread element MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In most cases this is unnecessary, but just occasionally it is necessary, notably in ternaries; for `a ? b : c.concat(…)` is very different from `(a ? b : c).concat(…)`. We could be cleverer about whether parentheses are needed, but Bublé doesn’t generally seem to go out of its way to *absolutely* minimise such things, and I’m also not sure whether there’s a good way to do that, or whether you have to start doing checks “is it a ParenthesizedExpression, CallExpression, NewExpression, &c.” With the opening parenthesis leaked into CallExpression.js, it’d become messier, too. 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. --- src/program/types/CallExpression.js | 6 ++-- src/utils/spread.js | 9 ++++-- .../classes-no-named-function-expressions.js | 2 +- test/samples/classes.js | 2 +- test/samples/spread-operator.js | 28 ++++++++++++------- 5 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/program/types/CallExpression.js b/src/program/types/CallExpression.js index 3671f526..412fe0ce 100644 --- a/src/program/types/CallExpression.js +++ b/src/program/types/CallExpression.js @@ -72,7 +72,9 @@ export default class CallExpression extends Node { _super.noCall = true; // bit hacky... if (this.arguments.length > 1) { - if (firstArgument.type !== 'SpreadElement') { + if (firstArgument.type === 'SpreadElement') { + code.prependRight(firstArgument.start, `( `); + } else { code.prependRight(firstArgument.start, `[ `); } @@ -85,7 +87,7 @@ export default class CallExpression extends Node { code.prependRight(firstArgument.start, `${context}, `); } else { if (firstArgument.type === 'SpreadElement') { - code.appendLeft(firstArgument.start, `${context}, `); + code.appendLeft(firstArgument.start, `${context}, ( `); } else { code.appendLeft(firstArgument.start, `${context}, [ `); } diff --git a/src/utils/spread.js b/src/utils/spread.js index ce2a10cc..184c7ccf 100644 --- a/src/utils/spread.js +++ b/src/utils/spread.js @@ -47,8 +47,13 @@ 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 parenthesize it to handle ternaries like [...a ? b : c]. + // (We could be smarter about it, but with the parenthesizing having + // spread to CallExpression.js, it sounds too much like hard work.) + if (start !== element.start) { + code.overwrite(start, element.start, '( '); + } // else CallExpression called us, and it puts the ( in itself. + code.overwrite(element.end, elements[1].start, ' ).concat( '); } else { code.overwrite(previousElement.end, element.start, ' ].concat( '); } diff --git a/test/samples/classes-no-named-function-expressions.js b/test/samples/classes-no-named-function-expressions.js index 0cc7fd3d..8a3a9e01 100644 --- a/test/samples/classes-no-named-function-expressions.js +++ b/test/samples/classes-no-named-function-expressions.js @@ -221,7 +221,7 @@ module.exports = [ var y = [], len = arguments.length - 1; while ( len-- > 0 ) y[ len ] = arguments[ len + 1 ]; - Bar.prototype.qux.apply(this, x.concat( y )); + Bar.prototype.qux.apply(this, ( x ).concat( y )); }; Foo.prototype.fob = function ( x, y ) { var this$1 = this; diff --git a/test/samples/classes.js b/test/samples/classes.js index 9fe7b314..d882790d 100644 --- a/test/samples/classes.js +++ b/test/samples/classes.js @@ -213,7 +213,7 @@ module.exports = [ var y = [], len = arguments.length - 1; while ( len-- > 0 ) y[ len ] = arguments[ len + 1 ]; - Bar.prototype.qux.apply(this, x.concat( y )); + Bar.prototype.qux.apply(this, ( x ).concat( y )); }; Foo.prototype.fob = function fob ( x, y ) { var this$1 = this; diff --git a/test/samples/spread-operator.js b/test/samples/spread-operator.js index 6cda801a..598432cb 100644 --- a/test/samples/spread-operator.js +++ b/test/samples/spread-operator.js @@ -302,27 +302,27 @@ module.exports = [ { description: 'transpiles multiple spread operators in an array', input: `var arr = [ ...a, ...b, ...c ];`, - output: `var arr = a.concat( b, c );` + output: `var arr = ( a ).concat( b, c );` }, { description: 'transpiles multiple spread operators in an array with trailing comma', input: `var arr = [ ...a, ...b, ...c, ];`, - output: `var arr = a.concat( b, c );` + output: `var arr = ( a ).concat( b, c );` }, { description: 'transpiles mixture of spread and non-spread elements', input: `var arr = [ ...a, b, ...c, d ];`, - output: `var arr = a.concat( [b], c, [d] );` + output: `var arr = ( a ).concat( [b], c, [d] );` }, { description: 'transpiles mixture of spread and non-spread elements in array with trailing comma', input: `var arr = [ ...a, b, ...c, d, ];`, - output: `var arr = a.concat( [b], c, [d] );` + output: `var arr = ( a ).concat( [b], c, [d] );` }, { @@ -385,14 +385,14 @@ module.exports = [ { description: 'transpiles multiple spread operators in function call', input: `var max = Math.max( ...theseValues, ...thoseValues );`, - output: `var max = Math.max.apply( Math, theseValues.concat( thoseValues ) );` + output: `var max = Math.max.apply( Math, ( theseValues ).concat( thoseValues ) );` }, { description: 'transpiles mixture of spread and non-spread operators in function call', input: `var max = Math.max( ...a, b, ...c, d );`, - output: `var max = Math.max.apply( Math, a.concat( [b], c, [d] ) );` + output: `var max = Math.max.apply( Math, ( a ).concat( [b], c, [d] ) );` }, { @@ -597,9 +597,9 @@ module.exports = [ `, output: ` - f(b).concat( ["n"]); - f(b).concat( ['n']); - f(b).concat( ["n"]); + ( f(b) ).concat( ["n"]); + ( f(b) ).concat( ['n']); + ( f(b) ).concat( ["n"]); ` }, @@ -608,7 +608,7 @@ module.exports = [ input: `[...A, () => 'B']`, - output: `A.concat( [function () { return 'B'; }])` + output: `( A ).concat( [function () { return 'B'; }])` }, { @@ -617,5 +617,13 @@ module.exports = [ input: `new X(...A, () => 'B')`, output: `new (Function.prototype.bind.apply( X, [ null ].concat( A, [function () { return 'B'; }]) ))` + }, + + { + description: 'transpiles a first spread element comprising a ternary operator', + + input: '[...a ? b : c, d]', + + output: '( a ? b : c ).concat( [d])', } ];