[3.16] Support semanticTokens missing methods

[ericdallo]

SPEC
lsp-mode is missing the implementation of:

• ↩️ textDocument/semanticTokens/full/delta witch should improve performance bringing only the tokens relative to existing ones
• ⬅️ workspace/semanticTokens/refresh not critical, which allows server request client to refresh the tokens at any time.

[ericdallo]

Hey @sebastiansturm willing to take a took? specially the delta request?

I implemented on clojure-lsp both range and full semantic-tokens, it's missing only the delta for full support :D

[sebastiansturm]

yep, will have a look!

[sebastiansturm]

if anyone would like to test this, I have a preliminary implementation at sebastiansturm/lsp-mode, but I haven't opened a PR yet as I'll need to test it with lsp-use-plists enabled, and with a language server that sends out refresh requests. Plan to do that later this week

[ericdallo]

cool @sebastiansturm I can test it, I implemented clojure lsp server support for semantic tokens recently, but no refresh yet, but I can do if it's needed

[ericdallo]

I noticed some bugs on lsp-mode regarding delaying the tokens updates and for a small amount of time the tokens just got white before apply the tokens instead of use the major mode faces, I hope this branch fixes it too!

[sebastiansturm]

I'm not a clojure developer but I'll gladly use the clojure server for testing if you'll implement support, otherwise I'll try with the lua language server (as far as I understand, someone recently had issues with lua lsp sending out refresh requests, so apparently the support is there).
As for the delay, I haven't noticed that even with the current implementation, except when tree-sitter-hl mode was enabled; that's something I'll have to take a look at. Did you use tree-sitter-hl when you noticed that issue?

[ericdallo]

I'm not sure when should server should send refresh notification, do you have an example?
Never heard of that tree-sitter-hl 😆

[ericdallo]

Hey @sebastiansturm, good news, my issue with some tokens not being updates were gone with your branch :D
I'll keep testing it with clojure and let you know if any bugs

[ericdallo]

Actually, still happens not so small files, check the gif:

Here, clojure-lsp return the purple color as a macro token for deftest, testing and is for example, it seems that the first request works, but if I scroll or go back it has a lot of tokens not purple...
Also if I edit the file, lsp-mode request new tokens and sometime works:

[sebastiansturm]

thanks for the bug report! I haven't seen this kind of behavior myself (unless, as mentioned, tree-sitter-hl was enabled), but I'll try to install clojure-lsp this evening and reproduce the issue myself. If that doesn't work out, I'll have a look this weekend at what happens with tree-sitter enabled, maybe for some reason the same underlying issue occurs with your configuration

[sebastiansturm]

better take this with a large grain of salt for now, but I guess the flickering might be caused by the underlying fontification layer extending its fontification region, in which case previously inserted token faces might get cleared, but wouldn't get reinserted because lsp-semantic-tokens only considers the originally requested fontification region. I've pushed an experimental commit to my repository that should fix this (if this really is the underlying issue; but it least it did eliminate flickering during my own 5 minutes of testing)

[ericdallo]

Ow, I'll update it here and keep testing, thanks!

[ericdallo]

@sebastiansturm the merged branch looks better than it was on master :)

But I found 2 issues:

