fix(colorscheme): link LSP semantic token groups to treesitter groups

[jdrouhard]

There has been a lot of confusion around the new LSP semantic token highlighting. There has been at least one github issue¹, many reddit posts²³⁴⁵, and the neovim chat questions with people who wonder why their syntax highlighting changes after a second or so with 0.9. Virtually all of the recommended "fixes" for these questions have been "just disable semantic tokens" without an explanation or reasoning as to why. It would be a shame for people to disable the feature entirely simply because their colorscheme didn't support LSP tokens when they first upgrade.

A large part of this confusion is that the default links bypass the @ groups that many themes have catered for use with treesitter highlighting. When LSP's semantic tokens kick in, they apply a higher priority highlight that reverts back to basic (less expressive) groups, which most modern themes don't style the same. I can't say this is 100% the reason, but it definitely contributes to many people's desire to turn the feature off ("treesitter highlighting is better, it just gets worse after a second or so!" is a common theme).

I understand that the LSP semantic token highlighting and treesitter highlighting are orthogonal, but realistically, they are both there to do one thing--provide a good experience for syntax highlighting in our favorite editor. There's nothing about the @type groups that an internal implementation detail couldn't make use of. I agree that from a documentation and recommendation perspective, we shouldn't mix them. They are different. But for the built-in defaults (which are non-exposed implementation details), we can do pretty much whatever we want.

That being said, the default links built in to the editor should provide the best out-of-the-box experience. Syntax highlighting (as a general "feature"--this includes all forms: syntax, treesitter, and LSP semantic tokens) priority is as follows: Syntax < treesitter < LSP semantic tokens. It makes sense that the link hierarchy should be the reverse: LSP -> treesitter -> syntax. This lets themes opt-in at any level to provide better/more fine tuned highlighting. The bonus is that many themes already have fine tuned treesitter groups, so if LSP semantic tokens kick in, it won't suddenly override all the colors everywhere, which has been a common complaint/source of confusion. For themes that don't color the @ groups at all, this PR won't change much at all since the links will fall through to the base groups.

@swarn @mfussenegger @gpanders @clason @lewis6991

