[clangd] Add highlighting modifier "constructorOrDestructor"

[ckandeler]

Summary

This is useful for clients that want to highlight constructors and
destructors different from classes, e.g. like functions.

[sammccall]

Hmm, semantic tokens doesn't really provide a good way of "subclassing" kinds, I suppose modifiers are the best we have.

It doesn't scale very well though: we're limited to 30 modifiers in total, this patch brings us up to 16, if we followed this class.constructor precedent for function.operator, class.constructor.copy enum.scoped etc we could end up exhausting this.
Is it important to distinguish constructor vs destructor or just those two vs other class refs?

I think this is patch is fine, just not sure where we sholud draw the line.

[ckandeler]

In D134728#3822653, @sammccall wrote:

It doesn't scale very well though: we're limited to 30 modifiers in total, this patch brings us up to 16, if we followed this class.constructor precedent for function.operator, class.constructor.copy enum.scoped etc we could end up exhausting this.

In light of this (and assuming it really has to be this way), would it make sense to introduce a generic modifier that changes its meaning depending on the main highlighting kind? It would have the drawback of not being self-documenting, but with the given constraints might just be a pragmatic solution:

enum class HighlightingModifier {
  // ...

  // Class -> Constructor/Destructor
  // Document new uses here
  Generic,

  // ...
};

[sammccall]

TL;DR: let's go with this patch as-is rather than borrowing problems from the future. This enum isn't growing very rapidly.

In D134728#3822775, @ckandeler wrote:

In D134728#3822653, @sammccall wrote:

It doesn't scale very well though: we're limited to 30 modifiers in total, this patch brings us up to 16, if we followed this class.constructor precedent for function.operator, class.constructor.copy enum.scoped etc we could end up exhausting this.

BTW, I remembered the other idea that could eat a bunch of the modifiers: rainbow highlighting a la https://github.com/clangd/clangd/issues/889. The idea is you could encode e.g. an 8 bit hash of the symbol identity as the presence/absence of 8 modifiers. But we never shipped it, and I suspect 8 would be enough.

In light of this (and assuming it really has to be this way)

FWIW the reason is that modifiers are encoded as an integer, and the spec says integers are <2^31-1 for interop purposes. (I'm not sure why they didn't go for 2^53-1, everyone has either doubles or 64-bit integers right? sigh).

would it make sense to introduce a generic modifier that changes its meaning depending on the main highlighting kind? It would have the drawback of not being self-documenting

Neat idea. That's workable at a technical level, but requires editors/themes to be pretty tightly coupled to this idiosyncracy of clangd. So I'm not sure it's a good fit for the loosely-coupled LSP ecosystem.

but with the given constraints might just be a pragmatic solution

Yeah, however the constraints aren't very tight at this point, I was just thinking out loud a bit. Let's go for the simple thing for now - we may never need something cleverer, and if we do then we'll have more examples to drive the design. I think that's worth the potential breaking change.

// Class -> Constructor/Destructor

We'd need Generic1 and Generic2 so you can distinguish constructor/destructor right?
If the distinction isn't important, maybe we should add a single "constructor or destructor" modifier...

[ckandeler]

In D134728#3822898, @sammccall wrote:

If the distinction isn't important, maybe we should add a single "constructor or destructor" modifier...

No, it's not. I will update the patch accordingly.

[ckandeler]

Now uses just one modifier instead of two, as discussed.

clang-tools-extra/clangd/SemanticHighlighting.h

enum class HighlightingModifier {
   Deduced,
   Readonly,
   Static,
   Abstract,
   Virtual,
   DependentName,
   DefaultLibrary,
   UsedAsMutableReference,
+  ConstructorOrDestructor,
 
   FunctionScope,
   ClassScope,
   FileScope,
   GlobalScope,
 
   LastModifier = GlobalScope
 };

clang-tools-extra/clangd/SemanticHighlighting.cpp

if (auto K = kindForType(L.getTypePtr(), H.getResolver())) {
       if (auto Mod = scopeModifier(L.getTypePtr()))
         Tok.addModifier(*Mod);
       if (isDefaultLibrary(L.getTypePtr()))
         Tok.addModifier(HighlightingModifier::DefaultLibrary);
     }
     return true;
   }
 
