[CS2] Fix #4487: Outdentation bug

[helixbass]

Fixes #4487

As described there, not sure if this fix could break enough existing code that it should be held back to 2.0?

[GeoffreyBooth]

I think we should probably only do this in 2, in case it introduces a breaking change we’re unaware of. Can you re-target the PR against the 2 branch?

Thanks for looking into this!

[helixbass]

@GeoffreyBooth sure, re-targeted against 2 branch. After merging 2 branch I'm now getting an error when running bin/cake test that I assume is related to my local testing/Node environment (rather than an actual failing test)? Here's part of the error message:

  /Users/jrosse/prj/coffeescript_orig/test/async.coffee:4
  test = async function(description, fn) {
               ^^^^^^^^
SyntaxError: Unexpected token function

[GeoffreyBooth]

You need to upgrade your Node. The minimum required version now for running the tests is 7.6, which has support for asynchronous functions without a flag.

[helixbass]

@GeoffreyBooth thanks, upgraded Node and tests passing

Let me know if you'd prefer that I get rid of the merge commit and condense this to a single commit by eg rebasing

[GeoffreyBooth]

We’ll squash it when merging it in, so there’s no need to rebase.

@lydell do you have any notes about this? Seems okay to me.

[lydell]

At a glance everything looks very convincing. But I’ve never understood how indentation works in CoffeeScript so I can’t say for sure. Merge if you feel like it :)

[xixixao]

Imho this should not be allowed. The example in the original issue should also throw syntax error. Instead of fixing the issue we broke the correct behavior.

In general, to keep programmers sane, we should require that code on the same AST level should be indented equally. Consider that

f()
  g()

is disallowed.

[GeoffreyBooth]

This goes back to why I was arguing that chaining should not be indented:

# This:
foo()
.bar()
.baz()

# Not this:
foo()
  .bar()
  .baz()

The indented version, in my opinion, is taking advantage of a too-forgiving compiler. Logically an indentation means “new block”, and this is all one expression, certainly not a new block.

I wouldn’t be opposed to forcing chaining to be unindented in 2, especially if doing so allows us to close some of the still-open chaining bugs.

[xixixao]

This PR and issue are completely unrelated to chaining, both syntactically and implementation-wise.

Syntactically it is reasonable imho that chained calls are indented, in the same way

f 1,
  2

is indented, because you are continuing an expression, as opposed to sequencing statements. The same goes for

x = a ||
  b

Implementation-wise the compiler treats

foo()
  .bar()
  .baz()

as

foo().bar().baz()