feat(lsp)!: semantic token modifiers, LspTokenUpdate

[swarn]

This is another take on how semantic token highlighting could work in neovim, cf #21576 and #21804. This PR has evolved quite a bit, and is no longer "rule-based."

Summary

• The default highlighting for semantic tokens adds a number of extmarks for each token, which cover most basic uses and can be higlighted by colorschemes.
• There is a new LspTokenUpdate autocommand event that allows more complex token highlighting.

Default Highlighting

For a token with n modifiers, 2n + 1 extmarks are applied:

• @lsp.type.<type>.<ft> for the type of the token, priority 125 (vim.highlight.priorities.semantic_tokens)
• @lsp.mod.<modifier>.<ft> for each modifier of the token, priority 126
• @lsp.both.<type>.<modifier>.<ft> for each modifier of the token, priority 127

For example, for the C++ code:

int function(int const p) { return p; }
//            with the cursor here ^

The :Inspect command shows:

Default highlight links

expand

The following captures are linked by default to standard highlight groups:

Token Type Highlight	Linked Group
@lsp.type.class	Structure
@lsp.type.decorator	Function
@lsp.type.enum	Structure
@lsp.type.enumMember	Constant
@lsp.type.function	Function
@lsp.type.interface	Structure
@lsp.type.macro	Macro
@lsp.type.method	Function
@lsp.type.namespace	Structure
@lsp.type.parameter	Identifier
@lsp.type.property	Identifier
@lsp.type.struct	Structure
@lsp.type.type	Type
@lsp.type.typeParameter	TypeDef
@lsp.type.variable	Identifier

The following semantic types are purposefully not linked:

Token Type Highlight	Note
@lsp.type	Don't add highlights for off-spec token types
@lsp.type.comment	This overrides @text.todo and other useful marks inside comments
@lsp.type.regexp	Let treesitter handle literals
@lsp.type.keyword	"
@lsp.type.string	"
@lsp.type.number	"
@lsp.type.operator	"
@lsp.type.event	Unclear to what this should link
@lsp.type.modifier	"

Examples: using default highlighting

expand

You can disable specific semantic highlights by clearing a highlight group (you probably want this code in a colorscheme autocommand):

vim.api.nvim_set_hl(0, '@lsp.type.function', {})

You can disable all semantic highlights. Note that while this means that no visible highlight are being applied, but the tokens are still there and are still available with vim.lsp.semantic_tokens.get_at_pos() and visible with :Inspect:

for _, group in ipairs(vim.fn.getcompletion("@lsp", "highlight")) do
  vim.api.nvim_set_hl(0, group, {})
end

The default links only use :h highlight-default groups. If you have a newer colorscheme with treesitter support, you can use the following to link to default treesitter groups, which potentially have more specific highlighting:

local links = {
  ['@lsp.type.namespace'] = '@namespace',
  ['@lsp.type.type'] = '@type',
  ['@lsp.type.class'] = '@type',
  ['@lsp.type.enum'] = '@type',
  ['@lsp.type.interface'] = '@type',
  ['@lsp.type.struct'] = '@structure',
  ['@lsp.type.typeParameter'] = 'TypeDef',
  ['@lsp.type.parameter'] = '@parameter',
  ['@lsp.type.variable'] = '@variable',
  ['@lsp.type.property'] = '@property',
  ['@lsp.type.enumMember'] = '@constant',
  ['@lsp.type.function'] = '@function',
  ['@lsp.type.method'] = '@method',
  ['@lsp.type.macro'] = '@macro',
  ['@lsp.type.decorator'] = '@function',
}
for newgroup, oldgroup in pairs(links) do
  vim.api.nvim_set_hl(0, newgroup, { link = oldgroup, default = true })
end

Note that some semantic token types have no treesitter equivalent, and there are both server-specific token types and server-specific token modifiers.

You can specialize by filetype. Make C++ functions red:

vim.api.nvim_set_hl(0, '@lsp.type.function.cpp', { fg = 'red' })

You can highlight all tokens with a given modifier by defining highlight group names:

vim.api.nvim_set_hl(0, '@lsp.mod.mutable.rust', { italic = true })
vim.api.nvim_set_hl(0, '@lsp.mod.readOnly', { fg = 'green' })

