Haddock needs to pass visible modules for instance filtering

Authored by harpocrates on Jan 6 2018, 11:35 AM.



The GHC-side getNameToInstancesIndex filters out incorrectly some
instances because it is not aware of what modules are visible. Using
runTcInteractive means that ie_visible gets initialized to a one
module set containing some dummy GHCi module. This is clearly not the
module set we want to check against to see if a given orphan instance
is visible or not.

In fact, GHC has no way of knowing what we want that module set to be
since it doesn't know ahead of time which modules Haddock is making its
docs for. The fix is just to pass that set in as an argument.

Diff Detail

rGHC Glasgow Haskell Compiler
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
harpocrates created this revision.Jan 6 2018, 11:35 AM

Here is the Haddock PR: https://github.com/haskell/haddock/pull/724

I just checked and Show Double, Show Float are also back in the instance lists for the Prelude!


harpocrates added a comment.EditedJan 6 2018, 12:25 PM

Actually, on closer inspection, this fixes a bunch of missing instance issues in base (I imagine outside of base too, but checking that would involve compiling a lot of large libraries with HEAD, and I don't have the time/willpower to do this now).

One missing instance it still does not find is instance (Ix a, Read a, Read b) => Read (Array a b) (see https://github.com/haskell/haddock/issues/469). I'll continue investigating this, but I reckon any change for that will be only on Haddock's side (changing exactly which modules are passed to getNameToInstancesIndex).

The build fails because this is a breaking change to a function Haddock uses and I've made the corresponding Haddock change only in the Haddock PR (https://github.com/haskell/haddock/pull/724), so naturally the Haddock bundled with GHC now fails to build. I don't know what the right way of handling this is (feel free to manually port over the Haddock change - it is literally one line).

@bgamari any chance of squeezing this tiny GHC-side change into 8.4?

@harpocrates we can merge your PR on Github; then you can include a haddock submodule bump here.
@bgamari let's put this into 8.4.1 O:-)

duog added a subscriber: duog.Jan 7 2018, 3:29 PM

Looks great, thanks!


I think this comment could be clearer, perhaps something like:

-- ^ Visible modules. An orphan instance will be returned if and only it is visible from at least one module in the list.
  • Better comment
harpocrates marked an inline comment as done.Jan 8 2018, 11:03 AM

@harpocrates Have you found out why this still misses some instances?

@harpocrates Have you found out why this still misses some instances?

No, and I'm unlikely to make real progress on this front in the next week (I'll try, but my day job is going to be busier).

The Read Array instance is located in Text.Read and the Array data type is defined in GHC.Arr. Just looking at documentation of base (assuming you remove the the right {-# OPTIONS_HADDOCK hide #-}) you'll see that the Read Array instance does show up under data Array. However, it does not show up under the re-exported data Array in the array.

To solve this, we just need to figure out why this is happening. Note that there are other instances, like Functor Array, which share pretty much all of the setup that Read Array has, but do show up in the re-exported array docs. My next point of debugging would be to figure out at what point Functor Array is different that Read Array. I reckon the answer to this will lead to a solution to this whole problem.

bgamari accepted this revision.Jan 21 2018, 10:50 AM

Looks reasonable to me.

This revision is now accepted and ready to land.Jan 21 2018, 10:50 AM
This revision was automatically updated to reflect the committed changes.