alephium / ralph-lsp

Ralph language server
7 stars 1 forks source link

Implement find references #289

Closed tdroxler closed 2 months ago

tdroxler commented 2 months ago

Find references request

I thought I already created an issue for that, but maybe I only mentioned it on slack.

This one is an easy win I think, because the results are the same as when asking the Jump to definition while being on the definition itself.

Even tho it gives the same result, I think it's fine to add this request as users might be used to use this request and have their shortcut defined for this request.

simerplaha commented 2 months ago

Hey @tdroxler, looks like this won't be an easy win. I've explained the issue below with an implementation.

Issue

The current "Go to Definitions" provider returns definitions or references depending on where the request was executed. This is an issue because if "Go to References" is executed on a reference itself, the search will return "jump to definitions" results instead of returning other reference locations. Similarly if "Go to Definitions" is executed on a definition itself, the search will jump to references.

The header also mentions "Definitions"

image

instead of "References"

image

An implementation (Test)

The branch request_references_v1 implements "Go to References" and "Find all References" using the current GoToDefinitionProvider with the flags isReferencesSearch and includeSelf, not a perfect solution.

But this does not completely resolve the issue, because executing "Go to References" on a reference still goes to "Definitions", which could be resolved with more flags and checks, also not ideal.

The headers are also not resolved.

Solution

GoToReferencesProvider should be separated from GoToDefinitionProvider, so they follow the protocol (no need for unnecessary flag and checks). If "Go to Definitions" is executed on a definition, it should return current definition node itself and not return references, to which VSCode dispatches a new "Go to References" request (I did not know this before, don't think it's mentioned in the documentation either).

This change also requires minor changes to the goTo tests that test usages vs definitions.

tdroxler commented 2 months ago

thx for the explanation, indeed a bit more involving than I thought.

So no need to rush on that issue, let's do it when time comes.

to which VSCode dispatches a new "Go to References" request

I guess that's a way to make it friendly for users