Add a Haskell style guide

[dhess]

PRIM-17

Over time, we should accrete a Haskell style guide. Our general approach will be to add things to the guide as they come up in review.

As we add more Haskell projects, we should maintain the style guide separately from Primer, but for now, this repo seems like the natural place for it. It seems likely that Primer will have some guidance that won't apply to Haskell projects in general, so we'll probably want to keep a supplemental Primer guide, in any case.

So far, we've agreed to the following inclusions:

• When writing comments, make use of proper Haddock markup
• When adding new functionality, write at least a few unit/property tests for it, unless it's trivial.
• Always use explicit imports & exports in libraries & executables.
  • Exception: Custom Prelude names can be imported unqualified.
• Explicit imports in tests are encouraged, but optional, as we want to keep the friction of writing tests to a minimum.
• Don't use leading underscores _ for lens names, unless it's necessary to avoid a name clash.
• Default to judiciously providing lenses for new types.
• Avoid using semicolons to string actions together on a single line.

A few more things seem obvious to me:

• Frequently-enabled GHC extensions should be considered for use in the Cabal file's default-extensions stanza.
• Start new Cabal projects with default-language: Haskell2010.
• Keep line length reasonable. (Let's agree on something, say 100 characters?)
• Code must pass project-specific lint & formatting checks before being committed to main. (Exceptions can be made.)
• Code must pass all tests before being committed to main.

Some other things we've discussed but haven't yet decided on:

• Whether we should generate lenses using generics or TH. We prefer generics, but are mindful of performance issues. (Decision pending the outcome of https://github.com/hackworthltd/primer/issues/141)
• Should we require/allow/ban explicit exports from test modules. (Decision pending the outcome of https://github.com/hackworthltd/primer/issues/142)

Finally, I would like to include the following things that we haven't discussed as a group yet:

• Primer is meant to be used as an online service, so it should never crash. Therefore, we should ban the use of error, undefined, and other partial functions that can crash the program (e.g., Prelude's head). Exceptions can be made in extreme cases, and it's fine to use these in tests.

[georgefst]

* Always use explicit imports & exports in libraries & executables.

It would be nice to have a CI check for this. AFAIK HLint isn't capable.

* Frequently-enabled GHC extensions should be considered for use in the Cabal file's `default-extensions` stanza.

140

* Keep line length reasonable. (Let's agree on something, say 100 characters?)

I'm happy with 100. I usually use 120 for personal projects, but I'm aware that's unusually high. By the way I'm hoping to get https://github.com/fourmolu/fourmolu/pull/102 merged in some form. So we'd be able to enforce this if we wanted.

[dhess]

* Always use explicit imports & exports in libraries & executables.

It would be nice to have a CI check for this. AFAIK HLint isn't capable.

Maybe Stan can do this, although I don't see anything similar in its ruleset: https://github.com/kowainik/stan/wiki/All-Inspections. I'm not sure whether it can be extended (easily).

By the way I'm hoping to get fourmolu/fourmolu#102 merged in some form. So we'd be able to enforce this if we wanted.

That would be very useful!

[brprice]

* Always use explicit imports & exports in libraries & executables.

It would be nice to have a CI check for this. AFAIK HLint isn't capable.

It looks like HLint can enforce explicit exports: https://github.com/ndmitchell/hlint/blob/master/hints.md#builtin-export, though I don't see anything similar for imports. https://github.com/ndmitchell/hlint/issues/1117 & https://github.com/ndmitchell/hlint/issues/769 imply that it is not currently possible, but patches may be welcome

[brprice]

It looks like HLint can enforce explicit exports

See https://github.com/hackworthltd/primer/pull/146

[dhess]

We should always use our new custom prelude Foreword, and lead with it like this:

import Foreword

import ...

[georgefst]

* Don't use leading underscores `_` for lens names, unless it's necessary to avoid a name clash.

I've just noticed that underscores at start of names can actually make GHC error messages a lot more confusing, because of the syntax already being used for unused variables and holes.

e.g. When forgetting to import one of our lenses, GHC assumes any attempted use is a hole. So we don't get useful messages like "perhaps you want to add 'x' to the import list of 'M'". Instead we get a warning about a hole, quite often drowned out in a sea of errors about ambiguous types.

This seems like all the more reason to rename our lenses.

[dhess]

As discussed in https://github.com/hackworthltd/primer/pull/195#discussion_r747447419, we should standardize on American spellings for names in code as well as in documentation. Apologies to my British teammates, but this is de facto Internet standard.

[dhess]

I'm not sure how we managed to miss this, but I think we should have some guidelines on what default deriving instances we should provide for our types. Show and Generic seem obvious, as does Eq for most cases. NFData seems quite common in modern Haskell code, as well.

[georgefst]

In #189, @dhess mentions:

taking full advantage of its [GHC-9.2's] features, like the new record syntax

We should discuss to what extent we intend to embrace this. Should we enable NoFieldSelectors globally, relying on dot syntax and lenses, and finally being able to have duplicate field names across different types without any concern?

This would allow us to ditch the ugly prefixes we have in record names for so many types. There are some particularly horrible field names due to the Def/TypeDef changes in #210. I'd love to go back and clean up this sort of thing, but it may not be the best use of time.

Another benefit would be that we can produce more compact JSON output without the need for workarounds like Primer.JSON.VJSONPrefix (which incidentally, we have almost entirely forgotten to use).

(see also #140)

[dhess]

I think I'm pretty much ready to give up on GHCJS, so unless something major on that front changes between now and when 9.2.2 ships, I'm in favor of embracing GHC 9.2 features and not looking back.

I do think we should "page in" these features by tackling them as the need/opportunity arises, rather than spending a significant effort back-porting them, however.

[dhess]

The style guide should indicate which versions of GHC the code must support (which should be enforced by CI checks, at least before merges to main). However, I don’t think we should worry about any versions earlier than our development compiler (currently 9.4.5) until we ship 1.0, as we needn’t worry about backwards compat until then.