Integrate hiedb #898

wz1000 · 2020-11-08T08:59:12Z

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:

Write documentation
Tests
Release hiedb on hackage

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.

wz1000 · 2020-11-08T09:12:11Z

Go to definition for dependencies works in the following way:

First, the user must index the .hie files for their dependencies by running the command ghcide hiedb index $DIR in the root directory of their project.

Then, whenever we report a definition in one of the dependencies(say Control.Lens), a read only file Control.Lens.hs is written out on demand in root/.cached-deps/lens-XXX/Control.Lens.hs. The contents of the file are obtained from the .hie file that was indexed in the previous step. The logic responsible for this is in lookupMod in Rules.hs.

The .cached-deps directory has to be inside the project directory, as otherwise vscode and other editors will try to spin up another instance of ghcide to

pepeiborra

I've taken a first look, there's loads of changes.

cabal.project

exe/Main.hs

exe/Arguments.hs

pepeiborra · 2020-11-08T11:03:32Z

exe/Arguments.hs

    ,argsVersion :: Bool
    ,argsShakeProfiling :: Maybe FilePath
    ,argsTesting :: Bool
    ,argsThreads :: Int
    ,argsVerbose :: Bool
+    ,argFilesOrCmd :: a


Can we call this simply argCmd ? Or even take it out of this record, and change the type of getArguments to (Arguments, IdeCmd).

src/Development/IDE/Core/Shake.hs

pepeiborra · 2020-11-08T11:29:32Z

src/Development/IDE/Core/Shake.hs

@@ -778,16 +819,16 @@ useWithStaleFast' key file = do
  liftIO $ case r of


Isn't the call to getValues made redundant by lastValueIO?

We still need to know whether the key is the map so that we can decide to block or not?

I don't see it, could you walk me through the cases?

There are three cases:

1a) The value isn't in the store, and we can't (or fail to) compute a stale persistent value - we want to wait for the rule to compute a fresh value (matching the previous semantics)
1b) The value isn't in the store but we can compute a stale persistent value - return the value
2) The value is in the store - we can just return it.

(sorry for the delay, hope you will be able to page this back in)

src/Development/IDE/Core/Shake.hs

src/Development/IDE/Spans/Common.hs

exe/Main.hs

wz1000 · 2020-11-08T14:34:02Z

The situation with the cached-deps files is unsatisfactory right now - we want a nice way to detect if a file is "fake" in this manner (something better than "cached-deps" isInfixOf filepath, and we also want to compute only a given whitelist of rules for such files, like GetHieAsts, but not things like GenerateCore.

pepeiborra

Second pass done.

Can you write a high-level comment describing the integration with hiedb?

In particular, regarding the paragraph below, for non FOIs, what things are now obtained from hiedb instead of from .hie files?

Under this paradigm, ghcide acts as an indexing service for hiedb, generating .hi and .hie files which are indexed and saved in the database, available for all future queries, even across restarts. A local cache of .hie files/typechecked modules is maintained on top of this to answer queries for the files the user is currently editing, while non-local information about other files in the project is accessed through the database.

src/Development/IDE/Core/Rules.hs

src/Development/IDE/Core/Shake.hs

src/Development/IDE/Spans/AtPoint.hs

pepeiborra · 2020-11-08T17:49:46Z

src/Development/IDE/Core/Rules.hs

+persistentHieFileRule :: Rules ()
+persistentHieFileRule = addPersistentRule GetHieAst $ \file -> runMaybeT $ do
+  res <- readHieFileForSrcFromDisk file
+  let refmap = generateReferencesMap . getAsts . hie_asts $ res
+  pure $ HAR (hie_module res) (hie_asts res) refmap (HieFromDisk res)
+
+readHieFileForSrcFromDisk :: NormalizedFilePath -> MaybeT IdeAction HieFile
+readHieFileForSrcFromDisk file = do
+  db <- asks hiedb
+  log <- asks $ L.logInfo . logger
+  row <- MaybeT $ liftIO $ HieDb.lookupHieFileFromSource db $ fromNormalizedFilePath file
+  let hie_loc = HieDb.hieModuleHieFile row
+  liftIO $ log $ "LOADING HIE FILE :" <> T.pack (show file)
+  readHieFileFromDisk hie_loc


I am trying to understand what the persistent rule buys us. Ghcide already reuses .hie files from the cache dir at startup if they are up-to-date, so in what cases does this make a difference?

Ghcide already reuses .hie files from the cache dir at startup if they are up-to-date

It doesn't, at least not for things like hover/definition etc.

With the persistent rule stuff we get:

Instant availability of select IDE features (hover/definition...) on startup, even before cabal configure etc. finish

If the project doesn't compile on startup, we can still use the above features from the stale .hie file.

That's awesome.

But surely ghcide will block until the cradle is loaded, as it usually happens on a Notification?

But surely ghcide will block until the cradle is loaded, as it usually happens on a Notification?

Ah, this is indeed the behavior in vscode. I didn't notice because my client sends the request immediately after initialize if the server isn't already running, so I was getting instant responses most of the time.

I think we can serialize notifications with respect to each other, but handle requests concurrently without blocking?

We decided to process notifications synchronously to ensure consistency after seeing test failures showing real inconsistency issues around e.g. completions. I don't see how to avoid such inconsistencies if we handle requests concurrently with notifications.

haskell/hie-bios#265 will make startup much faster when it lands, so we will get most of the benefits of quick startup even without async notifications. Do we still want to keep this mechanism to provide code navigation when the project doesn't compile on startup, or should I just remove it?

If we are keeping it, I think it needs a re-design. I'm thinking another variant of define in which we can return a stale value if the regular codepath fails.

I think you've answered yourself! Get rid of this version and leave the redesign for future work

I think that my assumption that ghcide blocks on a FileOpened Notification might be wrong.
The handler calls restartShakeSession but doesn't wait on the results, so it might not be actually blocking.
The open telemetry stuff will make it very easy to find out whether it blocks or not.

src/Development/IDE/Core/Rules.hs

pepeiborra · 2020-11-08T17:56:21Z

The situation with the cached-deps files is unsatisfactory right now - we want a nice way to detect if a file is "fake" in this manner (something better than "cached-deps" isInfixOf filepath, and we also want to compute only a given whitelist of rules for such files, like GetHieAsts, but not things like GenerateCore.

Sounds like extending the IsFileOfInterest rule and its result would be good for clarity and allow a better mechanism down the line

pepeiborra · 2020-11-08T17:59:17Z

First, the user must index the .hie files for their dependencies by running the command ghcide hiedb index $DIR in the root directory of their project.

What should $DIR point to?

If it's just the same folder where hie.yaml lives, can ghcide do it in the background?

src/Development/IDE/Core/Shake.hs

wz1000 · 2020-11-08T19:12:35Z

What should $DIR point to?

What ever -hiedir was set to.

You would put something like the following in a cabal.project(.local):

package *
    ghc-options: -fwrite-ide-info -hiedir <some-directory>

One limitation is that <some-directory> has to be an absolute path, or cabal will strew .hie files in random places.

I don't know if there is a way for ghcide to pass such options to cabal/stack, but it might not be a good idea anyway since it would force recompilation of all dependencies. If it is possible, we can have a special ghcide command that does this and indexes the resulting files.

/cc @fendor

pepeiborra · 2020-11-08T22:04:47Z

What should $DIR point to?

What ever -hiedir was set to.

You would put something like the following in a cabal.project(.local):
package *
    ghc-options: -fwrite-ide-info -hiedir <some-directory>
One limitation is that <some-directory> has to be an absolute path, or cabal will strew .hie files in random places.

I don't know if there is a way for ghcide to pass such options to cabal/stack, but it might not be a good idea anyway since it would force recompilation of all dependencies. If it is possible, we can have a special ghcide command that does this and indexes the resulting files.

/cc @fendor

This might just about work, but in the long term, do we want .hie files to be part of ghc package databases, just like interface files are?

pepeiborra · 2020-12-05T07:33:45Z

@wz1000 when do you expect to be able to push this forward?

wz1000 · 2020-12-05T09:31:17Z

Unfortunately I won't be able to get back to this for another 1-2 weeks at least.

Meanwhile, @jhrcek has very kindly volunteered to help out with tests and documentation for the hiedb library itself, before its hackage release.

Use db for findDef save source file location to db Find source for boot files Use DynFlags from HieDb instead of unsafeGlobalDynFlags Return multiple definitions don't typecheck files on load Add support for persistent stale values Add persistent hie file rule docs wip better typedef defs for deps update hiedb Fix for files with errors Fix build integrate hiedb commands and set dynflags on boot workspace symbol tweaks, cabal.project Write ifaces on save use real mtime for saved files safe indexing bump hiedb Proper refs for FOIs hlint Update exe/Main.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Review comments update hiedb Update src/Development/IDE/Core/Shake.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Update src/Development/IDE/Core/Rules.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Update src/Development/IDE/Core/Rules.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Update src/Development/IDE/Spans/AtPoint.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Update src/Development/IDE/Core/Rules.hs Co-authored-by: Pepe Iborra <pepeiborra@me.com> Apply suggestions from code review Co-authored-by: Pepe Iborra <pepeiborra@me.com> more careful re-indexing update for hiedb-0.1.0.0

wz1000 · 2020-12-29T09:42:30Z

I've removed all the stuff to do with non-project dependencies for now, will re-add in another PR.

pepeiborra · 2020-12-29T11:00:30Z

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 reviewed Nov 8, 2020

View reviewed changes

wz1000 force-pushed the hiedb-final branch from 7008126 to a1411c9 Compare November 8, 2020 14:26

pepeiborra reviewed Nov 8, 2020

View reviewed changes

src/Development/IDE/Core/Shake.hs Show resolved Hide resolved

wz1000 force-pushed the hiedb-final branch from 43773aa to bcaca71 Compare November 8, 2020 21:08

This was referenced Nov 9, 2020

Major steps required (Roadmap) haskell/haskell-language-server#1

Closed

Go to references haskell/haskell-language-server#211

Closed

jneira mentioned this pull request Nov 23, 2020

Cannot jump to definition haskell/haskell-language-server#486

Closed

wz1000 force-pushed the hiedb-final branch from bcaca71 to 920a131 Compare December 29, 2020 09:19

Remove cached-deps stuff for now

d17cf31

wz1000 added 4 commits December 29, 2020 15:35

explicit showSDoc

de5f402

docs in AtPoint

70c70b7

add doc comment about database consistency

0d64a71

add TODO for better position mapping from diff

1d760bb

pepeiborra closed this Dec 29, 2020

This was referenced Dec 29, 2020

References via hiedb haskell/haskell-language-server#704

Merged

Bring over unmerged PRs from ghcide haskell/haskell-language-server#705

Closed

jneira mentioned this pull request Jan 3, 2021

Go-to definition jumps to hs-boot files haskell/haskell-language-server#790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate hiedb #898

Integrate hiedb #898

wz1000 commented Nov 8, 2020 •

edited

wz1000 commented Nov 8, 2020

pepeiborra left a comment

pepeiborra Nov 8, 2020

pepeiborra Nov 8, 2020

wz1000 Nov 8, 2020

pepeiborra Nov 8, 2020

wz1000 Dec 29, 2020

wz1000 commented Nov 8, 2020

pepeiborra left a comment

pepeiborra Nov 8, 2020 •

edited

wz1000 Nov 8, 2020

pepeiborra Nov 8, 2020

wz1000 Nov 8, 2020

pepeiborra Nov 8, 2020

wz1000 Nov 21, 2020

pepeiborra Nov 21, 2020

pepeiborra Dec 5, 2020

pepeiborra commented Nov 8, 2020

pepeiborra commented Nov 8, 2020

wz1000 commented Nov 8, 2020

pepeiborra commented Nov 8, 2020 •

edited

pepeiborra commented Dec 5, 2020

wz1000 commented Dec 5, 2020

wz1000 commented Dec 29, 2020

pepeiborra commented Dec 29, 2020

		@@ -778,16 +819,16 @@ useWithStaleFast' key file = do
		liftIO $ case r of

Integrate hiedb #898

Integrate hiedb #898

Conversation

wz1000 commented Nov 8, 2020 • edited

wz1000 commented Nov 8, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wz1000 commented Nov 8, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

pepeiborra Nov 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepeiborra commented Nov 8, 2020

pepeiborra commented Nov 8, 2020

wz1000 commented Nov 8, 2020

pepeiborra commented Nov 8, 2020 • edited

pepeiborra commented Dec 5, 2020

wz1000 commented Dec 5, 2020

wz1000 commented Dec 29, 2020

pepeiborra commented Dec 29, 2020

wz1000 commented Nov 8, 2020 •

edited

pepeiborra Nov 8, 2020 •

edited

pepeiborra commented Nov 8, 2020 •

edited