Source hyperlinker

[panhania]

First part of my Google Summer of Code project. I am sure that there are many, many issues I am not aware of, so treat is as a beta for now. The real testing will begin once Cabal incorporates this to its pipeline.

[Fuuzetsu]

Presumably 0d0550c is for lens crash, right?

Could we perhaps make the typ helper used there less prone to crashing or more obvious that it may crash? For example we could try to catch the exception when it happens from GHC and do something (even crashing with better information would be an improvement). Probably better solution is to make the patterns explicit instead of using _ on LHS and blindly calling tcdLName. At least we'll then know when GHC moves things around and we need to update.

It is very good to see you adding tests. GHC is a moving target and without tests it's pretty much impossible to tell how/when something breaks.

What's the situation with unicode? I know it was a TODO and I haven't seen changes. Even if the linking doesn't work optimally, we need to operate as usual. What does HsColour behave like? Notably, I don't want a regression in functionality. If HsColour is able to highlight unicode stuff properly but we aren't then that'd be a regression IMHO.

I wonder if you thought about how we could go through migration from HsColour. I would not like to exclude the possibility to use it. We can't update old cabal versions so we should probably make sure that if they call Haddock how they used to, stuff still works (even if our own highlighter is used). I think I will create an issue over at cabal and see how they want to go about it and what needs they have: maybe some stuff on Hackage needs to be flipped too.

If we ship with some grave bug that we don't catch but it becomes unusable for people then the option to use HsColour as usual needs to be there, at least for a release or two. After it has been battle-tested a bit, we could potentially remove any extra stuff we don't need for it (is there any?).

It would be good for you to prepare some kind of release notes for this feature. What it does and how to use. If you prefer, you can write a sort of a blog post and publish it somewhere once we merge and I can link to that when announcing new release (though of course you're free to publish early with subject-to-change disclaimer). Eventually we'll also need some words in the documentation, notably updating the flag descriptions and adding any new ones.

For early adopters, we should tell them what flags to use before cabal makes it default (if it has to do any work at all). Luckily there is nowadays a --hadock-options flag we can piggy-back on if need be.

The problem is with GHC API where the AST node span for leftmost a includes everything until the end of the line (the problem does not exist for version with explicit outer forall). I suspect that might be a bug in GHC itself.

If it's a bug in GHC then we need to check if it's know or not and report it if necessary. @alanz is this stuff in your domain of reign?

Are there any bugs you know of or major features that are missing still? If there's something seriously broken then we need to fix it ahead of time and if not possible, ticket it. If there are non-critical issues missing then we need to open tickets. Assuming this PR works as advertised, I can't think off the top of my head of anything major except cross-package links which are out of scope for this right now or anything I haven't already mentioned/asked about.

I don't want to have Edward K on my case because Haddock is crashing or otherwise misbehaving on one of his libraries again.

Lastly, do you think what's here is OK for merging and use? Basically, do you have confidence in that it will work? I can only test so much myself before running out of time: if you can add tests for more cases then do! Stuff like the lens crash was only discovered because I happened to run on it but lens is only one package out of many thousand. If I see a test for something, I don't have to write or find a package that uses that feature saving me a lot of time and saving everyone time in the future.

I would like to merge within few days, maybe Saturday/Sunday if you give a heads up.

[panhania]

Presumably 0d0550c is for lens crash, right?

Yep.

Could we perhaps make the typ helper used there less prone to crashing or more obvious that it may crash? For example we could try to catch the exception when it happens from GHC and do something (even crashing with better information would be an improvement). Probably better solution is to make the patterns explicit instead of using _ on LHS and blindly calling tcdLName. At least we'll then know when GHC moves things around and we need to update.

Done the explicit pattern version. The problem is that constructors in the AST are sometimes quite intimidating, like with the ClassDecl case with 10 arguments. Fortunately (thanks to your suggestion for a hyperlinker test) I learned about RecordWildCards which made it a little bit nicer.

What's the situation with unicode? I know it was a TODO and I haven't seen changes. Even if the linking doesn't work optimally, we need to operate as usual. What does HsColour behave like? Notably, I don't want a regression in functionality. If HsColour is able to highlight unicode stuff properly but we aren't then that'd be a regression IMHO.

HsColour doesn't recognize unicode either. And lack of it isn't that terrible, just some operators like ≫ being rendered as identifiers instead of whatever was set in CSS file. For token classification is use standard functions like isUpper, isAlpha, isSpace etc. which (allegedly) support Unicode characters.

I wonder if you thought about how we could go through migration from HsColour. I would not like to exclude the possibility to use it. We can't update old cabal versions so we should probably make sure that if they call Haddock how they used to, stuff still works (even if our own highlighter is used). I think I will create an issue over at cabal and see how they want to go about it and what needs they have: maybe some stuff on Hackage needs to be flipped too.

If we ship with some grave bug that we don't catch but it becomes unusable for people then the option to use HsColour as usual needs to be there, at least for a release or two. After it has been battle-tested a bit, we could potentially remove any extra stuff we don't need for it (is there any?).

As long as you don't use --hyperlinked-source flag, everything works as before. There is not much extra stuff except for source-* flags for specifying anchor format (which are ignored when --hyperlinked-source is on). They are not very HsColour-specific though. I think having them does no harm and can come handy in case we decide to make hyperlinker an external tool or something.

It would be good for you to prepare some kind of release notes for this feature. What it does and how to use. If you prefer, you can write a sort of a blog post and publish it somewhere once we merge and I can link to that when announcing new release (though of course you're free to publish early with subject-to-change disclaimer). Eventually we'll also need some words in the documentation, notably updating the flag descriptions and adding any new ones.

For early adopters, we should tell them what flags to use before cabal makes it default (if it has to do any work at all). Luckily there is nowadays a --hadock-options flag we can piggy-back on if need be.

I updated the documentation file (I hope I did it right as I have never done anything in DocBook before). Essentially, there are only two new flags and that's it. I have also added some information about (previously undocumented) source path option in --read-interface flag which I use now to enable cross-package hyperlinking - I am not sure what was the use of that before, though.

Are there any bugs you know of or major features that are missing still? If there's something seriously broken then we need to fix it ahead of time and if not possible, ticket it. If there are non-critical issues missing then we need to open tickets. Assuming this PR works as advertised, I can't think off the top of my head of anything major except cross-package links which are out of scope for this right now or anything I haven't already mentioned/asked about.

I don't want to have Edward K on my case because Haddock is crashing or otherwise misbehaving on one of his libraries again.

Lastly, do you think what's here is OK for merging and use? Basically, do you have confidence in that it will work? I can only test so much myself before running out of time: if you can add tests for more cases then do! Stuff like the lens crash was only discovered because I happened to run on it but lens is only one package out of many thousand. If I see a test for something, I don't have to write or find a package that uses that feature saving me a lot of time and saving everyone time in the future.

I checked it on vector, lens and Cabal and it seems to work fine. But we can treat it as a beta feature, encourage people to try it out and just stick to HsColour for the time being if something goes wrong.

The only issues I know about:

• no module hyperlinks in imports
• incorrect type variable highlighting in certain cases (with RankNTypes)

I will create appropriate issues for them.

That being said, this is Haskell and tons of weird extensions I have never heard about. While it shouldn't crash no matter what (but obviously I can't guarantee that), it may produce wrong hyperlinks or syntax highlighting from time to time. But everything that HsColour does right now, my hyperlinker should be capable of as well.

[Fuuzetsu]

I have also added some information about (previously undocumented) source path option in --read-interface flag which I use now to enable cross-package hyperlinking

Can you elaborate? We don't have cross-package hyperlinking do we?

But we can treat it as a beta feature, encourage people to try it out and just stick to HsColour for the time being if something goes wrong.

I find that not many people tend to do this. It's easier for us to make it the default and then advise to report issues and use HsColour if there's a problem.

While it shouldn't crash no matter what (but obviously I can't guarantee that), it may produce wrong hyperlinks or syntax highlighting from time to time.