The default lsp.mod.* extmarks all have the same priority. If you add modifier highlights that all compose, or highlight modifiers that are mutually exclusive, then everything will work.

-- Deprecated default library functions will be both italic and strikethrough.
vim.api.nvim_set_hl(0, '@lsp.mod.defaultLibrary', { italic = true })
vim.api.nvim_set_hl(0, '@lsp.mod.deprecated',     { strikethrough = true })

-- Tokens can only have one scope in cpp, so there's no conflict here.
vim.api.nvim_set_hl(0, '@lsp.mod.fileScope.cpp',  { fg = 'red' })
vim.api.nvim_set_hl(0, '@lsp.mod.globalScope.cpp',{ fg = 'blue' })

The default lsp.both.* extmarks have higher priority, allowing for specialization:

-- Make declarations (class, function, etc...) bold.
vim.api.nvim_set_hl(0, '@lsp.mod.declaration', { bold = true })

-- But not variable declarations.
vim.api.nvim_set_hl(0, '@lsp.both.variable.declaration', { link = '@lsp.type.variable' })

LspTokenUpdate autocommand

The highlight-based customizations described above are straightforward, but limited. If a user wants arbitrary matching logic, they can use the LspTokenUpdate event to define callbacks that apply highlights to tokens. The event fires once per updated visible token. The vim.lsp.semantic_tokens.highlight_token function is provided to simplify creating a correct extmark for the token.

expand

Examples using LspTokenUpdate

Here is an example highlighting mutable global variables:

local st = vim.lsp.semantic_tokens
vim.api.nvim_create_autocmd("LspTokenUpdate", {
  callback = function(args)
    local token = args.data.token
    if
      token.type == "variable"
      and token.modifiers.globalScope
      and not token.modifiers.readonly
    then
      st.highlight_token(token, args.buf, args.data.client_id, "MyMutableGlobalHL")
    end
  end,
})

vim.api.nvim_set_hl(0, 'MyMutableGlobalHL', { fg = 'red' })

Here's an example that highlights variable names written in ALL_CAPS that aren't constant:

local st = vim.lsp.semantic_tokens
vim.api.nvim_create_autocmd("LspTokenUpdate", {
  callback = function(args)
    local token = args.data.token
    if token.type ~= "variable" or token.modifiers.readonly then return end

    local text = vim.api.nvim_buf_get_text(
      args.bufnr, token.line, token.start_col, token.line, token.end_col, {})[1]
    if text ~= string.upper(text) then return end

    st.highlight_token(token, args.buf, args.data.client_id, "Error")
  end,
})

[clason]

Thank you for that well reasoned and explained proposal! This sounds like a reasonable compromise to me, although I know some people will be apoplectic about colorschemes containing actual logic instead of being purely declarative...

Two bikeshedding comments:

1. @lsp instead of @sem?
2. Is it possible to instead of

st.delete_rule() -- delete all rules
st.restore_default_rule()

do just st.restore_default_rules()? What is the default_rule?

[swarn]

I know some people will be apoplectic about colorschemes containing actual logic instead of being purely declarative...

I think those concerns would be reasonable. If something like this is merged, I would strongly recommend that colorscheme authors stick to adding highlights, for all the @sem.type.* values and choice @sem.mod.* values.

If they do want to supply rules, I think they should offer them as opt-in and allow users to apply them manually.

st.rule_add(require('thecolorscheme').get_rule())

I see this as part of a broader trend of neovim plugins doing things on request, rather than doing things automatically.

1. @lsp instead of @sem?

Yeah, I feel the names can change. @lsp.type.function works as well as @sem.type.function.

do just st.restore_default_rules()? What is the default_rule?

I played with both versions of this: restore_default_rule calling delete internally, or not. I defaulted to more explicit control; you can add rules then apply the default afterwards, if you want. But, I don't have a strong opinion on this.

The default rule implements the behavior described above, and is simply

local default_rule = function(token, bufnr, priority, namespace)
  local ft = vim.bo[bufnr].filetype

  local mark = function(hl_group, delta)
    vim.api.nvim_buf_set_extmark(bufnr, namespace, token.line, token.start_col, {
      hl_group = hl_group,
      end_col = token.end_col,
      priority = priority + delta,
      strict = false,
    })
  end

  mark(string.format('@sem.type.%s.%s', token.type, ft), 0)

  for _, modifier in ipairs(token.modifiers) do
    mark(string.format('@sem.mod.%s.%s', modifier, ft), 1)
  end
end

[jdrouhard]

This looks great. Just a couple comments. You may have to mark some of the local helper functions as @private so gen_vimdoc.py doesn't try to add documentation for them.

[swarn]

To be clear, are you recommending the following?

1. Remove restore_default_rule
2. Remove the colorscheme option in add_rule

If so, I'll do both.

For later, it might be easiest to simply add colorscheme as a fourth scope (after global, ft, and buffer).

I'd strongly prefer to keep this PR focused on user customization

100% agreement from me. Previous discussion centered on colorschemes, so I wanted to address them. But I am comfortable just leaving a note that says "TBD: colorscheme rules" in this PR, if that's acceptable.

(In any case, I believe this is better solved at a lower level, e.g. adding a :highlight! clear that completely resets the highlight state to "factory defaults".)

I think it is useful to think about token rules as being the same kind of thing as treesitter queries or :syntax rules, because they are logic that applies highlights based on buffer contents. The complication is that both queries and syntax cleanly map to changes in filetype, while token rules may not.

So, if you specifically wanted to overload :highlight clear to reset rules, I disagree. Otherwise, I'm just making an off-topic point about separating highlighting from rules.

(Long-term, a better strategy will be to scope the rules to namespaces.)

Are you thinking of something fairly simple? Like this:

st.add_rule(fn1, { namespace = "foo"})
st.add_rule(fn2, { namespace = "foo"})

-- sometime later, delete all foo rules
st.delete_rule({namespace = "foo"})

If so, that's straightforward to implement as part of this PR.

[clason]

@Lsp instead of @sem?

Followup bike to shed: do we really need to specify .type. and .mod.? Is there a realistic danger for the same name to be used both as a type and a modifier and needing to be distinguished?

[swarn]

Followup bike to shed: do we really need to specify .type. and .mod.?

I agree that it's unlikely there will be clashes between token types and modifiers.

But, I think of that question like this: are we sure that no LSP server implementation will ever reuse the same name in the lists of tokens and modifier? My answer is, why wouldn't they? They're two separate lists.

So, I am OK with making the names slightly longer to ensure it's never a problem. This seems like a very low cost.

This also allows highlighting @lsp.type to catch all off-spec types.

[swarn]

@clason, I have not ignored your earlier parenthetical note:

I also would prefer not to set implicit priorities based on order

The implicit priority setting is not required for this PR, but I do think it is useful, and a sane default.

Consider the following code:

st.add_rule(function(token)
  if token.modifiers.a and token.modifiers.b then
    return "ab_highlight"
  end
end)

st.add_rule(function(token)
  if token.modifiers.c then
    return "c_highlight"
  end
end)

hl(0, 'ab_highlight', { fg = 'red' })
hl(0, 'c_highlight', { fg = 'blue' })

If a token has modifiers a, b, and c, is it red or blue? The API could just shrug and pass on this question, but I think the intent is clear. Consider:

vim.api.nvim_set_hl(0, 'Highlight', { fg = 'red' })
vim.api.nvim_set_hl(0, 'Highlight', { fg = 'blue' })

We know that the later call "wins." So, I think that having an incrementing default priority will prevent confusion by matching expectations.

I believe that strongly enough that I prefer having no default priority to having a constant default priority. That is, make the signature:

add_rule({rule}, {priority}, {opts})

Than means a user would write:

st.add_rule(function(token)
  if token.modifiers.a and token.modifiers.b then
    return "ab_highlight"
  end
end, 1000)

st.add_rule(function(token)
  if token.modifiers.c then
    return "c_highlight"
  end
end, 1000)

And the answer to "why is it sometimes one and sometimes the other?" would at least be right there in the rule creation.

In that case, it might be worthwhile to add a new value in vim.highlight.priorities that is guaranteed to be higher than any used by the default rule, but I'm not sure.

[clason]

I think this analogy is fraught, since highlight is a prior Vim API that we (need to) replicate, but add_rule is a new Lua function and should therefore behave like a Lua function.

So if you care about priority, you need to explicitly set priority. This is the same way as extmarks work, which this is much closer to.

[swarn]

Output of the modified :Inspect

[clason]

Further call for simplification (sorry that it's going piecemeal, but this PR is a huge wall of text):

1. What is the use case for a rule applying extmarks directly? Why is just returning a highlight name not enough? (I mean, a callback of course can do whatever it wants, including wiping your hard drive, but do we really need to document this as an intended scenario?)
2. Do we really need ft and bufnr as options? If you write a rule, you can just add your own check. For that it's sufficient to have the callback accept bufnr (which can be used to extract all manner of information, not just filetype) and token (in this order, which is the customary order throughout the API).

On a similar note, the callback can return a pair (string,number|nil), then priority doesn't need to be an extra argument.

[swarn]

(sorry that it's going piecemeal, but this PR is a huge wall of text):

I chose, for my first neovim PR, a user-facing API for a new feature. That was an objectively foolish choice. I really appreciate the time you're spending helping this fool improve his PR!

1. What is the use case for a rule applying extmarks directly?

The default rule shows one use case: applying multiple highlights, in this case one per modifier. If the API is too simple to create the default rule, it's too simple.

You might reasonably respond, "why not allow returning a table of highlights?" Sure, that can be added. But, what if the user wants them at different priority levels? Then, we could debate whether that seems reasonable, and if it is worth adding more complexity to the return type. And we could do it again for the next "what if."

The fact that users can set extmarks in rules means that the default callback behavior can be as simple as possible — one returned highlight or nil — while also supporting use cases that I haven't imagined. It is a core benefit of this callback-based approach. As an extra benefit, we don't have to debate what seems like a reasonable use of a tool that doesn't exist yet. In any case, you can't prevent users from setting extmarks in rules, all you can do is make it harder to do correctly.

I could bundle all the usually-unneeded arguments into a single table to tidy up the callback parameters. Something like callback(token, context) where context is a table with bufnr, ns, etc.

2. Do we really need ft and bufnr as options?

It's true, you could implement those checks in your own rules. For filetype-specific rules, you'd probably just add it inside the rule:

local function a_rule(token, bufnr)
  if vim.bo[bufnr].filetype ~= "lua" then return
  -- actual rule here
end

The fact that you might end up evaluating a bunch of rules for other filetypes is a performance pessimization that will likely be meaningless. You could load filetype-specific rules in a once filetype autocommand, if you feel like it.

For buffer-specific rules, maybe a decorator approach:

local function some_rule(token) ... end

local function buffer_scoped(rule, scope)
    return function(token, bufnr, pri, ns)
        if bufnr ~= scope then return end
        return rule(token, bufnr, pri, ns)
    end
end

st.add_rule(buffer_scoped(some_rule, a_bufnr), 130, { desc = "some_rule for buffer " .. a_bufnr })

I think most non-trivial rules will be server-specific. The ft argument was just an easy near-enough approximation to server type, while I had imagined using buffer-scoped rules in on_attach for getting it exactly right. But, on further consideration, that seems to needlessly duplicate a lot of rules. I could instead make the change to the callback arguments mentioned above, and add a server field to the context:

local function another_rule(token, context)
  if context.server ~= "clangd" then return
  -- actual rule here
end

There's another wall of text for you. To summarize:

• Extmarks in callbacks allow customization while keeping the API simple
• I will remove the ft and buffer args from add_rule
• I will change the callback signature to the two-argument form (token, context), and add server info to the second argument.

Thoughts?

[clason]

Extmarks in callbacks allow customization while keeping the API simple

Of course; I still think it's not something we need to advertise widely.

I will remove the ft and buffer args from add_rule

Agreed; premature performance optimization is the root of all evil. That being said, if there's a benchmark that shows noticeable impact, then having this option is of course good.

I will change the callback signature to the two-argument form (token, context), and add server info to the second argument.

Again, having a (much) simpler API is worth a (small) amount of boilerplate. I'm not a great fan of just stuffing everything into a context table, in particular here as this looks like a real grab bag:

• filetype can be easily looked up from bufnr, no need to pass it unless that can be demonstrated to have a performance impact;
• namespace has a very different purpose (it namespaces the rule itself, not the token), so it should go into an opts table of the add_rule function (if and when it is implemented, not now!)
• server in our context is more typically client.name, so I'd rather use the standard pattern of passing client directly (again, until it's shown to affect performance).

So add_rule(token, client, bufnr) sounds to me like a reasonable signature. But not sure we need bufnr if we have client then? I would welcome practical examples for its use. Any reasonable use, we'd add an example in the docstring showing the correct boilerplate.

[swarn]

• filetype can be easily looked up from bufnr, no need to pass it

I agree. I used filetype lookup in the default rule and in the example above.

namespace has a very different purpose (it namespaces the rule itself, not the token), so it should go into an opts table of the add_rule function

I believe we are thinking of two different uses of namespace. I'm not thinking of an argument to add_rule. I'm thinking of an argument supplied to the callback. It's needed so that any extmarks applied inside the callback can be correctly managed by the decoration provider for semantic token highlights. Basically, "use this namespace if you want your extmarks to be automatically updated correctly."

At some point in the future, it could be useful to name sets of rules — a conceptual namespace for rules. But I'm not sure why you'd want that to be a literal :h namespace.

server in our context is more typically client.name, so I'd rather use the standard pattern of passing client

Very good point, passing client is both more idiomatic and more useful.

But not sure we need bufnr if we have client then? I would welcome practical examples for its use.

Well, we've mentioned looking up filetype, and that is used by the default rule. Also, I've got an example in the "Complex Logic" section of the (frequently changing) first comment in this thread using nvim_buf_get_text. In general, the buffer number is the key to a lot of information.

This would end up with a callback function callback(token, bufnr, client, ns).

[clason]

I believe we are thinking of two different uses of namespace. I'm not thinking of an argument to add_rule. I'm thinking of an argument supplied to the callback. It's needed so that any extmarks applied inside the callback can be correctly managed by the decoration provider for semantic token highlights. Basically, "use this namespace if you want your extmarks to be automatically updated correctly."

Hmmm. I think that this needlessly complicates the API. I would prefer that

• add_rule is for adding a single rule, returning a highlight name and optionally a priority.
• the default_rule is special and can use internal stuff (in particular the (fixed!) namespace for semantic tokens).

If there is a strong(!) argument for adding a custom_rule that can (more conveniently) do lower-level stuff, we add it later. I strongly suggest not trying to shoehorn both "convenient simple" and "powerful complex" into a single function. I really don't think we should design add_rule around applying extmarks directly.

[clason]

To put it bluntly, I'm not convinced that either of the "complex logic" examples is something we should support; this is veering dangerously into "look at the neat stuff we could do", where "stuff" is actually the language server's job, not the client's. Feature creep is a very real problem.

Of all the examples you've shown, client-specific rules are the only ones I really see as compelling -- although those could also be done by adding the rule as part of the on_attach callback or LspAttached autocommand, which is our standard pattern for LSP setup and config and therefore a very strong precedent.

Maybe we should get some benchmarks for adding rules and highlighting with them before continuing the discussion? I would also find it very helpful to see an actual complete example how one would use this in the context of setting up a server (using vim.lsp.start()).

[swarn]

Maybe we should get some benchmarks

I'm happy to do some benchmarking. What numbers are you looking for? Given that the rule callbacks are only called for visible tokens, I can't imagine that we'll see any meaningful difference for reasonable numbers of rules. If it wasn't clear, I think you're right about eliminating the scope options (ft and bufnr) from add_rule, and handling that functionality via guards inside the rules. Wasn't that the only context for performance concerns?

how one would use this

If I only wanted to load needed rules, I might add rules in an autocommand like so:

local clangd_rule_specs = {
  {
    function(token, bufnr, client)
      if client.name ~= "clangd" then return end
      -- ...
    end, 130, "clangd rule a"
  },
  { function(token, bufnr, client) ... end, 131, "clangd rule b" },
  { function(token, bufnr, client) ... end, 132, "clangd rule c" },
}

local clangd_rules_added = false

vim.api.nvim_create_autocmd("LspAttach", {
  callback = function(args)
    local client = vim.lsp.get_client_by_id(args.data.client_id)

    if clangd_rules_added or client.name ~= "clangd" then
      return
    end

    clangd_rules_added = true
    for _, spec in ipairs(clangd_rule_specs) do
      local rule, pri, desc = unpack(spec)
      vim.lsp.semantic_tokens.add_rule(rule, pri, { desc = desc })
    end
  end,
})

You could do something similar in on_attach, especially if you wanted buffer-scoped rules. But, given the ability to guard on client name, that seems unnecessary.

this is veering dangerously into "look at the neat stuff we could do"

Ha! I'd be careful with that. We've been programming for a long time without semantic highlighting; is there anything you can do with it that can't be dimissed as merely "neat?" There are a lot of neovim features that are just neat! I like my global status bar!

I will say that many C++ codebases have naming guidelines, things like "parameters start with t_". It's great to automatically show violations of those guidelines, both false positives and false negatives. While I don't know that highlighting misleadingly-named variables rises above neat, it shows what you can do very easily with a simple callback mechanism that can get the text.

One point is to contrast this callback approach with a DSL approach — implemented inside highlight names or otherwise — which seems like vastly more work to achieve less functionality.

where "stuff" is actually the language server's job

The server has done its job: it's telling us what is a variable, and what is read-only. Deciding how that should look is the client's job, I think.

Hmmm. I think that this needlessly complicates the API.

So, I'm thinking about this callback signature:

callback(token, bufnr, client, ns)

Are you saying that all of bufnr, client, and ns are too much? They don't seem like "shoehorning" to me — though I understand that is the issue at hand. But, simple callbacks just use the first argument. The other arguments give you access to, respectively, information about the text, the LSP server, and the ability to set correctly-managed extmarks. It doesn't seem extravagant, while it does seem like it is sufficient to prevent needing any other complex_rule function (famous last words!).

the default_rule is special and can use internal stuff

I feel pretty strongly about this one. Dogfooding is a straightforward way to determine if your API is sufficient. If we can't implement the default rule with the API, then the API is not good enough. I don't have have a knock-down, must-have use case that demonstrates the need for the extra arguments; again, I don't think there are must-have semantic tokens use cases. You're probably tired of reading this, but one of my goals here is to support uses I haven't thought of. If the API can't support the — very simple! — default rule, then there are probably a lot of interesting uses that it can't handle.

[swarn]

Easier side question: what's a good name/behavior for a command to print a table of current rules? :LspRuleInfo in a pop-up?

[mfussenegger]

Not sure where to start given the amount of conversation that already happened, so I'll just leave some questions and thoughts. In no particular order:

• I like that it keeps semantic tokens enabled by default

• Why do we need to distinguish between LSP and Treesitter in the default highlight groups?

  • Advantage of not distinguishing:
    • Color schemes designed for Tree-sitter may yield better results to users if TS is not in use but LSP is
  • Disadvantage: Can't distinguish, but why is that a Problem? Tokens are usually not ambiguous. If something is a function, it's a function.

• Couldn't one already accomplish a similar form of customization by using nvim_set_decoration_provider with get_at_pos(...), or maybe an additional .get() to retrieve token information? Relates a bit to your point of making complex things possible. It would give even more power without having to introduce new systems (that people need to learn and understand). And making it easy to do complex things is not the main priority.

• I still miss a couple of concrete use-cases where I'd feel like "Oh yes, I'd want that". It's kinda hard to judge the proposed API against vague requirements. Good rule of thumb is to have 2-3 concrete use-cases to validate a design. E.g I can see how it's valuable to have a different highlighting on declarations, deprecated variables or readonly variables, but as I understood this would be possible with a design as initially proposed in Better semantic token modifiers #21576. Maybe this rule system is already entering Attractive nuisances territory?

• I'm still not sure how I feel about having a custom highlight rule system specifically for LSP. I'd kinda prefer it if the LSP highlighter were merely an implementation detail given that the notation of semantic tokens and modifiers is conceptionally not exclusive to LSP but it just happens that it's currently the only source for it. On the other hand, that didn't stop us from making other LSP exclusive APIs, so maybe this point is moot.

• I'm kinda in a weird spot with this as I don't care about highlighting much myself, so I mainly see this as maintenance burden, but you clearly put quite a lot of thought into this, so I don't want to say no to this either but I'd feel a lot better about this if in addition to @clason, @folke is also on board with this as he's also in the LSP reviewer group and maintains a popular colorscheme.

I can review some of the implementation details once it's clear that a rule system is really what we want to have.

[folke]

I'm a bit late to the this discussion, but just wanted to say I like this approach!

[clason]

For the record, here's Helix' PR for integrating semantic tokens -- their theming choices are worth a look (or, rather, vice versa, since we're a good deal farther; the link goes both ways):
helix-editor/helix#6102

[bfredl]

perhaps this has been discussed before, but I think the behavior would be predictable if

1. this code was moved before the @lsp.foo.bar code
2. highlight_token() would set token.marked (or at least provide an option to do so)
3. after the autocmd, we check again if token.marked was set and then disable the setting of the default highlight groups ( i e @lsp.whatever ) if this was done.

This would make it easier to completely override the behavior of a token without needing to counteract the effect of the default groups (which might set colors and attrs not set by the autocmd provided hl_group).

[clason]

Bikeshed, while we're at it: not crazy about both -- maybe typemod or combined or combination or somesuch would be more descriptive.

[jdrouhard]

perhaps this has been discussed before, but I think the behavior would be predictable if

1. this code was moved before the @lsp.foo.bar code

2. highlight_token() would set token.marked (or at least provide an option to do so)

3. after the autocmd, we check again if token.marked was set and then disable the setting of the default highlight groups ( i e @lsp.whatever ) if this was done.

This would make it easier to completely override the behavior of a token without needing to counteract the effect of the default groups (which might set colors and attrs not set by the autocmd provided hl_group).

I really like this idea, and might even consider it a must. The one part of this PR I'm not super excited about is that the default extmarks are being added no matter what a user has in their autocmd. This would resolve that nicely.

[mfussenegger]

Wouldn't this also mean that as soon as you want to have a single customisation you'd also need to apply all defaults as well if color schemes start using these?

I'd think it's more confusing if other highlights disappear. Afaik this was also discussed before. If there are no new substantial new arguments for a behaviour change I'd say we go ahead with this and let people actually try and use it before having another round of discussion going back and forth

---

Bikeshed, while we're at it: not crazy about both -- maybe typemod or combined or combination or somesuch would be more descriptive.

+1 for typemod

[jdrouhard]

Wouldn't this also mean that as soon as you want to have a single customisation you'd also need to apply all defaults as well if color schemes start using these?

Not if marking the token as added in the utility highlight function was optional (and false by default). Just giving the ability to override defaults for a particular token would be extremely useful and wouldn't really change the behavior this PR currently provides.

[swarn]

I made the change from @lsp.both.* to @lsp.typemod.*, which is more descriptive.

[bfredl]

Not if marking the token as added in the utility highlight function was optional (and false by default)

Right, this seems like the correct resolution. The default behavior should remain like it is, but we provide a realiable opt-in mechanism to completely override highlights for user who understand what it implies and wants that.

[swarn]

I agree that this would be useful and straightforward to implement, but I am fine with leaving it for a future PR.

I imagine that the opts argument would add a second field; something like:

---@param opts (table|nil) Optional parameters.
---       - priority: (number|nil) Priority for the applied extmark. Defaults
---         to `vim.highlight.priorities.semantic_tokens + 3`
---       - keep_default (boolean|nil) Defaults to true. If true, apply the
---         default `@lsp.*` highlights as usual. If false, do not apply the
---         default highlights for this token.

I don't think this is the same as previous discussion about "disabling the default rule." That was about disabling all default semantic highlighting, rather than allowing you to disable it per-token using the callback.

There was also speculation about a hypothetical change enabling return values from autocommand callbacks, and how that would obviate the need for a separate highlight_token() function. Adding more logic to highlight_token() doesn't make that impossible, but maybe less clean.

[mfussenegger]

I don't think this is the same as previous discussion about "disabling the default rule." That was about disabling all default semantic highlighting, rather than allowing you to disable it per-token using the callback.

Oh right.

I don't have a strong opinion on this but I'd really like to defer changing this to a separate PR.
If there are no other objections I'll go ahead and merge this in a bit