haskell / haskell-language-server

Official haskell ide support via language server (LSP). Successor of ghcide & haskell-ide-engine.
Apache License 2.0
2.66k stars 355 forks source link

Add support for going to definition of overloaded labels as field lenses #1756

Open arybczak opened 3 years ago

arybczak commented 3 years ago

optics-core supports using overloaded labels as field lenses (and constructor prisms) for types with a Generic instance since 0.4.

Consider the following module (full project for ease of reproducibility is here).

{-# LANGUAGE DeriveGeneric, OverloadedLabels #-}
module Main where

import Optics.Core
import GHC.Generics (Generic)

data User p = User { name :: String, age :: Int }
  deriving (Show, Generic)

getName_gotoWorks :: User p -> String
getName_gotoWorks u = name u

getName_gotoDoesntWork :: User p -> String
getName_gotoDoesntWork u = u ^. #name

setFields_ambiguousMiddleType :: User p -> User p
setFields_ambiguousMiddleType u = u & #name .~ "Tom"
                                    & #age .~ 45

main :: IO ()
main = pure ()

With name in the definition of getName_gotoWorks jumping to the definition works fine. However, trying to do the same using #name within getName_gotoDoesntWork doesn't.

Would it be possible to add a support for this? On hover the term #name is a dictionary for IsLabel "name" (Optic' A_Lens NoIx (User p) String), so in principle it's possible to go to the field definition as all the information is there.

A somewhat more complicated case is in setFields_ambiguousMiddleType: hovering over #name there shows just IsLabel, presumably due to undetermined phantom type variable (since getting rid of the type variable show the full type) :thinking: Hovering over #age gives all the dictionary info (IsLabel "age" (Optic A_Lens NoIx (User Any) (User p) Int Int).

Output of haskell-language-server --probe-tools:

haskell-language-server version: 1.0.0.0 (GHC: 8.10.4) (PATH: /home/unknown/.cabal/store/ghc-8.10.4/haskell-language-server-1.0.0.0-e-haskell-language-server-789994ec071fd92b667a696b70ccf6882ad277e8ef1bfac3a85c7a034386e70e/bin/haskell-language-server)
Tool versions found on the $PATH
cabal:          3.4.0.0
stack:          2.3.3
ghc:            8.10.4
Ailrun commented 3 years ago

Thank you for the request! However, this one looks like an optic-library specific feature, as that #name is not directly related to name outside of the library.

If you think this one is more general than optic, could you elaborate why do you think so?

arybczak commented 3 years ago

However, this one looks like an optic-library specific feature

That is unfortunately true, it's specific to optics. generic-lens offers something similar for lens-compatible optics, but it uses different types.

It's just that with tags-based navigation jumping to definition using #name works (admittedly somewhat by accident since # is not considered part of a symbol and dropped), while with HLS it doesn't.

It's quite unfortunate, because using labels as field lenses is the only way to handle fields without prefix in a sane manner in Haskell, so it would be great if it did

michaelpj commented 3 years ago

I can't think of a way to make this nice without a special case. The IsLabel instance really is in optics, and there's no obvious way to tell that it's "really" associated with one of the types that appears in the instance head, let alone a particular field of that type.

And even if we had a special case, it would sometimes be wrong. Where should we jump to if you write a function that's generic over all LabelOptic "name" k s t a b? You could still write #name, but where would it go? The only reasonable thing that makes sense in all contexts is the definition of fromLabel... but that's not what you want.

It's just that with tags-based navigation jumping to definition using #name works (admittedly somewhat by accident since # is not considered part of a symbol and dropped), while with HLS it doesn't.

I think "it works in my existing system due to a bug" is not a good argument for replicating that ;) This would of course go horribly wrong for any other use of OverloadedLabels. Or with multiple records with "name" fields, etc.

wz1000 commented 3 years ago

With GHC 9.0 we can implement "go to instance definition", like this. That won't take you to the record field, but it would take you to both the IsLabel instance and the Generic instance for your specific record.

arybczak commented 3 years ago

I can't think of a way to make this nice without a special case.

Right, I don't think it's possible.

The only reasonable thing that makes sense in all contexts is the definition of fromLabel... but that's not what you want.

Indeed, it's quite unfortunate.

@michaelpj Your points are valid, all I'm pointing out is that it's unfortunate that it doesn't work. Perhaps if optics was more prevalent in the ecosystem and the go to solution for labels-as-lenses, making it a special case would make more sense.

Perhaps we should wait for that to happen (assuming that it does) and re-evaluate. I'd obviously like that to happen sooner as an author and a user, but oh well :)

That won't take you to the record field, but it would take you to both the IsLabel instance and the Generic instance for your specific record.

@wz1000 Would it take me to a Generic instance definition for the type (which is almost always below field definitions)? If that was an option, it would be great.

wz1000 commented 3 years ago

@wz1000 Would it take me to a Generic instance definition for the type (which is almost always below field definitions)? If that was an option, it would be great.

Yes it would.