Skip to content

Go to definition doesn't work for record syntax #1102

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

Closed
newhoggy opened this issue Sep 16, 2019 · 11 comments
Closed

Go to definition doesn't work for record syntax #1102

newhoggy opened this issue Sep 16, 2019 · 11 comments

Comments

@newhoggy
Copy link

Doesn't work for constructor

image

Doesn't work for fields

image

@cocreature
Copy link
Contributor

Thanks for the bug report, that definitely sounds like something that we should fix. If you want to take a look yourself, my guess would be that getSrcSpanInfos is the culprit.

@jacg
Copy link
Contributor

jacg commented Sep 19, 2019

It seems to work fine here:

constructor

... both for constructors and fields (though for fields it still refers to the position of the constructor).

Can you provide a minimal failing example?

@newhoggy
Copy link
Author

I defined person0 and person1.

The only time I get the popup is when I hover over the Person constructor in person1.

image

And Go To Definition doesn't work anywhere in the person0 case.

image

image

@ndmitchell
Copy link
Collaborator

I get definitions for the Person constructor in both person0 (due to https://github.com/digital-asset/ghcide/blob/master/src/Development/IDE/Spans/Calculate.hs#L122) and person1 (due to https://github.com/digital-asset/ghcide/blob/master/src/Development/IDE/Spans/Calculate.hs#L121). I don't get information about name or age because it doesn't have any cases that examine the record fields. This whole piece of code was copied wholesale from Intero, upgraded through a few GHC versions, lightly hacked for "things that didn't work", and could do with someone giving it some love. However, it shouldn't be that hard to make it a lot more powerful. If using SYB was OK then finding all names and grabbing the nearest enclosing Loc would probably be perfect.

@jacg
Copy link
Contributor

jacg commented Sep 23, 2019

If using SYB was OK

Has SYB been proscribed?

@ndmitchell
Copy link
Collaborator

SYB seems to be responsible for a lot of problems, e.g. haskell/haskell-ide-engine#1371. It's not fast and it is memory hungry. That said, it is concise and if it can be formulated properly, it's probably correct by construction.

@jacg
Copy link
Contributor

jacg commented Sep 25, 2019

I got tired of trying to verify manually what works and what does not, so I wrote some automated tests, which you can find here.

The one entitled "plain data constructor" passes while the one entitled "record data constructor" fails, which seems to agree with what @newhoggy observes above, as well as my own observations in both VSCode and Emacs; but it disagrees with what @ndmitchell observes above.

I get definitions for the Person constructor in [...] person0 (due to https://github.com/digital-asset/ghcide/blob/master/src/Development/IDE/Spans/Calculate.hs#L122)

Removing line 122, has no observable effect on any of the tests I have written. (I have confirmed that removal of lines 120, 121, or 147 does break some of my tests).

@jacg
Copy link
Contributor

jacg commented Sep 25, 2019

This whole piece of code [...] could do with someone giving it some love.

I'd like to have a go, but would benefit immensely from some pointers on how best to get stuck in.

@ndmitchell
Copy link
Collaborator

@mpickering ported the hie code over as part of https://github.com/digital-asset/ghcide/issues/101#issuecomment-534498465. If we can use that implementation rather than our own that makes most sense - less code = better.

@mpickering
Copy link
Contributor

I have wanted to experiment with geniplate, in
order to generate efficient traversal functions. That might be worth looking into.

@jneira
Copy link
Member

jneira commented Sep 9, 2020

I've just test it in hls and works for me, i guess it should work in ghcide too.
@newhoggy feel free to reopen if it is no the case

@jneira jneira closed this as completed Sep 9, 2020
@pepeiborra pepeiborra transferred this issue from haskell/ghcide Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants