[semantic-tokens] Refresh tokens after apply workspaceEdit

ericdallo commented 3 years ago

Thank you for the bug report

[X] I am using the latest version of lsp-mode related packages.
[X] I checked FAQ and Troubleshooting sections
[X] You may also try reproduce the issue using clean environment using the following command
```
emacs -q -l lsp-start-plain.el
```
where lsp-start-plain.el can be downloaded here.

Bug description

We should refresh the semantic tokens of all affecter buffers by a workspaceEdit change sent from server.

Steps to reproduce

call LSP rename on some symbol that is used on multiple already opened buffers, after renaming the symbol on other buffers will have outdated semantic tokens.

Expected behavior

refresh the tokens after applying the workspaceEdit

Which Language Server did you use?

clojure-lsp and dart

OS

Linux

Error callstack

No response

Anything else?

c/c @sebastiansturm

ericdallo commented 3 years ago

It seems we are sending the request semantic token before we send didChange, causing these outdated tokens c/c @sebastiansturm

sebastiansturm commented 3 years ago

I do get stale tokens when doing this, though in my workspace log the didChange request precedes the corresponding token requests. Could you send me your workspace log (and maybe tell me what exactly you're renaming, I'm using the clojure-lsp repo as a test environment which I guess is what you're using too). Will have another look tomorrow

ericdallo commented 3 years ago

Yes, that happens with any clojure project, what I do to repro easily is:

open handlers.clj from clojure-lsp project in a buffer on left splitted window
open feature/semantic_tokens.clj in the right window
rename range-tokens to range-tokens-foo on feature/semantic_tokens.clj buffer
see outdated symbol f.semantic-tokens/range-tokens-foo on left buffer

@sebastiansturm the same happens with lsp-dart if you want to give a try as well, which makes me think that's probably a bud on lsp-mode, right?

sebastiansturm commented 3 years ago

hmm. I tried this again several times and never saw token requests precede their corresponding didChange notifications. Though I did see that only the buffer was being refreshed that I had currently been visiting, which I think is because lsp's on-change-hook only fires within the current buffer. One way to avoid this specific problem would be to call (lsp--semantic-tokens-request nil t) within lsp-after-apply-edits-hook, though if the user happens to rename a symbol within a hundred opened buffers, simultaneously emitting and processing that many token requests might cause some pretty annoying lag. Alternatively, I could use the lsp--semantic-tokens-request-full-token-set-when-idle mechanism by having it hold a list of to-be-fontified buffers, preferable starting with (current-buffer), followed by currently visible other buffers, and invisible buffers coming in last.

Or alternatively I could try to check tokens against the current document version on some window change/buffer focus hook, though that would sometimes cause avoidable flickering and I'm not too fond of using global hooks as teardown mistakes might easily go unnoticed and leave redundant no-op functions polluting often-invoked hooks. So I think I'd slightly prefer the fontification-on-idle list, even if this change would only fix rehighlighting after lsp-induced text editing operations, not after other operations that might possibly modify the contents of background buffers. Have you run into other instances of stale semantic highlights that were not related to lsp text edits?

ericdallo commented 3 years ago

I really tried fixing that before opening this issue listening to the lsp-after-apply-edits-hook and refreshing the tokens and didn't work 😅 not sure I did something wrong though.

Have you run into other instances of stale semantic highlights that were not related to lsp text edits?

I think with cider, but almost sure it's a corner case since cider has it's own fortification logic as well, besides that, there is also the case when user change the git branch of the project, but maybe server should handle didChangeWatchedFiles and update the token? (clojure-lsp ATM doesn't handle didChangeWatchedFiles for change operations, it's something on the backlog)

sebastiansturm commented 3 years ago

do those cases result in lsp--cur-version being increased? If so, maybe it'd be better to have lsp-mode directly enqueue refresh requests on lsp-on-change if semantic tokens are bound and enabled. Would make for somewhat tighter coupling, but doesn't seem entirely unreasonable to me

ericdallo commented 3 years ago

yeah, makes sense to me I think

sebastiansturm commented 3 years ago

I pushed a change to my fork that refreshes tokens on lsp-after-change, though I'm still not sure I'm too fond of this change, will try to make up my mind tomorrow. Haven't tested this extensively but at least the clojure-lsp example seems to refontify correctly

ericdallo commented 3 years ago

yes, I used this during the week and it works perfectly unless I jack in cider which I still have no idea how to fix that :/

emacs-lsp / lsp-mode