[CS2] Fix #3199: throw multiline implicit object

[helixbass]

Fixes #3199

The reason the kind of implicit object "splitting" described in #3199 happens is b/c the suppression of the INDENT following throw doesn't play nicely with the rewriter (normalizeLines() specifically). So to me it seems cleaner to preserve the INDENT and just add grammar rule THROW INDENT Expression OUTDENT

@GeoffreyBooth this relates to #1263 and your comment there - as you found, throw with implicit object worked b/c of INDENT/TERMINATOR suppression (via unfinished()). But then #3199 shows that it doesn't always work. And the same breakage applies to how you implemented returning implicit objects in #4532, ie this breaks:

if 1 then return
  a: 1
  b: 2

So I applied the same idea here to return and replaced your INDENT-suppressing implementation in #4532 with a grammar rule RETURN INDENT Expression OUTDENT which un-breaks the above example

In a more general way, #3199 and #1263 seem to reflect our feeling that we should be able to pass indented implicit object literals to function-call-ish things like throw and return the way that we can pass them to an actual function call. I definitely feel that way and am glad that #4532 was added. This PR just changes the way that it's supported to avoid bugs like #3199, I'm unaware if there's any additional downside to doing it via grammar rules allowing INDENT...OUTDENT variations

But then it does feel a little weird that both how this PR supports it and the existing behavior for throw and for return (since #4532) allow anything to be passed on following lines, not just implicit objects (which is what I think our brains tell us we should be able to do). Eg this has been legal:

throw
  'abc'

And as of #4532, you can do eg:

return
  x

I don't have a big problem with either of these but it does seem akin to how @jashkenas apparently doesn't like the idea of this being legal:

fn
  a

Because basically all three of these (throw, return, and actual function calls) all "feel" like function calls, so it would make sense that they follow similar rules

There's some interesting related discussion in #3098 and #1263 - this comment warns against allowing implicit indented objects with return and mentions throw, but given that the way it's supported for function calls is different from both existing support for throw (which the commenter @satyr added)/return (as of #4532) and from how this PR supports it, I'm not inclined to heed it

Put list of tokens in UNFINISHED per @jashkenas' request

[GeoffreyBooth]

@helixbass Thank you for tackling this knotty, difficult bug! I agree with everything you’ve written above. Yes, functions, throw and return should behave similarly or identically.

If I had to guess, I suppose that @jashkenas’ objection to

fn
  arg

Is more that he doesn’t want people to get in the habit of putting each argument on its own line, e.g.

fn
  arg1
  arg2
  arg3

and then we lose the idea of the indentation signifying a new block, such as an object. So if we can fix #3199 and get these three calling tokens to behave similarly, and still avoid allowing a confusing syntax that would lead to lots of ambiguity problems, that sounds like an ideal solution. Does this PR do that?

I’m totally fine with disallowing

return
  x

but I wonder if we should make that change on master too, since I opened the floodgates for such nonsense with 1.12.6 and we could close that door in 1.12.7 before people notice and start to take advantage of it (and then yell when it doesn’t work in 2). We have to release a 1.12.7 anyway to fix the #4568 regression, we could include this PR in 1.12.7 too to fix the above “regression.”

The PR itself looks good to me. @lydell?

[lydell]

I have no opinion. I guess it's fine, but I'm not really sure what this is all about anyway.

[GeoffreyBooth]

@helixbass can you please retarget this to master, or open a separate PR for master? I’m thinking that this should be one of the few fixes we make for the 1.x branch, because I want to fix the regression introduced in 1.12.6; and because the object splitting of #3199 is a pretty nasty bug that really deserves fixing even on 1.x.

[helixbass]

@GeoffreyBooth retargeted to master. This seems safe with regards to return (since the return support was just introduced) so I'd only worry slightly that this introduces incompatibilities with existing throw syntax (see below for an example)

As far as only allowing implicit objects/trying to align with function-call syntax, a couple thoughts:

• If we did want to restrict throw/return this way, the way I'd think to do it would be to change the new grammar rules to THROW INDENT Object OUTDENT and RETURN INDENT Object OUTDENT. But this wouldn't distinguish between implicit and explicit objects and I don't see a super clean way to make that distinction. So there'd still be a difference between throw/return vs function call allowed syntax
• there are some times when I'd personally enjoy being able to pass a non-Object as an indented arg to return or throw (without needing parens). Eg a JSX element. Really any complicated expression that may have nested sub-expressions/blocks of its own. But I'd similarly like to be able to do that for function calls and am not really arguing that that should be supported
• you make an important distinction that function calls accept multiple (indented) arguments - so I looked at throw/return in this light, it's a weird consequence of using INDENT-suppression/unfinished() that currently this:

throw
  a
  b

compiles to:

throw a;

b;

and similarly for return since 1.12.6. With this PR the above (with throw or return) would now fail (as I think it should - but this is an example of a breaking change that this would introduce into master)

• so considering the above points, I'd personally lean towards leaving this PR as is (thus allowing any expression to be an indented arg to throw/return) but am happy to try the above grammar-based restriction to Object (and/or look for a somewhat clean way to restrict that to just implicit objects) if you think it's more important to try and keep parity with allowed function-call syntax?

So if we can fix #3199 and get these three calling tokens to behave similarly, and still avoid allowing a confusing syntax that would lead to lots of ambiguity problems, that sounds like an ideal solution. Does this PR do that?

As far as I'm aware, yes, this PR won't introduce ambiguity and using a grammar-based solution avoids some of these problems (like #3199) that using INDENT-suppression/unfinished() can cause. The above are my thoughts about whether/how to get the three to behave similarly, possible breakages of existing syntax, and how that relates to whether (part/all) of this should be targeted against master (basically it makes me a little nervous but I understand the thinking that the object-splitting bug is worth fixing on master)

[helixbass]

@GeoffreyBooth just played around with postfix if/for as a possible source of weirdness and that has changed my opinion (sorry for the quick about-face). Consider:

Currently (on master) this:

return
  a for b in c

compiles to:

for (i = 0, len = c.length; i < len; i++) {
  b = c[i];
  return a;
}

ie the same as return a for b in c. Not intuitive. In this case this PR compiles to what I'd expect:

results = [];
for (i = 0, len = c.length; i < len; i++) {
  b = c[i];
  results.push(a);
}
return results;

But what about postfix if? Both currently and with this PR, this:

return
  a if b

compiles to the unintuitive

if (b) {
  return a;
}

And with throw this PR will currently generate invalid JS:

throw
  a if b

compiles to

throw if (b) {
  a;
}

and similarly with postfix for. I'm guessing that could be corrected by changing level or something? But it still seems ambiguous in this case what it should compile to

So these kinds of cases have made me think it's actually a really good idea to restrict it to just Objects (as indented args to return/throw). I'm updating this PR to use the grammar-based restriction to Object - all the existing tests (which just tested the intuitive case of an implicit object) pass. And all of the above postfix if/for examples fail to compile. As would things like

return
  a

But at that point I think it needs to be targeted against 2 since the potential breakage of existing throw code is much higher (people could have been throwing any old indented argument). I don't see a way (at least based on this approach) to safely fix the object-splitting #3199 bug without introducing some potential existing throw breakage, so as much as the object-splitting bug is bad I think it's best to only fix it (with the additional Object restriction) on 2

However, there's no reason that I can see not to go ahead and make the return changes on master since it was just introduced and as you say people could start using the looser syntax and then complain about it not being available on 2

So I'll go ahead and retarget this back to 2 with the restriction to just Object. And I'll open a separate PR against master for just the return part

At that point, here's my assessment of parity with function-call syntax:

• throw and return would both accept a non-implicit indented object literal so eg:

return
  {a: 1}

would work. If it's worth disallowing this, the cleanest way I can think of would be to pass an additional flag to both new Return() and new Throw() indicating that the indent-based rule was used and based on the value of this flag and @expression.generated, throw an error for an indented non-implicit object. A little ugly

• function calls can actually do things like:

f
  a: 1 if b

since the allowance of implicit-object first arguments is token-based (in rewriter.coffee) rather than grammar-based. throw and return wouldn't allow this

[jashkenas]

Just to quickly chime in — yes, I think that @GeoffreyBooth characterized my objection correctly.

throw
  value

and

return
  value

... are both poor CoffeeScript style, and we shouldn't really be encouraging them.

[GeoffreyBooth]

Thanks @helixbass, you’ve put more thought into this than I have. Your recommendation sounds good.

The code looks good too, though I have one suggestion: should we add some tests to confirm the function/throw/return parity (where it exists) and that the “bad” forms are truly disallowed?

[helixbass]

@GeoffreyBooth sure, added a couple tests each for "bad" forms of throw (just to this PR) and return/function-call (both here and #4605)

@jashkenas' and your opinion reinforces that restricting to just allowing indented Objects is the right way to handle this. I think it is fundamentally about having it match our intuition that return/throw should feel like function calls and thus allow implicit object indented args (but disallow other things). I'm not inclined to introduce the ugliness that I think is required to exclude explicit objects (to throw/return) but if anyone thinks it's worth excluding them, I'll do that too

[GeoffreyBooth]

I’m not inclined to introduce the ugliness that I think is required to exclude explicit objects

In other words, excluding this?

return
  {key: 1}

as opposed to implicit (braceless) or the { on the same line (return {)? Yes, I think that’s fine.

This PR looks good to me. @vendethiel or @jashkenas or @lydell or anyone else have any notes?

[vendethiel]

Seems good. I think it's fine not to differentiate between return { vs return\n{

[GeoffreyBooth]

@helixbass do you mind answering my last question, and resolving conflicts? Aside from that I think we’re ready to merge.

[helixbass]

@GeoffreyBooth merged 2 and resolved conflicts. And yes you understood correctly re explicit objects being allowed

[GeoffreyBooth]

Thanks @helixbass!