[PATCH] D77811: [clangd] Implement semanticTokens modifiers

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D77811#2533237 <https://reviews.llvm.org/D77811#2533237>, @nridge wrote:

> Thanks a lot for working on this! I agree this is well aligned with the goals of D77702 <https://reviews.llvm.org/D77702> (and in fact goes even further and meets the goals of D66990 <https://reviews.llvm.org/D66990> via the `declaration` modifier).

Ooh, I hadn't seen D66990 <https://reviews.llvm.org/D66990>, thanks for the pointer and also giving the upstream feedback to LSP folks! I'm also really happy about the multi-dimensional nature of highlightings in the new protocol, looks like you pushed it in that directyon.

>> - no split between primitive/typedef. (Is introducing a nonstandard kind is worth this distinction?)
>
> Here's my general take on this:
>
> - There is too much variety between programming languages to expect that a standard list of highlighting kinds is going to satisfactorily cover all languages.
> - If LSP aims to be the basis for tools that are competitive with purpose-built language-specific tools, it's reasonable to allow for clients willing to go the extra mile in terms of customization (e.g. use a language-specific theme which provides colors for language-specific token kinds).
> - Therefore, I would say yes, we should take the liberty of adding nonstandard highlighting kinds and modifiers where it makes sense from a language POV.

Totally agree, I should have been more explicit.
Introducing nonstandard kinds is **backwards-incompatible**. If the client doesn't understand primitiveType, then the token kind is now completely unknown. This could be a regression from the current state (type).
But for primitive type specifically, the information we're losing is "auto denotes a type" which the client's non-semantic highlighting should manage anyway. So I agree we should do this.

> That said... for typedef specifically, I wonder if it actually makes more sense as a modifier than a kind. That is, have the kind be the target type (falling back to Unknown/Dependent if we don't know), and have a modifier flag for "the type is referred to via an alias" (as opposed to "the type is referred to directly"). WDYT?

Agree. Do you think it should be the *same* modifier as `deduced` which I included here?
(In a similar vein, there's an argument for pointer, ref, rvalue-ref as modifiers)

>> - no nonstandard local attribute This probably makes sense, I'm wondering if we want others and how they fit together.
>
> Are you referring to local-scope variables (i.e. `FunctionScope` in D95701 <https://reviews.llvm.org/D95701>)? If so, I think the approach in that patch is promising.

Yes, exactly.



================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:264
+        $Class[[D]] $Field_decl[[E]];
+        static double $StaticField_decl_static[[S]];
+        static void $StaticMethod_decl_static[[bar]]() {}
----------------
nridge wrote:
> Presumably, the highlighting kinds `StaticField` and `StaticMethod` are going to be collapsed into `Field` and `Method` in a future change (after the removal of TheiaSemanticHighlighting, I guess)?
Yeah, merging any kinds that are exported with the same name should be NFC at that point.

Hmm, though currently StaticField --> Variable, not Field (similarly StaticMethod --> Method).
So we can have static fields be Variable+Static+ClassScope or Field+Static+ClassScope.

I can see arguments for either...


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:718
+      };
+      )cpp",
+  };
----------------
nridge wrote:
> Should we add a test case for `_deprecated` as well?
Oops, done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77811