lspitzner / brittany

haskell source code formatter
GNU Affero General Public License v3.0
691 stars 72 forks source link

Support sum type layouting #294

Open eborden opened 4 years ago

eborden commented 4 years ago

@lspitzner Getting the ball rolling on this topic.

This is a naive first pass at sum type support. This currently breaks 11 tests, some from comment misplacement, others in more exotic forms that previously required high levels of context to be correctly laid out. The solution for comment misplacement is probably trivial, but the highly contextualized forms may need a bit more alchemy.

  1) data type declarations record no matching single line layout
       expected: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 -- brittany { lconfig_allowSinglelineRecord: true }
                 data MyRecord = forall a . Show a => Bar
                   { foo :: abittoolongbutnotvery -> abittoolongbutnotvery
                   }

        but got: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 -- brittany { lconfig_allowSinglelineRecord: true }
                 data MyRecord = forall a
                 . Show a =>
                   Bar
                     { foo :: abittoolongbutnotvery -> abittoolongbutnotvery
                     }

  2) data type declarations record forall constraint multiline
       expected: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord
                   = forall a
                   . LooooooooooooooooooooongConstraint a =>
                     LoooooooooooongConstructor
                       { foo :: abittoolongbutnotvery -> abittoolongbutnotvery
                       }

        but got: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord = forall a
                 . LooooooooooooooooooooongConstraint a =>
                   LoooooooooooongConstructor
                     { foo :: abittoolongbutnotvery -> abittoolongbutnotvery
                     }

  3) data type declarations record forall constraint multiline more
       expected: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord
                   = forall a b
                   . ( Loooooooooooooooooooooooooooooooong a
                     , Loooooooooooooooooooooooooooooooong b
                     ) =>
                     MyConstructor
                       { a :: a
                       , b :: b
                       }

        but got: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord = forall a b
                 . ( Loooooooooooooooooooooooooooooooong a
                   , Loooooooooooooooooooooooooooooooong b
                   ) =>
                   MyConstructor
                     { a :: a
                     , b :: b
                     }

  4) data type declarations plain with forall and constraint
       expected: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyStruct
                   = forall a b
                   . ( Loooooooooooooooooooooooooooooooong a
                     , Loooooooooooooooooooooooooooooooong b
                     ) =>
                     MyConstructor (ToBriDocM BriDocNumbered)
                                   (ToBriDocM BriDocNumbered)
                                   (ToBriDocM BriDocNumbered)

        but got: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyStruct = forall a b
                 . ( Loooooooooooooooooooooooooooooooong a
                   , Loooooooooooooooooooooooooooooooong b
                   ) =>
                   MyConstructor (ToBriDocM BriDocNumbered)
                                 (ToBriDocM BriDocNumbered)
                                 (ToBriDocM BriDocNumbered)

  5) data type declarations record with many features
       expected: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord
                   = forall a b
                   . ( Loooooooooooooooooooooooooooooooong a
                     , Loooooooooooooooooooooooooooooooong b
                     ) =>
                     MyConstructor
                       { foo, foo2
                           :: loooooooooooooooooooooooooooooooong
                           -> loooooooooooooooooooooooooooooooong
                       , bar  :: a
                       , bazz :: b
                       }
                   deriving Show

        but got: Right
                 {-# LANGUAGE ScopedTypeVariables #-}
                 data MyRecord = forall a b
                 . ( Loooooooooooooooooooooooooooooooong a
                   , Loooooooooooooooooooooooooooooooong b
                   ) =>
                   MyConstructor
                     { foo, foo2
                         :: loooooooooooooooooooooooooooooooong
                         -> loooooooooooooooooooooooooooooooong
                     , bar  :: a
                     , bazz :: b
                     }
                   deriving Show

  6) data type declarations single record existential
       expected: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data Foo = forall a . Show a => Bar
                   { foo :: a
                   }

        but got: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data Foo = forall a
                 . Show a =>
                   Bar
                     { foo :: a
                     }

  7) data type declarations record multiple types existential
       expected: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data Foo = forall a b . (Show a, Eq b) => Bar
                   { foo  :: a
                   , bars :: b
                   }

        but got: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data Foo = forall a b
                 . (Show a, Eq b) =>
                   Bar
                     { foo  :: a
                     , bars :: b
                     }

  8) data type declarations record newline comment
       expected: Right
                 data MyRecord = MyRecord
                   { a :: Int
                     -- comment
                   , b :: Int
                   }

        but got: Right
                 data MyRecord = MyRecord
                   { a :: Int
                 -- comment
                   , b :: Int
                   }

  9) data type declarations comment before equal sign
       expected: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data MyRecord
                   -- test comment
                   = forall a b
                   . ( Loooooooooooooooooooooooooooooooong a
                     , Loooooooooooooooooooooooooooooooong b
                     ) =>
                     MyConstructor a b

        but got: Right
                 {-# LANGUAGE ExistentialQuantification #-}
                 data MyRecord
                   -- test comment
                                = forall a b
                 . ( Loooooooooooooooooooooooooooooooong a
                   , Loooooooooooooooooooooooooooooooong b
                   ) =>
                   MyConstructor a b

  10) data type declarations large record with a comment
       expected: Right
                 data XIILqcacwiuNiu = XIILqcacwiuNiu
                   { oyyFtvbepgbOge_pebzVmuftEijwuj     :: Jgtoyuh HessJvNlo
                   , wloQsiskdoxJop_xatiKrwedOxtu       :: Jgtoyuh [Inotg]
                   , mmmJjcqtemyIyo_ovosDoreKeeoyamvove :: Jgtoyuh Eujo
                   , mbiIatelofxOzr_uluxNngiiMjah       :: Jgtoyuh HessJvNlo
                   , obxIskfcxpkIkb_uuviTuevcSkrgo      :: Jgtoyuh Int
                   , wqrAtuvuecoHwr_ilotNxbuPleo        :: Jgtoyuh Ufaxdeq
                   , lofAfuebdhpLuv_cnekPoyFxmg         :: Jgtoyuh Ufaxdeq
                   , ouoFugtawzvUpk_oupiLzptugy         :: Jgtoyuh Eujo
                   , iqiXjtziwogNsa_uiyvSunaTtgUsf3     :: Jgtoyuh Oaivn
                   , odbIriaqnojUlz_onotoWuunehIpuy     :: Jgtoyuh Eujo
                   , opjUxtkxzkiKse_luqjuZazt
                       :: Jgtoyuh [(Eujo, Int, Int, Int, Int, Int, NELUxro)]
                   -- , jcqRaqznxfhIpa_ywevMezmoYkutuwa        :: Jgtoyuh ()
                   , vayOmuasyphOfd_bcsVljmvt               :: Jgtoyuh Eujo
                   , rifArahilooRax_ufikecqdImsv            :: Jgtoyuh Oaivn
                   , raqKtopcpszDwb_oqocubasZuqjcryoDojGkw  :: Jgtoyuh Oaivn
                   , mluJiilpcijUtt_gaisklifVekfeyagRmfbyzz :: Jgtoyuh Oaivn
                   , oqhPaahjupaSmi_gamwwoovKyxznecvEayluc  :: Jgtoyuh Oaivn
                   , mazFubimwebZpa_itidehDodiDlboz         :: Jgtoyuh Vrep
                   , jeyOcuesexaYoy_vpqn                    :: Jgtoyuh ()
                   }

        but got: Right
                 data XIILqcacwiuNiu = XIILqcacwiuNiu
                   { oyyFtvbepgbOge_pebzVmuftEijwuj     :: Jgtoyuh HessJvNlo
                   , wloQsiskdoxJop_xatiKrwedOxtu       :: Jgtoyuh [Inotg]
                   , mmmJjcqtemyIyo_ovosDoreKeeoyamvove :: Jgtoyuh Eujo
                   , mbiIatelofxOzr_uluxNngiiMjah       :: Jgtoyuh HessJvNlo
                   , obxIskfcxpkIkb_uuviTuevcSkrgo      :: Jgtoyuh Int
                   , wqrAtuvuecoHwr_ilotNxbuPleo        :: Jgtoyuh Ufaxdeq
                   , lofAfuebdhpLuv_cnekPoyFxmg         :: Jgtoyuh Ufaxdeq
                   , ouoFugtawzvUpk_oupiLzptugy         :: Jgtoyuh Eujo
                   , iqiXjtziwogNsa_uiyvSunaTtgUsf3     :: Jgtoyuh Oaivn
                   , odbIriaqnojUlz_onotoWuunehIpuy     :: Jgtoyuh Eujo
                   , opjUxtkxzkiKse_luqjuZazt
                       :: Jgtoyuh [(Eujo, Int, Int, Int, Int, Int, NELUxro)]
                 -- , jcqRaqznxfhIpa_ywevMezmoYkutuwa        :: Jgtoyuh ()
                   , vayOmuasyphOfd_bcsVljmvt               :: Jgtoyuh Eujo
                   , rifArahilooRax_ufikecqdImsv            :: Jgtoyuh Oaivn
                   , raqKtopcpszDwb_oqocubasZuqjcryoDojGkw  :: Jgtoyuh Oaivn
                   , mluJiilpcijUtt_gaisklifVekfeyagRmfbyzz :: Jgtoyuh Oaivn
                   , oqhPaahjupaSmi_gamwwoovKyxznecvEayluc  :: Jgtoyuh Oaivn
                   , mazFubimwebZpa_itidehDodiDlboz         :: Jgtoyuh Vrep
                   , jeyOcuesexaYoy_vpqn                    :: Jgtoyuh ()
                   }

  11) data type declarations records in sum
       expected: Right
                 -- brittany {lconfig_indentPolicy: IndentPolicyLeft }
                 data Foo
                   = Bar
                     { foo :: Int -- hello
                     , bar :: Foo
                     -- how are you
                     }
                   | Baz
                   | Biz
                     { foo :: Int
                     , bar :: Foo
                     }

        but got: Right
                 -- brittany {lconfig_indentPolicy: IndentPolicyLeft }
                 data Foo
                   = Bar
                     { foo :: Int -- hello
                     , bar :: Foo
                 -- how are you
                     }
                   | Baz
                   | Biz
                     { foo :: Int
                     , bar :: Foo
                     }
