Respect newlines between 'where' bindings

[mheinzel]

Thanks for merging the PR to preserve the grouping within do-blocks! The main thing of #74 that I'm missing now is about newlines between where bindings.

Is your feature request related to a problem? Please describe.

When defining multiple helper functions in a where clause, Ormolu will turn them into a single block that for me is hard to parse into individual bindings. Others seem to have similar issues and in some cases even added empty comments to visually separate the bindings.

Describe the solution you'd like

There where two approaches already described in #74, I'm fine with either one:

1. Newline between all bindings or none, depending on existing newlines (similar to type signatures)

   within a where block of bindings, they should either all have an intercalated newline or none of them. (source)

   Maybe this can be similar to type signature, where any blank line between two definitions causes this intercalation behaviour? (source)

   Note that this should always remove newlines between multiple clauses of the same definition, as it does for top-level definitions.

2. Individually allow single or no newline between bindings, depending on existing newlines (similar to do-blocks as)

   I'd argue that one might group let bindings by semantics as well [...] and putting newlines everywhere or nowhere might be too extreme? (source)

Describe alternatives you've considered

See above.

Additional context

An example for the two suggestions:

x = y
  where
    y = triangle 10
    triangle n = triangle' n 0

    triangle' 0 acc = acc
    triangle' n acc = triangle' (n - 1) (acc + n)

currently gets turned into

x = y
  where
    y = triangle 10
    triangle n = triangle' n 0
    triangle' 0 acc = acc
    triangle' n acc = triangle' (n - 1) (acc + n)

with proposal 1 will be turned into

x = y
  where
    y = triangle 10

    triangle n = triangle' n 0

    triangle' 0 acc = acc
    triangle' n acc = triangle' (n - 1) (acc + n)

and with proposal 2 will stay as it was.

[mrkkrp]

Should this also apply to let bindings?

[mboes]

Proposal (1) sounds more consistent to me: that way there is no difference between how we treat top-level-bindings and local bindings. Furthermore, it makes sense to allow no blank lines between definitions in let bindings and where blocks when it's a sequence of short one-line definitions (especially in let bindings). It doesn't hurt to have the same rule apply at the top-level as well, even if in practice it won't be used often at top-level because modules nearly never have only single line definitions (if only because of the type signatures). So the spec would be:

Definitions where at least one definition is multi-line are always all separated by a blank line. If all definitions are single-line, then there are no intercalating blank lines between any definitions.

This is in keeping with #74 (comment), which is essentially about keeping decision fatigue to a minimum.

[mheinzel]

It doesn't hurt to have the same rule apply at the top-level as well

Agreed, it could be the same. No strong preference either way from my side.

(edit: I also think the same rule could apply in a let, but it's not as important for me there as I usually only have very short definitions in let anyways)

Definitions where at least one definition is multi-line are always all separated by a blank line.

So instead of separating by blank line if any of the bindings are already separated by a blank line, it happens if any definition is multi-line?

So the bindings in the following example would get separated by empty lines? Are both binding2 and binding3 considered "multi-line definitions" or only the latter?

where
  binding1 = short
  binding2 =
    longerSoWePutItOnASeparateLine
  binding 3 =
    multiline
      expression

I'm not sure I like this conflation of being multi-line and getting separated, but I see how it reduces the number of possible layouts.

[mrkkrp]

Definitions where at least one definition is multi-line are always all separated by a blank line. If all definitions are single-line, then there are no intercalating blank lines between any definitions.

This sounds good, but if we look at actual results, they are not pretty. For example, here is a function from Ormolu's code base:

p_unboxedSum :: BracketStyle -> ConTag -> Arity -> R () -> R ()
p_unboxedSum s tag arity m = do
  let before = tag - 1
      after = arity - before - 1
      args = replicate before Nothing <> [Just m] <> replicate after Nothing
      f (x, i) = do
        let isFirst = i == 0
            isLast = i == arity - 1
        case x :: Maybe (R ()) of
          Nothing ->
            unless (isFirst || isLast) space
          Just m' -> do
            unless isFirst space
            m'
            unless isLast space
  parensHash s $ sep (txt "|") f (zip args [0 ..])

If we apply the logic, we see that the definition f is multiline, so we must insert newlines between each declaration in the let block:

p_unboxedSum :: BracketStyle -> ConTag -> Arity -> R () -> R ()
p_unboxedSum s tag arity m = do
  let before = tag - 1

      after = arity - before - 1

      args = replicate before Nothing <> [Just m] <> replicate after Nothing

      f (x, i) = do
        let isFirst = i == 0
            isLast = i == arity - 1
        case x :: Maybe (R ()) of
          Nothing ->
            unless (isFirst || isLast) space
          Just m' -> do
            unless isFirst space
            m'
            unless isLast space
  parensHash s $ sep (txt "|") f (zip args [0 ..])

I think it is pretty obvious that this is overly spaced without any improvement for readability.

[mrkkrp]

I think that if we go with something like 1, then the heuristic should be more clever. Actually 2 seems not such a bad choice. People will be able to add a bit of spacing exactly when they want it, it doesn't seem like a bad idea, or something that will make style inconsistent in any way.

[mboes]

I think (2) gives too much choice. Ormolu isn't just about consistent style, it's about automating away micro choices that are a chore to deal with. Should I add more space between these two definitions but not elsewhere? Should I do so everywhere? If there are two choices, can I at least flip between them easily with a single change in the input? etc.

Perhaps it would have to have more examples where newlines between bindings is really desirable. Looking again at the ticket description, perhaps a more meaningful discriminator than whether the body is multi-line, is whether there are multiple clauses and/or type signature? So e.g. the original input in @mheinzel's example would remain as-is, and so would @mrkkrp's above?

One thing I want to note about (2) is bindings are not like do-blocks: bindings are sets with no particular order, whereas statements in a do-block are an ordered sequence. It might make sense to group some elements in the sequence.

[aspiwack]

Yet another heuristic could be: is there at least one empty line (in the input source)?

• If there is one empty line, then all definitions are spaced out by one line
• If there is no empty line, then keep it as such.

[mheinzel]

What @aspiwack describes is what I had in mind for option 1, and probably still my preferred option. It seems like a good tradeoff, giving the user the possibility to influence layout (especially since I don't think there is a good and intuitive heuristic), while allowing only one additional binary choice ("separate everything or nothing"). This also seems in line with the goal of "Let some whitespace be programmable".

Another point worth discussing: If we pick some heuristic, it will decide to separate some bindings in code that was formatted with an old existing version of ormolu, potentially introducing many new lines when upgrading. On the other hand, if we base this on the input's formatting, existing code will stay as it is, requiring users to opt into the additional newlines per where binding.

Which of these is more desirable?

[mrkkrp]

I considered 1 in two flavors from the very beginning: one where we insert newlines when we have at least one multiline definition and another one when we have at least one blank line in the original input.

I'm unconvinced by this all or nothing approach though. There could be a bunch of quick one-liners and then one or more longer definitions which should be separated. If we're to come up with a heuristic, I'd just go for separation on per-definition basis. That is, short ones should be lumped together and longer ones should have blank lines around them.

[mrkkrp]

An update. I think now that it is not a good idea to rely on number of lines, because after re-formatting it may change and so there may be problems with idempotence. I checked what we do for type class instances and we just preserve user's choice, i.e. like with expressions in do-blocks.

If we want to be consistent, that's the way to go.

[mrkkrp]

We had a lengthy discussion about this and we can't decide about the best logic for this. I'm going to delay implementation of this feature, which seems much more minor than e.g. blank lines in do-blocks.

[ocharles]

I think it is pretty obvious that this is overly spaced without any improvement for readability.

I don't think this is obvious, that is exactly how we style our code internally.

[neongreen]

I think "If there is one empty line, then all definitions are spaced out by one line. If there is no empty line, then keep it as such." is a fine stop-gap solution and we can seamlessly transition from it to "respect all newlines in where". It's definitely better than "eat all newlines".

[mrkkrp]

@neongreen You've just repeated the option we discussed and rejected without adding any new argumentation.

It's definitely better than "eat all newlines".

This is neither "definite" nor obvious. If it were, the change would have been merged.

[neongreen]

This is neither "definite" nor obvious.

I do think it's definite and obvious.

The fact that I can't justify it in writing doesn't mean it's a useless datapoint — I think it's not. (Of course, I also don't think you /have/ to use it, though.)

[neongreen]

<cranky>

But if you want an argument, here you go:

I'm unconvinced by this all or nothing approach though.

I don't think it's As Good As It Gets, just a fine stop-gap solution.

There could be a bunch of quick one-liners and then one or more longer definitions which should be separated.

Yes, so we agree it's not perfect. I claim that "remove all newlines, always" has even more drawbacks. Right now "remove all newlines, always" already serves as a stop-gap solution, and I think that "either remove all newlines or add all newlines" is strictly better because it permits the current behavior while also allowing a different, often more desirable one. It also doesn't cause any formatting changes for people already using Ormolu.

If we're to come up with a heuristic, I'd just go for separation on per-definition basis. That is, short ones should be lumped together and longer ones should have blank lines around them.

Later you said you don't want that anymore, so I dismissed that part. (I also don't think it's a good idea.)

[neongreen]

My claim in a nutshell is that "either no newlines or all newlines, with the choice being left to the user" is strictly better than "always no newlines", and I don't see this claim being discussed and rejected earlier. If it was, I missed it.

The claim that "either no newlines or all newlines IS THE BEST SOLUTION PERIOD" is stronger, I never made it, and I will not defend it.

</cranky>

[pkamenarsky]

Just chiming it that having no control over grouping is a major blocker for me. From my POV, it's simple: just respect the input empty lines in let, where, do blocks (collapsing multiple empty lines into one is fine).

I don't agree at all with the "micro-decision" argument: strategically placed newlines improve readability dramatically. Code without newlines or newlines everywhere is absolutely undecipherable to me.

[leifmetcalf]

We had a lengthy discussion about this and we can't decide about the best logic for this.

I'm curious why you decided against treating where/let blocks like we already treat do blocks. It seems like the consistent thing to do.

[mboes]

There are very good arguments on both sides, but one thing that shouldn't be a deciding factor for a decision is a (IMO) false sense of consistency. The elements in let and do are not at all in the same syntactic category: we have bindings on the one hand and statements on the other. Bindings form a set, which the formatter is in principle free to reorder at will, whereas statements are a sequence, which we absolutely must not reorder. Any grouping in that sequence, introduced through whitespace, has a stronger semantic meaning than with bindings.

[mrkkrp]

After thinking about this again today, I think we should preserve blank lines found in original input between definitions in let and where bindings, like we do in do-blocks and instance definitions. @mboes said that the reason not to do it is that we want to be "automating away micro choices that are a chore to deal with". However, I think that:

• We already give the user some control over formatting by respecting their choice between single-line and multi-line layouts and in some other cases, so we already employ a composite approach. This means that we should not try to follow an extreme strategy (all manual or all automatic), but rather use common sense when drawing the lines between decisions that are automatic and decisions that are manual.
• Sometimes only human can decide when to place blank lines for best readability. For example, there could be a group of one-liners that are somehow related. Only human can see that a break between those definitions and the rest is helpful. This cannot be automated, and erasing the blank line or devaluating it by adding blank lines everywhere seems to be not the best strategy.
• I do not think that preserving blank lines erodes the stylistic consistency we want to achieve, it rather makes the tool more attentive to the subtleties of human intellection in code formatting.