As an aside, for the themes that have added support for semantic tokens, they basically just add these links back to the @ groups. I know tokyonight⁶, catppuccin⁷ and kanagawa⁸ have all needed to add some of these links back (with varying degrees of additional modifier groups they've tweaked).

Footnotes

1. https://github.com/neovim/neovim/issues/22956 ↩

2. https://www.reddit.com/r/neovim/comments/12g7ijr/treesitter_strange_highlighting_question/ ↩

3. https://www.reddit.com/r/neovim/comments/12g5qk3/neovim_v090_is_very_slow_in/ ↩

4. https://www.reddit.com/r/neovim/comments/12fidjh/treesitter_breaks_with_new_neovim_release/ ↩

5. https://www.reddit.com/r/neovim/comments/12fdlac/neovim_error_after_upgrading_to_090/ ↩

6. https://github.com/folke/tokyonight.nvim/blob/main/lua/tokyonight/theme.lua#L256-L264 ↩

7. https://github.com/catppuccin/nvim/blob/main/lua/catppuccin/groups/integrations/semantic_tokens.lua ↩

8. https://github.com/rebelot/kanagawa.nvim/blob/master/lua/kanagawa/highlights/lsp.lua ↩

[clason]

We have already discussed that on the original PRs, and the deliberate and reasoned decision was to not link these by default, as these may have similar names but there is no guarantee that they are used similarly. (In fact, that is the issue of contention: servers going overboard with highlighting indiscriminately even if there is no semantic information.)

If color schemes set these links themselves, they are welcome to do so, of course. And just as with tree-sitter, the expectation was always that a good experience requires customization; the defaults are just there to make sure people see that something is happening.

(And an argument like

But for the built-in defaults (which are non-exposed implementation details),
is a contradiction; if it's visible, it's exposed, and if it's not, how will changing it affect user experience?)

[jdrouhard]

I'm bringing the conversation back. I understand the pragmatic reason to keep them separate. I understand they might be used for different things. I understand the decisions from the previous discussions that led to where we are today.

We are getting more real world feedback now. There is a large amount of confusion by LSP making highlighting worse due to bypassing the @ groups.

The standard LSP groups actually are (standardized!) to be the same thing that the @ groups are for. I'm only proposing linking these groups that are standardized. Besides, as I said, these default links are implementation details. It makes no difference whatsoever what a server returns for tokens, this change just adds the @ groups to the highlight chain so themes can opt in at any level to add additional tuning.

If we use a fallback hierarchy like I propose here, people can be none the wiser and themes can continue to do what they do now. But the out of the box experience won't be jarring, it'll be more complementative.

[clason]

There is a large amount of confusion by LSP making highlighting worse due to bypassing the @ groups.

And this is the part I am not convinced about (and neither are you, as you admit in the PR description). I feel the first step is figuring out what the problem is before trying to fix it.

Before/after pictures with real-world colorschemes would go a long way here.

[jdrouhard]

There is a large amount of confusion by LSP making highlighting worse due to bypassing the @ groups.

And this is the part I am not convinced about (and neither are you, as you admit in the PR description). I feel the first step is figuring out what the problem is before trying to fix it.

I am convinced that bypassing the @ groups is a source of confusion--most of the complaints have been that LSP tokens change all of their highlighting so people are recommending they just disable it. It is "undoing" treeesitter highlighting for many themes. I admit this is likely not the only source of confusion. However, this PR is pretty unobtrusive for something that will make the default experience less jarring and more complementary.

Before/after pictures with real-world colorschemes would go a long way here.

Before/after the changes in this PR with LSP tokens enabled? Or before/after LSP tokens are enabled in neovim HEAD? The latter is available in some of those Reddit posts I linked and the GitHub issue.

[clason]

Before/after the changes in this PR with LSP tokens enabled?

That, of course. No point in replicating reddit posts.

[gpanders]

To ensure I understand the problem, I'll attempt to summarize:

Right now, the default highlights are defined like this:

@lsp.type.property -> Identifier
@property -> Identifier

this PR would instead have them work like this:

@lsp.type.property -> @property -> Identifier

Now, if a color scheme defines @property independently of Identifier, the semantic tokens will respect that definition.

For highlight groups that have builtin equivalents (Function, Macro, Comment, String) this is not as important, since (hopefully) colorschemes define @comment and Comment equivalently. But for groups without builtin equivalents (@variable, @property, @namespace) this is more meaningful. However, for consistency it makes sense to define all of the semantic token groups to the @ variants.

The change makes sense to me, though I agree with @clason that we should first try and verify that this does address the root issue (either via screenshots showing before/after or individual testers). Minimal reproduction steps would be helpful too (I haven't run into the issue myself and am not sure how to immediately reproduce it).

[jdrouhard]

@gpanders I think you've summarized it well. To reproduce, you just need to use a theme that has @ groups defined but not LSP where the @ groups are either more finely tuned or are just different than the base group highlights. This is surprisingly common. Then just make sure you are using both treesitter and LSP for highlighting.

Here's an example with the dracula theme:

No LSP/only treesitter (what appears prior to the LSP "pop in"):

Neovim 0.9 (w/ semantic tokens):

With this PR (w/ semantic tokens):

With this change, the semantic tokens just augment the treesitter highlighting instead of just totally changing everything on the screen.

[gpanders]

@jdrouhard Thanks, that's helpful.

I think there is a growing consensus that the builtin legacy highlight groups that we inherited from Vim are insufficient to support both Tree-sitter and semantic tokens, and attempting to adhere to those groups leads to issues like this. Eventually we will want to deprecate the legacy groups and define a new set of base highlight groups that map to Tree-sitter captures, and to which the semantic token highlights would link to. The legacy groups would then link to the new base groups instead of vice versa. This PR is a good step in that direction.

The difficulty right now is that (1) those new Tree-sitter groups (such as @function, @comment, @namespace, etc.) are not documented, and (2) are not yet even decided. That effort will also be coupled with development of the new default colorscheme. So this may change again in the future, but seeing as we are early in the 0.10 development cycle this is an acceptable risk.

[gpanders]

Will leave this open a bit longer for further comments.

[gpanders]

@jdrouhard Minor nit: can you change the commit message and PR description to use fix(lsp) rather than feat? I think this is a fix, and we will likely backport to a bugfix release.

[jdrouhard]

@jdrouhard Minor nit: can you change the commit message and PR description to use fix(lsp) rather than feat? I think this is a fix, and we will likely backport to a bugfix release.

Did you want me to change/remove the links that still link to the base groups (because there wasn't an obvious @ group to link to)?

I'll fix up the description and commit when I get a chance.

[mfussenegger]

Given that we already know that TS groups are likely to change, I'd rather not change the default links now.

Neovim 0.9.0 is out, any color scheme that wants to support that version will have to either work with vanilla vim syntax groups or add @lsp.* definitions.

If we add these links now (and backported that change), we'd encourage color schemes to rely on the default link to TS groups, but that would mean:

• Color schemes relying on this link would remain broken for Neovim 0.9.0.
• Color schemes relying on this link would break (both LSP, and TS highlighting) once we changed the TS groups.

If we don't make this change now:

• Color schemes (hopefully) get fixed
• If color schemes declare @lsp.*, without linking to TS groups, the LSP highlights at least won't break.

And I want to highlight that from the core perspective, treesitter is still experimental, and although there's some handling for TS groups (including some default links), they are undocumented. That's why I wouldn't consider this a fix.

[jdrouhard]

@jdrouhard Minor nit: can you change the commit message and PR description to use fix(lsp) rather than feat? I think this is a fix, and we will likely backport to a bugfix release.

@gpanders - Done.

[folke]

This makes a lot of sense to me!

[dundargoc]

@jdrouhard the tests for semantic highlighting (not in this PR, but in general) are extremely flaky and causes consistent CI failures. I considered disabling them for now but thought I contact you first. Do you know the reason for this flakiness, and would you mind taking a look at it?

[lhanson]

@jdrouhard I've been eagerly watching for this to be merged because the current user experience is pretty bad. Are the flaky tests the only blocker, and are there any plans to fix them?

[wookayin]

This looks good to me as well but, if we are to make this change, we should also have @function, @constant, @class for the remaining ones (Function, Constant, Structure), etc. This should be fixed or updated in the PR.

Note that these highlight groups are linked to their counterparts in the default highlight definitions, e.g. @function links to Function.

[clason]

If we do this, it should wait until the vim-treesitter capture rename PR is backported and upstreamed. Also, these @ groups have not been adapted to the new default colorscheme yet (for the same reason).

So I'd recommend waiting for @echasnovski to finish the work and then reconsider based on the state of things (which will be different).

[echasnovski]

I do indeed plan to update all @ groups to link to other @ groups as much as possible. And to avoid double work, I would indeed rather wait for 'nvim-treesitter' captures rename to be done (and maybe upstreamed to core if they end up are more or less one-to-one rename).

[echasnovski]

@clason, the changes from 649061b look perfect to me and are good to merge if CI is green.

@jdrouhard, would mind updating commit message? To something like fix(colorscheme): link LSP semantic tokens to treesitter groups.

[jdrouhard]

@jdrouhard, would mind updating commit message? To something like fix(colorscheme): link LSP semantic tokens to treesitter groups.

Done.

[echasnovski]

Thanks, @jdrouhard!