• If one copy/paste a piece of code or for some reason inserts a piece of code (via code actions, for example), emacs first shows without any faces and then color with the semantic tokens as you can see In the gif. It'd be nice to first use the default major mode face and only then change to the semantic-token face, WDYT?
• You can notice after the paste and after apply semantic tokens, that for some reason some tokens were placed wrong like (mapv, only after editing the file the tokens were corrected.

[sebastiansturm]

as for newly inserted text remaining unfontified at first: I'll have to try this out, though it makes sense it would behave that way; if parts of the buffer had previously been fontified already, semantic highlighting will be suspended until new data arrives from the language server. On the other hand, applying default fontification unconditionally (as we now do prior to language server initialization, thanks to the patch submitted by @Kha) might clear previous semantic-highlighting tokens within the region that is to receive font locking, so this might cause lots of flickering during regular typing.
If text insertion causes the jit-lock mechanism to request highlighting only for the newly inserted region, I guess I could look for face properties created by semantic highlighting within that region and use that as a heuristic to decide whether to fall back to default font locking. Sounds awkward and comes at some extra performance cost, but the latter should be minimal (we'd early-exit most of the time anyway), and despite being ugly it might work reasonably well. Or maybe there's another, cleaner way to do this?

as for the fragments of incorrect highlighting: have to check; are you using ranged or delta requests in that example? Maybe I messed up the implementation, will have a look (won't get to it today but hopefully on one of the next few evenings). Thanks a lot!

[ericdallo]

yeah, that looks hard to implement but I have no idea on how to implement that so feel free to test it if it works 😅

are you using ranged or delta requests in that example?

For now, only full and range as is the supported for clojure-lsp, but I'll implement delta soon.
Thanks for the help on this, I really would like to make semantic tokens work on lsp-mode.

Also, I know Dart lsp server is already using semantic tokens, so there is another server to test things: https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/tool/lsp_spec/README.md

[ericdallo]

@sebastiansturm I also noticed the faces doesn't match with current clojure major mode, do you know some way to improve the default faces for that without need to customize manually that?

like macro face:
Default

Semantic-tokens:

keywords, and others the same, all differ from elisp faces for example. It'd be nice to use the same from the major modes to avoid those changing for users before and after LSP, WDYT?

Any ideas @yyoncho ?

[ericdallo]

@sebastiansturm I noticed lsp-mode is not updating the tokens after moving parens via paredit or something like that:

Before:

After barfing:

I know lsp already advice some function and call rangeFormatting, we probably should update the tokens at the same time as well

[yyoncho]

To me, this looks like a bug in the server or in the client.

[sebastiansturm]

@sebastiansturm I also noticed the faces doesn't match with current clojure major mode, do you know some way to improve the default faces for that without need to customize manually that?

like macro face:
Default

Semantic-tokens:

keywords, and others the same, all differ from elisp faces for example. It'd be nice to use the same from the major modes to avoid those changing for users before and after LSP, WDYT?

Any ideas @yyoncho ?

I tried to base semhl faces on reasonable defaults (that are used by font-lock elsewhere, so should be properly themed) though I probably could have tried harder in some cases. No matter how good our defaults are though, you'd probably still run into edge cases here and there. One way to at least simplify major-mode-specific setups could be to provide an interactive function that iterates over the current token set, compares those tokens' semhl faces to the faces selected by the underlying font-lock mechanism and based on that proposes a custom setting for lsp-semantic-token-faces.
How well that works in practice should depend on whether your major-mode's distinct faces map to distinct token types, will have to check

[sebastiansturm]

@sebastiansturm I noticed lsp-mode is not updating the tokens after moving parens via paredit or something like that:

Before:

After barfing:

I know lsp already advice some function and call rangeFormatting, we probably should update the tokens at the same time as well

I pushed a commit with some debugging helper methods that capture buffer snapshots before and after every fontification run, and export those snapshots using htmlize. Haven't seen visual artefacts using rust-mode, will try with the clojure language server and lispyville (which I assume is close enough to paredit to reproduce the issue, let's see)

[sebastiansturm]

I'm just revisiting my semantic-tokens branch and for some reason lsp--semantic-tokens-cache refuses to update, which I think is because I'm using lsp-put as if it modified its argument in place (testing with lsp-use-plists set to t, not sure if I had used that setting the last time). Since lsp-put resolves to plist-put in those cases, whose docstring recommends to "(setq x (plist-put x prop val)) to be sure to use the new value", I figure that is what I should be doing.
OTOH, it seems to me that all current uses of lsp-put save for one within lsp-completion.el rely on lsp-put modifying its argument in place; can anyone shed some light on whether these are bugs in the making or whether (more likely) I'm misunderstanding something here? Also, I'm using native-compiling Emacs with a somewhat risky comp-speed of 3; might perhaps play a role here

[ericdallo]

c/c @yyoncho

[sebastiansturm]

I had a look at the fontification issues using both the clojure-lsp language server and its source code repository. When opening src/clojure_lsp/shared.clj and inserting one empty line after L125 (the (cond within debounce-by, I think I'm getting a ranged token response that still contains tokens on the now-empty line 126. Since semantic-tokens-fontify doesn't check whether returned tokens overstep their containing line's bounds, those tokens push into the next line (or at least I guess that is what happens, stopped looking too closely into the matter once I saw the presumed-faulty part of the response). @ericdallo could you please have a look at the annotated workspace log (grep for ^->) and the annotated range response (221), in case I misread something here?

[ericdallo]

@sebastiansturm I confirmed the issue, and it seems really a issue on clojure-lsp, the issue is that clojure-lsp handle the didChange requests async, so when it receives the semantingTokens request, its db doesn't contain the actual text yet 😔
Not sure how to solve that easily though

[ericdallo]

@sebastiansturm I did a quick fix for this issue for now (You can test if run a make on clojure-lsp to generate the new executable), and I could confirm the issue is gone including this one and this :)
Thank you for the help, I'll keep testing that branch!

[ericdallo]

Also I created this #2826 which solves the different faces issue :)

[yyoncho]

lsp--semantic-tokens-cache

lsp-put is more of a workaround and it is supposed to be avoided as much as possible. plist-put returns a new list when it is called with nil object. We might fix that limitation by making lsp-get work with setf.

[sebastiansturm]

@sebastiansturm I did a quick fix for this issue for now (You can test if run a make on clojure-lsp to generate the new executable), and I could confirm the issue is gone including this one and this :)
Thank you for the help, I'll keep testing that branch!

great, happy to hear that! Will have a look now at your face overrides PR, thanks!

[sebastiansturm]

lsp--semantic-tokens-cache

lsp-put is more of a workaround and it is supposed to be avoided as much as possible. plist-put returns a new list when it is called with nil object. We might fix that limitation by making lsp-get work with setf.

thanks for clearing that up. My current use of lsp-put doesn't make much sense anyway, will get rid of it

[sebastiansturm]

@ericdallo your changes look good to me, haven't noticed any issues. I pushed an interactive function named lsp-semantic-tokens-suggest-overrides to my fork that autogenerates token type overrides in a compatible format, though modifiers aren't currently taken into consideration

[ericdallo]

Nice, I think both will work nice. I'll keep using both during the week, but semantic tokens look way better than before :)

[FelipeLema]

are tokens for "current buffer" saved somewhere / somehow? can they be available so external packages can use that info for other purposes?

on that matter, when should a package use such information? should something like lsp-semantic-tokens-received-hook be added?

[yyoncho]

@FelipeLema we can arrange saving them and making them public.

on that matter, when should a package use such information?

One can implement a method - go to the next token with the same type(e. g. next variable declaration).

[FelipeLema]

I can write such "saving them and making them public"

I'm interested in doing so because I'm working on implementing semanticTokens in ccls and it was pointed out that I'm leaving out a tightly-coupled feature of ccls with Emacs.

I thought that a) other packages could benefit from having the current tokens b) decoupling rainbow semantic highlight from ccls-the-server and ccls-the-package is a good idea.

