Lexer: Breaking change in multiple row function chaining

[algesten]

#4535 introduced a breaking change in function chaining over multiple rows.

I bisected master and this worked in 1.12.5, but 51c0657 broke it for 1.12.6.

Turns out I rely on it pretty heavily to avoid christmas trees.

test "Function chaining on separate rows", (cb) ->
  func = -> Promise.resolve().then ->
      true
  .then cb

  func()

[GeoffreyBooth]

Well we’re not simply going to revert that bugfix, so the question is whether your specific case can be accommodated. Can you please post some example code that broke as a result of that change?

[algesten]

Our internal code is mostly coffeescript, and we've encountered this problem at least two places there.

Here's an external example https://github.com/algesten/ductile/blob/master/src/ductile.coffee#L31

[algesten]

Is it fair to call that commit a bug fix? It's changing the interpretation.

Obviously stricken up the language is fine, but maybe not in a patch release given that 2.x is around the corner?

[jashkenas]

Here's the minimal case that is now broken, with unexpected .:

->
  a
.b

Note that with .b indented to the level of a, or anywhere further in, in compiles as an immediate access against a

->
  a
  .b

It seems like the first case ought to compile? And close the function body and then call against it?

It's not great CoffeeScript style, but...

[algesten]

So. Any chance this is getting fixed?

[GeoffreyBooth]

You’re welcome to submit a bugfix. Or just use parentheses:

test "Function chaining on separate rows", (cb) ->
  func = (-> Promise.resolve().then ->
      true
  ).then cb

  func()

[algesten]

I mean the easiest would be to back out that breaking commit, but I guess there's a case there that you want to defend against. Where do you suggest I start trying to fix this?

[GeoffreyBooth]

Look in lexer.coffee and rewriter.coffee for code related to chaining or unclosed lines.

[helixbass]

@algesten @GeoffreyBooth I looked at this a bit

@jashkenas it doesn’t look like your example has ever compiled:

->
  a
.b

but I agree that it seems like it should (as (-> a).b). By adding a grammar rule SimpleAssignable: Code Accessor it seems to start to work (and not break any other tests). The most sensible use case for this I can think of is eg

->
  a
.call(...)

I don’t think that case is particularly related to the issue raised here in terms of implementation, but it seems to me that it is in terms of expected meaning. If @jashkenas’ example becomes allowed, then the previous parsing/what @algesten wants it to return to seems even weirder. I guessed wrong when I looked at his example. And @GeoffreyBooth you did too, your suggested parenthesization isn’t what it used to compile to, it was actually:

test "Function chaining on separate rows", (cb) ->
  func = (-> Promise.resolve().then( ->
      true
  ).then cb)

  func()

But now with the added grammar rule, it would compile to what I (and @GeoffreyBooth) expected (rather than not compile, as it does currently, or compile to what it used to in 1.12.5). This seems consistent with allowing @jashkenas’ example and with at least my own intuitive sense of chaining - that a chained call should be chained at the level that it’s visually indented at

So @algesten I know it sucks that this breaking change was inadvertently introduced in a patch release but rather than restoring its behavior, I’m recommending adding that new grammar rule so that it parses in accordance with others’ expectations and with @jashkenas’ example (which would start working)

What do you mean by avoiding Christmas trees? The way I’d indent your example would be like:

test "Function chaining on separate rows", (cb) ->
  func = ->
    Promise
    .resolve()
    .then ->
      true
    .then cb

  func()

or you could do:

test "Function chaining on separate rows", (cb) ->
  func = ->
    Promise.resolve()
    .then -> true
    .then cb

  func()

[algesten]

@helixbass on some level I agree with your reasoning and some not. Maybe it is more clear if I remove the first function.

func = Promise.resolve(true).then(something).then(another) ->
  true
.then ->
  false

compiles to (both previously and currently)

func = Promise.resolve(true).then(something).then(another)(function() {
  return true;
}).then(function() {
  return false;
});

Which is hard to argue against, what else could it do?

For me it then follows by analogy that:

func = -> Promise.resolve(true).then(something).then(another) ->
  true
.then ->
  false

compiles to (the previous behaviour)

func = function() {
  return Promise.resolve(true).then(something).then(another)(function() {
    return true;
  }).then(function() {
    return false;
  });
};

I.e. the previous behaviour.

What do you mean by avoiding Christmas trees? The way I'd indent your example would be like:

Your examples introduce one more level of indentation I previously didn't need. So the solution is more christmas trees.

In the spirit of semver and 2.0.0 around the corner, wouldn't it be better to restore the previous behaviour for 1.x.x and not have this break in patch releases?

[algesten]

I think the minimal cases that we're debating are

a ->
  42
.b

vs

-> a ->
  42
.b

[GeoffreyBooth]

@helixbass Do you mind opening a PR with this new grammar rule? Seems like something we should try to slip in before 2.0.0.

@algesten I’m sorry about the inadvertent breaking change, but we can’t move backward. It wouldn’t make much sense to revert this in a potential 1.12.8, just for the 1.12.6-7 behavior to come back again in 2.0.0. The 1.x versions are closed to new development, and probably won’t get any more releases after 2.0.0 comes out, which is imminent. If you don’t wish to adjust your code for this change, is there a reason you can’t just stay at 1.12.5 indefinitely?

[GeoffreyBooth]

See #4665. That PR makes the examples in @helixbass’ comment work.

@algesten Looking at your final example:

func = -> Promise.resolve(true).then(something).then(another) ->
  true
.then ->
  false

You want this output:

func = function() {
  return Promise.resolve(true).then(something).then(another)(function() {
    return true;
  }).then(function() {
    return false;
  });
};

but (as of #4665, at least) the compiler is generating this output:

func = (function() {
  return Promise.resolve(true).then(something).then(another)(function() {
    return true;
  });
}).then(function() {
  return false;
});

I know it’s frustrating that the output changed from 1.12.5 to 1.12.6, but the first output was simply incorrect. If you look at the CoffeeScript in the top code block, the last then is at the same indentation level as func, so it should be chaining off func, as it does in the current output.

If you want to chain it off Promise instead, you need to line up the indentation that way:

func = ->
  Promise.resolve(true).then(something).then(another) ->
    true
  .then ->
    false

---

func = function() {
  return Promise.resolve(true).then(something).then(another)(function() {
    return true;
  }).then(function() {
    return false;
  });
};

Or if you’re determined to not increase your indentation at all, use parentheses to ensure that the final then attaches itself to Promise instead of func:

func = -> Promise.resolve(true).then(something).then(another)( ->
  true
).then ->
  false
# (same output as previous)

Closing as there isn’t really a bug here to fix. #4665 adds new functionality.

[algesten]

Oh well. It introduces strange inconsistencies, but I don't have to like it. Life goes on.