LSP: Add Go To Definition support for Ast::ModuleAccess
#623
Conversation
Sija
added
enhancement
jansul
force-pushed
the
def-module-access
branch
from
9e0e5c0
to
c3ddc13
Compare
jansul
force-pushed
the
def-module-access
branch
from
2baf49b
to
53e8033
Compare
|
OK, turns out it was less straightforward than I thought 😅 I forgot that you can:
This PR should now work correctly with both. For the former, it will now link to all instances of a module. Here's how that looks like in VSCode: 2023-08-05.14-59-45.mp4 |
| @@ -4,7 +4,7 @@ module Mint | |||
| class Definition < LSP::RequestMessage | |||
| property params : LSP::TextDocumentPositionParams | |||
|
|
|||
| def execute(server) : LSP::LocationLink | LSP::Location? | |||
| def execute(server) : Array(LSP::LocationLink | LSP::Location) | LSP::LocationLink | LSP::Location | Nil | |||
There was a problem hiding this comment.
I'd prefer a more unified return type Array(LSP::LocationLink | LSP::Location)
There was a problem hiding this comment.
I thought the above matched the specification, but it turned out it doesn’t!
The return type for “Go to definition” can be any of Location | Location[] | LocationLink[] | null, but a singular LocationLink is not allowed - so we are currently out of spec 😅
Simplifying it to Array(LSP::LocationLink | LSP::Location) might be a good option, will have to have a think about how best to refactor all the definition methods.
There was a problem hiding this comment.
Simplifying it to
Array(LSP::LocationLink | LSP::Location)might be a good option, will have to have a think about how best to refactor all the definition methods.
IMO, that can be in a separate PR.
| @@ -4,7 +4,7 @@ module Mint | |||
| class Definition < LSP::RequestMessage | |||
| property params : LSP::TextDocumentPositionParams | |||
|
|
|||
| def execute(server) : LSP::LocationLink | LSP::Location? | |||
| def execute(server) : Array(LSP::LocationLink | LSP::Location) | LSP::LocationLink | LSP::Location | Nil | |||
There was a problem hiding this comment.
Simplifying it to
Array(LSP::LocationLink | LSP::Location)might be a good option, will have to have a think about how best to refactor all the definition methods.
IMO, that can be in a separate PR.
Fairly straightforward one, this adds Go To Definition support for
Ast::ModuleAccessas demonstrated in the video below:2023-08-05.11-55-11.mp4
While allowed by the language, some of these examples will error when running them in the browser. If you like I can have a look at why next if you like?
As part of this PR, I have removed the file
src/parsers/constant_variable.crand replaced it with the new method for parsing constants introduced in #594. This old version still had the bug which allowed invalid constants.