Skip to content
This repository was archived by the owner on Jan 2, 2021. It is now read-only.

Locate Documentation html page using Haddock interface file #861

Closed

Conversation

DunetsNM
Copy link
Contributor

A WIP for #860 , not ready for review

So far can make it work only for GHC 8.10.2 , having issues with haddock-api in other versions.

@wz1000
Copy link
Collaborator

wz1000 commented Oct 11, 2020

This seems like the correct approach, but I'm a bit concerned about reading the interface file on each hover. Perhaps we should have a global IORef that maintains a cache of the ifLinkEnvs for all the interface files we have already read.

@DunetsNM
Copy link
Contributor Author

afaik tooltip is not generated per mouse hover but once per opened .hs file/module. I was concerned too that reading all interface files in advance can be a bottleneck but it looked fast enough to me, I didn't profile memory though, should be OK as it retains only paths to html doc files in memory not the whole interface data structure

@pepeiborra
Copy link
Collaborator

afaik tooltip is not generated per mouse hover but once per opened .hs file/module. I was concerned too that reading all interface files in advance can be a bottleneck but it looked fast enough to me, I didn't profile memory though, should be OK as it retains only paths to html doc files in memory not the whole interface data structure

Neither. mkDocMap is called on every keystroke, so I would prefer caching the results of readInterfaceFile in a Shake rule. In case of doubt we have a benchmark suite: just run cabal bench and look at the results

@wz1000
Copy link
Collaborator

wz1000 commented Oct 21, 2020

I don't think we need a shake rule, since nothing has a 'dependency' on this, and nothing has to be recomputed if we read in an interface file. An IORef or a Var should be sufficient, as it is just a transparent cache.

@DunetsNM DunetsNM closed this Oct 23, 2020
@DunetsNM DunetsNM deleted the locate-docs-via-haddock-interface branch October 23, 2020 00:08
@DunetsNM
Copy link
Contributor Author

accidentally closed because of force-push, reopening

@DunetsNM DunetsNM reopened this Oct 23, 2020
@DunetsNM DunetsNM force-pushed the locate-docs-via-haddock-interface branch from 2e35f73 to cfdfcf9 Compare October 23, 2020 00:24
@DunetsNM
Copy link
Contributor Author

is it OK to cache locations of all names from haddock interface files, or only names that are actually used in the code
Former is simple and needs only one readInterfaceFile per package but requires more memory that will be mostly unused.
Only used names will result in multiple reads of same file when code is being typed and more names brought into scope.

@wz1000
Copy link
Collaborator

wz1000 commented Oct 23, 2020

I think we should cache the complete LinkEnv

@DunetsNM
Copy link
Contributor Author

@wz1000 do you mind if I partially revert #875 to bring GhcMonad back to Documentation? I need it to useH.nameCacheFromGhc deep inside - the doc lookup doesn't work with H.freshNameCache

As Session.hs already mentions it's probably because "It's important to keep the same NameCache though for reasons that I do not fully understand" - and neither do I

@wz1000
Copy link
Collaborator

wz1000 commented Oct 28, 2020

You can use the NameCache in ShakeExtras?

@DunetsNM
Copy link
Contributor Author

DunetsNM commented Oct 30, 2020

haddock-library is designed to compile across multiple versions of GHC, but haddock-api is tightly bound to a particular version of GHC - and of haddock-library:

8.6: haddock-api-2.22.* haddock-library-1.7.*
8.8: haddock-api-2.23.* haddock-library-1.8.*
8.10: haddock-api-2.24.* haddock-library-1.9.*

There are two ways to solve it:

  1. variate haddock-library based on GHC version. It works but requires a lot of #ifdef-ing (especially in tests) and can result in versioning hell on the long run, and also locks out older GHC from getting improvements of newer versions.

  2. or, link haddock-api to version haddock-library that it needs but use one fixed haddock-library version for everything else (we use 1.8.0 at the moment)

I like the second option more, because haddock-api usage is very limited but had to resort to a nasty hack as I couldn't find a clean way to use two versions of same package in stack.yaml. Is it possible to do at all ?


P.S. In fact, 8.10 here uses haddock-library-1.9.0 which potentially also needs the hack - it works only by a lucky coincidence because it's similar enough to 1.8.0

@DunetsNM
Copy link
Contributor Author

Also 8.10-windows build is flaky now (I saw it red and green with no code change, with both compilation or test errors)
Could be also related to haddock-library-1.9.0 so another reason to keep it at 1.8.0, just need to find a clean way to do it.

Apart that it's ready for review

@DunetsNM DunetsNM marked this pull request as ready for review October 30, 2020 02:04
-- Exposed for testing.
Q(..),
Q(..),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trimming whitespace needed

Copy link
Member

@jneira jneira Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the project has a .editorconfig that enforces that, do you have installed the needed plugin in your editor to honour it?
It saves a lot of time fixing those little things automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the hint

Comment on lines 18 to 21
- github: haskell/haddock
commit: 1f704e30d5f40fa5113045b252c0e48aebd842be
subdirs:
- haddock-api
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why and how long do we need this custom version of haddock-api? I'm not keen on merging as it will prevent a Hackage release

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was removed from stack-ghc-lib.yaml - haddock-api uses ghc and can't be used from ghc-lib mode anyway (still uses old documentation lookup).

similar workaround with github dependency still there in stack-810.yaml: haddock-api-2.24.0 from hackage doesn't compile with haddock-library-1.9.0 although it should.
Same for 8.6 (stack.yaml)

This is easy to fix by making a new release of haddock-api upstream, but I'm not sure if that's the best way to go because it also will lock us with separate versions of haddock-library per GHC version

@pepeiborra
Copy link
Collaborator

There are two ways to solve it:

  1. variate haddock-library based on GHC version. It works but requires a lot of #ifdef-ing (especially in tests) and can result in versioning hell on the long run, and also locks out older GHC from getting improvements of newer versions.
  2. or, link haddock-api to version haddock-library that it needs but use one fixed haddock-library version for everything else (we use 1.8.0 at the moment)

I like the second option more, because haddock-api usage is very limited but had to resort to a nasty hack as I couldn't find a clean way to use two versions of same package in stack.yaml. Is it possible to do at all ?

Which of these two options is compatible with uploading a package to Hackage that builds with GHC 8.6.x, 8.8.x and 8.10.x?

@DunetsNM
Copy link
Contributor Author

DunetsNM commented Oct 30, 2020

Which of these two options is compatible with uploading a package to Hackage that builds with GHC 8.6.x, 8.8.x and 8.10.x?

First option is definitely feasible, but according to haddock's README the second option looks more correct:

haddock-library: is concerned with the parsing and processing of the
Haddock markup language. Unlike the other packages, it is expected to build
on a fairly wide range of GHC versions.

First is easy to do: just need to release latest commits of upstream haddock branches ghc-8.6 and ghc-8.10 to hackage and use them here in ghcide (plus some #ifdef-ing here and there, which can get worse over time).

Second option also requires upstream release of mentioned haddock branches , plus some way to compile them with their private copies of haddock-library.
I just forked haddock and renamed haddock-library to haddock-library-sandbox which did the trick but is obviously bad (see stack.yaml).
I'm not aware of any clean way to do it, maybe putting it into yet another package can do it e.g. haddock-api-wrapper that exposes only readInterfaceFile function? Any hints are very welcome.

@pepeiborra

@jneira
Copy link
Member

jneira commented Oct 30, 2020

@DunetsNM I would go to use each combination of haddock-api and haddock-library official versions in the cabal file

haddock-api ^= 2.22 || ^= 2,23 || ^= 2.23
haddock-library ^=1.7 || ^=1.8 || ^=1.9

cabal/stack should pick the correct version based on base library bounds.
we are dropping support for ghc versions slowly so cpp conditions would not grow indefinitely.

And create a special module Development.IDE.Haddock.Compat similar to the existing Development.IDE.GHC.Compat with all the cpp conditions so the rest of the code can be keep clean of them.

Not sure if i fully understand your proposals but i think it is your first option. Forks from official libraries should be avoided imo, moreover if they are established and widely used ones.

A new package (haddock-compat?) with Development.IDE.Haddock.Compat with other name could be useful outside ghcide but i would start putting it here.

@DunetsNM
Copy link
Contributor Author

Not sure if i fully understand your proposals but i think it is your first option.

@jneira correct, this is my first option. OK I'll proceed with it then.
Converting PR back to draft meanwhile: as of now haddock-api-2.22.0 and 2.24.0 that are on hackage simply fail to compile, I'll try my best but it can take a while before fixed version is released upstream (as I understood all dependencies for ghcide must be on hackage)

@DunetsNM DunetsNM marked this pull request as draft October 31, 2020 02:07
pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Nov 11, 2020
Does not work reliably on all platforms.

Reenable when haskell#861 lands
pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Nov 12, 2020
Does not work reliably on all platforms.

Reenable when haskell#861 lands
pepeiborra added a commit to pepeiborra/ghcide that referenced this pull request Nov 14, 2020
Does not work reliably on all platforms.

Reenable when haskell#861 lands
pepeiborra added a commit that referenced this pull request Nov 15, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when #861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
@pepeiborra
Copy link
Collaborator

The ghcide Github project is becoming archived and merged into https://github.com/haskell/haskell-language-server

This PR will need to be reopened in the HLS repo. To do that, create a new branch from HLS HEAD in your HLS repo and do:

git remote add ghcide https://github.com/georgefst/ghcide.git
git fetch ghcide
git merge ghcide disable-warnings

Thanks!

@pepeiborra pepeiborra closed this Dec 29, 2020
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
@DunetsNM
Copy link
Contributor Author

alright, will take a look after the holidays. This PR is still blocked by upstream haddock which seems to lack ownership so not sure when that can be merged anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants