WIP Goto dependency definition by nlander · Pull Request #3704 · haskell/haskell-language-server

nlander · 2023-07-11T10:53:22Z

For users using cabal HEAD, enables indexing of dependency HIE files so that the source can be written out on calls to gotoDefinition.

michaelpj · 2023-07-11T14:40:52Z

Tests?

wz1000 · 2023-07-13T11:04:20Z

ghcide/src/Development/IDE/Core/Actions.hs

+      liftIO $ atomically $
+        unGetTQueue indexQueue $ \withHieDb -> withHieDb $ \db -> do
+          HieDb.addSrcFile db hieFile writeOutPath False
+          putMVar completionToken ()


This should be outside the withHieDb retry block

wz1000 · 2023-07-13T11:06:42Z

ghcide/src/Development/IDE/Core/Actions.hs

+                ms -> foldr1 (</>) ms
+            breakOnDot :: FilePath -> [FilePath]
+            breakOnDot = words . map replaceDotWithSpace
+            replaceDotWithSpace :: Char -> Char


See Language.Haskell.Syntax.Module.Name.moduleNameSlashes

wz1000 · 2023-07-13T11:07:46Z

ghcide/src/Development/IDE/Core/Rules.hs

+            HAR
+                (Compat.hie_module hieFile)
+                (Compat.hie_asts hieFile)
+                mempty


we can fill in the RefMap using Compat.generateReferencesMap

wz1000 · 2023-07-13T11:11:46Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+                  (libraryDir : _) -> libraryDir
+                hieDir :: FilePath
+                hieDir = pkgLibDir </> "extra-compilation-artifacts"
+            modIfaces <- mapMaybeM loadModIface modules


why do we need to load .hi files here?

wz1000 · 2023-07-13T11:15:11Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+                    mkModule (unitInfoId package) moduleName
+                -- When module is re-exported from another package,
+                -- the origin module is represented by value in Just
+                makeModule (_, Just otherPackageMod) = otherPackageMod


modules from other packages won't have a .hie file in the extra-compilation directory of the package under consideration, so we can just skip them from the list.

wz1000 · 2023-07-13T11:19:34Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+            let hiePath :: NormalizedFilePath
+                hiePath = toNormalizedFilePath' $
+                  hieDir </> toFilePath (moduleName $ mi_module modIface) ++ ".hie"
+            hieCheck <- checkHieFile recorder se "newHscEnvEqWithImportPaths" hiePath


we might want to be less granular with the HieAlreadyIndexed check - each package can contain hundreds/thousands of modules, and we don't want to check all these modules on every startup.

It's also wasteful because once we successfully index a whole unit/package we can assume that it doesn't change. So maybe we need to only check if a package has been indexed rather than checking for each file in the package.

This might require another table in hiedb to keep track of the packages we have indexed.

wz1000 · 2023-07-13T11:21:52Z

ghcide/src/Development/IDE/Plugin/TypeLenses.hs