pbrisbin commented 4 years ago

Super excited to see progress here :clap:, thanks Evan!

I just happened to notice something in the test cases that might be worth a little bike-shedding, if it's still easy to change things at this stage:

       expected: Right
                 -- brittany {lconfig_indentPolicy: IndentPolicyLeft }
                 data Foo
                   = Bar
                     { foo :: Int -- hello
                     , bar :: Foo
                     -- how are you
                     }
                   | Baz
                   | Biz
                     { foo :: Int
                     , bar :: Foo
                     }

This is interesting to me in terms of consistency, given the possible non-sum styles of:

-- (1)
data Foo = Bar
  { foo
  }

-- (2)
data Foo
  = Bar
  { foo
  }

-- (3)
data Foo
  = Bar
    { foo
    }

In our own code base, we've shifted between (1) and (2): our "before my time" Style Guide has examples in style (2), but we aren't sure it was intentional and most of the current team prefers (1), so we most often use that style now.

Doing (1) for non-sums feels consistent with this sum test case: i.e. you only drop the = if some |s are coming, and { is always indented one column further than above.

Doing (2) for non-sum feels inconsistent to me since the = is always dropped but the { is only indented for sum-type cases.

I actually think aesthetically I do like this PR as-is, but I thought it was worthwhile to raise the consistency point. At the end of the day, "I don't care as long as it's automated" always wins (and sums-with-records is something we avoid generally), but if there's ever a time to actually dig in on a style discussion, it's probably when that automation itself is being written :smile:

lspitzner commented 4 years ago

Sorry that I have not given any feedback yet, and thanks for starting on this feature. First I got sidetracked by refactoring around butcher/czipwith/barbies/commandline-/config-parsing and today I started restructuring the test-suite.

This PR is a good start. but it needs more tests. Both to clear up the discussion on desired layout similar to what @pbrisbin mentions (and some existential/constraint/multiple-constructor/non-single-line combinations are non-trivial) and to get a tiny bit of coverage over the implementation.

I noticed the docPar (docLit "") trick that is very useful for several different cases and I had not realized it. Can probably clean up some existing layouter code with it :-) (which of course will be unrelated to this PR.)

(there are also some redundant patterns in this PR's code, but I'll just clean them up while fixing comment behaviour probably.)

My workspace is currently a half-broken mess; I'll try to push something in the next days.

vaclavsvejcar commented 4 years ago

Hello, is there any progress on this PR or is it stuck on something? It would be absolutely fantastic to have support for sum types in Brittany.

eborden commented 4 years ago

@vaclavsvejcar This isn't stuck for any particular reason. I think myself and @lspitzner have just not had the time to dedicate to it.