Add "Go to type" hyperlinks in the hover popup (like Rust has) #4691

dnikolovv · 2025-08-13T16:39:14Z

This is how it looks in Rust:

dschrempf · 2025-08-14T05:34:24Z

Thanks! I had a quick look at the popups in my editor, and the "Go to ..." renders well and looks great. A few questions/suggestions:

Can we sort the list of types in the same order they appear in the type signature of the highlighted definition?
Does it make sense to write "Go to type ..." because we are only listing types afterwards.
The hint only shows up when symbols on the value-level are highlighted. (See attached screenshots).

dnikolovv · 2025-08-14T07:00:09Z

Hi @dschrempf

Can we sort the list of types in the same order they appear in the type signature of the highlighted definition?

I need to check if the order they come in is deterministic when we query the locations.

Does it make sense to write "Go to type ..." because we are only listing types afterwards.

Yes, I just copied the Rust one.

The hint only shows up when symbols on the value-level are highlighted. (See attached screenshots).

Yeah. For some reason the locations query does not return anything when you're on the type level. This is how Go To Definition works right now. It might be a bug though?

dschrempf · 2025-08-14T08:04:10Z

Ad "Go to type ... not showing for symbols on the type level": Actually, it makes sense, but only somewhat.

When the cursor is on X (see picture)

Go to definition goes to the definition of the data type X. That's good.
Go to type does nothing (error: Not found for: X) -> Makes sense, the type of X is Type, or Star, we could go there.

However, when the cursor is on x on the type level (see picture)

Go to definition goes to x on the value level (one line below). Does this make sense? I guess it does.
Go to type gives the same error as above (error: Not found for: x). This should actually go to X, or suggest a list of types involved in defining x, shouldn't it? And this is also why you do not get a list of types @dnikolovv

dnikolovv · 2025-08-14T09:14:23Z

@dschrempf

Go to type gives the same error as above (error: Not found for: x). This should actually go to X, or suggest a list of types involved in defining x, shouldn't it? And this is also why you do not get a list of types @dnikolovv

Exactly - the definitions in the hover use the same functionality underneath. I don't know if this is a bug or intended behavior. Maybe @fendor knows?

fendor · 2025-08-14T09:54:02Z

I don't know exactly, I presume it is an artefact of HIE file structure or how GHC defines the AST and its source locations. So, probably a "bug".

I don't think it is something worth investing too much time into right now (i.e., not for this PR in particular), and we should rather focus on the presentation in the hover.

dnikolovv · 2025-08-14T16:42:20Z

Not sure why this pipeline fails. Running cabal test ghcide-tests locally with GHC 9.12.2 gives me All 398 tests passed (287.79s).

fendor · 2025-08-16T07:57:18Z

@dnikolovv The testsuite in HLS is unfortunately quite flaky. You can likely ignore the testcase, and wait for an admin to restart the job. If the failing test case cannot be reproduced locally, then you can ignore it for now.

jian-lin · 2025-08-17T16:55:16Z

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

-        pTypes :: [T.Text]
-        pTypes
-          | Prelude.length names == 1 = dropEnd1 $ map wrapHaskell prettyTypes
-          | otherwise = map wrapHaskell prettyTypes
+        pTypes :: M.Map Name Location -> [T.Text]
+        pTypes locationsMap =
+          case names of
+            [_singleName] -> dropEnd1 $ prettyTypes Nothing locationsMap
+            _             -> prettyTypes Nothing locationsMap


a bit offtopic:

Why do we dropEnd1 when length names == 1?

Why don't we dropEnd1 when length names /= 1?

I'm tempted to remove this (actually I did but then reverted it) because it seems weird and unneeded to me as well. I tested a build without it and it seemed fine.

However, this is 4 year old code introduced by 2fef041 so I can't really comment.

My guess for the motivation behind dropEnd1 is that the author wants to avoid showing the same type/signature in pTypes as the one shown in prettyName. If that is the case, I think we should also do dropEnd1 when length names /= 1 (for example, names has 1 "actual" name and an evidence name), at least in today's GHC versions.

I also need to deal with this duplication when implementing signature help. I decide to filter out the same type instead of dropEnd1.

Actually dropEnd1 was introduced in 5dfee4c. Maybe @wz1000 has some ideas to share?

~~Why just change it to use intelecate.~~

@jian-lin your explanation is correct. If we have the expression x where x :: Int, then in the hover, we would get both the lines x :: Int for the variable at this point and _ :: Int for the entire expression. The point of the code is to remove the duplication.

Thanks @wz1000. However, my main question is that we should always (not just when length names == 1) filter out the same type. For example, the current hover info for == in x = (==) True contains duplicated type forall a. Eq a => a -> a -> Bool.

== :: forall a. Eq a => a -> a -> Bool Defined in ‘GHC.Classes’ (ghc-prim-0.11.0) Documentation Source * * * Evidence of constraint Eq Bool using an external instance Defined in ‘GHC.Classes’ * * * _ :: Bool -> Bool -> Bool * * * _ :: forall a. Eq a => a -> a -> Bool * * * infix 4 ==

fendor

Thank you for implementing this nice feature!

Could we add a testcase? Otherwise, this looks good to me and we can merge after a test has been added :)

fendor

Some more review

fendor · 2025-09-01T07:38:37Z

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

+module Development.IDE.Core.LookupMod (lookupMod, LookupModule) where
+
+import           Control.Monad.Trans.Maybe       (MaybeT (MaybeT))
+import           Development.IDE.Core.Shake      (HieDbWriter, IdeAction)
+import           Development.IDE.GHC.Compat.Core (ModuleName, Unit)
+import           Development.IDE.Types.Location  (Uri)
+
+-- | Gives a Uri for the module, given the .hie file location and the the module info
+-- The Bool denotes if it is a boot module
+type LookupModule m = FilePath -> ModuleName -> Unit -> Bool -> MaybeT m Uri
+
+-- | Eventually this will lookup/generate URIs for files in dependencies, but not in the
+-- project. Right now, this is just a stub.
+lookupMod ::
+  -- | access the database
+  HieDbWriter ->
+  -- | The `.hie` file we got from the database
+  FilePath ->
+  ModuleName ->
+  Unit ->
+  -- | Is this file a boot file?
+  Bool ->
+  MaybeT IdeAction Uri
+lookupMod _dbchan _hie_f _mod _uid _boot = MaybeT $ pure Nothing


Is this still needed?

~~Nope, removed.~~

That was a bit too hasty. This was old code that I had to move around to avoid circular references. It doesn't do anything functionally but maybe the author wants to go back to it at some point.

In my opinion, and if it is dead code, it should go. We can revive it later if need be.

We want to get back to it eventually, but I also think this is not needed as is and barely gives any advantage right now. So, let's just get rid of it, and then merge :)

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

… redundant dropEnd1" This reverts commit 723d56c.

dnikolovv · 2025-09-13T07:58:25Z

Thank you for implementing this nice feature!

Could we add a testcase? Otherwise, this looks good to me and we can merge after a test has been added :)

@fendor

Added tests.

fendor · 2025-09-14T01:53:54Z

There was one concern that this might explode the Hover message if maaaanny types are involved.

Do we perhaps want to impose a hard upper-limit on the number of elements we display, such as 10? What do you think about this?

fendor

LGTM, modulo my last comment, thank you for taking care of this!

fendor · 2025-09-14T01:58:48Z

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

+module Development.IDE.Core.LookupMod (lookupMod, LookupModule) where
+
+import           Control.Monad.Trans.Maybe       (MaybeT (MaybeT))
+import           Development.IDE.Core.Shake      (HieDbWriter, IdeAction)
+import           Development.IDE.GHC.Compat.Core (ModuleName, Unit)
+import           Development.IDE.Types.Location  (Uri)
+
+-- | Gives a Uri for the module, given the .hie file location and the the module info
+-- The Bool denotes if it is a boot module
+type LookupModule m = FilePath -> ModuleName -> Unit -> Bool -> MaybeT m Uri
+
+-- | Eventually this will lookup/generate URIs for files in dependencies, but not in the
+-- project. Right now, this is just a stub.
+lookupMod ::
+  -- | access the database
+  HieDbWriter ->
+  -- | The `.hie` file we got from the database
+  FilePath ->
+  ModuleName ->
+  Unit ->
+  -- | Is this file a boot file?
+  Bool ->
+  MaybeT IdeAction Uri
+lookupMod _dbchan _hie_f _mod _uid _boot = MaybeT $ pure Nothing


We want to get back to it eventually, but I also think this is not needed as is and barely gives any advantage right now. So, let's just get rid of it, and then merge :)

dnikolovv requested a review from wz1000 as a code owner August 13, 2025 16:39

dnikolovv changed the title ~~Add "Go to type" hyperlinks in the hover popup.~~ Add "Go to type" hyperlinks in the hover popup (like Rust has) Aug 13, 2025

jian-lin reviewed Aug 17, 2025

View reviewed changes

fendor self-requested a review August 20, 2025 07:55

fendor added the status: needs review This PR is ready for review label Aug 20, 2025

fendor requested changes Sep 1, 2025

View reviewed changes

fendor mentioned this pull request Sep 4, 2025

Implement signature help #4626

Merged

dnikolovv force-pushed the type-definition-hyperlinks branch from 6021b15 to b1eab33 Compare September 13, 2025 06:45

dnikolovv added 4 commits September 13, 2025 10:57

Add "Go to type" hyperlinks in the hover popup.

72f866f

Get rid of redundant dropEnd1

753b8e7

Actually realize that the dropEnd1 is old code and revert "Get rid of…

7f91fad

… redundant dropEnd1" This reverts commit 723d56c.

Add tests for the 'Go to' links in the hover.

f21e011

dnikolovv force-pushed the type-definition-hyperlinks branch from 859c323 to f21e011 Compare September 13, 2025 07:57

fendor approved these changes Sep 14, 2025

View reviewed changes

Uh oh!

Add "Go to type" hyperlinks in the hover popup (like Rust has) #4691

Are you sure you want to change the base?

Add "Go to type" hyperlinks in the hover popup (like Rust has) #4691

Uh oh!

Conversation

dnikolovv commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dschrempf commented Aug 14, 2025

Uh oh!

dnikolovv commented Aug 14, 2025

Uh oh!

dschrempf commented Aug 14, 2025

Uh oh!

dnikolovv commented Aug 14, 2025

Uh oh!

fendor commented Aug 14, 2025

Uh oh!

dnikolovv commented Aug 14, 2025

Uh oh!

fendor commented Aug 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jian-lin Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulomoon Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jian-lin Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnikolovv Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

dnikolovv commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fendor commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnikolovv commented Aug 13, 2025 •

edited

Loading

jian-lin Aug 18, 2025 •

edited

Loading

soulomoon Aug 19, 2025 •

edited

Loading

jian-lin Sep 3, 2025 •

edited

Loading

dnikolovv Sep 13, 2025 •

edited

Loading

dnikolovv commented Sep 13, 2025 •

edited

Loading

fendor commented Sep 14, 2025 •

edited

Loading