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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

dnikolovv wants to merge 3 commits into haskell:master

from dnikolovv:type-definition-hyperlinks

Open

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

dnikolovv wants to merge 3 commits into haskell:master from dnikolovv:type-definition-hyperlinks

+146 −45

Conversation

dnikolovv

Copy link

@dnikolovv dnikolovv commented Aug 13, 2025 •

edited

Loading

go-to-types

This is how it looks in Rust:

image

@dnikolovv


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

5924bb9

@dnikolovv dnikolovv requested a review from wz1000 as a code owner

August 13, 2025 16:39

@dnikolovv 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

@dschrempf

Copy link

Collaborator

dschrempf commented Aug 14, 2025

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).

image image

@dnikolovv

Copy link

Author

dnikolovv commented Aug 14, 2025

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

Copy link

Collaborator

dschrempf commented Aug 14, 2025

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)
image

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)
image

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

Copy link

Author

dnikolovv commented Aug 14, 2025

@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

Copy link

Collaborator

fendor commented Aug 14, 2025

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

Copy link

Author

dnikolovv commented Aug 14, 2025

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

Copy link

Collaborator

@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

jian-lin reviewed

Aug 17, 2025

View reviewed changes

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

Comment on lines -268 to +288

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

Copy link

Collaborator

@jian-lin jian-lin Aug 17, 2025

Choose a reason for hiding this comment

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

a bit offtopic:

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

Copy link

Author

@dnikolovv dnikolovv Aug 18, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Collaborator

@jian-lin jian-lin Aug 18, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

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.

Copy link

Collaborator

@jian-lin jian-lin Aug 18, 2025

Choose a reason for hiding this comment

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

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

Copy link

Collaborator

@soulomoon soulomoon Aug 19, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

~~(削除) Why just change it to use intelecate. (削除ここまで)~~

Copy link

Collaborator

@wz1000 wz1000 Sep 3, 2025

Choose a reason for hiding this comment

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

@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.

Copy link

Collaborator

@jian-lin jian-lin Sep 3, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

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 ==

dnikolovv added 2 commits

August 18, 2025 09:44

@dnikolovv


 Get rid of redundant dropEnd1

723d56c

@dnikolovv


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

b1eab33

... redundant dropEnd1"
This reverts commit 723d56c.

@fendor fendor self-requested a review

August 20, 2025 07:55

@fendor fendor added the status: needs review label

Aug 20, 2025

fendor

fendor requested changes

Sep 1, 2025

View reviewed changes

Copy link

Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

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

fendor requested changes

Sep 1, 2025

View reviewed changes

Copy link

Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Some more review

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

Comment on lines +1 to +24

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

Copy link

Collaborator

@fendor fendor Sep 1, 2025

Choose a reason for hiding this comment

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

Is this still needed?

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

Comment on lines +270 to +271

locationsWithIdentifier <- runIdeAction "TypeCheck" shakeExtras $ do

runMaybeT $ gotoTypeDefinition withHieDb (lookupMod hiedbWriter) opts har pos

Copy link

Collaborator

@fendor fendor Sep 1, 2025

Choose a reason for hiding this comment

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

I think we might have to worry about start-up time until a hover request can be served.
But this should mostly pull data from hiedb, iirc, so maybe it is completely fine.

@fendor fendor mentioned this pull request

Sep 4, 2025

Implement signature help #4626

Merged

Labels

status: needs review

6 participants

@dnikolovv @dschrempf @fendor @wz1000 @soulomoon @jian-lin

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

Conversation

@dnikolovv 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!

@jian-lin jian-lin Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@dnikolovv dnikolovv Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

@jian-lin 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!

@jian-lin jian-lin Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

@soulomoon 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!

@wz1000 wz1000 Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

@jian-lin 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 fendor left a comment

Choose a reason for hiding this comment

Uh oh!

@fendor fendor left a comment

Choose a reason for hiding this comment

Uh oh!

@fendor fendor Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

@fendor fendor Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@dnikolovv dnikolovv commented Aug 13, 2025 •

edited

Loading

@jian-lin jian-lin Aug 18, 2025 •

edited

Loading

@soulomoon soulomoon Aug 19, 2025 •

edited

Loading

@jian-lin jian-lin Sep 3, 2025 •

edited

Loading