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

Feature Request: Produce fewer semantic tokens if the client uses augmentsSyntaxTokens #12783

Open
frankplow opened this issue Jul 17, 2022 · 7 comments
Labels
A-highlighting (semantic) token highlighting C-feature Category: feature request E-medium

Comments

@frankplow
Copy link

rust-analyzer produces a lot of semantic tokens, including those for simple syntax features such as parentheses and semicolons. Most other LSP servers do not produce such a large set of tokens and so perform better than rust-analyzer for me when semantic tokens are enabled.

I believe the intended use of the augmentsSyntaxTokens client capability is to indicate to the server that these simple tokens which do not require semantic analysis will be highlighted independently by the client and do not need to be provided by the server.

@flodiebold flodiebold added E-medium C-feature Category: feature request A-highlighting (semantic) token highlighting labels Jul 17, 2022
@Veykril
Copy link
Member

Veykril commented Jul 17, 2022

That is one confusing capability, the documentation basically states nothing about what it means at all 😕

@frankplow
Copy link
Author

frankplow commented Jul 17, 2022

@Veykril Yes it's not very helpful really. The use case described in my OP is the only reason I can see the client needing to communicate this to the server, so I've made this presumption. There's the other issue of a very fuzzy line between what's better served by static grammar and what's better served by semantic analysis as well (strings jump to mind). I think the feature is still worth trying to implement though - the difference in performance compared to, for example, clangd is substantial (to the extent where I turn off semantic highlighting when editing Rust).

@Veykril
Copy link
Member

Veykril commented Jul 17, 2022

The question really is, what semantic tokens are relevant and what aren't for this. To me this capability sounds borderline useless, though I guess just not emitting semantic tokens for stupid punctuation would be a decent start. Instead of the capability this really sounds like the server should just offer configurations for disabling certain kinds of semantic tokens though (like we have a setting for disabling string highlighting).

Regarding performance, are you referring to the client not handling semantic tokens well if there are a lot of them?

@flodiebold
Copy link
Member

@Veykril I think it'd be fine to leave out purely syntax-based tokens, but err on the side of sending slightly too much. Having ways to disable additional token types would be helpful on top of that.

Regarding performance, in my testing our semantic highlighting was pretty unusable in Emacs for example, so I think this would be great to have.

@frankplow
Copy link
Author

frankplow commented Jul 17, 2022

@Veykril Yes by performance I mean the client struggling with that many tokens rather than rust-analyzer itself. There's a slight performance difference between the two in terms of the time taken to respond to the request as well, but this is insignificant compared to the time for the client to apply the tokens.

sounds like the server should just offer configurations for disabling certain kinds of semantic tokens

I've never quite understood how the tokenTypes in the client capabilities is meant to work to be honest. Most servers (rust-analyzer included I believe, though correct me if I'm wrong) just seem to discard this.

Not producing those punctuation tokens in the first place is just as good a solution to the problem for me.

bors added a commit that referenced this issue Aug 23, 2022
Add some more highlighting configurations

The following can be enabled/disabled now in terms of highlighting:
- doc comment injection (enabled by default)
- punctuation highlighting (disabled by default)
- operator highlighting (enabled by default)
- punctuation specialized highlighting (disabled by default)
- operator specialized highlighting (disabled by default)
- macro call bang highlighting (disabled by default)

This PR also changes our `attribute` semantic token type to the `decorator` type which landed upstream (but not yet in lsp-types).

Specialized highlighting is disabled by default, as all clients will have to ship something to map these back to the standard punctuation/operator token (we do this in VSCode via the inheritance mapping for example). This is a lot of maintenance work, and not something every client wants to do, pushing that need to use the user. As this is a rather niche use in the first place this will just be disabled by default.

Punctuation highlighting is disabled by default, punctuation is usually something that can be done by the native syntactic highlighting of the client, so there is no loss in quality. The main reason for this though is that punctuation adds a lot of extra token data that we sent over, a lot of clients struggle with applying this, so disabling this improves the UX for a lot of people. Note that we still highlight punctuations with special meaning as that special entity, (the never type `!` will still be tagged as a builtin type if it occurs as such)

Separate highlighting of the macro call bang `!` is disabled by default, as I think people actually didn't like that change that much, though at the same time I feel like not many people even noticed that change (I prefer it be separate, but that's not enough reason for it to be enabled by default I believe :^) )

cc #12783 #13066
@Veykril
Copy link
Member

Veykril commented Aug 23, 2022

So, I wonder what exactly we should interpret this flag as. I assume it really just means only sent tokens over that differ from their syntax (like usages of const parameters, which syntactically look like any other one segment path etc). It seems like an odd setting since the server has no idea what the client can differentiate on its own and what not honestly.

@frankplow
Copy link
Author

Here is the discussion leading to the addition of the capability.

Yeah it seems that it's intended to indicate that the client is doing some traditional static highlighting, and the server only needs to send tokens which require semantic analysis.

@Veykril Veykril self-assigned this May 10, 2023
bors added a commit that referenced this issue May 11, 2023
Add basic support for `augmentsSyntaxTokens` and non-standard semantic token config

cc #12783
Closes #13066
@Veykril Veykril removed their assignment May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-highlighting (semantic) token highlighting C-feature Category: feature request E-medium
Projects
None yet
Development

No branches or pull requests

3 participants