Display instances on hover for type and data constructors #3963

DeviousStoat · 2024-01-15T19:24:13Z

An attempt to implement the requested enhancement in #3098

It displays the list of typeclass instances a type has when requesting hover information.
It should work on type constructors as well as the data constructors, using the related type.

It is added as a new section at the end of the hover information.

Here is an example of how it looks when hovering on Maybe or Just or Nothing

It seems to also be scope aware, if I import new instances then they will be added to the list. For example if I install hashable and import Data.Hashable with the Hashable instance for Int and hover on Int I get an additional

which is pretty cool.

soulomoon · 2024-01-16T12:00:39Z

It looks cool, 🤔 how is the result sorted, should we sort the result?

soulomoon

look good, some questions

soulomoon · 2024-01-16T12:01:52Z

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

@@ -59,6 +60,7 @@ import           Data.List.Extra                      (dropEnd1, nubOrd)

 import           Data.Version                         (showVersion)
 import           Development.IDE.Types.Shake          (WithHieDb)
+import           GHC                                  (getInstancesForType)


Can we get it from Compat?

Since in general we import ghc stuff from Development.IDE.GHC.Compat

so I should just export it in Development.IDE.GHC.Compat? Why is this needed? It seems to be available in this module in all the ghc versions supported by hls

I'm unsure of our policy here, wdyt @wz1000 ?

michaelpj

Looks plausible to me, I'd like a review from @wz1000 . And it needs some tests.

I note that we have a problem in our hovers in that there are no explanations for what the various sections of the hover mean. In this case I think it's obvious, but perhaps we could take the opportunity to add titles to the sections? For this bit I think it could just be Instances in scope for <name>?

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

michaelpj · 2024-01-17T09:42:52Z

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

+            where
+                instancesForName :: IO (Maybe [ClsInst])
+                instancesForName = runMaybeT $ do
+                    typ <- MaybeT . pure $ lookupNameEnv km n >>= tyThingToType


It looks like we look things up repeatedly in km. Should we do this once and then just pass in the TyThing to all the places that need it?

I am not sure what you mean. I think the lookup only happens once per Name and we need to look it up for each Name

I mean in the other functions we call around here. Probably not a huge deal

michaelpj · 2024-01-17T09:43:16Z

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

@@ -308,6 +311,24 @@ atPoint IdeOptions{} (HAR _ hf _ _ (kind :: HieKind hietype)) (DKMap dm km) env
            UnhelpfulLoc {} | isInternalName name || isSystemName name -> Nothing
            _ -> Just $ "*Defined " <> printOutputable (pprNameDefnLoc name) <> "*"

+        prettyInstances :: (Either ModuleName Name, IdentifierDetails hietype) -> IO (Maybe T.Text)
+        prettyInstances (Right n, _) =


In cases like this I'd usually just make the function take what it needs (just a Name) and get the caller to worry about massaging the list into the right form.

Sounds better, I switched it up

michaelpj · 2024-01-17T09:44:06Z

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

@@ -59,6 +60,7 @@ import           Data.List.Extra                      (dropEnd1, nubOrd)

 import           Data.Version                         (showVersion)
 import           Development.IDE.Types.Shake          (WithHieDb)
+import           GHC                                  (getInstancesForType)


I'm unsure of our policy here, wdyt @wz1000 ?

DeviousStoat · 2024-01-19T05:29:30Z

About the titles, I personally don't think it is too useful, imo it would just add unecessary clutter in the hover text.
You say In this case I think it's obvious but I don't really see any other section that is not obvious.
But of course if you all think it is a good idea I am willing to add them (I am kind of starting in the haskell world so I am a sucker for some small not too tricky tasks to improve :D) but maybe in another PR afterwards as it would be awkward to have only one title here. And adding them all in this PR is a bit out of scope.

DeviousStoat · 2024-01-19T06:28:08Z

hum, I used it a bit more and there are a few issues with this feature as it is currently implemented actually.
Hovering on some imported datatypes that are supposed to have instances don't show anything on hover. And it also doesn't show anything for datatypes defined in the current module with instances. I am not sure what's going on yet. I will be looking into it but I guess this PR is still WIP then.

michaelpj · 2024-01-19T09:16:15Z

Yes fine, we can leave the title thing for later

DeviousStoat · 2024-02-07T21:57:05Z

I looked a bit into it and I might need some guidance, there are some things I don't really understand.
So the issue I have is that the instances coming from external libraries (apart from GHC base libraries) and from the current file are not shown at all.

I believe the issue comes from there

clsInst <- liftIO $ evalGhcEnv env $ getInstancesForType typ

It seems that the env doesn't have the required modules loaded at this point.

Doesn't the GHCSession action takes care of this? I tried using GHCSessionDeps but it didn't change anything. Do you know what the difference is between the 2?

I found this function in the hls-eval-plugin. It seems to have to manually load the modules in the session. Is it something I would probably have to do to execute interactive functions like the getInstancesForType? If so I don't exactly understand what this env is all about, is it just an empty ghc execution context?

I am guessing there are some things like types and docstrings that are cheap to reload but instances is not part of these.

soulomoon · 2024-02-23T08:39:45Z

Doesn't the GHCSession action takes care of this?

Perhaps we are not taking care of things that is not used yet, you can dig deeper to see how do we manage to include the information in the session.

DeviousStoat requested a review from pepeiborra as a code owner January 15, 2024 19:24

DeviousStoat force-pushed the display-instances branch from 7adc0ac to eb7c90b Compare January 15, 2024 19:25

fendor requested review from wz1000 and fendor January 16, 2024 08:31

soulomoon reviewed Jan 16, 2024

View reviewed changes

michaelpj reviewed Jan 17, 2024

View reviewed changes

DeviousStoat force-pushed the display-instances branch from eb7c90b to e80d081 Compare January 19, 2024 05:09

soulomoon added the WIP label Jan 23, 2024

DeviousStoat added 4 commits February 11, 2024 22:51

Display instances on hover for type and data constructors

f4393e6

Rename tyThingToType

a67792e

prettyInstances takes Name argument

cdefac2

Avoid creating an empty section if no instances are found

cf69ce5

DeviousStoat force-pushed the display-instances branch from 39a6e25 to cf69ce5 Compare February 11, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display instances on hover for type and data constructors #3963

Display instances on hover for type and data constructors #3963

DeviousStoat commented Jan 15, 2024

soulomoon commented Jan 16, 2024 •

edited

soulomoon left a comment

soulomoon Jan 16, 2024

soulomoon Jan 16, 2024

DeviousStoat Jan 16, 2024

michaelpj Jan 17, 2024

michaelpj left a comment

michaelpj Jan 17, 2024

DeviousStoat Jan 19, 2024

michaelpj Jan 19, 2024

michaelpj Jan 17, 2024

DeviousStoat Jan 19, 2024

michaelpj Jan 17, 2024

DeviousStoat commented Jan 19, 2024

DeviousStoat commented Jan 19, 2024

michaelpj commented Jan 19, 2024

DeviousStoat commented Feb 7, 2024 •

edited

soulomoon commented Feb 23, 2024

Display instances on hover for type and data constructors #3963

Are you sure you want to change the base?

Display instances on hover for type and data constructors #3963

Conversation

DeviousStoat commented Jan 15, 2024

soulomoon commented Jan 16, 2024 • edited

soulomoon 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

michaelpj 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

Choose a reason for hiding this comment

DeviousStoat commented Jan 19, 2024

DeviousStoat commented Jan 19, 2024

michaelpj commented Jan 19, 2024

DeviousStoat commented Feb 7, 2024 • edited

soulomoon commented Feb 23, 2024

soulomoon commented Jan 16, 2024 •

edited

DeviousStoat commented Feb 7, 2024 •

edited