Add type synonym formatting

[ruhatch]

Add layouter for type synonyms using a similar style to type signatures. It calls layoutType on the right-hand side and handles the left-hand side similiarly to the function name in a Sig. It might be possible to refactor the Sig and SynDecl code to share more structure.

I might have missed some possible cases, so please let me know if there are missing tests!

[ruhatch]

@eborden, @tfausak - would you mind trying this on your codebases to try and spot missing cases/errors?

[tfausak]

We don't have that many type synonyms in our codebase. Looks like only 25. Regardless, formatting everything with this PR changed one file and all the tests still passed. 👍

[eborden]

We also don't have too many type aliases, but I ran and everything looks fine. It only reformatted two of them and they looked reasonable :man_shrugging:

[lspitzner]

Some cases:

-- comment move around a bit
type Foo a -- fancy type comment
  = -- strange comment
    Int

-- crashes due to pattern match failure
type (a `Foo` b) c = (a, b, c)

-- Typeoperators already work. I would like a testcase for them.
type (a :+: b) = (a, b)

I can have a look on the comment thingy; those are tricky. Would be nice to have the failure fixed, even though I might not even view it as a blocker - it should be a rare occasion to write things this way.

(oh, and rebase onto latest master please; CI was still borked. master is finally clean.)

[ruhatch]

I have fixed the pattern matching error, but then the parens are a bit of a rabbit hole:

type (a :-: b) c = (a, b)

type ((a :%: b) c) = (a , c)

type ((a :*: b) c) d = (a , c)

These are stored as AnnOpenPs with relative DPs, so I'm not sure how to go about laying them out properly. A simple solution that produces at least syntactically valid results is to drop all but the parens around the operator.

[ruhatch]

Also the fixity testing on 8.0.2 is a bit tricky - the first example above prints as

type :-: a b c = (a, b)

because there's no backtick and no fixity declaration to check. Any ideas how to detect that?

[lspitzner]

I have had a bit of a look at the infix-detection and parenthesis stuff. I think it should be possible to hack around this by considering a combination of hasAnnKeyword name AnnBackquote, hasAnnKeyword name AnnOpenP (on the Unqual) and testing on whether the name starts with an ":" (which I believe is the only way for type operators).

As ghc-exactprint demonstrates in its test-suite (in TypeBrackets.hs, just if you are curious) there is enough information to do things properly on ghc-8.0.2, too. But it looks really messy and I cannot make any sense of how it works - the fact that there are negative offsets in the relevant DPs is really confusing to me. (I am no exactprint expert, but in general I can make rough sense of that stuff ..)

As a consequence, I agree with dropping down to "normalize while formatting" behaviour, i.e. to turn: type (((MyType))) = Int into type MyType = Int and type ((a `OtherType` b) c) = Int into type (a `OtherType` b) c = Int, even when brittany usually refrains from any sort of "parenthesis normalization".

Please say if this is getting too complex or if the goal of keeping things consistent over different ghc versions causes too much headache; I can give it a go when I find time too, or we can drop to exactprint for complex cases or even declare that the more-than-two-type-args-while-infix case is so rare that we don't support it (for now).

Looking at the comment thing is still on my to-do list.

[ruhatch]

Okay, so I got infix stuff working for 8.0.2 by checking the first character of the name. I assume it's an infix operator if it's not an upper-case letter or an opening bracket, which I believe is reasonable.

To complicate things though, the brackets in type (a :+: b) c = (a, b, c) are stored as comments in 8.4.3, so we get missing comment warnings! Is there a way to safely drop them?

I think here the "normalize while formatting" approach seems fine - the extra parens don't seem to make it more readable anyway ;)

Other than that 8.4.3 thing the comments are the only thing left. I also left a couple of other pending tests in place so that someone could come back to this if they want.

[ruhatch]

Okay I added another commit that deals with parens on 8.4.3 - it actually works out better than on the other two versions, because the comments are better placed. It required some small changes in layoutBriDocM, so it would be good to look closely at that.

Only thing left is dealing with comments now!

[ruhatch]

I made some progress with comments, but the example you gave above has a problem.

-- comment move around a bit
type Foo a -- fancy type comment
  = -- strange comment
    Int

=>

-- comment move around a bit
type Foo a -- fancy type comment
  =  -- strange comment
    Int

Notice the extra space before -- strange comment. This problem exists in type signatures as well actually. I'm not sure how best to fix this. I tried adding a hasAnyCommentPrior but I'm not sure how to modify the layout once we know the type has prior comments.

[ruhatch]

I fixed that up by using appSep instead of an explicit space, so this is actually all good to go!

It performs 'normalisation while formatting' for parens on 8.0.2 and 8.2.2, but everything else seems to work perfectly :D

[lspitzner]

Sorry for the delay. The fix for comments looks good. I'd just like to confirm that layoutMoveToCommentPosX is indeed necessary as I'd prefer not baking more implicit assumptions into the backend state fields, it is sufficiently confusing as is :p. Can you give feedback on the comment?

The resulting behaviour looks good to me across ghc versions, although I have not tested 8.6 yet. The ghc-8.6 branch should be working as of today, but it has merge conflicts. I plan to merge this PR first, and then rebase the 8.6 branch, so this PR will not need to be 8.6-ready.

[lspitzner]

@ruhatch I added one commit that (hopefully) should work just as well as before, and it fixes a case of superfluous spaces being retained on ghc-8.4. Being a bit more selective in using docSeparator also makes the one backend hack unnecessary.

-- e.g.
type ((a :%: b  ) c  ) = (a, c)
-- yielded
type ((a :%: b ) c ) = (a, c)

(there was the x - 1 in the code before, but that was not sufficient if in the input the x was >1.)

Maybe it is time to add testcases for non-fixpoints to the suite, i.e. have (input, expected output) pairs. The bug I describe above cannot currently be tested against, which is unfortunate.

If you have any interesting (non-fixpoint) testcases lying around it would be nice if you could test against the latest commit. Otherwise I'd say this is good to merge.

thanks!

[ruhatch]

@lspitzner that all looks good to me! Thank you!

[lspitzner]

thanks!