lsp-rust: add semantic token modifiers

[alanz]

This is more of a proof-of-concept than anything else.

It works for the "mutable" modifier, adding an underline. This matches what the VsCode client does, but clashes with the lsp-mode default for read vs write highlighted variables. The rest have a no-operation face assigned, but need to be there to get the numbers right.

It is based on the approach in lsp-terraform.el. I tried setting it in the client registration as per lsp-clojure.el, but the final mapping of fonts to indexes does not work for some reason, as the call to lsp--semantic-tokens-initialize-workspace does not get the correct value from (lsp-semantic-tokens--modifier-faces-for client), despite them being set elsewhere. I suspect a timing or update problem.

I set (setq lsp-semantic-tokens-enable t) in (use-package lsp-mode in my personal init file to enable this.

[sebastiansturm]

I missed the fact that lsp-terraform-ls uses lsp-terraform-ls--set-tokens; that should probably be cleaned up in a separate PR; I can do that tomorrow or on the weekend. As for the present PR: there already is an option :semantic-tokens-faces-overrides for lsp-register-client that should allow you to define type and modifier faces on a per-client basis, cf. lsp-clojure.el. In your case, replacing :types by :modifiers and pasting in your list of face definitions should just work. Could you please try that and adapt your PR accordingly (unless of course you find some bug along the way that first needs fixing)? thanks!

[alanz]

there already is an option :semantic-tokens-faces-overrides for lsp-register-client

I tried this first, and could not get it to work. See my description in the initial PR comment (https://github.com/emacs-lsp/lsp-mode/pull/3668#issue-1333849483). I agree it would be the cleanest way to do it.

Note that lsp-clojure sets the fonts only, not modifiers.

[sebastiansturm]

oh, sorry! I'll have a look this evening; if the face overrides don't work out of the box, that should at least be easy enough to fix

[alanz]

if the face overrides don't work out of the box, that should at least be easy enough to fix

I think the problem is in lsp-semantic-tokens--replace-alist-values. It only replaces fonts for the set of modifiers already in lsp-semantic-token-modifier-faces, when called from lsp-semantic-tokens--modifier-faces-for. So perhaps those parameters should be swapped, or the ones in the new modifiers list should be added to the set.

[sebastiansturm]

good catch, thank you! Just wanted to start debugging and then I noticed you already found the culprit :) I opened a mini-pr but haven't tested it yet

[psibi]

I missed the fact that lsp-terraform-ls uses lsp-terraform-ls--set-tokens; that should probably be cleaned up in a separate PR; I can do that tomorrow or on the weekend. As for the present PR: there already is an option :semantic-tokens-faces-overrides for lsp-register-client that should allow you to define type and modifier faces on a per-client basis, cf. lsp-clojure.el.

@sebastiansturm I tried the approach for terraform today and for some reason it doesn't seem to be working. The language server is not getting initialized with the right values. I can test your PR with terraform ls server, if you get to it.

[sebastiansturm]

@psibi I changed my PR to take face overrides into account when sending out client capabilities; I'd appreciate it if you could test that against terraform ls (with appropriate face overrides used in lsp-register-client)

[psibi]

@sebastiansturm Thanks! I tested your PR against terraform and left my comment here: https://github.com/emacs-lsp/lsp-mode/pull/3680#issuecomment-1235749313

[alanz]

What still has to happen to get this PR to land?

[sebastiansturm]

I think it's fine, though I'd prefer it to use semantic token overrides, which in turn requires #3680 (and that one still requires an approving review). I'll ping the other thread

[alanz]

I think it's fine, though I'd prefer it to use semantic token overrides, which in turn requires https://github.com/emacs-lsp/lsp-mode/pull/3680 (and that one still requires an approving review). I'll ping the other thread

I updated this to use the overrides, hopefully we can now land it. Except I see some stray changes came in with the rebase. Will update.

[alanz]

I made a variable for each specific semantic modifier, since the set of modifiers is fixed for a given version of rust-analyzer, and has not changed yet.

Rather than having a fallback default font that does nothing, I made a face for each font, so you can use describe-char to understand what modifiers are active.

[sebastiansturm]

thanks, that's very thorough :). LGTM, though personally I'd probably still have used a defvar/defcustom instead of a defun for lsp-rust-analyzer--semantic-modifiers (or perhaps even a defconst if each of its entries is now separately customizable -- not sure). I'm fine with merging this as-is; @yyoncho / @ericdallo : do you have a strong opinion on the use of defun here?

[ericdallo]

Both are ok to me, but personally, I'd use a variable or const for better performance trading CPU for memory

[alanz]

From my side I would like to see it land, and then the exact custom process can be optimized. Probably not by me :)