References via hiedb

[wz1000]

Continuing from haskell/ghcide#898

Includes support for references, workspace symbols, faster loading by loading (stale) .hie files from disk and jump to definition for arbitrary dependencies.

As described here and here

TODO:

• Release hiedb on hackage
• Tests

I'll be pressed for time in the near future, so would appreciate any help to get this over the finish line, especially with the first two points above.

[peterwicksstringfield]

I wrote some tests in HLS, for looking up references within one module, between a module and a module that imports it, between a module and a module that imports it transitively, and between a package and another package on which it depends (with the two packages being in the same cabal project).

textDocument/references has an extra flag includeDeclaration. When this flag is false, looking up the symbol "foo" in "foo = 2; bar = foo + 3" should pick up "... = foo + ..." as a reference, but not "foo = ...". The references provider in this branch currently ignores this flag. There's a test with an expected failure documenting this.

There's a test in test/functional/TypeDefinition failing too. "find definition of parameterized data type". It looks like, when we have this code:

module Lib where

data Parameter a = Parameter a

parameterId :: Parameter a -> Parameter a
parameterId pid = p[CURSOR HERE]id

And we ask for the type definition of the symbol pid, we get the "data Parameter a = Parameter a" line that defines the type Parameter a AND the "parameterId :: Parameter a -> Parameter a" line that defines the type of parameterId. That's new, is it supposed to be doing that?

...

Also I hacked up a bunch of HLS to make it compile (at least on 8.10.2). Don't take that part of the code, it's the unit tests I am interested in here. I just needed them to run. (Hacked up HLS works in vscode though, and get all references does what it's supposed to. XD)

(The tactics and class plugins will need to be updated to account for this:

 data HieAstResult
-  = HAR
+  = forall a. HAR
   { hieModule :: Module
-  , hieAst :: !(HieASTs Type)
-  , refMap :: RefMap
+  , hieAst :: !(HieASTs a)
+  , refMap :: RefMap a
   -- ^ Lazy because its value only depends on the hieAst, which is bundled in this type
   -- Lazyness can't cause leaks here because the lifetime of `refMap` will be the same
   -- as that of `hieAst`
+  , hieKind :: !(HieKind a)
+  -- ^ Is this hie file loaded from the disk, or freshly computed?
   }

)

I also fixed a couple hlint hints, and a couple ghcide tests.

wz1000#1

[wz1000]

And we ask for the type definition of the symbol pid, we get the "data Parameter a = Parameter a" line that defines the type Parameter a AND the "parameterId :: Parameter a -> Parameter a" line that defines the type of parameterId. That's new, is it supposed to be doing that?

It's returning the definition of the type variable a (which is defined by the implicit forall on the type declaration), though I can see how it may be confusing. The idea is to support things that have something like type Foo (Bar Baz), returning the definition of Foo, Bar and Baz.

[peterwicksstringfield]

That makes sense. Symbol "pid" has type "Parameter a" ... and if I want the definition of "Parameter a", then that comes in two parts: the definition of "Parameter" ("data Parameter = ...") and the definition of "a" ("forall a. ..."). Okay question answered. That test needs to be updated to expect two type definitions. (I will make this change on my PR.)

[wz1000]

I think this is ready for review while I work on final tweaks to tests and the HLS CLI parser.

[pepeiborra]

I'm not convinced of replacing showName here, which is a more intentional operation than showGhc.
In the future showGhc could change in unexpected ways, while showName would be more stable

[wz1000]

Completions uses showGhc all over the place, lets fix this in another PR.

[pepeiborra]

but this PR is making it worse, right? At least previously they calling showName was expressing the intent. All I am asking is to preserve that intent

[pepeiborra]

I thought we were keeping showName?

[wz1000]

I used showNameWithoutUniques instead of resurrecting showName.

[wz1000]

I've squashed this down into one commit with a clear commit message.

[pepeiborra]

The benchmarks do not show any regressions, and I think this is ready to merge. Super awesome, thank you @wz1000 !!!

It's a big change, please land it asap so that HEAD users have a chance to play with it before the next release.

[wz1000]

I think it would be good to have a "stable" release before this change lands, since that would buy us some time to address any issues that may arise.

[pepeiborra]

I think it would be good to have a "stable" release before this change lands, since that would buy us some time to address any issues that may arise.

There hasn't been an HLS release in a while and the last one had a pretty annoying diagnostics bug, so I think this is a good idea (for HLS).

But for ghcide, there was a release pretty recently and I don't think it's a priority to make another one.

[wz1000]

@jneira sorry to bother you, but would you have time to do a release of HLS so that this can land? I would do it, but I'm not familiar enough with the HLS release process.

[jneira]

Sure, I will try to do asap
I want to complete the actual doc about releases and involve other people (@Ailrun f.e. 😬 if agree), to share the knowledge, but it can be done after the next release

[Ailrun]

If there's a detailed documentation, I'm good for that.

[michaelpj]

Is there any user documentation we need as part of this? e.g. is it possible to clear out the hiedb files if it's misbehaving? Should we add some troubleshooting notes?

[wz1000]

@michaelpj there is a whole hiedb subcommand added to hls and ghcide to allow you to query and modify the db. Ideally, the changes in this PR and the existence of the database should be invisible to users. Of course, this is not going to be the case, and we would need to directly inspect the database to debug issues. The sub-command exists for this precisely purpose (with automatically generated help messages). I would like to see what kinds of issues people actually end up running into before offering any kind of debugging advice.