Improve lambda formatting

[novalis]

#19

The style guide says:
"A line is never broken adjacent to the arrow in a lambda, except that
a break may come immediately after the arrow if the body of the lambda
consists of a single unbraced expression."

There are two changes here:

1. Don't put a newline after the arrow.
2. When the only argument to a function is a lambda, don't put
   a newline after the open-paren of the function. I think
   this newline was getting added because a lambda is a single
   expression that is longer than (the remainder of) a line. But
   generally, it's prettier to break inside the lambda.

[JakeWharton]

These indents are now wrong.

[jbduncan]

I agree with Jake. IMO, entrySet() and all the lines afterwards should be indented +4, rather than, say, +12, as is the case currently.

[novalis]

Fixed, I think. Thanks.

[jbduncan]

IMO, lines like this would look better with a break after the arrow ->, but no break before the arrow parameter list. So something like this:

...
.flatMap(m ->
    m.getFieldValues()
        .stream()
        .filter(...)
        ...);

[novalis]

My colleagues (who use a lot of lambdas) vastly prefer having fewer line-breaks. I'm presently trying to sell them on this tool, and I think they will have a hard time buying with the extra newline. (Of course I could maintain a fork, but for a formatting tool, it's better to stay as close to upstream as possible, since it's a matter of industry-wide standardization).

[jbduncan]

That's fair enough.

The reason I suggested this change is because, when I last used Ruby about 3 years ago, I seem to remember that the preferred formatting style of lambda expressions, when used with Ruby's functional equivalent of for-loops, followed this style. And I just happened to remember that style recently, and thought to myself that it introduces a sort of visual break that makes things a bit easier to scan IMO, compared to your current proposal of having more on the same line but consequently more tightly-packed and IMO more difficult to read.

I hope that this clarifies my viewpoint, and that perhaps it convinces you and your colleagues to consider this option. :)

[jbduncan]

@novalis ^

[novalis]

I'll run it by them again.

[cushon]

This change causes the formatter to never break in these places, even if breaks are necessary to prevent >100 character lines, e.g.:

class T {
  {
    ffffffffffffffffffffffffffffffffffffffffffffffffff(
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -> {});
  }
}

[novalis]

Well, I can definitely see why this issue has been open for a while :). Will try again.

[novalis]

OK, so I hacked together something that works for @cushon's f...(x -> ...) case. But I'm deeply unhappy with it, because (a) it doesn't yet work if you put another pair of parens around the lambda, although I think I can fix that with yet more code, and (b) more importantly, because it doesn't look like any of the other code in the formatter.

My idea was: if you've got a lambda that's an argument to a function, figure out how much space you have on the right to decide whether to break. To do this, I look back in the positionTokenMap until I find the open-paren for the function, and check its column in the positionToColumn map. Then I look at the formatted length of the argument to the lambda, and compare all that to 100; if it's greater, then we add a breakToFill(). Else, we don't.

This does preserve my result for all of the test cases that I modified, as well as a few that I added for @cushon's case and a few related cases (except, as noted above, the case where the lambda is for
some reason wrapped in parentheses).

One interesting example is

    ffffffffffffffffffffffffffffffffffffffffffffffffff(
        (morxxxxxxxxxxxxxxxx, fleeeeeeeeeeeeeeem) -> {});

There is enough horizontal space to put the first arg to the lamba on the line with the enclosing function call, but that would be much uglier wrapping before the args. (Notes that this problem comes up regardless of whether we take @jbduncan's suggestion about wrapping after arrows).

Anyway, my hack is probably not how this code is intended to work. What I imagine is something higher-level, like attaching a "priority" to each break, where the higher-priority breaks are taken before the lower-priority ones. So in f((a, b) -> c.d) the break after f( would be higher-priority than the break after the comma, and the break after the dot would be even higher-priority. If the break after the dot doesn't fix the problem, we add one after f(, and then finally we break the arg list. But this seems like a lot of general-purpose machinery for a rare edge case (and I don't know what it would do to the rest of the formatting code).

So I wonder if it is better or just do the hack?

[MrIvanPlays]

Any progress on this?

[novalis]

Not on my end -- it seems really hard to do right, and I haven't had time to look at it lately. Sorry.

[cushon]

What I imagine is something higher-level, like attaching a "priority" to each break, where the higher-priority breaks are taken before the lower-priority ones

This is roughly what clang-format does, e.g.: https://youtu.be/s7JmdCfI__c?t=649

That approach has merit, and it was considered when we settled on the break-at-the-highest-syntactic-level approach used by google-java-format. I don't see a good way to adopt pieces of the clang-format approach to make a targeted change to lambda formatting. Switching the entire formatting algorithm to something closer to clang-format would be an invasive change, and come with trade-offs.