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

Support "get type of the expression" functionality #389

Closed
matklad opened this issue Dec 31, 2018 · 18 comments · Fixed by #9693
Closed

Support "get type of the expression" functionality #389

matklad opened this issue Dec 31, 2018 · 18 comments · Fixed by #9693
Labels
E-medium good first issue S-actionable Someone could pick this issue up and work on it right now

Comments

@matklad
Copy link
Member

matklad commented Dec 31, 2018

It's very useful to be able to get the type of the expression at the cursor position. The way this feature should ideally work is that you place the cursor on the target expression, like 1 +<|> 2.foo(), then press "extend selection" several times until you select the the whole expression, and then invoke "show type".

Unfortunatelly, LSP does not have an API for this sort of functionality yet, so we need to hack it a bit.

I propose the following plan:

  • define type_of_expression(&self, FileRange) -> Option<String> API on the Analysis
  • implement textDocument/hover on top of it. Hover talks about positions, not ranges, so the implementation should first find an ast::Expr at the given offset (using find_node_at_offfset) and then invoke type_of_expression.
  • Implement our own extension ra-lsp/typeOfExpression which sends a range instead of a single offset.

See this issue for the implementation tips.

@h-michael
Copy link
Contributor

I try this, before #388.

@h-michael
Copy link
Contributor

@matklad
I think type_of_infer needs Hir::Expr and that is discussed at #386.
Did I get that right?

  • add Hir::Expr
  • add type_of_expression to ra_analysis
  • add textDocument/hober
  • add ra-lsp/typeOfExpression that might be as textDocument/codeAction

@matklad
Copy link
Member Author

matklad commented Jan 1, 2019

@h-michael I am not sure: we already have Analysis::type_of which works with ranges, here:

https://github.com/rust-analyzer/rust-analyzer/blob/5a866a772c863ba7e3ec8bad353c1b6997a7a62a/crates/ra_analysis/src/lib.rs#L373-L375

So we only need to expose that to the LSP layer, by writing appropriate handlers in ra_lsp_server crate.

hir::Expr is required to make the implementation of Analysis::type_of more powerful, but it shouldn't affect the interface.

@h-michael
Copy link
Contributor

@matklad
Now, ra_lsp_server#handle_hover shows Analysis#doc_text_for's results.
Might we merge Analysis#type_of's result with above, or replace with Analysis#type_of's result?

@matklad
Copy link
Member Author

matklad commented Jan 3, 2019

I think we should prefer to show docs over showing the type.

@h-michael
Copy link
Contributor

I think we should prefer to show docs over showing the type.

What you said is that we show type by ra-lsp/typeOfExpression or textDocument/codeAction instead of textDocument/hober ?

@matklad
Copy link
Member Author

matklad commented Jan 3, 2019

No, I mean the logic should be as roughly follows:

fn handle_hover(position) {
    lf let Some(docs) = analysis.doc_text_for(position) {
        return docs
    }
    if let Some(type_of) = analysis.type_of(position) {
        return type
    }
}

@h-michael
Copy link
Contributor

I see, thanks for explaining a detail!

@DJMcNab
Copy link
Contributor

DJMcNab commented Jan 3, 2019

This should be handled in description_of, btw.

@h-michael
Copy link
Contributor

This should be handled in description_of, btw.

I also think it is better.

@h-michael
Copy link
Contributor

But I think if NavigationTarget#description were failed, type_of may be also failed.
I try to implement by handling in NavigationTarget#description for now.

bors bot added a commit that referenced this issue Jan 5, 2019
414: textDocument/hover returns both type name and doc_text r=matklad a=h-michael

implement #389

Co-authored-by: Hirokazu Hata <h.hata.ai.t@gmail.com>
@matklad
Copy link
Member Author

matklad commented Jul 15, 2020

Blocked on upstream: microsoft/language-server-protocol#377

@lnicola lnicola added the S-unactionable Issue requires feedback, design decisions or is blocked on other work label Jan 25, 2021
@matklad matklad added S-actionable Someone could pick this issue up and work on it right now and removed S-unactionable Issue requires feedback, design decisions or is blocked on other work labels May 27, 2021
@matklad
Copy link
Member Author

matklad commented May 27, 2021

Actually, there's no reason to wait for the upstream -- we can just impletent this extension on the server, emacs and vim folks will probably pick it up, and then we can poke upstream until this is documented. So, mentoring instructions:

@alexfertel
Copy link
Contributor

@matklad Can I take a jab at implementing this?

@matklad
Copy link
Member Author

matklad commented Jul 5, 2021

@alexfertel sure!

@lnicola
Copy link
Member

lnicola commented Jul 5, 2021

@alexfertel it's yours.

@alexfertel
Copy link
Contributor

Hey, guys, sorry to be this slow, I haven't had a lot of time and there's a lot of reading (both code and not code) to be done, so I can tackle this implementation.

I promise that I will eventually submit a PR, hopefully in the next few days.

@Veykril
Copy link
Member

Veykril commented Jul 18, 2021

No rush, it takes time to get familiar with unknown codebases. If you got questions about things you don't understand or which seem unclear feel free to hit up the zulip channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium good first issue S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants