Tree sitter compatibility w/ semantic tokens

[theol0403]

I use semantic highlighting to improve the coloring of my c++ files. I was looking for better syntax hl for non-semantic identifiers and found tree-sitter.

However, when tree sitter is enabled, all semantic highlights disappear. Even for identifiers that don't have a tree sitter face assigned to them, the semhl face is still gone. It would be ideal if semhl would override tree-sitter.

Default font-lock:

Semhl:

Semhl + tree-sitter:

As you can see, the orange parameter semhl is removed. Running describe-char on the most-bottom "spline" identifier in the screenshot shows that there is no tree-sitter face being assinged to it, yet the semhl face is removed.

Semhl:

Semhl + tree-sitter:

Which Language Server did you use
Clangd 12

OS
Fedora 35 w/ doom-emacs w/ ptk-nativecomp from git tree.

Thanks!

[yyoncho]

cc @sebastiansturm @ubolonton

[sebastiansturm]

could you please try that with PR #2790? That should improve semhl compatibility with underlying font-lock mechanisms. Also, I guess it might matter in what order tree-sitter-hl-mode and lsp are enabled; during my personal tests, I typically activated tree-sitter first, lsp thereafter

[theol0403]

I pinned my lsp-mode package in doom to 0a298c4, rebuilt (and it did properly checkout the commit), and I did not notice any difference in behavior.

The order might matter, but I'm not sure how I can control that within doom. Ideally, it should not matter, of course.

[ericdallo]

Something I noticed (even in the PR branch) not related with tree-sitter is that if you write new text, there will be no face applied to the text until client receive the semantic tokens response, it'd be really nice if we keep using the emacs(maybe tree sitter) face and only after the response apply the face

[sebastiansturm]

strange, I just tried this both with C++ (clangd) and rust (rust-analyzer), with both tree-sitter and (the PR's) semantic fontification enabled. In both cases I'm seeing semantic highlights + tree-sitter highlights (tree-sitter where no semantic tokens are available, otherwise semhl wins).
For C++ I'm using a dumb barebones mode that essentially only sets up tree-sitter and lsp-mode, as I haven't been happy with cc-mode in the past. Could you perhaps first try with rust/rust-analyzer to check if layering in principle works the same way on your machine as it does on mine?

[sebastiansturm]

I'm also using master (+nativecomp), +pgtk so should be pretty close to your setup

[yyoncho]

I'm also using master (+nativecomp), +pgtk so should be pretty close to your setup

Offtopic: you are on the bleeding edge - how does pgtk feel?

[theol0403]

I won't be able to look further into this until a bit later, but it is worth mentioning that I'm toggling tree-sitter-hl-mode manually, it is not in a config. I feel that might be ensuring that tree-sitter is loaded last. Also, since the buffer remains unchanged, no new tokens are being sent from the server, which means no chance to regain control of the faces.

Indeed, I just confirmed that after enabling tree-sitter, if I restart the lsp server, it properly redraws the semantic hl. This is still using the PR so I can't confirm if it works without it.

I'm not sure what the solution to this problem looks like - maybe having a hook to redraw the semhl whenever font lock changes drastically.

[sebastiansturm]

I'm also using master (+nativecomp), +pgtk so should be pretty close to your setup

Offtopic: you are on the bleeding edge - how does pgtk feel?

I'm afraid I can't really judge pgtk's merits because I'm not using Wayland, and I didn't have any UI-toolkit-related performance issues with non-pgtk to start with. That being said, I haven't noticed any way in which it's worse than non-pgtk, nor did I encounter any instabilities, so I just keep using it for the time being. Maybe if there's some reliable childframe-related benchmark (something with lsp signatures perhaps?) I could run that on both branches and see if there is a difference

[yyoncho]

Is this issue still present?

[sebastiansturm]

I believe this should be fixed; @theol0403 do you agree?

[theol0403]

Haven't used emacs for a bit - I'll try to check later tonight 👍

[theol0403]

Unfortunately, this issue still occurs:

Steps:

• open file with lsp semhl
  
• toggle tree-sitter-hl-mode
  
  The semhl is gone.

Same system as before, with unpinned lsp package in doom emacs.

This issue only happens when tree-sitter is enabled after the lsp is already running.

[sebastiansturm]

I'm sorry, I must have made a mistake then when I retried yesterday. Will have another look tomorrow and make sure to get lsp/tree-sitter initialization order "right" (as in, "fault-triggering"). Sorry again for wasting your time @theol0403

[sebastiansturm]

I've had another look today and as @theol0403 says, tree-sitter-hl will override lsp-mode's semantic highlighting if enabled after lsp-mode (for good reason, as tree-sitter-hl covers all the highlighting provided by typical font-lock implementations).
Not sure how to best handle this apart from "well, don't do that". One possibility would be to have lsp-semantic-tokens.el attach to tree-sitter-hl-mode-hook and reactivate itself in the correct order if tree-sitter-hl-mode is enabled after lsp:

(with-eval-after-load 'tree-sitter-hl
  (add-hook
   'tree-sitter-hl-mode-hook
   (lambda ()
     (when (and lsp-mode lsp-semantic-tokens-enable (boundp 'tree-sitter-hl-mode) tree-sitter-hl-mode)
       (lsp-warn "It seems you have configured tree-sitter-hl to activate after lsp-mode.
To prevent tree-sitter-hl from overriding lsp-mode's semantic token highlighting, lsp-mode
will now disable both itself and tree-sitter-hl mode and subsequently re-enable both, starting
with tree-sitter-hl-mode. Please adapt your config to prevent unnecessary mode reinitialization
in the future.")
       (lsp-mode -1)
       (tree-sitter-hl-mode -1)
       (tree-sitter-hl-mode t)
       (lsp)))))

This appears to work AFAICT (@theol0403: would be nice if you could check if that works for you, too), but personally I hate it. With both lsp and doom-emacs trying so hard to avoid unnecessary overhead, I wouldn't really like to be disabling and re-enabling heavyweight modes such as lsp just to get around a relatively minor configuration issue. Though my hope is that the in-your-face warning message would at least guide users toward the preferred activation order, while also providing them with a working setup out of the box in case they can't be bothered to work on their Emacs config the very moment they run into this issue.

With that rationalization, I might be able to convince myself that this is a (borderline) acceptable solution; @yyoncho @ericdallo what is your opinion? Also, if we decide to do this, is this the best way to cleanly disable and re-enable lsp-mode?

[ericdallo]

Do we need to disable and enable again the whole lsp-mode? can't we do that only for lsp-semantic-tokens-mode?

Regarding the warning message, IMO it makes sense, users will only fix their config or what they want when seeing warnings like that if that really means there is something that should be fixed/improved on user-config.

[sebastiansturm]

yes, after posting that it dawned on me that calling lsp--semantic-tokens-teardown and reenabling the tokens setup should suffice, but you were faster :) will check if that works as intended

[theol0403]

Putting the code snippet shown above in my config works as expected. Thanks!

[sebastiansturm]

@theol0403 could you also try this somewhat less heavyweight version? Thanks!

(with-eval-after-load 'tree-sitter-hl
  (add-hook
   'tree-sitter-hl-mode-hook
   (lambda ()
     (when (and lsp-mode lsp--semantic-tokens-teardown
                (boundp 'tree-sitter-hl-mode) tree-sitter-hl-mode)
       (lsp-warn "It seems you have configured tree-sitter-hl to activate after lsp-mode.
To prevent tree-sitter-hl from overriding lsp-mode's semantic token highlighting, lsp-mode
will now disable both semantic highlighting and tree-sitter-hl mode and subsequently re-enable both,
starting with tree-sitter-hl-mode.

Please adapt your config to prevent unnecessary mode reinitialization in the future.")
       (funcall lsp--semantic-tokens-teardown)
       (setq lsp--semantic-tokens-teardown nil)
       (tree-sitter-hl-mode -1)
       (tree-sitter-hl-mode t)
       (lsp--semantic-tokens-initialize-buffer)))))

[theol0403]

Also seems to work well 👍

[ericdallo]

Nice, since this should only enable/disable semantic tokens mode, I'm ok with that :)