This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by daiyousei-qz on Apr 16 2023, 8:18 PM.

Details

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.

Diff Detail

Event Timeline

daiyousei-qz created this revision.Apr 16 2023, 8:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 8:18 PM
daiyousei-qz requested review of this revision.Apr 16 2023, 8:18 PM
daiyousei-qz retitled this revision from Implement configs to stop clangd produce a certain semantic tokens to [clangd] Implement configs to stop clangd produce a certain semantic tokens.Apr 16 2023, 8:21 PM
daiyousei-qz edited the summary of this revision. (Show Details)
daiyousei-qz added a reviewer: nridge.

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

Fix unittest

Add a test in SemanticHighlightingTests.cpp

Fix an issue where the previous change is lost.

clang-tools-extra/clangd/SemanticHighlighting.cpp
1279

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".

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

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

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

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 requested changes to this revision.May 7 2023, 11:59 PM

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

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

587

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

This revision now requires changes to proceed.May 7 2023, 11:59 PM

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

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

nridge added inline comments.May 8 2023, 11:48 PM
clang-tools-extra/clangd/SemanticHighlighting.cpp
400

Good point, I overlooked that.

daiyousei-qz updated this revision to Diff 521967.EditedMay 13 2023, 10:07 PM

Pass filter by value.

Redo arc as some changes are missing from previous update.

Fix format issue

nridge accepted this revision.May 16 2023, 11:29 PM

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

clang-tools-extra/clangd/SemanticHighlighting.cpp
406

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

This revision is now accepted and ready to land.May 16 2023, 11:29 PM

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

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

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

  • Address review comment and remove a unnecessary flag

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!

  • Fix reversed logic of including inactive region token
nridge accepted this revision.May 26 2023, 12:14 AM

Thanks, LGTM!

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?

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 :)