lsp-rust: add semantic token modifiers #3668

alanz · 2022-08-09T22:27:32Z

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.

yyoncho · 2022-08-16T06:03:12Z

clients/lsp-rust.el

+(defvar font-lock-documentation-modifier-face 'font-lock-nop-face
+  "Face name to use for documentation items.")
+
+(defvar font-lock-declaration-modifier-face 'font-lock-nop-face


does it make sense to move these in lsp-semantic-tokens? Also, can you at least prefix them with lsp- and if these are rust specific you can prefix with lsp-rust

I am prefixing them with lsp-rust-analyzer. I see there are already other faces defined in lsp-rust.el, such as lsp-rust-analyzer-inlay-face. Perhaps they should stay here, to keep all the rust config in one place.

clients/lsp-rust.el

sebastiansturm · 2022-08-16T22:02:31Z

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 · 2022-08-16T22:07:29Z

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 (#3668 (comment)). I agree it would be the cleanest way to do it.

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

sebastiansturm · 2022-08-17T09:04:14Z

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 · 2022-08-17T17:39:11Z

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 · 2022-08-17T21:45:42Z

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 · 2022-08-19T15:42:40Z

clients/lsp-rust.el

@@ -945,7 +946,9 @@ or JSON objects in `rust-project.json` format."
  :library-folders-fn (lambda (_workspace) lsp-rust-analyzer-library-directories)
  :after-open-fn (lambda ()
                   (when lsp-rust-analyzer-server-display-inlay-hints
-                     (lsp-rust-analyzer-inlay-hints-mode)))
+                     (lsp-rust-analyzer-inlay-hints-mode))
+                   (when lsp-semantic-tokens-enable


Does this work for you ?

I tried this approach in terraform mode and I could see that the terraform language server was not getting initialized with the proper values.

It does work for me for lsp-rust and rust-analyzer using my branch https://github.com/alanz/lsp-mode/tree/semantic-tokens-plus-optimise-modifiers which is a combination of #3668 and #3670

I tried again and these are my observations:

The buffer local variable is actually set properly.

The terraform language server is not initialized with the right values. I believe the after-open-fn is too late in the lifecycle process to change the initialization values.

It could be that your other changes in your branch may cause this to work. I will wait till your changes are merged, before giving it an attempt again.

I think a more proper fix for this would be to allow proper initialization in the make-lsp-client function itself.

It might relate to the initial capability negotiation. The rust language server ignores the capabilities the client sends, and always returns a fixed modifiers legend (fixed for the given version of the language server).

Yeah, that's probably it.

psibi

Minor comment.

psibi · 2022-08-19T17:38:16Z

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 · 2022-08-28T16:56:39Z

@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 · 2022-09-02T17:31:29Z

@sebastiansturm Thanks! I tested your PR against terraform and left my comment here: #3680 (comment)

alanz · 2022-10-03T18:42:43Z

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

sebastiansturm · 2022-10-03T20:12:09Z

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 · 2022-10-06T21:45:07Z

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

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.

sebastiansturm · 2022-10-07T19:53:54Z

clients/lsp-rust.el

@@ -926,6 +927,37 @@ or JSON objects in `rust-project.json` format."
 (lsp-defun lsp-rust--analyzer-debug-lens ((&Command :arguments? [args]))
  (lsp-rust-analyzer-debug args))

+(defun lsp-rust-analyzer--semantic-modifiers ()


first, thanks a lot for your patience! Sorry the PR has been blocked so long. I'm fine with your PR as-is, though I wonder if there's a reason this is a defun as opposed to a variable, defcustom or something? Not trying to be annoying here, honest question :)

The simple answer is I repurposed the prior version, and changed the function name to plug it into a new place. I will take a look when I have time to make it something more idiomatic.

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 "reference" modifier has the bold attribute set. The rest have a no-operation face assigned, but need to be there to get the font caluclation numbers right, and to be customized by users.

alanz · 2022-10-09T19:54:45Z

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 · 2022-10-10T18:55:16Z

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 · 2022-10-10T19:11:03Z

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

alanz · 2022-10-10T20:01:41Z

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

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 "reference" modifier has the bold attribute set. The rest have a no-operation face assigned, but need to be there to get the font caluclation numbers right, and to be customized by users.

github-actions bot added client One or more of lsp-mode language clients rust labels Aug 9, 2022

alanz force-pushed the semantic-tokens branch from aa5811e to d85d496 Compare August 10, 2022 18:03

github-actions bot added the documentation label Aug 10, 2022

alanz mentioned this pull request Aug 15, 2022

semantic-tokens: cache modifier lookups #3670

Merged

yyoncho reviewed Aug 16, 2022

View reviewed changes

alanz force-pushed the semantic-tokens branch from 7726e6e to ea62e68 Compare August 16, 2022 20:22

psibi reviewed Aug 19, 2022

View reviewed changes

sebastiansturm mentioned this pull request Sep 18, 2022

Permit semantic token overrides to extend base set #3680

Merged

alanz force-pushed the semantic-tokens branch from ea62e68 to eeafcdb Compare October 6, 2022 21:43

github-actions bot added the semantic-tokens label Oct 6, 2022

alanz force-pushed the semantic-tokens branch from eeafcdb to ece4cbc Compare October 6, 2022 21:56

github-actions bot removed the semantic-tokens label Oct 6, 2022

sebastiansturm reviewed Oct 7, 2022

View reviewed changes

alanz force-pushed the semantic-tokens branch from ece4cbc to e67818b Compare October 9, 2022 19:51

ericdallo approved these changes Oct 10, 2022

View reviewed changes

ericdallo merged commit 759fb79 into emacs-lsp:master Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lsp-rust: add semantic token modifiers #3668

lsp-rust: add semantic token modifiers #3668

alanz commented Aug 9, 2022

yyoncho Aug 16, 2022 •

edited

Loading

alanz Aug 16, 2022

sebastiansturm commented Aug 16, 2022

alanz commented Aug 16, 2022

sebastiansturm commented Aug 17, 2022

alanz commented Aug 17, 2022

sebastiansturm commented Aug 17, 2022

psibi Aug 19, 2022

alanz Aug 19, 2022

psibi Aug 20, 2022

alanz Aug 20, 2022

psibi Aug 24, 2022

psibi left a comment

psibi commented Aug 19, 2022

sebastiansturm commented Aug 28, 2022

psibi commented Sep 2, 2022

alanz commented Oct 3, 2022

sebastiansturm commented Oct 3, 2022

alanz commented Oct 6, 2022 •

edited

Loading

sebastiansturm Oct 7, 2022

alanz Oct 7, 2022

alanz commented Oct 9, 2022

sebastiansturm commented Oct 10, 2022

ericdallo commented Oct 10, 2022

alanz commented Oct 10, 2022

lsp-rust: add semantic token modifiers #3668

lsp-rust: add semantic token modifiers #3668

Conversation

alanz commented Aug 9, 2022

yyoncho Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastiansturm commented Aug 16, 2022

alanz commented Aug 16, 2022

sebastiansturm commented Aug 17, 2022

alanz commented Aug 17, 2022

sebastiansturm commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psibi left a comment

Choose a reason for hiding this comment

psibi commented Aug 19, 2022

sebastiansturm commented Aug 28, 2022

psibi commented Sep 2, 2022

alanz commented Oct 3, 2022

sebastiansturm commented Oct 3, 2022

alanz commented Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanz commented Oct 9, 2022

sebastiansturm commented Oct 10, 2022

ericdallo commented Oct 10, 2022

alanz commented Oct 10, 2022

yyoncho Aug 16, 2022 •

edited

Loading

alanz commented Oct 6, 2022 •

edited

Loading