[yyoncho]

I can write such "saving them and making them public"

ok, just coordinate your effort with @sebastiansturm

I don't understand 100% what will be the use case of decoupling of the rainbow semantic highlight from ccls if only ccls can provide that data.

[sebastiansturm]

I have a pending PR on semantic tokens that is a bit out of date by now and still produces some CI warnings; think it's best I'll rebase and clean up that PR on the weekend, so it can be used as a basis for @FelipeLema's potential extensions. @FelipeLema: does the ccls extension "abuse" the standard semanticTokens protocol in some way (say, by using otherwise undefined token ids), or does it transmit that extra info out of band?

[ericdallo]

Yeah, @sebastiansturm last time I checked your PR was OK, I think if you update it, I can re-test it with Dart and Clojure and we can merge it as it improves semantic tokens, but I can't test the delta tokens yet :/

[FelipeLema]

@sebastiansturm It does not: the extension leverages the token information to present extra meta-information to the user using fontification.

I've never used it, but as far as I understand, it "rainbows elements" (like rainbow-parens) within a certain group. The code can be found here

Maybe I'm missing some technical detail, but that's the core of the functionality there. The whole handling of the token information was done before LSP 3.16 so there is some intersection, although I think it should be decoupled from ccls (maybe into something like lsp-semantic-token-rainbows.el