Server-side Semantic Tokens #3159

kjeremy · 2020-02-15T21:51:49Z

Takes the output of syntax_highlighting and converts it to the proposed semantic tokens API (so far only textDocument/semanticTokens. There's a lot of cool stuff we could do with this and the "Inspect Editor Tokens and Scopes" vscode command (pic below) is a cool way to see what has tokens and what doesn't.

Incredibly hacky and could panic due to unwraps, panic! etc. To use: run with code-insiders --enable-proposed-api matklad.rust-analyzer. If you try to run this without --enable-proposed-api it will crash.

@matklad I'm mostly looking for design feedback.

kjeremy · 2020-02-15T21:55:14Z

crates/ra_lsp_server/src/caps.rs

@@ -57,6 +59,49 @@ pub fn server_capabilities() -> ServerCapabilities {
        execute_command_provider: None,
        workspace: None,
        call_hierarchy_provider: Some(CallHierarchyServerCapability::Simple(true)),
+        semantic_tokens_provider: Some(SemanticTokensServerCapabilities::SemanticTokensOptions(


I'm thinking the token and modifier lists should be exposed in a semantic_tokens.rs file in ra_ide so they can be reused in conv.rs. Maybe as arrays or strings that then get converted to SemanticTokenType and SemanticTokenModifer via into() when we fill in the server caps?

kjeremy · 2020-02-15T21:57:20Z

crates/ra_lsp_server/src/main_loop/handlers.rs

+    data: Vec<SemanticToken>,
+}
+
+impl SemanticTokensBuilder {


A straight forward port of the typescript version but without the support for edits.

Veetaha · 2020-02-15T22:01:20Z

editors/code/vscode.proposed.d.ts

@@ -0,0 +1,1737 @@
+/*---------------------------------------------------------------------------------------------


What is the rationale behind copying it here?

Followed instructions for proposed extensions. It will be removed before merging.

Veetaha · 2020-02-15T22:09:28Z

crates/ra_lsp_server/src/conv.rs

+    type Output = (SemanticTokenType, Vec<SemanticTokenModifier>);
+
+    fn conv_with(self, _world: &WorldSnapshot) -> (SemanticTokenType, Vec<SemanticTokenModifier>) {
+        match self {


I would also put a function-local use SemanticTokenType as Stt; here to decrease verbosity, but I am not sure that everyone likes this style...

Yeah, I am not a fan of local use * or renames.

To make this less verbose, I'd ranter

let tag = match { // many trivial branches; } let mods = match { // copule of non-trivial branches _ => vec![] }

It also makes sense to refactor our internal repr to use tag + mods apporach, but that should be done separately.

Or

let simple_token_type = match self { tags::KEYWORD => Stt::KEYWORD, tags::SOMETHING => return (Stt::SOMETHING, vec![MODIFIER]) } return (simple_token_type, vec![])

crates/ra_lsp_server/src/conv.rs

Veetaha · 2020-02-15T22:26:32Z

crates/ra_lsp_server/src/conv.rs

+                SemanticTokenType::COMMENT,
+                SemanticTokenType::KEYWORD,
+                SemanticTokenType::STRING,
+                SemanticTokenType::NUMBER,
+                SemanticTokenType::REGEXP,
+                SemanticTokenType::OPERATOR,
+                SemanticTokenType::NAMESPACE,
+                SemanticTokenType::TYPE,
+                SemanticTokenType::STRUCT,
+                SemanticTokenType::CLASS,
+                SemanticTokenType::INTERFACE,
+                SemanticTokenType::ENUM,
+                SemanticTokenType::TYPE_PARAMETER,
+                SemanticTokenType::FUNCTION,
+                SemanticTokenType::MEMBER,
+                SemanticTokenType::PROPERTY,
+                SemanticTokenType::MACRO,
+                SemanticTokenType::VARIABLE,
+                SemanticTokenType::PARAMETER,
+                SemanticTokenType::LABEL,


num-derive?

matklad · 2020-02-22T12:03:21Z

Haven't looked too closely into this, but looks 👍 on the first glance!

kjeremy · 2020-02-22T12:27:14Z

It's still WIP. I haven't had a chance to do much with it this week.

…

On Sat, Feb 22, 2020, 7:03 AM Aleksey Kladov ***@***.***> wrote: Haven't looked too closely into this, but looks 👍 on the first glance! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3159>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBACRGWP4CI2GMMUGOFL23REEIAVANCNFSM4KV4SRIA> .

kjeremy · 2020-02-25T01:54:39Z

@matklad I've refactored a few things and I think the rust side is in good shape. However it's not useful from vscode yet. In order to get it to work you have to:

Use "enabledProposedApi": "true" in package.json
Run code or code-insiders with --enable-proposed-api matklad.rust-analyzer
On vscode stable you have to set editor.semanticHighlighting.enable

So I think I would like to back out the typescript changes and leave those as an exercise to anyone who would like to mess with tokens.

kjeremy · 2020-02-25T02:01:13Z

I've removed the client side changes.

kjeremy · 2020-02-25T02:02:48Z

This only supports whole file token support. Once accepted I can tackle edits

matklad · 2020-02-25T09:59:11Z

bors r+

Once accepted I can tackle edits

Edits are not important. They add quite a bit of complexity for questionable performance benefits. What is important to support are ranges for narrowing down the highlighted bit.

bors · 2020-02-25T10:09:09Z

Build succeeded

matklad · 2020-02-25T10:14:50Z

We also should remove our custom client-side hacks. It's ok if this regresses functionality for non-insiders builds.

Veetaha · 2020-02-25T10:23:41Z

Let's not rush with removing working hacks, I personally don't use code insiders...

matklad · 2020-02-25T10:26:17Z

Aggressively not accumulating technical debt is an explicit design guideline for rust-analyzer. We should actively migrate from hacks to insiders functionality. It's OK to regress the experience of users who don't use insiders.

bjorn3 · 2020-02-25T10:35:52Z

I disagree with both requiring the insiders version, which 99% of the people don't use, and having to manually pass an argument to vscode every time you want to start it. In this particular case the old implementation is kind of broken anyway, so it is not a very big problem, but in the general case I would prefer that users of the latest stable vscode release can keep using features.

matklad · 2020-02-25T10:38:53Z

In this particular case the old implementation is kind of broken anyway

Yeah, this is exactly my reasoning here: we replacing something completely ad hoc and broken with something on path to stabilization.

We should migrate from hacks, but we shouldn't change something that already works just because a new shiny thing just came out.

lnicola · 2020-02-25T10:49:47Z

crates/rust-analyzer/src/conv.rs

+impl Conv for &'static str {
+    type Output = (SemanticTokenType, Vec<SemanticTokenModifier>);
+
+    fn conv(self) -> (SemanticTokenType, Vec<SemanticTokenModifier>) {


We could do a more direct conversion here, avoiding the Vec allocation, right?

I think smallvec could be more appropriate here?

The problem is using &'static str type in the first place. It doesn't make sense to optimize impl details while the API itself is wrong. I think we should move to a model closer to tag+modifier of LSP.

matklad · 2020-02-25T10:50:41Z

While we are at it, this is why I think it's important to migrate to new APIs:

this immediately decrees overall maintenance burden. All the time when the new API is available but we use the old one is accumulated debt interest.
nightly APIs are in general higher quality than the hacky ones. Ones we migrated extend selection in a similar way, we immediately got the undo functionality working
probably most important one: we can't not report bugs with the new API unless we fully embrace it. And it's important to report bugs (especially design bugs) while the API is nightly.
these "I'll do it later" things tend to acquire mass and momentum with time. Getting rid of hacks now is much less effort than deciding to do so some time in the future.

kjeremy · 2020-02-25T12:25:05Z

@matklad I noticed last night that edits aren't used in the extensions so will focus on ranges. Thanks.

matklad · 2020-02-26T13:08:39Z

@kjeremy should I do anything else to test this besides code-insiders --enable-proposed-api matklad.rust-analyzer .? Doesn't work for me so far, probably I am doing something stupid like using an old version of extension or what not, but I can't find the problem immediately

kjeremy · 2020-02-26T13:13:38Z

@matklad

You have to also set "enableProposedApi": "true" in your package.json and in client.ts either:

call res.registerProposedFeatures() instead of registering CallHierarchyFeature
OR import and register SemanticTokensFeature

kjeremy · 2020-02-26T13:14:40Z

Note that range requests and possibly edits are not implemented yet in vscode: microsoft/vscode#86415 (comment)

matklad · 2020-02-26T13:17:17Z

Oh, right... @kjeremy do you have client-side implemenation around by any chance? If you have, could you send a PR?

kjeremy · 2020-02-26T13:29:08Z

You mean the client.ts changes I mentioned above? I can't do that because it will kill the client on stable and insiders without that command line switch... maybe we can catch and ignore the error somehow.

matklad · 2020-02-26T13:38:58Z

It's still useful to have at least a PR

kjeremy · 2020-02-26T13:43:24Z

Done! #3321

matklad · 2020-02-26T13:45:15Z

Also, I think there was a problem that requests are canceld with ContentModifed, but are not retried by the client.. Am i remmebering this correctly? Have we reported an upstream issue?

kjeremy · 2020-02-26T13:47:24Z

I have not reported that issue. I THINK I saw it yesterday but then ran into the panics in #3310 which may be the actual problem.

kjeremy commented Feb 15, 2020

View reviewed changes

Veetaha reviewed Feb 15, 2020

View reviewed changes

kjeremy force-pushed the semantic-tokens branch 3 times, most recently from a176cec to f49e942 Compare February 19, 2020 14:33

Veetaha mentioned this pull request Feb 24, 2020

PunkE01: In vscode, types can't be styled differently from their field name #3266

Closed

kjeremy force-pushed the semantic-tokens branch 2 times, most recently from 7fb8741 to 38ae123 Compare February 25, 2020 01:34

Teach the server about Semantic Tokens proposed LSP

9f0cfb7

kjeremy force-pushed the semantic-tokens branch from 38ae123 to 9f0cfb7 Compare February 25, 2020 02:00

kjeremy changed the title ~~WIP Early semantic tokens support~~ Server-side Semantic Tokens Feb 25, 2020

Add docs

17ffdf9

bors bot merged commit 1fe48a0 into rust-lang:master Feb 25, 2020

lnicola reviewed Feb 25, 2020

View reviewed changes

kjeremy deleted the semantic-tokens branch February 25, 2020 12:17

kjeremy mentioned this pull request Feb 26, 2020

Enable SemanticTokens on the client #3321

Merged

runbmp mentioned this pull request Mar 3, 2020

Semantic Highlighting #604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server-side Semantic Tokens #3159

Server-side Semantic Tokens #3159

kjeremy commented Feb 15, 2020

kjeremy Feb 15, 2020

kjeremy Feb 15, 2020

Veetaha Feb 15, 2020

kjeremy Feb 16, 2020

Veetaha Feb 15, 2020

matklad Feb 15, 2020

Veetaha Feb 15, 2020

Veetaha Feb 15, 2020

matklad commented Feb 22, 2020

kjeremy commented Feb 22, 2020 via email

kjeremy commented Feb 25, 2020

kjeremy commented Feb 25, 2020

kjeremy commented Feb 25, 2020

matklad commented Feb 25, 2020

bors bot commented Feb 25, 2020

matklad commented Feb 25, 2020

Veetaha commented Feb 25, 2020

matklad commented Feb 25, 2020

bjorn3 commented Feb 25, 2020

matklad commented Feb 25, 2020 •

edited

Loading

lnicola Feb 25, 2020

Veetaha Feb 25, 2020

matklad Feb 25, 2020

matklad commented Feb 25, 2020

kjeremy commented Feb 25, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

kjeremy commented Feb 26, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020 •

edited

Loading

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

		@@ -0,0 +1,1737 @@
		/*---------------------------------------------------------------------------------------------

Server-side Semantic Tokens #3159

Server-side Semantic Tokens #3159

Conversation

kjeremy commented Feb 15, 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

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 Feb 22, 2020

kjeremy commented Feb 22, 2020 via email

kjeremy commented Feb 25, 2020

kjeremy commented Feb 25, 2020

kjeremy commented Feb 25, 2020

matklad commented Feb 25, 2020

bors bot commented Feb 25, 2020

Build succeeded

matklad commented Feb 25, 2020

Veetaha commented Feb 25, 2020

matklad commented Feb 25, 2020

bjorn3 commented Feb 25, 2020

matklad commented Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Feb 25, 2020

kjeremy commented Feb 25, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

kjeremy commented Feb 26, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020 • edited Loading

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

matklad commented Feb 26, 2020

kjeremy commented Feb 26, 2020

matklad commented Feb 25, 2020 •

edited

Loading

kjeremy commented Feb 26, 2020 •

edited

Loading