[PATCH] D77702: [clangd] Use token modifiers and more token types for semanticTokens

Thu Apr 9 11:39:07 PDT 2020

sammccall added a comment.

Some context for me: I did also start some work on supporting modifiers, and after some experiments with VSCode concluded it was too early (no support in any shipped themes, I saw some hard-to-explain interactions with the default syntax highlighting...).
My plan was to revisit once this was in the standard and VSCode had some out-of-the-box support. If there's a goal to implement already in other editors (Theia?) that's a good reason to move forward sooner, but we should be wary of getting too far ahead of VSCode I think, as they're the ones who will mostly decide how the standard mostly gets interpreted.
The unfinished code is in D77811 <https://reviews.llvm.org/D77811> for what it's worth (no tests, not sure if the scope is right etc). With that out of the way...

---

A couple of observations about the spec:

1: LSP modifiers are clearly designed to be orthogonal to the token kind (in principle at least, even if `static namespace` isn't a thing).
Some of the most useful modifiers mentioned in the spec (e.g. readonly and definition) are certainly going to apply to a wide range of kinds. And I imagine these will often be orthogonal in presentation too (e.g. underline all definitions, color indicates what it is).
I think this makes a lot of sense and would like our design to reflect this internally (e.g. modifiers as boolean flags in HighlightingToken>) rather than having modifiers+kind encode some internal enum. While it's OK to have some modifiers that aren't orthogonal internally, I worry about *starting* there. And I'm skeptical that some of those added here are best implemented this way.

2: The "predefined" modifiers/kinds in the semanticToken spec are a bit of a dilemma.
We care about lots of clients, and know from experience that a) we have very little influence on how vscode interprets the spec and b) we have some influence on other clients, but they mostly copy vscode by default 
Because servers, client plugins, editors, and themes typically come from different vendors, getting modifiers/kinds highlighted well out-of-the-box is going to be really hard unless there's alignment on what the modifiers/kinds *are*.
I think the predefined values in the spec are probably the best/most likely way to get this alignment, so there's a lot of value in sticking to them closely.
**On the other hand** we'll never capture all interesting C++ semantics in a language-neutral spec, and there are some seriously questionable choices ("member" as a kind rather than an attribute or at least method/field?).
There's no "subclasses" at least for kinds, so we have to make a call one way or the other. I do think we should stick to the standard names where we possibly can, and fight (and probably lose) to get `member` fixed, `local` added etc in the spec.

---

I'm not sure that making the results of the new API easy to exactly map back onto the results of the old API is something we should be taking as a hard design goal, which I know makes transition a bit painful :-(
Similar to the first point, we don't want our design constrained too much by an interface that will go away.

================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:450
+  None = 0,
+  Local = 1,
+  Static = 2,
----------------
I like this a lot. There's a function `computeScope` in Quality.cpp that determines if something's class/function/file scoped, though for function-local I guess `Decl::isLexicallyWithinFunctionOrMethod()` is enough.

================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:451
+  Local = 1,
+  Static = 2,
+  Member = 4,
----------------
I think we should probably call this StaticMember (internally), to avoid accidentally conflating the various meanings of `static`.

(Which reminds me, I'd love to make hover on `static` explain what it means here!)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77702