tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

vertical whitespace explosion #456

Closed tonyday567 closed 4 years ago

tonyday567 commented 4 years ago

I have a line of haskell that looks like this:

cs = vs <> maybe [] (\x -> [Chart (RectA x) (maybeToList (SpotRect <$> (fmap (* maybe 1 realToFrac (l ^. #innerPad)) <$> styleBoxes vs)))]) (l ^. #legendFrame)

and ormolu formatting is idempotent.

But if there is a linebreak somewhere eg

cs = vs <> maybe [] (\x -> [Chart (RectA x) (maybeToList (SpotRect <$> 
  (fmap (* maybe 1 realToFrac (l ^. #innerPad)) <$> styleBoxes vs)))]) (l ^. #legendFrame)

I get this:

    cs =
      vs
        <> maybe
          []
          ( \x ->
              [ Chart
                  (RectA x)
                  ( maybeToList
                      ( SpotRect
                          <$> (fmap (* maybe 1 realToFrac (l ^. #innerPad)) <$> styleBoxes vs)
                      )
                  )
              ]
          )
          (l ^. #legendFrame)

That's some serious vertical spacing. Not saying it's wrong, but can someone give me a sense of what's happening?

neongreen commented 4 years ago

You've cut the AST in (almost) the worst possible place.

The algorithm works as follows: if one of the subtrees is multiline, the whole tree will be multiline.

So, when you make SpotRect <$> (fmap (* maybe 1 realToFrac (l ^. #innerPad)) <$> styleBoxes vs) multiline, you force the enclosing expression to be multiline too. And then multiline-ness gets propagated up the tree.

neongreen commented 4 years ago
neongreen commented 4 years ago

If you want Ormolu to keep the expression as-is, chime in at https://github.com/tweag/ormolu/issues/435.

neongreen commented 4 years ago

You can also play with your expression at https://monadfix.com/ormolu/ to see how exactly it behaves when you insert the linebreak at different places. The worst possible subexpression to break is (l ^. #innerPad), which gives you:

cs =
  vs
    <> maybe
      []
      ( \x ->
          [ Chart
              (RectA x)
              ( maybeToList
                  ( SpotRect
                      <$> ( fmap
                              ( *
                                  maybe
                                    1
                                    realToFrac
                                    ( l
                                        ^. #innerPad
                                    )
                              )
                              <$> styleBoxes vs
                          )
                  )
              )
          ]
      )
      (l ^. #legendFrame)
mrkkrp commented 4 years ago

I think in this case the behavior is expected.

tonyday567 commented 4 years ago

Thanks for the explanations.

You've cut the AST in (almost) the worst possible place.

So my personal, probably suspect, style choice in this case is single line but cut any old place (ie not have a good sense of exactly what the tree is) to not go past column 80.

Is there any scope for this sort of rule, between single and multi-line?

mrkkrp commented 4 years ago

Sorry, I do not understand what you are asking. In particular I do not understand what you mean by "old place".

tonyday567 commented 4 years ago

Sorry. "any old place" means "arbitrary" or "random".

I would like to retain the single line formatting, but with an option to break a line after 80 characters, and for this not to signal multi-line treatment.

mrkkrp commented 4 years ago

"any old place" means "arbitrary" or "random".

That's an interesting idiom, especially when used in this context :)

I would like to retain the single line formatting, but with an option to break a line after 80 characters, and for this not to signal multi-line treatment.

There is no way to do it. We do not have any counter for columns, so we can't tell when a line is "too-long", we don't even have a configuration setting to define "too-long". Ormolu works by selecting layouts from the two options: single line vs multiline. The layout is chosen by the user.

tonyday567 commented 4 years ago

Ok, that's a radical departure from existing practice.

I'm liking it!

Thanks again for your help.