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

feat: Add the Hover Range capability which enables showing the type of an expression #9693

Merged
merged 19 commits into from
Jul 28, 2021
Merged

feat: Add the Hover Range capability which enables showing the type of an expression #9693

merged 19 commits into from
Jul 28, 2021

Conversation

alexfertel
Copy link
Contributor

@alexfertel alexfertel commented Jul 25, 2021

Closes #389

This PR extends the textDocument/hover method to allow getting the type of an expression. It looks like this:

type_of_expression

Edit: One thing I noticed is that when hovering a selection that includes a macro it doesn't work, so maybe this would need a follow-up issue discussing what problem that may have.

(PS: What a great project! I am learning a lot! 🚀)

@matklad
Copy link
Member

matklad commented Jul 26, 2021

Oh my, I didn't realise that it's possible to convince VS Code to actually use this! My original plan was to just implement this in the server and wait for emacs/vim to pick this up, and then bug vs code maintainers to follow the suite. You, however, managed to implement this in vscode, kudos!

@matklad
Copy link
Member

matklad commented Jul 26, 2021

Note that we already have a custom hover request (we customize the return type), so you only need to add custom params:

diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs
index f89b7f5d7..8a115ec30 100644
--- a/crates/rust-analyzer/src/handlers.rs
+++ b/crates/rust-analyzer/src/handlers.rs
@@ -867,7 +867,7 @@ pub(crate) fn handle_signature_help(
 
 pub(crate) fn handle_hover(
     snap: GlobalStateSnapshot,
-    params: lsp_types::HoverParams,
+    params: lsp_ext::HoverParams,
 ) -> Result<Option<lsp_ext::Hover>> {
     let _p = profile::span("handle_hover");
     let position = from_proto::file_position(&snap, params.text_document_position_params)?;
diff --git a/crates/rust-analyzer/src/lsp_ext.rs b/crates/rust-analyzer/src/lsp_ext.rs
index 7aed93f99..627cacc9c 100644
--- a/crates/rust-analyzer/src/lsp_ext.rs
+++ b/crates/rust-analyzer/src/lsp_ext.rs
@@ -376,24 +376,25 @@ pub struct SnippetTextEdit {
 pub enum HoverRequest {}
 
 impl Request for HoverRequest {
-    type Params = lsp_types::HoverParams;
+    type Params = HoverParams;
     type Result = Option<Hover>;
     const METHOD: &'static str = "textDocument/hover";
 }
 
-pub enum HoverRangeRequest {}
-
-impl Request for HoverRangeRequest {
-    type Params = HoverRangeParams;
-    type Result = Option<Hover>;
-    const METHOD: &'static str = "rust-analyzer/hoverRange";
-}
-
-#[derive(Deserialize, Serialize, Debug)]
+#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
-pub struct HoverRangeParams {
+pub struct HoverParams {
     pub text_document: TextDocumentIdentifier,
-    pub range: Range,
+    pub position: PositionOrRange,
+    #[serde(flatten)]
+    pub work_done_progress_params: WorkDoneProgressParams,
+}
+
+#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
+#[serde(untagged)]
+enum PositionOrRange {
+    Position(lsp_types::Position),
+    Range(lsp_types::Range),
 }
 
 #[derive(Debug, PartialEq, Clone, Deserialize, Serialize)]

@matklad
Copy link
Member

matklad commented Jul 26, 2021

And don't forget to document the extension in the lsp-extensions.md and add a corresponding server capability for it.

@alexfertel
Copy link
Contributor Author

Note that we already have a custom hover request (we customize the return type), so you only need to add custom params:

This is it!!! My god, I struggled so much because I thought the name of the param had to change 😭

Thanks for the quick response! I'll try to revisit this today 😁

@flodiebold
Copy link
Member

If it's just an extension to the existing hover request, I think this should be very easy to add in Emacs as well.

@alexfertel
Copy link
Contributor Author

alexfertel commented Jul 26, 2021

@matklad I think this is fairly complete :) No clue on how to approach adding the client/server capability 😅 Halp!

/// Shows additional information, like the type of an expression or the documentation for a definition when "focusing" code.
/// Focusing is usually hovering with a mouse, but can also be triggered with a shortcut.
///
/// image::https://user-images.githubusercontent.com/48062697/113020658-b5f98b80-917a-11eb-9f88-3dbc27320c95.gif
Copy link
Member

Choose a reason for hiding this comment

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

This needs to use only two slashes, otherwise it won't be picked up by our docgen infra.

@@ -423,6 +423,15 @@ impl Analysis {
self.with_db(|db| hover::hover(db, position, config))
}

/// Returns a short text displaying the type of the expression.
pub fn hover_range(
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a second function, let's change the existing one to take a range, and let's use empty range for the case where the input is a single position.

@@ -656,6 +656,31 @@ interface TestInfo {
}
```

## Hover Range

**Issue:** https://github.com/microsoft/language-server-protocol/issues/377
Copy link
Member

Choose a reason for hiding this comment

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

This should have **Experimental Server Capability:** { "hoverRange": boolean }, see how thouse are defiened for some other requests in this file.

Copy link
Member

Choose a reason for hiding this comment

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

@alexfertel alexfertel marked this pull request as ready for review July 27, 2021 22:32
@alexfertel alexfertel changed the title WIP: Add the Hover Range capability which enables showing the type of an expression feat: Add the Hover Range capability which enables showing the type of an expression Jul 27, 2021
crates/ide/src/hover.rs Outdated Show resolved Hide resolved
@alexfertel alexfertel requested a review from lnicola July 28, 2021 10:59
@matklad
Copy link
Member

matklad commented Jul 28, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 28, 2021

@bors bors bot merged commit 068ede0 into rust-lang:master Jul 28, 2021
@lnicola
Copy link
Member

lnicola commented Jul 28, 2021

Not exactly, but...

changelog feat (first contribution) add "Hover Range" capability to get the type of an expression

@simrat39
Copy link
Contributor

simrat39 commented Aug 11, 2021

Can the hover response have actions (hover actions) attached to them? I guess not since its only the value of an expression.

@Veykril
Copy link
Member

Veykril commented Aug 11, 2021

That should be doable just fine since we show a type we can show type hover actions, done with #9856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "get type of the expression" functionality
6 participants