-                )
-    tmr <- fst <$> (
-                handleMaybeM "Unable to TypeCheck"
+    case Shake.getSourceFileOrigin nfp of


We have pluginFileType in PluginDescriptor which specifies which filetypes plugins want to handle. We should extend this concept to extend to specifying whether plugins want to handle FromDependency files.

wz1000 · 2023-07-13T11:23:41Z

ghcide/src/Development/IDE/Core/Shake.hs

+
+getSourceFileOrigin :: NormalizedFilePath -> SourceFileOrigin
+getSourceFileOrigin f =
+    case isInfixOf ".hls/dependencies" (show f) of


isInfixOf and show are the wrong functions to use here.

You probably want to inspect the path using functions from System.FilePath and use fromNormalizedFilePath to convert NormalizedFilePath to FilePath.

wz1000 · 2023-07-13T11:26:32Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+        deleteMissingDependencySources :: IO ()
+        deleteMissingDependencySources = do
+            completionToken <- newEmptyMVar
+            atomically $ unGetTQueue (indexQueue $ hiedbWriter se) $


I don't think we want unGetTQueue here.

nlander · 2023-07-15T11:14:22Z

ghcide/src/Development/IDE/Core/Shake.hs

+
+getSourceFileOrigin :: NormalizedFilePath -> SourceFileOrigin
+getSourceFileOrigin f =
+    case [".hls", "dependencies"] `isInfixOf` (splitDirectories file) of


@wz1000 What do you think of this? It still uses isInfixOf but not show. I don't see any predicate in System.FilePath that is like isInfixOf but for directories being contained in a FilePath.

This seems alright to me

This reverts commit 80b9a96.

wz1000 · 2023-07-29T08:47:32Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+newHscEnvEqWithImportPaths envImportPaths recorder se hscEnv deps = do

-    let dflags = hsc_dflags hscEnv
+    indexDependencyHieFiles


We probably want to do this asynchronously.

wz1000 · 2023-07-29T08:48:19Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+            pure $ projectDir </> ".hls"
+        deleteMissingDependencySources :: IO ()
+        deleteMissingDependencySources = do
+            completionToken <- newEmptyMVar


I don't think we need to wait for this to complete, we can just queue it up

wz1000 · 2023-07-29T08:49:06Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

+                lookupPackage db unit
+            case moduleRows of
+                [] -> traverse_ (indexModuleHieFile hieDir) modules
+                _  -> return ()


comments explaining that we have already indexed the package

wz1000 · 2023-07-29T08:49:33Z

ghcide/src/Development/IDE/Types/HscEnvEq.hs

          (evaluate . force . Just $ listVisibleModuleNames hscEnv)

    return HscEnvEq{..}
+    where


I think we probably want to move this code to another module.

wz1000 · 2023-07-29T08:51:04Z

ghcide/src/Development/IDE/Spans/AtPoint.hs

+      -- for any reason.
+      -- See the function `lookupMod` in Development.IDE.Core.Actions
+      -- for where dependency files get created and indexed in hiedb.
+      fileExists <- liftIO $ doesFileExist src


I don't think we need to do this, as long as we check for .hls on startup.

This reverts commit f0b7360.

This reverts commit a562811.

This reverts commit 1027229.

joyfulmantis · 2023-08-08T12:56:35Z

Closed as per request by @nlander on Matrix

nlander requested review from fendor and pepeiborra as code owners July 11, 2023 10:53

nlander marked this pull request as draft July 11, 2023 10:53

wz1000 mentioned this pull request Jul 13, 2023

HLS prioritization and product development #3715

Open

wz1000 reviewed Jul 13, 2023

View reviewed changes

nlander force-pushed the goto-dependency-definition branch from 3c686d9 to 68edc00 Compare July 14, 2023 00:30

nlander commented Jul 15, 2023

View reviewed changes

nlander added 14 commits July 21, 2023 06:48

Add gotoDefinition other file tests

4b4f8a7

Use correct position mapping in getDefinition

3b64fbb

Implement lookupMod function

e23922f

Create .hls directory in lookupMod

fdfffb7

Add ShakeExtras arg to newHscEnvEq

838ce20

Call indexHieFile in newHscEnvEq

429bd9a

Factor out loading ModIfaces

fd7f95e

Index hidden module hie files

354bcb6

Handle loadHieFile error

599c191

Log only on loadHieFile failure

f872627

Use loadModIface in ExportsMap creation

df0e93e

Check if dependency HIE files already indexed

d2b3eec

Check SourceFileOrigin in GetHieAst

352f3c2

Index .hls/dependencies files in lookupMod

50df9cf

nlander added 8 commits July 21, 2023 07:04

Remove unecessary getModIface

a1d88b8

Generate RefMap from HieFile

93aecea

Avoid indexing redundant modules

a807ae8

Use more System.FilePath functions

095dadd

Correct completion token placement

af1141c

Add PluginFileType

54b253a

Revert "Prevent GhcSession call on open dependency"

f4ba2b9

This reverts commit 80b9a96.

Check package before indexing

9a4b009

nlander force-pushed the goto-dependency-definition branch from 0c7a635 to 9a4b009 Compare July 21, 2023 12:35

nlander added 3 commits July 21, 2023 08:46

Disable write and execute permissions for dependency sources

94d3b07

Add gotoDefinition dependency test

a562811

Add check for indexing message

f0b7360

wz1000 reviewed Jul 29, 2023

View reviewed changes

nlander added 10 commits July 31, 2023 20:29

Make goto dependency test in ghcide

b7a097e

Use async for dependency test

11518ce

Revert "Add check for indexing message"

462f811

This reverts commit f0b7360.

Revert "Add gotoDefinition dependency test"

38e6cf5

This reverts commit a562811.

Calculate transitive dependencies

44cfd69

Fix indexHiefile ready message

39ba758

Index dependencies asynchronously

3e9d307

Don't wait for dependency file unindexing

4cae640

Move dependency indexing to its own module

7f8038a

Revert "Check that hiedb source files exist"

68b06f2

This reverts commit 1027229.

nlander mentioned this pull request Aug 8, 2023

Goto dependency definition #3749

Open

joyfulmantis closed this Aug 8, 2023

Uh oh!

Comments

Conversation

nlander commented Jul 11, 2023

Uh oh!

michaelpj commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyfulmantis commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants