Binary operators starting on new lines

[edemaine]

I think this is a bug:

extra.split(/\s+/).filter &
++ [state.start, state.goal]

Expected compilation:

extra.split(/\s+/).filter($ => $)
.concat([state.start, state.goal])

Actual compilation:

extra.split(/\s+/).filter($ =>
  $.concat([state.start, state.goal])
);

[STRd6]

Related #1280

[edemaine]

I notice that the same issue occurs without & altogether:

extra.split(/\s+/).filter x
++ [state.start, state.goal]

The real issue is that we handle binary operators that start on the next line, and then there's a question of which expression to attach that to.

I guess our intuition is that the implicit function call (on filter) should end by the newline, but that's not how things work currently.

[edemaine]

Inspired by the bulleted arrays PR (#1361), I have an idea for how to address this: when entering ImplicitArguments, push an artificial indentation of the current indentation + 1, so that to remain inside the function call argument, you need to be strictly indented. This would mean:

filter foo
+ rest
↓↓↓
filter(foo)
+ rest

// but
filter foo
  + rest
↓↓↓
filter(foo
  + rest)

The bad news is that chaining multiple implicit function calls on the same line would require additional indentation, like so:

f1 f2 f3 f4 f5 x
     + y
// ^needs 5 spaces because 5 function calls

I don't think that's good, but I'm also not quite sure how to fix it.

Maybe instead of increasing the indentation by 1, we add a boolean flag to the indentation saying "> i" where i is the current indentation, instead of the usual meaning of ">= i". And then we won't repeatedly add 1; instead we just repeatedly change the flag to ">" which has no effect beyond the first. Not the simplest solution, but maybe workable...

Let me know if you have better ideas!

[edemaine]

I reviewed this idea some more, and realize that we have almost this exact functionality, via ForbidNewlineBinaryOp (forbid newline followed by nested or indented binary op). If I just add this to ImplicitArguments, then both

extra.split(/\s+/).filter &
++ [state.start, state.goal]

extra.split(/\s+/).filter &
  ++ [state.start, state.goal]

parse as

extra
  .split(/\s+/)
  .filter(($) => $)
  .concat([state.start, state.goal]);

And no existing tests fail.

The question is whether we want this behavior, or whether we'd like the latter example to bring the concat inside the argument to filter. If the latter, it's tempting to redefine ForbidNewlineBinaryOp to mean forbid newline followed by nested binary op, but still allow binary ops that are indented further. Unfortunately, we need to forbid the latter in examples like this:

switch x
  > 0  // condition, not x > 0

So if we want these to parse differently, we could add ForbidNestedBinaryOp. Pretty similar to the > idea above, but just for binary operators. (And maybe it makes sense to use for other things too...)

---

I tried looking at how postfix accesses behave for comparison. This one makes sense:

extra.split(/\s+/).filter &
.sort()
---
extra
  .split(/\s+/)
  .filter(($) => $)
  .sort();

But this one does not:

extra.split(/\s+/).filter &
  .sort()
---
($) => extra.split(/\s+/).filter & $.sort();

Presumably that's a bug, though, and & needs to be treated like a placeholder when it's followed by an access strictly indented on the next line instead of immediately after. Presumably we should match this behavior for binary operators too? Or at least be consistent one way or the other.