GHC source code improvement ideas

[Simon Marlow]

Twan van Laarhoven wrote:
> Hello GHC people,
> 
> I was trying my hand at some GHC hacking, and I couldn't help but notice 
> that much of the code looks (IMHO) horrible. There are some things that 
> look as if they haven't been touched since the Haskell 1.3 days. The 
> most striking is the use of `thenXX` instead of do notation.
> 
> I was thinking of cleaning up some of this. In particular:
> 
>  1a. Use do notation where possible instead of `thenXX`.

Yes.

>  1b. Even better, use Applicative where possible. For example
> 
>         ds_type (HsFunTy ty1 ty2)
>           = dsHsType ty1            `thenM` \ tau_ty1 ->
>             dsHsType ty2            `thenM` \ tau_ty2 ->
>             returnM (mkFunTy tau_ty1 tau_ty2)
> 
>      Could be rewritten as
> 
>         ds_type (HsFunTy ty1 ty2)
>           = mkFunTy <$> dsHsType ty1 <*> dsHsType ty2

No, IMO.  This just adds another abstraction that a potential GHC 
contributor has to learn about.  Including me :-)  It doesn't hurt to write 
  out the code in full sometimes, and as the GHC coding style guidelines say:

   Remember: other people have to be able to quickly understand what you've
   done, and overuse of abstractions just serves to obscure the really
   tricky stuff, and there's no shortage of that in GHC.

http://hackage.haskell.org/trac/ghc/wiki/Commentary/CodingStyle

(I think I might be quoting myself there, in which case it doesn't really 
add much credibility to my argument :).

Also bear in mind that we have to be able to compile GHC with older 
versions of itself (currently back to 6.2.2), which explains why we're 
often conservative with respect to using external libraries.

>  2. Investigate the need for all these mappM functions. To quote the 
> source: "Standard combinators, but specialised for this monad (for 
> efficiency)". Wouldn't a SPECIALIZE pragma work just as well?

It does look like we could do away with some of those, yes.

>  3. In general, use standard functions when they are available. 
> Currently GHC has its own sort function and its own versions of 
> FiniteMap/Data.Map and Text.PrettyPrint. Using standard functions/data 
> structures will a) make GHC smaller and therefore easier to maintain, 
> and b) will make the code easier to understand for someone who knows the 
> standard library.

In general yes.  But bear in mind there's another principle at work here: 
we want to insulate GHC as much as possible from its environment, to reduce 
the chances of spurious failures or performance regressions.

I believe our version of Text.PrettyPrint has diverged from the library 
version - last time I looked it wasn't obvious that we could replace it.

Regarding Data.Map, I'd be interested in trying out AVL trees instead, to 
see if they have better performance, but then we'd have to import the code 
of course.

>  4. A more radical change would be introducing hierarchical modules. 
> This could be just a matter of renaming the directories to start with an 
> upper case character and changing the import declarations. This gives 
> module names like "Typecheck.TcGadt". The tc is redundant here, so 
> perhaps it should be renamed to "Typecheck.Gadt" or "Typecheck.GADT". 
> Perhaps even better would be "GHC.Typecheck.GADT", this way some modules 
> can be exposed as part of the GHC api.

Yes, I've wondered about doing this in the past.  It does mean more typing, 
though: clearly 'import BasicTypes.Id' is more painful than just 'import 
Id' and doesn't add much, so IMO we'd need to put some thought into a 
structure that makes sense.

> How do the GHC developers feel about this? Would such paches be gladly 
> excepted? Or will they be directly forwarded to the bit bucket?

Within the constraints I've outlined above, cleanups are definitely welcome.

Another worthwhile gardening-type activity is to go around elimianting 
warnings.  Many modules have {-# OPTIONS -w #-} at the top, waiting for 
someone to go in and clean up all the warnings.  They do catch real bugs, 
so this is not a waste of time by any means.

In addition to this, things like

  - identifying and removing dead code
  - commoning up duplicate code fragments
  - improving test coverage
  - profiling and optimising hotspots

are all useful tasks.

Cheers,
	Simon