Array spread breaks ternaries due to not parenthesising them #177
Comments
chris-morgan
added a commit
to chris-morgan/buble
that referenced
this issue
Jan 10, 2019
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 bublejs#177.
chris-morgan
added a commit
to chris-morgan/buble
that referenced
this issue
Jan 23, 2019
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 bublejs#177.
chris-morgan
added a commit
to chris-morgan/buble
that referenced
this issue
Jan 23, 2019
In most cases this is unnecessary, but in some cases 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 bublejs#177.
adrianheine
pushed a commit
that referenced
this issue
Jan 23, 2019
In most cases this is unnecessary, but in some cases 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Minimal test case:
Result:
Expected:
These two are wildly different!
The workaround is manual parenthesisation:
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.)The text was updated successfully, but these errors were encountered: