Support only GHC 9.0

[tfausak]

This fixes #352. It is an alternative to #356.

This pull request adds support for GHC 9.0 but drops support for all other versions of GHC. Since I am not really familiar with the Brittany codebase, it was challenging for me to deal with that and CPP at the same time.

These changes mostly work, but I'm having some trouble with spaces. Somehow final newlines went away, and spaces between arguments to constructors are mysteriously missing.

[tfausak]

There are two failing test cases. They're both for type class instances that contain data (or newtype) declarations. The problem is that they're not inserting spaces between constructor arguments.

-- instance-with-data-family-below-method
-- expected
data MyData = Test Int Int
-- actual
data MyData = TestIntInt

-- instance-with-newtype-family-and-deriving
-- expected
newtype Bar () = Baz ()
-- actual
newtype Bar () = Baz()

https://github.com/tfausak/brittany/runs/4111574000?check_suite_focus=true#step:12:661

I think layoutDataFamInstDecl is ultimately responsible for this. Removing the call to stripWhitespace changes the output, but it doesn't fix the constructor arguments.

[tfausak]

So layoutDataFamInstDecl just hands things off to briDocByExactNoComment, which seemingly works fine in other contexts. And also there's not a lot of room for things to go wrong there, assuming that ghc-exactprint is working correctly. I'm going to work on checking that assumption.

Perhaps it's possible that this is a GHC bug? I know there are a few gnarly ones in GHC 9.0.1. Unfortunately there's not yet a GHC 9.0.2 to test against.

[tfausak]

Well at least ghc-exactprint seems to be doing the right thing:

$ cat GH357.hs 
{-# language TypeFamilies #-}
instance C T where
    data X T = X A B C

$ cabal exec -- brittany --exactprint-only GH357.hs 
{-# language TypeFamilies #-}
instance C T where
    data X T = X A B C

$ cabal exec -- brittany GH357.hs 
{-# language TypeFamilies #-}
instance C T where
    data X T = XABC

[tfausak]

It looks like something is going wrong when converting the AST to BriDoc. That probably sounds obvious, but I feel it at least narrows things down. This isn't a problem with outputting the BriDoc, it's a problem with generating the BriDoc in the first place.

$ cabal exec -- brittany --suppress-output --dump-bridoc-raw GH357.hs

Click to see output.

---- bridoc raw ----
BDLines []
---- bridoc raw ----
BDLines
  [ BDExternal
      AnnKey {abstract:RealSrcSpan} (CN "ClsInstD")
      fromList
        [ AnnKey {abstract:RealSrcSpan} (CN "ClsInstD")
        , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
        , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
        , AnnKey {abstract:RealSrcSpan} (CN "HsAppTy")
        , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
        , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
        ]
      False
      pack "{-# language TypeFamilies #-}\ninstance C T where"
  , BDEnsureIndent
      BrIndentRegular
      BDIndentLevelPop
        BDIndentLevelPushCur
          BDLines
            [ BDExternal
                AnnKey {abstract:RealSrcSpan} (CN "DataFamInstDecl")
                fromList
                  [ AnnKey {abstract:RealSrcSpan} (CN "DataFamInstDecl")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "ConDeclH98")
                  , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "HsTyVar")
                  , AnnKey {abstract:RealSrcSpan} (CN "Unqual")
                  , AnnKey {abstract:RealSrcSpan} (CN "False")
                  , AnnKey {abstract:RealSrcSpan} (CN "[]")
                  ]
                False
                pack "data X T = XABC"
            ]
  ]

[tfausak]

This was implied, but to make it explicit: The latest version of Brittany pretty prints that file without issue.

$ brittany --version
brittany version 0.13.1.2
Copyright (C) 2016-2019 Lennart Spitzner
Copyright (C) 2019 PRODA LTD
There is NO WARRANTY, to the extent permitted by law.

$ brittany GH357.hs
{-# language TypeFamilies #-}
instance C T where
  data X T = X A B C

Click to see raw BriDoc output. It's the same except for the source spans.

---- bridoc raw ----
BDLines []
---- bridoc raw ----
BDLines
  [ BDExternal
      AnnKey {GH357.hs:(2,1)-(3,22)} (CN "ClsInstD")
      fromList
        [ AnnKey {GH357.hs:(2,1)-(3,22)} (CN "ClsInstD")
        , AnnKey {GH357.hs:2:10} (CN "HsTyVar")
        , AnnKey {GH357.hs:2:10} (CN "Unqual")
        , AnnKey {GH357.hs:2:10-12} (CN "HsAppTy")
        , AnnKey {GH357.hs:2:12} (CN "HsTyVar")
        , AnnKey {GH357.hs:2:12} (CN "Unqual")
        ]
      False
      pack "{-# language TypeFamilies #-}\ninstance C T where"
  , BDEnsureIndent
      BrIndentRegular
      BDIndentLevelPop
        BDIndentLevelPushCur
          BDLines
            [ BDExternal
                AnnKey {GH357.hs:3:5-22} (CN "DataFamInstDecl")
                fromList
                  [ AnnKey {GH357.hs:3:5-22} (CN "DataFamInstDecl")
                  , AnnKey {GH357.hs:3:10} (CN "Unqual")
                  , AnnKey {GH357.hs:3:12} (CN "HsTyVar")
                  , AnnKey {GH357.hs:3:12} (CN "Unqual")
                  , AnnKey {GH357.hs:3:16} (CN "Unqual")
                  , AnnKey {GH357.hs:3:16-22} (CN "ConDeclH98")
                  , AnnKey {GH357.hs:3:18} (CN "HsTyVar")
                  , AnnKey {GH357.hs:3:18} (CN "Unqual")
                  , AnnKey {GH357.hs:3:20} (CN "HsTyVar")
                  , AnnKey {GH357.hs:3:20} (CN "Unqual")
                  , AnnKey {GH357.hs:3:22} (CN "HsTyVar")
                  , AnnKey {GH357.hs:3:22} (CN "Unqual")
                  , AnnKey {<no location info>} (CN "False")
                  , AnnKey {<no location info>} (CN "[]")
                  ]
                False
                pack "data X T = X A B C"
            ]
  ]

[tfausak]

Here's an even simpler failing test case:

data instance A B = C D

As it stands on this branch, Brittany removes the space between C and D. Adding some tracing to docExt suggests that this is indeed a problem with exactPrint:

>>> docExt AnnKey TestFakeFileName.hs:1:1-23 CN "DataFamInstD"
>>> input: data instance A B = C D
>>> input (debug): {TestFakeFileName.hs:1:1-23}
data instance A{tc} {TestFakeFileName.hs:1:17}
                    B{tc}
  = {TestFakeFileName.hs:1:21-23}
    forall. C{d} D{tc}
>>> output: "data instance A B = CD"

[tfausak]

Hmm, this is puzzling. I wrote a script to parse a module with ghc-exactprint and print it back out with exactPrint. I expected the same output as from Brittany's test suite, but it worked fine.

Click to see script.

import qualified GHC.Utils.Error
import qualified GHC.Utils.Outputable
import qualified Language.Haskell.GHC.ExactPrint
import qualified Language.Haskell.GHC.ExactPrint.Utils

main :: IO ()
main = do
    parseResult <- Language.Haskell.GHC.ExactPrint.parseModule "TestFakeFileName.hs"
    (anns, parsedSource) <- either
        ( fail
        . GHC.Utils.Outputable.showSDocUnsafe
        . GHC.Utils.Outputable.sep
        . GHC.Utils.Error.pprErrMsgBagWithLoc
        )
        pure
        parseResult

    putStrLn ">>> showSDocUnsafe"
    putStrLn
        . GHC.Utils.Outputable.showSDocUnsafe
        $ GHC.Utils.Outputable.ppr parsedSource

    putStrLn ">>> showSDocDebug_"
    putStrLn
        . Language.Haskell.GHC.ExactPrint.Utils.showSDocDebug_
        $ GHC.Utils.Outputable.ppr parsedSource

    putStrLn ">>> exactPrint"
    putStr $ Language.Haskell.GHC.ExactPrint.exactPrint parsedSource anns

>>> showSDocUnsafe
data instance A B = C D
>>> showSDocDebug_
{TestFakeFileName.hs:1:1}
{TestFakeFileName.hs:1:1-23}
data instance A{tc} {TestFakeFileName.hs:1:17}
                    B{tc}
  = {TestFakeFileName.hs:1:21-23}
    forall. C{d} D{tc}
>>> exactPrint
data instance A B = C D

I thought maybe the annotations were different, but I printed those out too and they looked fine.

[tfausak]

Maybe eyeballing the annotations wasn't sufficient 😆

Changing anns to addAnnotationsForPretty [] x anns fixes this problem but breaks a bunch of other test cases. So it looks like this case is failing because the annotations are wrong for some reason.

[tfausak]

Dang this just keeps getting more confusing.

Changing layoutDataFamInstDecl to use addAnnotationsForPretty, either building from the existing anns or starting from mempty, does not change the output. That suggests the annotations on the DataFamInstDecl are fine but some annotations outside of it are problematic? I'm not entirely sure.

Here's the difference between the "bad" annotations from this branch and the "good" annotations from the script I posted earlier (#357 (comment)):

2a3,11
> ({ TestFakeFileName.hs:1:1 }
>  Just (Ann (DP (0,0)) [] [] [(AnnEofPos,DP (1,0))] Nothing Nothing)
>  (HsModule
>   (VirtualBraces
>    (1))
>   (Nothing)
>   (Nothing)
>   []
>   [
66c75,77
<         []))))))))
---
>            []))))))))]
>   (Nothing)
>   (Nothing)))

The AnnEofPos is strange. I thought it went away? #357 (comment)

It does live on as a field on ApiAnns: https://downloads.haskell.org/~ghc/9.0.1/docs/html/libraries/ghc-9.0.1/GHC-Parser-Annotation.html#v:apiAnnEofPos

But that doesn't explain how it shows up in ghc-exactprint's output.

[tfausak]

I narrowed it down to these lines:

brittany/src/Language/Haskell/Brittany/Internal.hs

Lines 465 to 466 in 42cf56b

	filteredAnns <- mAsk
	<&> \annMap -> Map.findWithDefault Map.empty declAnnKey annMap

If I comment out the second line, then Brittany correctly prints type families. Unfortunately a bunch of other things break.

[tfausak]

Both #198 and 9236673 seem relevant to this problem.

[tfausak]

I tested this against my work codebase, which was previously formatted with Brittany 0.13.1.2. This branch didn't change anything, which is great! Everything seems to be working as expected.

[tfausak]

73 commits later, here’s where this stands:

• It works with GHC 9.0. All the tests pass. Running the updated executable against various code bases that I maintain appears to work fine.
• It doesn’t work with any older versions of GHC. Removing the CPP made things easier for me to maintain.
• The basic work to get this building with GHC 9.0 wasn’t too obnoxious, but I don’t really want to maintain compatibility with multiple versions of GHC at the same time.

In short: mission accomplished.

I would merge this PR, but I don’t have admin access to this repo. That means I can’t add secrets, which means I can’t automatically release new versions through GitHub Actions. I have all the set up on my fork, so I will probably continue developing it there. We’ll see how that plays out moving forward.

My next goal is to get Brittany building with GHC 9.2. Unfortunately many dependencies don’t yet build with 9.2, so that’s going to be challenging. Although I haven’t looked too closely yet, I suspect my overall plan will involve trying to reduce the number of dependencies. For example I think butcher could be dropped for something like optparse-applicative or even System.Console.GetOpt.

[tfausak]

https://github.com/tfausak/brittany/releases/tag/0.14.0.0

[arybczak]

Thank you for the work! Supporting a single compiler version looks like the right move, it's a bit sad that maintaining compatibility for multiple GHC API versions is so hard right now.

[jneira]

@tfausak many thanks for your work with this, hls will be able to enable brittany for ghc-9.0.1 thanks to.
Sadly the new hackage version 0.14 mixed two important breaking changes: support for aeson >= 2.0 (only) and ghc == 9.0.1.
Due to that combination we can't jump in hls to aeson 2.0 for ghc < 9.0.1 and we will have to keep compatibility for aeson < 2.0 until we drop support for ghc 8.10, so for a long time (or droping the brittany plugin)
Any chance to release a intermmediate version with support for ghc < 9.0.1 and aeson-2.0? thanks!

[tfausak]

I'll have to see what changes I'd need to make to support Aeson 2. From what I remember there wasn't too much to do there.