+  bool VisitCXXDestructorDecl(CXXDestructorDecl *D) {
+    if (auto *TI = D->getNameInfo().getNamedTypeInfo()) {
+      SourceLocation Loc = TI->getTypeLoc().getBeginLoc();
+      H.addExtraModifier(Loc, HighlightingModifier::ConstructorOrDestructor);
+      H.addExtraModifier(Loc, HighlightingModifier::Declaration);
+      if (D->isThisDeclarationADefinition())
+        H.addExtraModifier(Loc, HighlightingModifier::Definition);
+    }
+    return true;
+  }
+
+  bool VisitCXXMemberCallExpr(CXXMemberCallExpr *CE) {
+    if (isa<CXXDestructorDecl>(CE->getMethodDecl())) {
+      if (auto *ME = dyn_cast<MemberExpr>(CE->getCallee())) {
+        if (auto *TI = ME->getMemberNameInfo().getNamedTypeInfo()) {
+          H.addExtraModifier(TI->getTypeLoc().getBeginLoc(),
+                             HighlightingModifier::ConstructorOrDestructor);
+        }
+      }
+    }
+    return true;
+  }
+
   bool VisitDeclaratorDecl(DeclaratorDecl *D) {
     auto *AT = D->getType()->getContainedAutoType();
     if (!AT)
       return true;
     auto K =
         kindForType(AT->getDeducedType().getTypePtrOrNull(), H.getResolver());
     if (!K)
       return true;
findExplicitReferences(
           if (isVirtual(Decl))
             Tok.addModifier(HighlightingModifier::Virtual);
           if (isDependent(Decl))
             Tok.addModifier(HighlightingModifier::DependentName);
           if (isDefaultLibrary(Decl))
             Tok.addModifier(HighlightingModifier::DefaultLibrary);
           if (Decl->isDeprecated())
             Tok.addModifier(HighlightingModifier::Deprecated);
+          if (isa<CXXConstructorDecl>(Decl))
+            Tok.addModifier(HighlightingModifier::ConstructorOrDestructor);
           if (R.IsDecl) {
             // Do not treat an UnresolvedUsingValueDecl as a declaration.
             // It's more common to think of it as a reference to the
             // underlying declaration.
             if (!isa<UnresolvedUsingValueDecl>(Decl))
               Tok.addModifier(HighlightingModifier::Declaration);
             if (isUniqueDefinition(Decl))
               Tok.addModifier(HighlightingModifier::Definition);
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, HighlightingKind K) {
   llvm_unreachable("invalid HighlightingKind");
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, HighlightingModifier K) {
   switch (K) {
   case HighlightingModifier::Declaration:
     return OS << "decl"; // abbreviation for common case
   case HighlightingModifier::Definition:
     return OS << "def"; // abbrevation for common case
+  case HighlightingModifier::ConstructorOrDestructor:
+    return OS << "constrDestr";
   default:
     return OS << toSemanticTokenModifier(K);
   }
 }
 
 bool operator==(const HighlightingToken &L, const HighlightingToken &R) {
   return std::tie(L.R, L.Kind, L.Modifiers) ==
          std::tie(R.R, R.Kind, R.Modifiers);
llvm::StringRef toSemanticTokenModifier(HighlightingModifier Modifier) {
   case HighlightingModifier::Virtual:
     return "virtual";
   case HighlightingModifier::DependentName:
     return "dependentName"; // nonstandard
   case HighlightingModifier::DefaultLibrary:
     return "defaultLibrary";
   case HighlightingModifier::UsedAsMutableReference:
     return "usedAsMutableReference"; // nonstandard
+  case HighlightingModifier::ConstructorOrDestructor:
+    return "constructorOrDestructor"; // nonstandard
   case HighlightingModifier::FunctionScope:
     return "functionScope"; // nonstandard
   case HighlightingModifier::ClassScope:
     return "classScope"; // nonstandard
   case HighlightingModifier::FileScope:
     return "fileScope"; // nonstandard
   case HighlightingModifier::GlobalScope:
     return "globalScope"; // nonstandard

clang-tools-extra/clangd/test/initialize-params.test

 # CHECK-NEXT:            "deduced",
 # CHECK-NEXT:            "readonly",
 # CHECK-NEXT:            "static",
 # CHECK-NEXT:            "abstract",
 # CHECK-NEXT:            "virtual",
 # CHECK-NEXT:            "dependentName",
 # CHECK-NEXT:            "defaultLibrary",
 # CHECK-NEXT:            "usedAsMutableReference",
+# CHECK-NEXT:            "constructorOrDestructor",
 # CHECK-NEXT:            "functionScope",
 # CHECK-NEXT:            "classScope",
 # CHECK-NEXT:            "fileScope",
 # CHECK-NEXT:            "globalScope"
 # CHECK-NEXT:          ],
 # CHECK-NEXT:          "tokenTypes": [
 # CHECK-NEXT:            "variable",
 # CHECK:               ]

clang-tools-extra/clangd/test/semantic-tokens.test

 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "data": [
 #                  First line, char 5, variable, declaration+globalScope
 # CHECK-NEXT:      0,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      16387
+# CHECK-NEXT:      32771
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "1"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{
   "textDocument": {"uri":"test:///foo.cpp","version":2},
   "contentChanges":[{"text":"int x = 2;\nint y = 3;"}]
 }}

 # CHECK-NEXT:    "edits": [
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "data": [
 #                      Next line, char 5, variable, declaration+globalScope
 # CHECK-NEXT:          1,
 # CHECK-NEXT:          4,
 # CHECK-NEXT:          1,
 # CHECK-NEXT:          0,
-# CHECK-NEXT:          16387
+# CHECK-NEXT:          32771
 # CHECK-NEXT:        ],
 #                    Inserted at position 1
 # CHECK-NEXT:        "deleteCount": 0,
 # CHECK-NEXT:        "start": 5
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "2"
 # CHECK-NEXT:  }
 ---
 # Incremental token request with incorrect baseline => full tokens list.
 {"jsonrpc":"2.0","id":2,"method":"textDocument/semanticTokens/full/delta","params":{
   "textDocument": {"uri":"test:///foo.cpp"},
   "previousResultId": "bogus"
 }}
 # CHECK:       "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "data": [
 # CHECK-NEXT:      0,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      16387,
+# CHECK-NEXT:      32771,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      4,
 # CHECK-NEXT:      1,
 # CHECK-NEXT:      0,
-# CHECK-NEXT:      16387
+# CHECK-NEXT:      32771
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "resultId": "3"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

)cpp",
       }
       template<typename $TemplateParameter_def[[T]]>
       struct $Class_def[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
         typename $TemplateParameter[[T]]::$Type_dependentName[[A]]* $Field_decl[[D]];
       };
       $Namespace[[abc]]::$Class[[A]]<int> $Variable_def[[AA]];
       typedef $Namespace[[abc]]::$Class[[A]]<int> $Class_decl[[AAA]];
       struct $Class_def[[B]] {
-        $Class_decl[[B]]();
-        ~$Class[[B]](); // FIXME: inconsistent with constructor
+        $Class_decl_constrDestr[[B]]();
+        ~$Class_decl_constrDestr[[B]]();
         void operator<<($Class[[B]]);
         $Class[[AAA]] $Field_decl[[AA]];
       };
-      $Class[[B]]::$Class_def[[B]]() {}
-      $Class[[B]]::~$Class[[B]]() {} // FIXME: inconsistent with constructor
+      $Class[[B]]::$Class_def_constrDestr[[B]]() {}
+      $Class[[B]]::~$Class_def_constrDestr[[B]]() {}
       void $Function_def[[f]] () {
         $Class[[B]] $LocalVariable_def[[BB]] = $Class[[B]]();
-        $LocalVariable[[BB]].~$Class[[B]]();
+        $LocalVariable[[BB]].~$Class_constrDestr[[B]]();
         $Class[[B]]();
       }
     )cpp",
       R"cpp(
       enum class $Enum_decl[[E]] {
         $EnumConstant_decl_readonly[[A]],
         $EnumConstant_decl_readonly[[B]],
       };
)cpp",
         $EnumConstant_decl_readonly[[EC]],
       };
       class $Class_def[[Foo]] {};
       class $Class_def[[Bar]] {
       public:
         $Class[[Foo]] $Field_decl[[Fo]];
         $Enum[[En]] $Field_decl[[E]];
         int $Field_decl[[I]];
-        $Class_def[[Bar]] ($Class[[Foo]] $Parameter_def[[F]],
+        $Class_def_constrDestr[[Bar]] ($Class[[Foo]] $Parameter_def[[F]],
                 $Enum[[En]] $Parameter_def[[E]])
         : $Field[[Fo]] ($Parameter[[F]]), $Field[[E]] ($Parameter[[E]]),
           $Field[[I]] (123) {}
       };
       class $Class_def[[Bar2]] : public $Class[[Bar]] {
-        $Class_def[[Bar2]]() : $Class[[Bar]]($Class[[Foo]](), $EnumConstant_readonly[[EC]]) {}
+        $Class_def_constrDestr[[Bar2]]() : $Class[[Bar]]($Class[[Foo]](), $EnumConstant_readonly[[EC]]) {}
       };
     )cpp",
       R"cpp(
       enum $Enum_decl[[E]] {
         $EnumConstant_decl_readonly[[E]],
       };
       class $Class_def[[Foo]] {};
       $Enum_deduced[[auto]] $Variable_def[[AE]] = $Enum[[E]]::$EnumConstant_readonly[[E]];
)cpp",
             void operator()(int, int, const int &);
             int &operator[](int &);
             int operator[](int) const;
         };
         struct $Class_def[[ClassWithStaticMember]] {
             static inline int $StaticField_def_static[[j]] = 0;
         };
         struct $Class_def[[ClassWithRefMembers]] {
-          $Class_def[[ClassWithRefMembers]](int $Parameter_def[[i]])
+          $Class_def_constrDestr[[ClassWithRefMembers]](int $Parameter_def[[i]])
             : $Field[[i1]]($Parameter[[i]]),
               $Field_readonly[[i2]]($Parameter[[i]]),
               $Field[[i3]]($Parameter_usedAsMutableReference[[i]]),
               $Field_readonly[[i4]]($Class[[ClassWithStaticMember]]::$StaticField_static[[j]]),
               $Field[[i5]]($Class[[ClassWithStaticMember]]::$StaticField_static_usedAsMutableReference[[j]])
           {}
           int $Field_decl[[i1]];
           const int &$Field_decl_readonly[[i2]];
)cpp",
           const $Class[[ClassWithOp]] $LocalVariable_def_readonly[[c2]];
           $LocalVariable[[c]]($LocalVariable[[val]]);
           $LocalVariable[[c]](0, $LocalVariable_usedAsMutableReference[[val]]);
           $LocalVariable[[c]](0, 0, $LocalVariable[[val]]);
           $LocalVariable[[c]][$LocalVariable_usedAsMutableReference[[val]]];
           $LocalVariable_readonly[[c2]][$LocalVariable[[val]]];
         }
         struct $Class_def[[S]] {
-          $Class_def[[S]](int&) {
+          $Class_def_constrDestr[[S]](int&) {
             $Class[[S]] $LocalVariable_def[[s1]]($Field_usedAsMutableReference[[field]]);
             $Class[[S]] $LocalVariable_def[[s2]]($LocalVariable[[s1]].$Field_usedAsMutableReference[[field]]);
 
             $Class[[S]] $LocalVariable_def[[s3]]($StaticField_static_usedAsMutableReference[[staticField]]);
             $Class[[S]] $LocalVariable_def[[s4]]($Class[[S]]::$StaticField_static_usedAsMutableReference[[staticField]]);
           }
           int $Field_decl[[field]];
           static int $StaticField_decl_static[[staticField]];