Good enough. Logic bugs we can fix later, crashes are immediate problems.

But everything that HsColour does right now, my hyperlinker should be capable of as well.

Good!

[Fuuzetsu]

Oh yeah, and

The problem is that constructors in the AST are sometimes quite intimidating, like with the ClassDecl case with 10 arguments. Fortunately (thanks to your suggestion for a hyperlinker test) I learned about RecordWildCards which made it a little bit nicer.

Alternatively you could have done something like

typ (ClassDecl { decl = decl }) = …

or

typ c@(ClassDecl {}) = … decl c …

[panhania]

Can you elaborate? We don't have cross-package hyperlinking do we?

We do if there is interface file available. For example, assume our package depends on filepath. So first we generate docs for filepath:

cabal haddock --html --haddock-options="--hyperlinked-source --dump-interface=filepath.iface"

And then you can do something like:

FILEPATH_ROOT="~/Desktop/filepath"
FILEPATH_IFACE="${FILEPATH_ROOT}/filepath.iface"
FILEPATH_HTML="${FILEPATH_ROOT}/dist/doc/html/filepath"
FILEPATH_SRC="${FILEPATH_HTML}/src"
cabal haddock --html --haddock-options="--hyperlinked-source --read-interface=${FILEPATH_HTML},${FILEPATH_SRC},${FILEPATH_IFACE}"

... except I just did it and something seems to be broken. </>s are hyperlinked but FilePaths are not for some reason. It worked before, I will look into this tomorrow.

Alternatively you could have done something like

Thanks, I wasn't aware you can do partial record unpacking.

[panhania]

... except I just did it and something seems to be broken. </>s are hyperlinked but FilePaths are not for some reason. It worked before, I will look into this tomorrow.

Mystery solved: FilePaths are not hyperlinked because they originate from base (Prelude, System.IO), not from filepath. However, when Haddock generates documentation it is somehow able to associate FilePath with System.FilePath.Windows. The magic happens in Haddock.Interface.Rename module but I have not yet looked at it in detail.

Generally, these cross-package hyperlinking issues should happen only for things that are somewhat duplicated which is quite uncommon for things outside base I think.

There are also some good news: when searching for the source of a problem, I did a bit of refactoring and that gave me an idea how to solve problem with imports hyperlinking.

[Fuuzetsu]

Awesome.

[panhania]

I have investigated how Haddock is able to hyperlink FilePath to System.FilePath.Windows of filepath instead of System.IO of base and I think I am not gonna do the same thing in hyperlinker. It creates link environments as a heuristic (sort of?) and that may possibly lead to sometimes incorrect links. Whether it is better to have incorrect link instead of none is of course debatable, but:

• it would add some twisted logic to hyperlinker
• it happens only in pathological cases
• in real world it will be quite uncommon to have interface for one dependency and to not have for another

And once you have interface file and hyperlinked source for base (by the way, one more library tested!) FilePaths work as expected. So, I think we can live with this.

---

I have finally fixed issue with hyperlinking modules in import statements. I suspect AST does not have Module reference in these because multiple packages can share a module (maybe, I don't know). If that's the case, then hyperlink will point to arbitrary file declaring this module.

---

Anyway, I think we are ready for a merge. Or maybe there are some new thoughts?

[Fuuzetsu]

I'll try to merge today.

[Fuuzetsu]

Please open issues/feature requests discussed throughout this PR and any other you can think of.

We will no doubt mess with the code from this PR here and there in the future but you should now focus on the second part of your GSOC proposal.