[PATCH] D136594: [clangd] Add support for semantic token type "operator"

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D136594#3940143 <https://reviews.llvm.org/D136594#3940143>, @nridge wrote:

> I believe highlighting of **built-in** operators should be out of scope for semantic highlighting, at least in the default mode; client-side highlighting should be sufficient for these, similar to strings and literals.

I'm not sure if "built-in" (as opposed to overloaded) is the right distinction here: whether `a == b` can be highlighted client-side seems unrelated to whether the type is `int` or `string`.
But I agree we only need to provide this (by default) if it's something that can't be determined by lexing.

An (IMO) useful distinction that can't be found by the lexer is the use of `*` as a declarator (`int *x`) vs an operator (`return *x`). These are both potentially "built-in". https://github.com/helix-editor/helix/pull/4278 has a fuller example. (In practice, clients are going to highlight declarator-stars as operators on the client-side, so to make this work we have to highlight them as something else rather than just mark operators. But this patch looks like the right direction).

> 2. An alternative to assigning (user-provided) operators a new token kind would be to assign them the same token kind as the entity they invoke (i.e. function or method). Both approaches have their advantages:
>   - If we use the function/method kinds, then uses of user-provided operators will be highlighted differently from built-in operators even when using a default / standard theme that doesn't know about clangd-specific token types.
>   - If we use a dedicated operator kind, users can configure different styles for operators vs. function/methods (and they may want different styles given that syntactically the two look quite different).
>
>     One way to get the best of both worlds could be to use the function/method kinds in combination with an `operator` **modifier**. That would color overloaded operators out of the box while also allowing users to customize the style based on the presence of the modifier. What do you think about this approach?

I think since LSP specifies an `operator` SemanticTokenType, we should use it unless there's a *really* strong reason not to.
A well-behaved editor will provide a themeable color for (client-side) highlighting of operators, and also map LSP's `operator` SemanticTokenType onto that color by default. If we decide to do our own different thing, we'll break that happy path.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
(Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
sammccall wrote:
> Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
> (Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)
as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5

If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76
   ConstructorOrDestructor,
+  UserProvided,
 
----------------
sammccall wrote:
> sammccall wrote:
> > Can you give some background here or on the bug tracker about what kind of distinction you're trying to draw here and why it's important?
> > (Most clients are never going to benefit from nonstandard modifiers so they should be pretty compelling)
> as well as being jargony, "user-provided" has a specific technical meaning that I don't think you intend here. For example, `auto operator<=>(const S&) const = default` is not user-provided (defaulted on first declaration). https://eel.is/c++draft/dcl.fct.def.default#5
> 
> If we need this and can't get away with reusing `defaultLibrary` (which would include `std::`) then maybe we should add something like `builtin` which seems quite reusable.
Since we often can't say whether an operator is user-provided or not (in templates), we should consider what we want the highlighting to be in these cases.
(If templates should be highlighted as built-in, we should prefer a modifier like `UserProvided`, if they should be highlighted as user-provided, we should prefer a modifier like `Builtin`)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136594/new/

https://reviews.llvm.org/D136594