[clangd] Implement configs to stop clangd produce a certain semantic tokens

[daiyousei-qz]

Summary

This patch introduces the following configurations to .clangd:

SemanticTokens:
    DisabledKinds: [ ... ]
    DisabledModifiers: [ ... ]

Based on the config, clangd would stop producing a certain type of semantic tokens from the source file.

[daiyousei-qz]

Please hold off from reviewing this. I have been figuring out how arc tools work and the unittest of the current version is broken.

[daiyousei-qz]

Fix unittest

[daiyousei-qz]

Add a test in SemanticHighlightingTests.cpp

[daiyousei-qz]

Fix an issue where the previous change is lost.

clang-tools-extra/clangd/SemanticHighlighting.cpp

1281 ↗

(On Diff #514086)

I'm not sure if we should parse the enums here or in parsing configs using EnumSwitch. The problem is that I don't want to introduce dependencies to "SemanticHighlighting.h" in "ConfigCompile.cpp".

[daiyousei-qz]

Add some comments to the config headers. Resubmit to retrigger build test since I cannot reproduce the failure.

[kadircet]

before you dive any deeper into the patch, could you give some reasoning about why this is needed/useful?

[nridge]

In D148489#4276634, @kadircet wrote:

before you dive any deeper into the patch, could you give some reasoning about why this is needed/useful?

https://github.com/clangd/clangd/discussions/1598 contains some context/motivation

[daiyousei-qz]

In D148489#4276634, @kadircet wrote:

before you dive any deeper into the patch, could you give some reasoning about why this is needed/useful?

The discussion is at https://github.com/clangd/clangd/discussions/1598.

The original motivation is that in the latest clangd, we start to assign semantic token "operator" to all operators in the source code. Since the semantic tokens take priority over the textmate rules in vscode, now we cannot theme different operators differently anymore. For example, we cannot color "new" in blue while have "+" in white.

To address the issue, this patch adds configurable filter to the semantic token to filter out unwanted kinds and modifiers. We should have the following benefits with such filter in place.

1. Address the issue mentioned above about unwanted token kinds and modifiers so textmate rules could fallback.
2. Improve the performance of semantic tokens (if anyone is taking advantage of the config OFC) since we could strip away unwanted tokens
3. Not directly supported by this patch, but the filter could also be used to support adding semantic tokens that are disabled by default. Say if we want to support augmentsSyntaxTokens in the future.

[nridge]

Thanks for the patch!

I think this is a nice and general solution, which rather than solving just a specific problem (e.g. highlighting of new as an operator vs. a keyword), gives users a broader ability to fine-tune clangd's highlightings to suit their needs.

Could I kindly ask you to make a PR that updates https://github.com/llvm/clangd-www/blob/main/config.md to cover the new config keys, as well?

clang-tools-extra/clangd/SemanticHighlighting.cpp

400 ↗

(On Diff #514530)

For good measure, let's static_assert(HighlightingModifier::LastModifier < 32)

590 ↗

(On Diff #514530)

This needs to be stored by value, otherwise it will dangle

[daiyousei-qz]

Sorry, I'm a little occupied lately and don't have time to fix the test failure. I'll try to fix that in this week.

clang-tools-extra/clangd/SemanticHighlighting.cpp

400 ↗

(On Diff #514530)

IIRC, we already have such static_assert after the definition of HighlightingModifier in SemanticHighlight.h

[nridge]

clang-tools-extra/clangd/SemanticHighlighting.cpp

400 ↗

(On Diff #514530)

Good point, I overlooked that.

[daiyousei-qz]

Pass filter by value.

[daiyousei-qz]

Redo arc as some changes are missing from previous update.

[daiyousei-qz]

Fix format issue

[nridge]

Thanks! Let me know if you need me to commit.

clang-tools-extra/clangd/SemanticHighlighting.cpp

406 ↗

(On Diff #521977)

The filter can be passed by const reference in the constructor (and it's better to do so to avoid making an additional copy), it just needs to be stored by value as a class member

[daiyousei-qz]

Yes please.

[nridge]

(Do you plan to address the last comment before I merge?)

[daiyousei-qz]

In D148489#4362990, @nridge wrote:

(Do you plan to address the last comment before I merge?)

Oh, okay. Let me address that. Didn't see that.

[daiyousei-qz]

• Address review comment and remove a unnecessary flag

[daiyousei-qz]

Sorry for the inactivity. I have replace the parameter with const ref. By the way, I also removed the existing IncludeInactiveRegionTokens flag and uses this filter instead. Please help review the change. Thanks!

[daiyousei-qz]

• Fix reversed logic of including inactive region token

[nridge]

Thanks, LGTM!

[nridge]

I went ahead and merged this.

When you get a chance, could you update https://github.com/llvm/clangd-www/pull/85 as well so we can merge that too?

[daiyousei-qz]

Thank you! Since the documentation shouldn't be available until LLVM 17, I'm slowly working on it. Let me update the PR in this weekend :)

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

 //==- SemanticHighlightingTests.cpp - SemanticHighlighting tests-*- C++ -* -==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
 #include "TestTU.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
o]] [[bar]])cpp";
   EXPECT_EQ(Toks[2].deltaStart, 0u);
   EXPECT_EQ(Toks[2].length, 1u);
 
   // bar
   EXPECT_EQ(Toks[3].deltaLine, 0u);
   EXPECT_EQ(Toks[3].deltaStart, 2u);
   EXPECT_EQ(Toks[3].length, 3u);
 }
+
+TEST(SemanticHighlighting, WithHighlightingFilter) {
+  llvm::StringRef AnnotatedCode = R"cpp(
+int *$Variable[[x]] = new int;
+)cpp";
+  Config Cfg;
+  Cfg.SemanticTokens.DisabledKinds = { "Operator" };
+  Cfg.SemanticTokens.DisabledModifiers = { "Declaration", "Definition" };
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  checkHighlightings(AnnotatedCode, {}, ~ScopeModifierMask);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang