search

emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.8k stars 892 forks source link

[semantic-tokens] tokens not entire applied on buffer #3154

Open ericdallo opened 3 years ago

ericdallo commented 3 years ago

Thank you for the bug report

Bug description

Sometimes when opening a buffer the semantic tokens are applied only for the current visible region (probably range semantic tokens) but if scrolling the buffer up or down, I don't see tokens for those other regions unless I M-x lsp again, even so other regions would not contain the tokens.

print: image You can see how just part of the buffer has the green tokens

I tested on vscode with clojure-lsp and it works fine, so it seems a issue on lsp-mode.

Steps to reproduce

You can clone https://github.com/clojure-lsp/clojure-lsp/ and open server.clj buffer, it's possible to see in other buffers as well.

Expected behavior

All tokens correctly applied for the entire buffer.

Which Language Server did you use?

clojure-lsp

OS

Linux

Error callstack

No response

Anything else?

@sebastiansturm could you help understand why that happens? I was thinking that was some issue on server but after implementing semantic tokens support on Calva(VScode extension to code clojure) here, everything worked perfectly there. I think this is the only issue I see with semantic tokens right now, but it's quite annoying as it happens a lot :/

sebastiansturm commented 3 years ago

thanks for the report; I've had a look and I think this is because clojure-mode often widens the region to be fontified and I'm not handling that correctly. Will prepare a PR to fix that; as a workaround, (setq lsp-semantic-tokens-allow-ranged-requests nil) should hopefully provide correct highlighting until a fix is pushed

ericdallo commented 3 years ago

Thank you for taking a look @sebastiansturm!

ericdallo commented 3 years ago

I'm also having issues with cider package that is the REPL package that almost every clojure user uses it, it seems cider apply a lot of font lock and I can't disable that accurately, but I think this is other issue :)

sebastiansturm commented 3 years ago

actually, I'm not sure how cleanly I'll be able to fix this -- within the font-lock response to a ranged request, I'll first apply the underlying (major-mode-provided) fontifier, which may widen the fontification region beyond the bounds of my ranged request and clear semantic highlights in that extended region. I'd now intersect the widened region with the region I got tokens for, limit fontification to that smaller region and defer full fontification to lsp--semantic-tokens-request-full-token-set-when-idle (which ordinarily wouldn't re-font-lock if the ranged request had already been processed, so my ugly solution to that would be to introduce another field to lsp--semantic-tokens-cache indicating whether a fontification request had been truncated). However, tokens within the narrow region would then be rendered twice, and if one were to scroll between the fully fontified (major mode + semhl) inner region (which should typically coincide with the bounds of the currently visible window) and other parts only highlighted by the major mode's own fontifier, one would see partially highlighted regions until the response to lsp--semantic-tokens-request-full-token-set-when-idle comes in.

That would still be better than the current situation, but in cases where we're building on major modes that aggressively widen fontification regions, disabling ranged responses might just be a better default (we might also provide a way to adjust our request ranges to the major-mode's widening logic, though I'm not sure if such a rather esoteric option would see much use -- might have to provide good defaults via lsp-clients then; and I'm not sure whether other font-lock mechanisms such as tree-sitter would further interfere). What do you think?

ericdallo commented 3 years ago

Good point, I'm not sure as well but I think we can test those options, I'm not familiar with the lsp-mode semantic tokens code but I'm with the server side code, even so LMK if you need any help with testing any of those options

sebastiansturm commented 3 years ago

had another look at clojure-mode and noticed it merely pushes a new element onto font-lock-extend-region-functions (of which I had been unaware); will try if applying the same to our request regions yields good results.

sebastiansturm commented 3 years ago

didn't seem like such a good fit either because font-lock requests chunk boundaries that don't coincide with the window boundaries I'm using for ranged requests. As a compromise, I'm now overshooting by several jit-lock-chunk-sizes in both directions, so the newly tokenized region is likely to include any extended region close to the current viewport, while still being potentially much smaller than the entire buffer. Though as we're later requesting the full set on idle anyway, I'm not sure if this actually is a useful optimization or just unneeded overhead. Anyway, I'll push the change to my repo at sebastiansturm/lsp-mode in a few minutes, would appreciate if you could try that out. Have tested it a few times on clojure buffers and one rust buffer, but haven't used it in anger yet

ericdallo commented 3 years ago

Great @sebastiansturm! Thanks and sure, I can test it!

ericdallo commented 3 years ago

@sebastiansturm it does look better than the current one :) I'll keep testing this week and I give you feedback as soon as I found any issues, thank you for the help on this

ericdallo commented 3 years ago

@sebastiansturm I didn't find any bugs until now with your branch, it does look fixed :) I still have issues with cider, but I'll try to debug further more and open another issue later with more details, I think we should merge your branch! thanks for the awesome work!

sebastiansturm commented 3 years ago

thanks a lot! Will have a look at the refresh issue so I can prepare a joint PR for both

ericdallo commented 3 years ago

Gentle ping @sebastiansturm :)

sebastiansturm commented 3 years ago

sorry for not following up sooner. My intention was to combine this with a fix to issue #3155 which during my cursory testing seemed to work, but I'd like to see that verified. Though on the other hand, no point in delaying the PR any further so I'll open one and outline what I think should best be re-evaluated by someone other than me

ericdallo commented 3 years ago

thanks @sebastiansturm I just pinged you because your branch seems to really improve the range tokens for me, feel free to wait if you think it makes sense, both works to me :)