haskellPackages: doHaddockInterfaces

[pepeiborra]

Motivation for this change

Adds a new builder option doHaddockInterfaces to enable the -haddock flag in GHC,
which results in Haddock comments parsed at compile-time and embedded in
interface files. These are used by the :doc command in GHCi, as well as by IDE
tools like ghcide and hls to display docs on hover.

The -haddock flag has been around since at least 8.2, even though it does not
get a mention in the GHC Users guide.

There are two downsides to turning on this flag:

1. Increased compile times, since Haddocks must be parsed and then encoded
2. Haddock parse errors now become compile errors

Item 2 in particular can be quite problematic for benchmarks and test suites,
where Haddock parse errors would not be a consideration.
This explains the default setting for doHaddockInterfaces being doHaddock && !doCheck .
Once the problem is fixed in
GHC, the doHaddockInterfaces default value can be updated to be always on in
certain GHC versions.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[cdepillabout]

@pepeiborra Would you be able to explain a little bit more about what this does, and why we would want to enable it? (And possibly link to upstream documentation? Although it sounds like it might not be documented upstream...)

In my experience, many packages have incorrect haddocks, so I'd be somewhat hesitant to enable this by default if it causes compile errors.

[pepeiborra]

@pepeiborra Would you be able to explain a little bit more about what this does, and why we would want to enable it? (And possibly link to upstream documentation? Although it sounds like it might not be documented upstream...)

In my experience, many packages have incorrect haddocks, so I'd be somewhat hesitant to enable this by default if it causes compile errors.

The -haddock flag instructs GHC to parse doc comments with Haddock, and embed the results inside .hi files, which are compilation artifacts and get distributed together with libraries. This makes Haddocks available to ghci and other tools like ghcide/hls, which is the main motivation to enable it.

Your concern is fair, some packages have incorrect haddocks (specially in test suites) and -haddock will break the build. But I believe this PR should not introduce a problem, because:

• The default value for doHaddockInterfaces is doHaddock && !doCheck which equals False as the default value for doCheck is true
• There are lib functions to change doHaddockInterfaces on a per library basis.

[cdepillabout]

@pepeiborra Ah, so you're saying that anywhere we are already building haddocks, then the addition in this PR shouldn't cause any new build failures?

In that case, it seems pretty safe to add this.

Could you rebase this on the haskell-updates branch, as well as change the base branch here on github?

---

cc @peti and @maralorn about this as well

[pepeiborra]

@pepeiborra Ah, so you're saying that anywhere we are already building haddocks, then the addition in this PR shouldn't cause any new build failures?

Kind of, except that cabal haddock doesn't parse comments in exe/test/bench suites, but this change will.

Unfortunately I believe there is no way to instruct Cabal to use a GHC option only in the library stanza. This has been raised in the Cabal issue tracker, but no mention of progress:

• Does cabal need configuration option to enable -haddock and -fwrite-ide-info for supported compilers haskell/cabal#6825

[cdepillabout]

@pepeiborra Normally what happens with these types of PRs is that @peti will try merging it into the haskell-updates branch, and we see how many additional compilation errors it causes.

If it is just a few, then we fix them up by hand. If it is a bunch, then we merge it in but keep the option off by default.

[cdepillabout]

Lets see if ofborg is able to build a couple common packages for us:

@GrahamcOfBorg build haskellPackages.lens
@GrahamcOfBorg build haskellPackages.xmonad
@GrahamcOfBorg build haskellPackages.spago

[maralorn]

What I really don‘t like about this change is, that it favors package without tests.
I think having incentive "I wish I had haddock informations in this hover" -> "lets just disable the tests in nixpkgs" could be a bad idea.

On the other hand I am obviously hugely in favor for improving the IDE experience. And if we regard this as an experiment to be improved upon, I am in favor.

[maralorn]

Also: This shows that I would be awesome to have better tuned control over different cabal outputs in nix.

[pepeiborra]

What I really don‘t like about this change is, that it favors package without tests.
I think having incentive "I wish I had haddock informations in this hover" -> "lets just disable the tests in nixpkgs" could be a bad idea.

On the other hand I am obviously hugely in favor for improving the IDE experience. And if we regard this as an experiment to be improved upon, I am in favor.

I am not a huge fan either, but this is what I arrived at after much thinking and tinkering. If you have a better idea, I would love to hear it.

Even when/if the issue gets fixed in GHC, backwards compatibility with previous GHC releases will limit our options here.

[cdepillabout]

At the very least, this now works with lens, spago, and xmonad (and all their dependencies).

[peti]

I don't like the constraint doHaddock && !doCheck very much. "Haddock information" feels like a totally different thing than "test suites", and I'd rather not create some connection between the two ... because there is none.

How about we enable the new Haddock feature for all packages, test build the package set, and see how many actual errors we'll have? If it's just a handful, then we could simply open PRs for the respective upstreams and fix those issues?

[pepeiborra]

I don't like the constraint doHaddock && !doCheck very much. "Haddock information" feels like a totally different thing than "test suites", and I'd rather not create some connection between the two ... because there is none.

How about we enable the new Haddock feature for all packages, test build the package set, and see how many actual errors we'll have? If it's just a handful, then we could simply open PRs for the respective upstreams and fix those issues?

I suspect it's quite a few packages that have comments in their test suites that trip up the Haddock parser. Even if we open PRs for all of them, it would take a long time for all those PRs to get merged and eventually released. And it looks like this issue will be fixed in GHC 8.12 anyway.

Would you object to this PR if the default value for the new option was doHaddockInterfaces ? false ?

[peti]

I suspect it's quite a few packages that have comments in their test suites that trip up the Haddock parser. Even if we open PRs for all of them, it would take a long time for all those PRs to get merged and eventually released.

This is speculation, though. I'd rather know how many packages actually break, and we have the means to find out thanks to Hydra.

Would you object to this PR if the default value for the new option was doHaddockInterfaces ? false ?

No, I think that would be perfectly fine.

[pepeiborra]

I suspect it's quite a few packages that have comments in their test suites that trip up the Haddock parser. Even if we open PRs for all of them, it would take a long time for all those PRs to get merged and eventually released.

This is speculation, though. I'd rather know how many packages actually break, and we have the means to find out thanks to Hydra.

I'm not that familiar with Nix and Hydra. Do I just push a change to my branch to turn the setting on and wait for results?

[pepeiborra]

I have pushed a change to turn on DoHaddockInterfaces by default. What's next?

[peti]

I have merged your changes into the branch pr-91557. That branch is built continuously by https://hydra.nixos.org/jobset/nixpkgs/pr-91557, so any changes we'll push there will be tested by Hydra (with a slightly delay because our job doesn't have a very high priority).

[pepeiborra]

4131 failures, most of them caused by a parse error in the random-1.1 testsuite.
It's not even clear what causes the parse error: https://github.com/haskell/random/blob/v1.1/tests/T7936.hs

My view is that this is a bug in GHC, not something that needs to be fixed in packages downstream, and the bugfix will be included in 8.12 (or 8.14 if it doesn't make the cut).

With that in mind, I have changed the default value to enable the setting only in GHC >= 8.12

[cdepillabout]

@GrahamcOfBorg build haskellPackages.lens
@GrahamcOfBorg build haskellPackages.xmonad
@GrahamcOfBorg build haskellPackages.spago
@GrahamcOfBorg build haskellPackages.random

[pepeiborra]

good to merge now or are there any changes requested?

[peti]

What does ghc-8.12.x have to do with this PR?

[pepeiborra]

8.12.x will turn Haddock errors into warnings, provided !2377 lands before the cut

[peti]

4131 failures, most of them caused by a parse error in the random-1.1 testsuite.

Now is the time to start fixing those failures ...

My view is that this is a bug in GHC.

Really? That doesn't look like a compiler bug to me at all. You just re-format the line

--- a/test-legacy/T7936.hs
+++ b/test-legacy/T7936.hs
@@ -3,8 +3,8 @@
 --
 -- Used to fail with:
 --
--- $ cabal test T7936 --test-options="+RTS -M1M -RTS"
--- T7936: Heap exhausted;
+-- > $ cabal test T7936 --test-options="+RTS -M1M -RTS"
+-- > T7936: Heap exhausted;
 
 module T7936 where

properly, and then it compiles just fine.

[pepeiborra]

4131 failures, most of them caused by a parse error in the random-1.1 testsuite.

Now is the time to start fixing those failures ...

Respectfully, we don't know how many failures there are, maintainers will be reluctant to cut a release just to change a comment in a test suite, and all that is wasted effort once !2377 is merged.

My view is that this is a bug in GHC.

Really? That doesn't look like a compiler bug to me at all. You just re-format the line

There is no error message referencing the problematic markup, Haddock, or even the line that caused the error:

tests/T7936.hs:9:1: error: parse error on input ‘module’
  |
9 | module Main where
  | ^^^^^^

The -haddock option is not mentioned in the GHC Users' guide and Cabal doesn't know about it, so even if we fixed all the problems in Hackage, it would only be a short matter of time before they appeared again. Imho the only viable way forward is waiting until !2377 is merged to enable the option.

[peti]

I have changed the default value to enable the setting only in GHC >= 8.12

I'd rather not a time bomb like that included in Nixpkgs. If you are not going to support any efforts to fix those errors, then the only way I want that new feature included is if it's off by default.

[pepeiborra]

That's fine by me, I'll submit another PR to enable conditionally once !2377 has landed in GHC.

[pepeiborra]

!2377 has been scheduled for merging[1], so I would like to amend this PR to enable doHaddockInterfaces by default on 8.12

[1] https://gitlab.haskell.org/ghc/ghc/-/merge_requests/2377#note_289455

[maralorn]

I think @peti's point about the time bomb here is, that while this might be perfectly reasonable code, we can't test this before we have 8.12 in nixpkgs.

Maybe we can merge this PR now and instantly create a new Pr which switches the default on 8.12, but only merge that one, when have tested it? I mean we don't loose anything by merging something that only affects 8.12 only when we actually have 8.12, right?

[expipiplus1]

How badly does this affect compile times?

Haskell compile times are already bad, and Haskell compile times with Nix are already worse (frequent rebuilds, ×2 time for profiling builds, ineffective CPU utilization), it would be sad to see this get even worse.