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.

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

Implement textDocument/typeDefinition #201

Closed
kjeremy opened this issue Nov 5, 2018 · 17 comments
Closed

Implement textDocument/typeDefinition #201

kjeremy opened this issue Nov 5, 2018 · 17 comments
Labels
E-easy E-has-instructions Issue has some instructions and pointers to code to get started good first issue

Comments

@kjeremy
Copy link
Contributor

kjeremy commented Nov 5, 2018

From #197

@aochagavia :

Regarding type checking, we need to be able to:

Get the type of the expression under the cursor
List the available functions for the type (which means knowing which traits are implemented by it)

On the LSP front this would involve implementing textDocument/typeDefinition.

@DJMcNab
Copy link
Contributor

DJMcNab commented Nov 6, 2018

We also need to handle type parameters properly (I.e. typeDefinition should jump to the function signature/impl <trait>)

@matklad
Copy link
Member

matklad commented Nov 6, 2018

@kjeremy note that "get type of the epxression" is actually required for signature info as well.

For something like

(foo)(1, 2, 3)

You need to figure out that (foo) expressin has a funciton type

@matklad matklad added help wanted good first issue E-easy E-has-instructions Issue has some instructions and pointers to code to get started labels Dec 29, 2018
@h-michael
Copy link
Contributor

I'll try it.

@h-michael
Copy link
Contributor

type_of function needs FileRange, but TextDocumentPositionParams does not have range.
Is there function which build FileRange from params which TextDocumentPositionPrams has.

If there is not, I'd like to add that function.
Where should I add to that function?

@matklad
Copy link
Member

matklad commented Dec 30, 2018

Oh, wait, typeDefinition is not "show the type of selected expression", it is "like go to declaration, but instead go to the type of the declaraions". So, if you have let x = Foo::new(); x<|>, typeDefinition on the second x should focus the editor on the file where Foo is defined.

Looks like there's no built-in support for get type of the expression on the LSP at the moment, but it souldn't be too hard to add an extension for this. First, one needs to define TypeOfExpression request here:

https://github.com/rust-analyzer/rust-analyzer/blob/e2bc57e3c89e469eee772aa8e42df1d453fea922/crates/ra_lsp_server/src/req.rs#L28-L47

Params would be a text document id and a range, response would be a string

Then one should add & register handler as described above.

Then, for VS Code, in typescript, one should define a new ra-lsp command here:

https://github.com/rust-analyzer/rust-analyzer/blob/e2bc57e3c89e469eee772aa8e42df1d453fea922/editors/code/package.json#L78-L81

register it here:

https://github.com/rust-analyzer/rust-analyzer/blob/e2bc57e3c89e469eee772aa8e42df1d453fea922/editors/code/src/extension.ts#L46

and write the actual implementation here

https://github.com/rust-analyzer/rust-analyzer/tree/e2bc57e3c89e469eee772aa8e42df1d453fea922/editors/code/src/commands

The implemntation should work like the one for extend selection, and use VS Code API to show a message with type to the user.

@DJMcNab
Copy link
Contributor

DJMcNab commented Dec 30, 2018

In Typescript, the type is shown on hover. Should we try and emulate that too/instead.

@matklad
Copy link
Member

matklad commented Dec 30, 2018

Good point! I think we should have both: hover doesn't allow selecting the expression, and it is useless for folks who don't use mouse :)

@h-michael
Copy link
Contributor

Oh, wait, typeDefinition is not "show the type of selected expression", it is "like go to declaration,

I see that.

Looks like there's no built-in support for get type of the expression on the LSP at the moment, but it souldn't be too hard to add an extension for this. First, one needs to define TypeOfExpression request here:

I also think geting the type of expression might be implemented as hover for now.

@h-michael
Copy link
Contributor

hover doesn't allow selecting the expression

LanguageClient-neovim handles textDocument/hover by mapped command and current cursor word.
We can use it without mouse.

@h-michael
Copy link
Contributor

BTW, implementing showing type definition whether textDocument/hover or textDocument/codeAction, I want to know the way calling type_of function from textDocument/typeDefinition which does not have FileRange.
Because I think that implementing go to type definition need knowing current cursor expression type.

@flodiebold
Copy link
Member

flodiebold commented Dec 30, 2018

You can get the expression under the cursor using the find_node_at_offset function, e.g. find_node_at_offset::<ast::Expr>(file.syntax(), offset). You can then get that expression's type.

For typeDefinition, you'll probably want to change type_of to return the actual Ty instead of just a string (the string is just ty.to_string()). You can then walk through the Ty to get a location: For Ty::RawPtr and Ty::Ref, we probably want to recurse into the inner type (i.e. for a variable of type &A, we want to jump to A); when you encounter a Ty::Adt, you can get the location of the definition from its def_id. If you encounter anything else, there's probably currently nothing to jump to.

I think we should show types of local variables on hover, but it would also be nice to have a way to get the type of an arbitrary expression (e.g. in foo.bar().baz(), what's the type of foo.bar() without first extracting it to a local?).

(By the way, Emacs' lsp-ui shows the hover values for everything in the current line on the sideline. That can get a bit overwhelming, but can also be pretty nice...)

@h-michael
Copy link
Contributor

find_node_at_offset needs specific type annotation of ast::.
In this case, I think there are a lot of syntax kind candidates.
So I think we might to add function that builds FileRange from TextDocumentPositionParams.
The LSP server receives TextDocumentPositionParams, but I think that we may have other cases where not only position but also range is needed.

BTW, I think textDocument/typeDefinition target is not only TYPE_DEF.
TYPE_DEF syntax kind is means type alias on Rust context.
What syntax kinds are we going to include to for this feature?

@DJMcNab
Copy link
Contributor

DJMcNab commented Dec 31, 2018

I think that the Ty type should have a way to get the types SourceFileItemId, so we can just use that. That will go to the struct def or union def etc.

@h-michael
Copy link
Contributor

@matklad
Do you have other thoughts and plans?

Sorry for everyone to take some time to understand that.

@matklad
Copy link
Member

matklad commented Dec 31, 2018

I am going to close this issue and split it into the two, because I am myself confused as to what exactly we are discussing :-)

  • one about textDocument/typeDefinition
  • one about hover/type of expression at point.

@matklad
Copy link
Member

matklad commented Dec 31, 2018

@h-michael I've split off #389 and #388. I think we should do #389 first, I've added some more concrete instructions for that. If anything is unclear, do ask questions at that issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy E-has-instructions Issue has some instructions and pointers to code to get started good first issue
Projects
None yet
Development

No branches or pull requests

5 participants