New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support reexported-modules #563

Open
Profpatsch opened this Issue Dec 23, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@Profpatsch

Profpatsch commented Dec 23, 2016

It would be nice if Haddock could handle the reexported-modules list, otherwise those are not shown at all.

For example, ghcjs-dom reexports modules from its dependencies, but no documentation is generated at all:
https://hackage.haskell.org/package/ghcjs-dom

Similarly, ghcjs-dom-jsffi reexports and renames said modules, those are then used (and reexported) by ghcjs-dom again:
https://hackage.haskell.org/package/ghcjs-dom-jsffi-0.7.0.1/ghcjs-dom-jsffi.cabal

Also, the documentation is broken in that case (ghcjs-dom references the renamed modules internally).

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Dec 23, 2016

Contributor

Agreed with @Profpatsch. Unfortunately, fixing this is not as simple as just fixing Haddock, because both Hackage and Stackage manually reimplemented the module listing logic, so that logic needs to be adjusted as well for module reexports. This seemed to involve some sort of massive refactor which is why I ended up putting it off (the other yak I need to shave being making it easy to deploy changes to Hackage...)

Contributor

ezyang commented Dec 23, 2016

Agreed with @Profpatsch. Unfortunately, fixing this is not as simple as just fixing Haddock, because both Hackage and Stackage manually reimplemented the module listing logic, so that logic needs to be adjusted as well for module reexports. This seemed to involve some sort of massive refactor which is why I ended up putting it off (the other yak I need to shave being making it easy to deploy changes to Hackage...)

@Profpatsch

This comment has been minimized.

Show comment
Hide comment
@Profpatsch

Profpatsch Dec 23, 2016

Is there any part of the Haskell infrastructure that is currently not maintained by you? oO

Is there any part of the Haskell infrastructure that is currently not maintained by you? oO

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Dec 24, 2016

Contributor

Nah, I don't have commit or deploy bits for Hackage ;)

Contributor

ezyang commented Dec 24, 2016

Nah, I don't have commit or deploy bits for Hackage ;)

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Dec 24, 2016

@ezyang I would also like to have this feature, since this one also leads to contradicting recommendations (hlint etc). I didn't really understand your comment about the module listing logic. Are there larger architectural changes planned or is it possible to fix that in a quick way that just works?

minad commented Dec 24, 2016

@ezyang I would also like to have this feature, since this one also leads to contradicting recommendations (hlint etc). I didn't really understand your comment about the module listing logic. Are there larger architectural changes planned or is it possible to fix that in a quick way that just works?

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Dec 24, 2016

Contributor

I have already written a patch to change the Haddock generated index page to handle reexported modules, the patch is here: #547 But as I mentioned above, this only affects the index page that Haddock creates, which is NOT the one that Hackage/Stackage uses.

I've never worked with the hackage-server codebase before, so I didn't get very far trying to adjust hackage-server (once again, mostly because I knew, even if I patched hackage-server, I didn't know how I was going to go about deploying my changes. At this point I decided to terminate the yakstack.)

Here is what I did find: there is a reimplementation of the index logic in the following modules:

I didn't spend very much time figuring out how to make this work.

There is another possible approach to solving this problem, which is to go about doing the Haddock patch differently: instead of just linking to the reexport module documentation, you just create another copy of the documentation from the current package. But this is a bit annoying because Haddock's interface files don't seem to save enough information to actually do this (it's the difference between InstalledInterface and Interface in https://github.com/haskell/haddock/blob/master/haddock-api/src/Haddock/Types.hs). I tried making Interface serializable but it had some references to GHC types so I gave up. Maybe there's a different way we could go about doing it.

Contributor

ezyang commented Dec 24, 2016

I have already written a patch to change the Haddock generated index page to handle reexported modules, the patch is here: #547 But as I mentioned above, this only affects the index page that Haddock creates, which is NOT the one that Hackage/Stackage uses.

I've never worked with the hackage-server codebase before, so I didn't get very far trying to adjust hackage-server (once again, mostly because I knew, even if I patched hackage-server, I didn't know how I was going to go about deploying my changes. At this point I decided to terminate the yakstack.)

Here is what I did find: there is a reimplementation of the index logic in the following modules:

I didn't spend very much time figuring out how to make this work.

There is another possible approach to solving this problem, which is to go about doing the Haddock patch differently: instead of just linking to the reexport module documentation, you just create another copy of the documentation from the current package. But this is a bit annoying because Haddock's interface files don't seem to save enough information to actually do this (it's the difference between InstalledInterface and Interface in https://github.com/haskell/haddock/blob/master/haddock-api/src/Haddock/Types.hs). I tried making Interface serializable but it had some references to GHC types so I gave up. Maybe there's a different way we could go about doing it.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Dec 24, 2016

Thx for the detailed explanation!

minad commented Dec 24, 2016

Thx for the detailed explanation!

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Jan 19, 2017

Contributor

My thinking on this matter has changed; I think that it is probably best if we teach Haddock how to reproduce documentation for reexported modules, rather than try to do something funny with redirecting links to the sources of them. So maybe there is some way to reconstruct the GHC types next time we're running (maybe by reading in their interfaces) and going from there.

Contributor

ezyang commented Jan 19, 2017

My thinking on this matter has changed; I think that it is probably best if we teach Haddock how to reproduce documentation for reexported modules, rather than try to do something funny with redirecting links to the sources of them. So maybe there is some way to reconstruct the GHC types next time we're running (maybe by reading in their interfaces) and going from there.

@Profpatsch

This comment has been minimized.

Show comment
Hide comment
@Profpatsch

Profpatsch Jan 20, 2017

next time we're running

While we’re at it maybe implement hashing (incremental rebuild) as well? The interface files don’t have the comments I assume? But hashing over the input hs files should be a good first approximation (up to edge cases) probably?

next time we're running

While we’re at it maybe implement hashing (incremental rebuild) as well? The interface files don’t have the comments I assume? But hashing over the input hs files should be a good first approximation (up to edge cases) probably?

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Jan 22, 2017

Contributor

@Profpatsch File another bug for that ;)

Contributor

ezyang commented Jan 22, 2017

@Profpatsch File another bug for that ;)

@crockeea

This comment has been minimized.

Show comment
Hide comment
@crockeea

crockeea Feb 28, 2017

I've just been trying to re-export modules with Haddock, so I'll just post a couple of things I discovered in the process:

  1. If you export renamed modules as in
    module Foo (module X) where
    import module A as X
    import module B as X
    
    documentation for A and B do not appear in the documentation for Foo.
  2. If you export unrenamed modules, documentation for A and Bdoes appear in docs for Foo:
    module Foo (module A, module B) where
    import module A
    import module B
    
  3. If you import a module hiding some items, and then re-export the module, the hidden items still appear in the documentation:
    module Foo (module A, module B) where
    import module A hiding (a)
    import module B
    
    In this example, documentation for a would appear in the docs for Foo, even though a is not exported from Foo. This is rather annoying.

(All of this assumes that modules A and B are hidden in the cabal file, so that their contents should be dumped into Foo in an ideal world.)

crockeea commented Feb 28, 2017

I've just been trying to re-export modules with Haddock, so I'll just post a couple of things I discovered in the process:

  1. If you export renamed modules as in
    module Foo (module X) where
    import module A as X
    import module B as X
    
    documentation for A and B do not appear in the documentation for Foo.
  2. If you export unrenamed modules, documentation for A and Bdoes appear in docs for Foo:
    module Foo (module A, module B) where
    import module A
    import module B
    
  3. If you import a module hiding some items, and then re-export the module, the hidden items still appear in the documentation:
    module Foo (module A, module B) where
    import module A hiding (a)
    import module B
    
    In this example, documentation for a would appear in the docs for Foo, even though a is not exported from Foo. This is rather annoying.

(All of this assumes that modules A and B are hidden in the cabal file, so that their contents should be dumped into Foo in an ideal world.)

@ezyang

This comment has been minimized.

Show comment
Hide comment
@ezyang

ezyang Mar 7, 2017

Contributor

Note that Haskell level reexported modules are different from Cabal reexported-modules (confusingly!)

Second, the behavior for how module reexports is specified in the docs: https://www.haskell.org/haddock/doc/html/ch03s04.html#idm140354810861024 I do think that Haddock's default here is wrong though, and we should make embedding the docs the default. This has gotten GHC.Exts into trouble http://ghc.haskell.org/trac/ghc/ticket/10886 so it's worth fixing (although maybe there should be another ticket for it.)

Contributor

ezyang commented Mar 7, 2017

Note that Haskell level reexported modules are different from Cabal reexported-modules (confusingly!)

Second, the behavior for how module reexports is specified in the docs: https://www.haskell.org/haddock/doc/html/ch03s04.html#idm140354810861024 I do think that Haddock's default here is wrong though, and we should make embedding the docs the default. This has gotten GHC.Exts into trouble http://ghc.haskell.org/trac/ghc/ticket/10886 so it's worth fixing (although maybe there should be another ticket for it.)

@crockeea

This comment has been minimized.

Show comment
Hide comment
@crockeea

crockeea Mar 7, 2017

Ah, I didn't realize there was a difference. My comments can now be found in #584.

crockeea commented Mar 7, 2017

Ah, I didn't realize there was a difference. My comments can now be found in #584.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Mar 7, 2017

@ezyang @crockeea When I made my comment, I also confused that and really meant #584. But lately I've also found that cabal reexports don't work, which is this issue :)

minad commented Mar 7, 2017

@ezyang @crockeea When I made my comment, I also confused that and really meant #584. But lately I've also found that cabal reexports don't work, which is this issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment