Add import and module support

[sniperrifle2004]

Adds import and module Layouters. I refer to the tests for the layout choices I made (I tried to keep close to the style used in the brittany source).

Things to note:

• I was not able to figure out how to deal with the importColumn config value. I suspected the column limit should be altered locally, but I couldn't find a way to do that from the layouter.

• I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where.

• I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked.

[lspitzner]

Nice work! This looks (sufficiently) well-tested and the code looks fine too, so I could have merged as-is, really.

• I was not able to figure out how to deal with the importColumn config value.

  Consider the alignment in Brittany.Internal.Prelude. All imports are in a certain column, and that is what I had in mind for the config value to express. I'll bikeshed a bit more below.

• I had to use what feels like a trick to get the comments of a module to keep appearing above the module, since using docWrapNode on the module doc caused them to appear right after the where.

  Yeah that is a bit wonky, but I don't see a much better way. I refactored it to use a docSeq
  with an empty node which simplifies the logic a bit, but ultimately it is somewhat confusing that the comments before the module are in this Ann field - if ghc-exactprint has some thought-out logic which field it actually places comments in, this logic has escaped me so far.

  (and yes, this also means that there indeed are probably side-cases with unusual comment placements that break, but i don't mind too much.)

• I added tests to cover reasonably expected use cases and certain edge cases, but I do not guarantee that there aren't things I overlooked.

  i think the important bits are covered. with regards to comments, there are a lot of untested side-cases around in general, unfortunately. But this should not stop this from getting merged.

I have done mostly some superficial refactoring, hope you don't mind. Mostly replacing let in with where or do let to reduce general indentation.

[lspitzner]

As for the bikeshedding: I think I prefer to have all imports layouted in a certain way, that is

--     111111111 22222222222222222222222222222222 3333333333333333333
import           Foo
import           Foo                            ( foo )
import qualified Foo
import           Foo                           as Foo
import           Foo                            ( foo
                                                , bar
                                                )

where the columns as marked are "reserved" for certain things: 1 for the potential "qualified" keyword, 2 for the module name, 3 for the binding names. further:

• for more "special" qualifiers ({-# SOURCE #-} etc.) I would not mind reverting to some less constrained approach. But those are rather rare.
• If module name is too long, newline, indent to col 50 again and place entities.
• keep the space for qualified keyword even if there are no qualified imports yet in this module (for general consistency).
• I essentially vote for hanging indentation here, but only because the usual downsides don't apply: Because we have absolute hanging indentation, local changes are not at risk of affecting surrounding code (and causing large diffs).

If that is the goal, the current approach of using docCols is not perfect in a couple of places. For example, the Import module header would currently be formatted like this:

module Language.Haskell.Brittany.Internal.Layouters.Import (layoutImport) where

#include "prelude.inc"

import Language.Haskell.Brittany.Internal.Types
-- ..
import RdrName (RdrName(..))
import GHC
  ( unLoc
  , runGhc
  , GenLocated(L)
  , moduleNameString
  , AnnKeywordId(..)
  , Located
  )
import           HsSyn
-- ..
import qualified FastString
import           BasicTypes

RdrName(..) and unLoc are not aligned, and qualified only affects stuff up to GHC (a side-effect of how docCols is currently configured to work).

Stylish-haskell seems to have sensible behaviour for imports, too. I'd have to do some more testing with it to figure out how they treat some of the more special cases.

(a good alternative for docCols might be to use docEnsureIndent.)

[sniperrifle2004]

Ah, I see. I can do that. I am a little confused by the Foo winding up in the third column. I would have expected it to belong in column two. What about an aliased import with bindings? And what about hiding?

[lspitzner]

It is for cases like

import qualified Brick.ReflexMain as Brick
import qualified Reflex as R
import qualified Reflex.Host.Class as R
import qualified Reflex.PerformEvent.Base as R
import qualified Reflex.Host.App as RH
import qualified Graphics.Vty as V
import qualified Control.Monad.Ref as Ref

where you cannot the actual names at all on a glance. Seems to be much nicer as

import qualified Brick.ReflexMain              as Brick
import qualified Reflex                        as R
import qualified Reflex.Host.Class             as R
import qualified Reflex.PerformEvent.Base      as R
import qualified Reflex.Host.App               as RH
import qualified Graphics.Vty                  as V
import qualified Control.Monad.Ref             as Ref

For hiding and qualified+list: not sure. perhaps

import           Foo                           as F
import           Foo                           as F        ( abc )
import           Foo                           as F        ( abc
                                                           , def
                                                           )
import           Foo                           as F hiding ( abc )
import           Foo                           as F hiding ( abc
                                                           , def
                                                           )

import           Foo                            ( abc )
import           Foo                            ( abc
                                                , def
                                                )
import           Foo                     hiding ( abc )
import           Foo                     hiding ( abc
                                                , def
                                                )

but I think i mostly use import+whitelist, import+blacklist, import-qualified, and import-qualified-as. So I don't really know about the other combinations.

[lspitzner]

stylish-haskell turns the last example (plus some more items to test the linebreaking behaviour) into:

import           Foo as F
import           Foo as F (abc)
import           Foo as F (abc, def, def, def, def, def, def, def, def, def,
                           def, def, def)
import           Foo as F hiding (abc)
import           Foo as F hiding (abc, def, def, def, def, def, def, def, def,
                           def)

import           Foo (abc)
import           Foo (abc, def, def, def, def, def, def, def, def, def, def,
                      def, def, def)
import           Foo hiding (abc)
import           Foo hiding (abc, def, def, def, def, def, def, def, def, def,
                      def, def, def, def, def, def, def, def, def, def, def)

[sniperrifle2004]

Well, even if the use cases are rare, the formatter has to do something, preferably nice.

How about expanding a little vertical space to maintain the horizontal alignment:

import qualified Foo                     hiding ( foo )
import qualified Foo                           as F
import qualified Foo                           as F
                                                ( foo )
import qualified Foo                           as F
                                         hiding ( foo )
import qualified Foo                           as F
import           Foo                     hiding ( foo )

This maintains the binding column entirely. The downside is that it makes it a little harder to see what module the binding under an alias belongs to.

It even looks reasonable if the module name overlaps the column:

import qualified FooFooFooFooFooFooFooFooFooFooFoo           
                                               as F
                                         hiding ( foo )

[lspitzner]

I like that.

[lspitzner]

Looks good!

I noticed the staticDefaultConfig contains layoutColumn 60, but i used 50 above. Do you have an opinion on the "best" default? (If the default changes, the tests need to be adapted again, or we need to override the test config.) (this question does not block merging, to be clear.)

I noticed and fixed this case in the above commit:

module TestModule where
import Prelude ( (<$>) )

Two to-dos:

• Applying it to brittany's Prelude.hs still gives errors due to omitted comments, unfortunately.
• (mostly a reminder for myself) I would like to review if rdrNameToText vs lrdrNameToTextAnn could cause problems in any other cases. Probably not, because module names cannot be operators.

[tfausak]

I’m excited for Brittany to get this functionality! Thanks for opening this pull request, @sniperrifle2004.

I looked over the context free test cases and they seem a little weird to me. I would expect no extra spaces when formatting imports in a context free manner. In other words, no spaces for qualified, no spaces to align import lists, and no spaces to align aliases.

Am I reading those test cases right?

[sniperrifle2004]

@lspitzner There is obviously a trade off between room for module names and room for aliases/ThingsWith/long(ish) import bindings. I suspect twenty characters is a little tight for bindings, so I think 50 is probably better.

Concerning the lrdrNameToTextAnn issue, I think that there may be a potential instance in the ThingWith case of layoutIE (For classes with operators).

[sniperrifle2004]

@tfausak You are reading them right. The import layout is completely context free. The alignment is defined purely around the elements in the import node and the importColumn config value, and is not affected by the surrounding elements or the alignment rules. So the context free layout is the same as the layout with context.

[tfausak]

I feel like the imports don't match the rest of the the context free tests. I get that it's not exactly context sensitive. We had a discussion about what that means before: #66 (comment)

I would prefer some way to make Brittany format imports like this:

import Data.List (sort)
import Data.Maybe (fromMaybe)
import qualified Data.Map as Map
import Data.HashMap.Strict as HashMap

That is, don't align anything.

[sniperrifle2004]

Well, it is always possible to add a configuration option for that.

[tfausak]

I thought that's what the "context free" tests were testing, which is why I remarked on them. Specifically the settings are IndentPolicyLeft and ColumnAlignModeDisabled.

[sniperrifle2004]

Column alignment mode deals with alignment elements over multiple syntactic constructs, like binding lines and IndentPolicyLeft means that britanny may not increase the indentation level to improve alignment. And the spacing isn't technically indentation (if I understand the mechanics correctly) nor added by the britanny transformer. It is applied by the layouter itself.

[lspitzner]

@tfausak @sniperrifle2004

I had a long-wided reply typed, but that took too long, so I'll make it brief: I think you are both correct. The current behaviour is context-free but the tests never were meant to test that (apparently i indeed should have demanded a different name in the other PR..)

We can always make a flag in the opposite direction: One that relaxes IndentPolicyLeft/ColumnAlignModeDisabled for import statement layouting. (Because the actual intention behind IndPolLeft and CAMDisabled was context-free-ness, and this layouting does not violate that. Cue some more discussion about definitions, meanings and intended meanings..)

@sniperrifle2004 it is up to you if you want to implement this additional behaviour. If not, I might be lazy and simply disable reformatting of imports in the presence of IndPolLeft or CAMDisabled. But then it probably is relatively straight-forward to actually implement it.. considering that, @tfausak maybe you want to contribute this bit?

[sniperrifle2004]

I fixed the ommitted comments for the prelude, but it positions them a little awkwardly because it retains the absolute position for most of them

[lspitzner]

@sniperrifle2004 there is one further (special) case: Some very long identifier:

import           Foo                            ( loooooooooooooooooooooooooooooooooooong )

This overflows the column limit. I am fine with that, but it is maybe worth a testcase to make it explicit.

[sniperrifle2004]

@lspitzner @tfausak

I'll construct a condensed layout based on the indent policy being IndentPolicyLeft. Would such a layout also try to keep bindings on the import line? Or would it still expand them by default?

[lspitzner]

there still are these broken:

#test import-with-comments-2

import           Test                                     ( abc
                                                          , def
                                                          -- comment
                                                          )

#test import-with-comments-3

import           Test                                     ( abc
                                                          -- comment
                                                          )

I can fix those too if this comment stuff is becoming too annoying. Probably involves splitFirstLast. But I don't want to push while you work. (And I'll be sleeping now anyways.)

Would such a layout also try to keep bindings on the import line? Or would it still expand them by default?

You mean whether to permit import Foo (foo, bar) when it does not overflow? I'd default to "yes", but there may be counterarguments.

[sniperrifle2004]

It isn't really. I want to get a feeling for how to deal with them, since I want to contribute more in the future so I had better learn how to deal with them. I am having difficulty anticipating both brittany and ghc-exact-print and improving bad results with more specific instructions to brittany is also harder than I expect.

Let's take your tests as an example:

#test import-with-comments-2

import           Test                                     ( abc
                                                          , def
                                                          -- comment
                                                          )

#test import-with-comments-3

import           Test                                     ( abc
                                                          -- comment
                                                          )

Both these comments end up on the (Located [LIE]) and not on the LIE for abc and def. This means that ghc-exact-print thinks that the comment belongs to the entire binding list or rather to the ) which semantically means the same thing. It goes even further than that:

#test import-with-comments-4


import           Test                                     ( abc
                                                          , def -- comment
                                                          )

I would consider any comments on the right of some element to belong to the element on its left (There might be a case, where there is a more semantically correct element directly below the comment), but ghc-exact-print still associates this with the llies.

I find this annoying and confusing because I suspected all comments for LIEs to be handled by layoutIE. So I go through hoops to get britanny to process this annotation for llies with the last LIE.

And when I do that the result is this:

import           Test                                     ( abc
                                                          -- comment
                                                               , def 
                                                          )

What? Why is that even a thing? I can understand that it now considers the comment to be a part of the BriDoc, but the annotation's location data should still indicate that the placement should be below the LIE. And why the ,def shifts to the right (or the comment is right associative with respect to the , def which is then forced to the base offet) is even more of a mystery to me. And then we had the comments for the module which shifted to after the where. In that case I agree with ghc-exact-print that the comments belong to the module node. I personally rarely place comments below the element, on occasion to the right of an element, but mostly above an element. And there is no reason not to place them above the element in that context (Since there are no elements there)

So the behaviour of both ghc-exact-print and brittany is far from intuitive to me.

End rant 😛

[sniperrifle2004]

I figured it out, but those delta positions sure are odd. I am not sure why certain comments are attached to the llies at all, while the delta positions clearly still indicate they are relative to def or abc.

[lspitzner]

@sniperrifle2004 agreed. See the discussion at alanz/ghc-exactprint#53. I made some suggestions there that I think would be saner approaches, but then I have no much of an idea of how the actual parsing and processing logic for DPs works. (There is also #31 where the strange DPs make things very hard/impossible for brittany to do the correct thing.)

[lspitzner]

Ah, and of course new-build treats "--ghc-options" differently. I was so evil to revert that merge, let me fix dev branch first.

[tfausak]

@sniperrifle2004 thanks for humoring my alignment-free lifestyle 😄 This PR is looking great!

[lspitzner]

@sniperrifle2004 and you are right,

import           Test                                     ( abc
                                                          -- comment
                                                               , def 
                                                          )

looks really strange, and is probably a bug hiding in the lower layers of brittany. I don't understand how you triggered it however, as the current code does not expose this and still seems to go through the hoop you mentioned. (Btw. other layouters in Expr.hs use the same approach, iirc.)

Do you currently have uncommitted/unpushed stuff? There is a couple of minor fixups I wish to make.

[sniperrifle2004]

@lspitzner It was partly because I did not use docWrapNode llies on just the last element at the time. I used it on both abc and , def (The first and the last element). I did not realize how wrong this was until I used dump-annotations. In addition the current code only affects def not the docSeq with the docCommaSep which seems to be significant as well, since just adding docWrapNode llies to abc now results in

( abc
-- comment
, def
)

If I also change the layoutAnnAndSepLLIEs to call docWrapNode on the , def docSeq then I get:

( abc
-- comment
     , def
)

I do not currently have any pending changes (I have been a bit busy).

[lspitzner]

@sniperrifle2004 oh, i should probably remark on that last refactor: My main concern was not that the code was bad, but it was not clear to me how the sharing worked (i.e. usage of docShareWrapper etc.). To be fair, sharing is probably irrelevant because there are no arbitrarily nested nodes below in the import/export statements anyways, so the complexity is linear anyways, but it does not hurt to properly implement the sharing either.

[sniperrifle2004]

I added a compact layout triggered when the indentPolicy is IndentPolicyLeft.

In addition, I removed the fieldLabel part of the ThingWith. I realized this couldn't be executed at all, since the information to distinguish fields and other functions isn't available yet.

I also added code to handle the case where there are comments in a ThingWith. This also means those lists can now expand over multiple lines. The only case where this is troublesome is the case where a single item with a list would expand resulting in this:

import           Test                                     ( Long( List
                                                                , Of
                                                                , Things
                                                                ) )

I tried several things to get this to look a little better, but nothing quite worked. So it seems this is the price for keeping such a line from (excessively) overflowing the column limit.

@lspitzner I think that is closer to what I intended to do (ie. share the list between alternatives). I wasn't really concerned with whether it had to be shared. I just thought that it would be better if it was.

[tfausak]

This is actually not context free anymore, right? The indentation here depends on the length of the module name being imported.

[sniperrifle2004]

More on the position of the first binding, which I chose to stick to the import line. So you rather want this:

import MoreThanSufficientlyLongModuleNameWithSome
  (items, that, ...)

An eventually:

import MoreThanSufficientlyLongModuleNameWithSome
  ( items
  , that
  , ...
  )

[tfausak]

👍 Yup!

[lspitzner]

@tfausak can/did you have another look at the last commit?

@sniperrifle2004 i have not forgotten about this; i have been toying a bit with the ThingWith case too, without finding a nice(r) solution yet either. But perhaps it is better to not let ThingWith hold up the PR. I have reviewed the last commits and would like to merge before this gets even larger, unless you have something planned still - have you?

[tfausak]

These are the only ones that look weird to me. The indentation depends on the length of the Thing identifier. I don't think I've ever actually had an import like this, but I think I'd format it as:

import Test
  ( Thing
    ( With
    -- Comments
    , and
    -- also
    , items
    -- !
    )
  )

[sniperrifle2004]

Ah I should have expected this. I'll take a look.

[sniperrifle2004]

@lspitzner I have nothing else planned really. I do suspect that it might be a good idea to add an option to choose the compact layout independent of the indent policy, but I don't know that for certain. Adding that later is no problem.

Another thing that comes to mind is that the import column is still set to 60. This is probably the best time to change it, since only the tests depend on it.

[lspitzner]

For the record, this was not closed intentionally, in fact I did not ever press any such button, and I was not aware that deleting a branch would have this effect. Sorry again for leaving this PR hanging for so long, it is certainly still on my to-do list for the next release.