Skip to content
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.

Sign up for GitHub

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

Jump to instance definition and explain typeclass evidence #4392

Merged
merged 8 commits into from
Oct 24, 2024
Merged

Jump to instance definition and explain typeclass evidence #4392

merged 8 commits into from
Oct 24, 2024

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Aug 29, 2024

Rebased and slightly modified version of #1983

Still requires some tests and docs.

@fendor fendor changed the title Jump to instance definition and explain typeclass evidence for Jump to instance definition and explain typeclass evidence Aug 29, 2024
@fendor fendor force-pushed the fendor/wip/evidence-info branch 2 times, most recently from 0ccc4d3 to 5568ca3 Compare August 29, 2024 15:35
@michaelpj
Copy link
Collaborator

Just at a high level, am I right that if I have

foo :: Num x => x
foo  = 1

let x :: Int = foo

and I call "go to implementation" on "foo" I will get the option to go to the Num Int definition? i.e. not to the "implementation" of "foo" in any real sense. (It's probably still the right method to use, just thinking)

@fendor
Copy link
Collaborator Author

fendor commented Aug 29, 2024

Yes, that's exactly what will happen!

michaelpj
michaelpj reviewed Aug 29, 2024
View reviewed changes
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Definitely needs some tests, it's not at all clear to me how it's going to show up.

ghcide/src/Development/IDE/LSP/HoverDefinition.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs
@@ -307,6 +331,39 @@ atPoint IdeOptions{} (HAR _ hf _ _ (kind :: HieKind hietype)) (DKMap dm km) env
UnhelpfulLoc {} | isInternalName name || isSystemName name -> Nothing
_ -> Just $ "*Defined " <> printOutputable (pprNameDefnLoc name) <> "*"

-- We want to render the root constraint even if it is a let,
-- but we don't want to render any subsequent lets
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this makes sense to someone who knows how evidence is represented in GHC...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't quite understand this either, maybe @wz1000 can help us out here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We just going to leave this without explanation? I guess so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an explanation, feel free to review :)

@michaelpj
Copy link
Collaborator

Can we include a test to see whether this now gives us some kind of useful hover information on overloaded record dot usage?

@fendor fendor force-pushed the fendor/wip/evidence-info branch 2 times, most recently from dfb7002 to dacc0b7 Compare September 17, 2024 13:21
@fendor fendor force-pushed the fendor/wip/evidence-info branch 8 times, most recently from 13b79de to 45fe83f Compare October 18, 2024 16:30
@fendor
Copy link
Collaborator Author

fendor commented Oct 18, 2024

I think this is good to go, there are still minor improvements to be made in the future, but I implemented one of them:

image

The Link for the source location is clickable and sends you to the source location in the editor.

Are there any crucial tests I am missing?

michaelpj
michaelpj approved these changes Oct 20, 2024
View reviewed changes
docs/features.md Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs
@@ -307,6 +331,39 @@ atPoint IdeOptions{} (HAR _ hf _ _ (kind :: HieKind hietype)) (DKMap dm km) env
UnhelpfulLoc {} | isInternalName name || isSystemName name -> Nothing
_ -> Just $ "*Defined " <> printOutputable (pprNameDefnLoc name) <> "*"

-- We want to render the root constraint even if it is a let,
-- but we don't want to render any subsequent lets
Copy link
Collaborator

Choose a reason for hiding this comment

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

We just going to leave this without explanation? I guess so

ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/Spans/AtPoint.hs Outdated Show resolved Hide resolved
ghcide/test/exe/FindImplementationAndHoverTests.hs Outdated Show resolved Hide resolved
@fendor
Copy link
Collaborator Author

fendor commented Oct 20, 2024

I figured out the Let evidence.

This is how it looks like when we skip single indirections:

image

But without skipping:

image

So, in other words, we try to avoid edges in the evidence tree if they essentially don't add any information.

@fendor fendor force-pushed the fendor/wip/evidence-info branch 2 times, most recently from 0508cdf to 37cb2b5 Compare October 20, 2024 16:57
@fendor fendor mentioned this pull request Oct 21, 2024
Open
@michaelpj
Copy link
Collaborator

(I think merge when you're happy, @fendor !)

wz1000 and others added 5 commits October 23, 2024 16:53
@wz1000 @fendor
improve hover rendering
fdba4e5
@fendor
Add "Goto Implementation" LSP handler
dc997a9 
Adds the necessary instances for handling the request type
`Method_TextDocumentImplementation`.
Further, wire up the appropriate handlers for the "gotoImplementation"
request.
@fendor
Add docs for 'Jump to Implementation' request
267a001
@fendor
Add Tests for 'Goto Implementation' feature
25f17fb
@fendor
Add pretty link for source location to hover
eed6ba4
@fendor fendor force-pushed the fendor/wip/evidence-info branch 2 times, most recently from 827d94d to 7679ed3 Compare October 23, 2024 14:59
@fendor fendor enabled auto-merge (squash) October 23, 2024 14:59
@fendor fendor mentioned this pull request Oct 23, 2024
Closed
4 tasks
@fendor fendor merged commit d923d82 into haskell:master Oct 24, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wz1000 wz1000 wz1000 approved these changes

@michaelpj michaelpj michaelpj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fendor @michaelpj @wz1000