Permit semantic token overrides to extend base set

[sebastiansturm]

please don't merge -- I haven't tested this yet. As @alanz noted, the current implementation of lsp--client-semantic-tokens-faces-overrides doesn't allow client package authors to add new faces or modifiers to the base set; the present mini-PR should fix that (plan to try this out tomorrow, won't get to it today)

[yyoncho]

Something like that seems to be more natural:

(let* ((copy-base (copy-alist base)))
  (mapc (-lambda ((key . value)) (setf (alist-get key copy-base) value)) overrides)
  copy-base) 

[yyoncho]

How I tested it:

(let* ((base '((key1 . value1) (key2 . value2)))
       (overrides '((key1 . override1)
                    (new-key . override2)))
       (copy-base (copy-alist base)))
  (mapc (-lambda ((key . value)) (setf (alist-get key copy-base) value)) overrides)
  copy-base)

[sebastiansturm]

thanks!

[alanz]

Something to be aware of: the list of faces supplied for modifiers has a specific order, which is indexed by the bitmap when applying them.
So the client-supplied override sequence should be preserved when doing the override.
I actually think it should be a replace operation, given the complexity.

For example, here is the initialization reply capabilites from rust analyzer, which I have annotated with the bitmap positions

     "semanticTokensProvider": {
      "legend": {
        "tokenTypes": [
        .......
        ],
        "tokenModifiers": [
          "documentation",  0
          "declaration",    1
          "definition",     2
          "static",         3
          "abstract",       4
          "deprecated",     5
          "readonly",       6
          "defaultLibrary", 7
          "async",          8
          "attribute",      9
          "callable",       10
          "constant",       11
          "consuming",      12
          "controlFlow",    13
          "crateRoot",      14
          "injected",       15
          "intraDocLink",   16
          "library",        17
          "mutable",        18
          "public",         19
          "reference",      20
          "trait",          21
          "unsafe"          22
        ]
      },
      "range": true,
      "full": {
        "delta": true
      }
    },

[alanz]

In fact, the spec says that the SemanticTokensLegend defines how the modifiers will be used by the server. This is a vector of names corresponding to the bitmap order the server will use. So the client needs to arrange things so that those face names line up with the bitmap positions when they are applied. Which sounds like there should be another stage of processing to do this lining up, ready for use.

The simplest way to do this is to let the client registration specify the fonts in the order required, and not to change the order. Otherwise explicitly rearrange once the name -> face mapping is in place.

[sebastiansturm]

that ordering should be established by lsp-semantic-tokens-build-face-map, which is supposed to map the client-supplied list of recognized token types/modifier types to a vector (containing nil for unmapped types) that can then directly be indexed by numerical token/modifier values. But since you mention it, I guess you found that it doesn't work as intended? Haven't checked in a while, so I'd better have a look

[alanz]

lsp-semantic-tokens-build-face-map just uses the value in the client of semantic-tokens-faces-overrides, which needs to keep its order as I stated in #3680 (comment), and that will work if the client and server match each other exactly.

But if the server ever changes the modifiers it uses, it may be more robust to explicitly use the order provided by the server in its initialize response, which is the semanticTokenLegend, as I mentioned #3680 (comment)

[sebastiansturm]

yes, I believe that's what lsp-semantic-tokens-build-face-map does: within lsp--semantic-tokens-initialize-workspace, it calls lsp--semantic-tokens-as-defined-by-workspace, which in turn refers to the client-provided semanticTokenLegend. To each of these client-provided token/modifier types, a face (possibly nil) is then assigned using semantic-token[-modifier]-faces and semantic-tokens-faces-overrides

[psibi]

@sebastiansturm I like this PR and it kind of works for terraform client. But For terraform, I don't want to extend the base set - since it doesn't recognize some of them. I want only the one which I defnine and I need full control over it. For eg: there are no such tokens like this in terraform:

    ("dependent" . lsp-face-semhl-type)
    ("concept" . lsp-face-semhl-interface)

But because it's already defined in the variable lsp-semantic-token-faces, it passes it to the language server.

[sebastiansturm]

thanks for trying out the PR! I would expect any extra faces passed to the server to be ignored by said server, so they won't end up in the face vector and shouldn't cause any overhead. Do you see any practical downside to the current approach? If so, we could of course just remove all definitions explicitly overridden by (<identifier-string> . nil)

[psibi]

@sebastiansturm I think this should be fine, but I guess I would prefer as a client user - if I didn't have to pass any additional parameters to the language server if it's not required. But this definitely improves the current situation, so thank you for that! And I'm planning to patch the terraform client once this PR get's merged.

Also one thing worth mentioning is that the parameters that are passed to the language server doesn't seem to preserve the order. While this is not a problem for terraform language server, I'm not sure if it will cause issues for other language server.

Also, probably it's worth having two options:

• :semantic-tokens-faces-overrides
• :semantic-tokens-faces-extend

Where the fist option completely overrides the one defined in lsp-semantic-tokens.el and the second one just extends the one defined there (current behavior).

[sebastiansturm]

thanks for the hint, not sure if I'll have time this week but I'll check the spec to see if token/modifier order is somehow important. I slightly changed my PR so that default tokens/modifiers can now be completely replaced by your overrides, depending on two new (optional) elements :discard-default-types and :discard-default-modifiers of semantic-tokens-faces-overrides

[psibi]

but I'll check the spec to see if token/modifier order is somehow important.

I could be mis-understanding, but from reading @alanz's comment here - I think that ordering matters: #3680 (comment)

[alanz]

I think that ordering matters: #3680 (comment)

A later response by @sebastiansturm clarifies that it actually uses the order returned in the initialize response.

So the key thing is to make sure all the required mappings are in place.

[sebastiansturm]

I guess this could be merged, or are there any outstanding issues I'm currently not aware of? @alanz have you tried to use this in your pr #3668?

[alanz]

I have tested this, and it works, using

  :semantic-tokens-faces-overrides `(:discard-default-modifiers t
                                     :modifiers
                                     ,(lsp-rust-analyzer--semantic-modifiers))

in mak-lsp-client

[sebastiansturm]

thank you! I think this should be good to go; @yyoncho or @ericdallo do you agree?

[alanz]

Should there be a documentation update with it, or as well, related. It is not clear how to use this in practice.

[sebastiansturm]

true. I added a comment to semantic-tokens-faces-overrides, do you think this description will be helpful to new users?

[sebastiansturm]

I think this can be merged; or (@yyoncho @ericdallo) is there anything else you'd like me to change?

[ericdallo]

Sorry for the delay!