Hover actions #4729

vsrs · 2020-06-03T12:31:13Z

This PR adds a hoverActions LSP extension and a Go to Implementations action as an example:

kjeremy · 2020-06-03T12:50:11Z

Since this is a protocol extension we might want a heavy test for when this is and isn't supported.

crates/rust-analyzer/src/lsp_ext.rs

vsrs · 2020-06-03T13:19:15Z

@kjeremy thanks!

I wasn't aware of #[serde(flatten)]. Looks very useful.
I'll use it and add some heavy tests.

kjeremy · 2020-06-03T14:55:38Z

crates/ra_ide/src/hover.rs

+impl Default for HoverConfig {
+    fn default() -> Self {
+        Self { implementations: true }
+    }
+}


Since the client has to opt in to this feature it makes more sense for the default to be false.

Initially it was.

matklad said(#4659 (comment))

I think in ra_ide we generally enable everything by deafault, and than rust-analyzer explicitelly disables stuff it doesn't like.

kjeremy · 2020-06-03T14:56:04Z

crates/ra_ide/src/hover.rs

    pub fn to_markup(&self) -> String {
-        self.results.join("\n\n---\n")
+        self.results.join("\n\n___\n")


Is this correct? We just went back and forth on this a few PRs ago.

I believe yes, it is. As I can see '---' was replaced by '___', but not in this place.

--- cause problems if hover content is just a single line.

some test ---

Becomes

some test

while

some test ___

some test

Btw, hover actions are rendered on the client and this method is not used for them at all.

kjeremy · 2020-06-03T19:15:35Z

You'll need to rebase ~~and regenerate the docs it looks like~~. Then I think @matklad should give it a once over.

vsrs · 2020-06-03T19:54:46Z

Rebased.

matklad · 2020-06-05T10:24:30Z

crates/rust-analyzer/tests/heavy_tests/main.rs

+            "range": { "end": { "character": 13, "line": 2 }, "start": { "character": 7, "line": 2 } }
+        })
+    );
+}


I'd rather not add heavy tests for this functionality:

the cost of a heavy test is high, as it spawns an external process

the additional confidence it provides is low, as it only verifies the JSON we produce, and not how the other side interprets the JSON.

So, almost all code in handlers.rs and conv.rs is deliberately not tested, and we just rely on types and manual testing for correctness, and on keeping these modules trivial-ish.

So far, it worked ok -- the regressions I remember usually involve bugs in https://github.com/Microsoft/vscode-languageserver-node/

I'm not against the deleting. Just one consideration:

additional confidence it provides is low, as it only verifies the JSON we produce,

I do not think so. Such tests let us to ensure that we produce valid JSON.

For example, these tests helped me to find and fix a problem: with disabled hoverActions client capability, the server was still adding empty actions field to the Hover response. Of course, LSP defines that an unknown property should be ignored, but it is not always the case.

I still think it makes sense to delete. I agree that this can help to find some bugs, probably quite a few of them. But, over the lifetime of the project, the cost of running&maintaining these test seems like it would be higher than the additional benefit.

(as a more general design note, I find that for ironing bugs on the integration boundary the most efficient strategy is release trains (so that folks can try things out in various real world configurations) and speedy release process (so that you can submit hotfix and make a new release quckly))

yea, this strategy should work well for such an active project :)

matklad · 2020-06-05T10:29:31Z

docs/dev/lsp-extensions.md

+  | TITLE _Action1_ | _Action2_ |  <- second group
+  +-----------------------------+
+  ...
+```


Hm, I feel like this suffers the same problem as codeLens request -- it necessitates client-specific code. I guess it's not something we can fix on our side.

The client has to explicitly set the capability field and to implement some commands anyway (rust-analyzer.showReferences in this case) therefore I think it is ok.

Besides, I hope that it would be possible to support hover actions functionality in editors.
I've checked: in vscode it is very easy to add, in sublime it already exists and one just need to get information from the new Hover response :)

matklad · 2020-06-05T10:30:44Z

editors/code/src/client.ts

+        return md;
+    }
+    return obj;
+}


Is this really trusted? Should we do some kind of validation/escaping here?

Good catch!

Indeed someone can write in a doc comment something like
[title]('command:some.vscode.command')
and we will render the corresponding link in the hover without any validation. I left this code in the hope it would be useful to implement intra-doc links later. But now I think I'd better remove it, and if we will support intra-doc links we have to add some validation. But not in this PR.

Command links added to the hover bottom are safe as we fully control them. So this code is safe in turn: hover.contents.push(renderHoverActions(actions));

matklad · 2020-06-05T10:34:20Z

crates/ra_ide/src/hover.rs

 /// Contains the results when hovering over an item
 #[derive(Debug, Default)]
 pub struct HoverResult {
    results: Vec<String>,
+    actions: Vec<HoverAction>,
 }


Yup, this API makes sense to me!

matklad · 2020-06-05T10:34:55Z

LGMT, with testing strategy changed to not rely on heavy tests.

Hover contents might be extracted from raw doc comments and need some validation.

vsrs · 2020-06-05T12:39:02Z

It seems to be OK now.

matklad · 2020-06-05T15:10:04Z

bors r+

Thanks @vsrs !

bors · 2020-06-05T15:22:34Z

Build succeeded:

Summary: The change introduces support _hover actions_, a LSP extension modeled after the Rust Analyzer one (introduced [here](rust-lang/rust-analyzer#4729)). Hover actions are appended, on the client side, as _command links_ to the bottom of a hover popup. They are disabled by default for now. The LSP extension works by extending the standard `Hover` result with optional actions. Change Highlights: * Introduce two configuration values: ` hoverActions_enable` (to control hover actions as a whole) and `hoverActions_docLinks_enable` (to control hover actions of type `docLink`) * Introduce the LSP extension * Introduce a `DocLink` action to point to external docs, when available Reviewed By: alanz Differential Revision: D48154706 fbshipit-source-id: 45a1850a03e350d44cad046f4222f66a1f981d56

kjeremy reviewed Jun 3, 2020

View reviewed changes

crates/rust-analyzer/src/lsp_ext.rs Show resolved Hide resolved

kjeremy reviewed Jun 3, 2020

View reviewed changes

crates/rust-analyzer/src/lsp_ext.rs Show resolved Hide resolved

kjeremy reviewed Jun 3, 2020

View reviewed changes

crates/rust-analyzer/src/lsp_ext.rs Outdated Show resolved Hide resolved

vsrs force-pushed the hover_actions_lsp_ext branch from 30d516a to 4193ad7 Compare June 3, 2020 14:50

kjeremy reviewed Jun 3, 2020

View reviewed changes

kjeremy requested a review from matklad June 3, 2020 19:15

vsrs force-pushed the hover_actions_lsp_ext branch from 4193ad7 to a0dfb72 Compare June 3, 2020 19:52

matklad reviewed Jun 5, 2020

View reviewed changes

vsrs added 9 commits June 5, 2020 14:59

Add hover actions as LSP extension

7d0dd17

Add hover actions LSP extension documentation.

da7ec4b

Code formatting

b147e6e

Rebase on the latest master.

5d0c1aa

Add enum hover action test.

92cfc0f

Apply suggestions from @kjeremy review

e35418c

Add capabilities tests.

0fe43a1

Remove hover contents marking as trusted.

78c9223

Hover contents might be extracted from raw doc comments and need some validation.

Remove hover actions heavy tests.

bd9d7b6

vsrs force-pushed the hover_actions_lsp_ext branch from a0dfb72 to bd9d7b6 Compare June 5, 2020 12:27

bors bot merged commit 4029628 into rust-lang:master Jun 5, 2020

vsrs deleted the hover_actions_lsp_ext branch June 5, 2020 15:35

fannheyward mentioned this pull request Jun 8, 2020

LSP Extensions support fannheyward/coc-rust-analyzer#256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hover actions #4729

Hover actions #4729

vsrs commented Jun 3, 2020

kjeremy commented Jun 3, 2020

vsrs commented Jun 3, 2020

kjeremy Jun 3, 2020

vsrs Jun 3, 2020

kjeremy Jun 3, 2020

vsrs Jun 3, 2020

kjeremy Jun 3, 2020

vsrs Jun 3, 2020

kjeremy commented Jun 3, 2020 •

edited

Loading

vsrs commented Jun 3, 2020

matklad Jun 5, 2020 •

edited

Loading

vsrs Jun 5, 2020

matklad Jun 5, 2020

matklad Jun 5, 2020

vsrs Jun 5, 2020

matklad Jun 5, 2020

vsrs Jun 5, 2020

matklad Jun 5, 2020

vsrs Jun 5, 2020

matklad Jun 5, 2020

matklad commented Jun 5, 2020

vsrs commented Jun 5, 2020

matklad commented Jun 5, 2020

bors bot commented Jun 5, 2020

Hover actions #4729

Hover actions #4729

Conversation

vsrs commented Jun 3, 2020

kjeremy commented Jun 3, 2020

vsrs commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

some test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjeremy commented Jun 3, 2020 • edited Loading

vsrs commented Jun 3, 2020

matklad Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jun 5, 2020

vsrs commented Jun 5, 2020

matklad commented Jun 5, 2020

bors bot commented Jun 5, 2020

kjeremy commented Jun 3, 2020 •

edited

Loading

matklad Jun 5, 2020 •

edited

Loading