[clangd] Added highlighting for class and enum types

[jvikstrom]

Summary

Added highlighting for class and enum types using VisitTypeLoc. Ignoring namespace qualifiers as for now.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

68 ↗

(On Diff #208208)

We are only interested in TagDecl, maybe use the VisitTagLoc callback, so that you can get rid of the filtering code above.

72 ↗

(On Diff #208208)

nit: auto => const auto *, we usually spell out the pointer type explicitly.

73 ↗

(On Diff #208208)

Here is the case:

class Foo {
   ~Foo
 // ^~~ we get a TypeLoc whose TagDecl is a cxxRecordDecl.
}

not sure this is expected in clang AST, but it is unreasonable in highlighting context -- ideally, we want to highlight ~Foo as a destructor (we may encounter a tricky case, like ~ /*comment*/ Foo(), but I assume this is rarce, should be fine), @sammccall, @ilya-biryukov, thoughts?

99 ↗

(On Diff #208208)

nit: clang-format

214 ↗

(On Diff #208208)

entity.name.type.class.cpp?

clang-tools-extra/clangd/SemanticHighlighting.h

29 ↗

(On Diff #208208)

Type is general, can match different types, I think we want distinguish different types (class, enum, etc).

Since this patch is highlighting class, I think the name should be Class?

[jvikstrom]

Type -> Class
Also added more tests.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

68 ↗

(On Diff #208208)

With just VisitTagLoc it does not catch this case:

namespace abc {
  template<typename T>
  struct $Type[[A]] {};
}
abc::$Type[[A]]<int> $Variable[[AA]];

I guess I could add a bunch of `Visit*TypeLoc``` methods but I can't seem to find the correct Visit method for the case above...

[sammccall]

clang-tools-extra/clangd/SemanticHighlighting.cpp

61 ↗

(On Diff #208354)

should you highlight DependentNameTypeLoc::getNameLoc()? (As some generic "type" rather than "class" as we can't resolve the name) Or is the unqualified name highlighted by traversing some other node?

62 ↗

(On Diff #208354)

Nit: consider splitting this || into two statements so you can comment each.

The comments should ideally say what drives the actual behavior in this situation, e.g. "For elaborated types, the underlying type will be highlighted when visiting the inner typeloc"

70 ↗

(On Diff #208354)

The patch description says "non-builtin types".

It's fine to just handle tag types (struct class enum union) in this patch, but there are other cases you may want to handle later (e.g. typedefs/using).

Random thought for the future: you could highlight auto differently based on the actual underlying type (e.g. class vs enum vs pointer...)

73 ↗

(On Diff #208354)

This code is doing something really weird: you've seen a mention of a class, now you're checking to see if the destructor's declaration encloses the mention so you can not highlight it.

First, I'm not convinced highlighting it is bad or worth any complexity to avoid, happy to discuss further.

Second, this will get a bunch of cases wrong:

• a call to the destructor (a->~Foo()) will still be highligthed
• the out-of-line definition of the destructor (Foo::~Foo() { ... }) will still be highlighted
• a (separate) mention of the class name within the body of the destructor (defined inline) will not be highlighted

100 ↗

(On Diff #208354)

consider implementing enum in this patch too: it's a TagDecl so you've already done almost all the work, just need to add another if here

68 ↗

(On Diff #208208)

Your logic seems sound to me, but please add a comment like "This covers classes, class templates, etc"

99 ↗

(On Diff #208208)

RecordDecl, unless you're trying to distinguish C++ from C for some reason

[jvikstrom]

Addressed comments.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

73 ↗

(On Diff #208208)

Do we want to highlight the entire "~Foo" or only the ~"Foo" for destructors?

[jvikstrom]

Clang formatted.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

73 ↗

(On Diff #208208)

based on our discussion, we'd just highlight ~"Foo" as a class type for now.

[hokein]

could you please also update the patch description? "non-builtin" types are not precise, this patch only highlights the class and enum types.

clang-tools-extra/clangd/SemanticHighlighting.cpp

71 ↗

(On Diff #208416)

nit: you could simplify the code like

if (const auto* D = TL.getXXX)
  addToken(D->getLocation(), D);
return true;

81 ↗

(On Diff #208416)

nit: move this around if (isa<RecordDecl>(D)) { since they are related to Class, and we should have a comment describing the highlighting behavior of class, constructor, and destructor.

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

87 ↗

(On Diff #208416)

could we split the enum case into a separate testcase?

Thinking it further, we may want to highlight the enumerator as well.

101 ↗

(On Diff #208416)

this test case can be merged into the above case (we could add constructor/destructor to class B there)

[sammccall]

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

87 ↗

(On Diff #208416)

(please don't add enum value highlighting in this patch, though)

[jvikstrom]

Addressed comments.

[jvikstrom]

Changed commit message.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

81 ↗

(On Diff #208416)

I don't really know what you mean with this comment after the move RecordDecl around (or rather where to put the comment and what to put in it)
I wrote a comment but don't know if that's really helpful

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #208622)

I don't understand this comment -- when visiting the constructor AST, we get a TagTypeLoc, and its underlying Decl is a CXXConstructorDecl

73 ↗

(On Diff #208622)

From the comment of getTypePtr():

This function requires that the type not be NULL. If the type might be NULL, use the (slightly less efficient) \c getTypePtrOrNull().`

I think we should use getTypePtrOrNull if you want to check the type ptr is null.

if (const auto* Type = TL.getTypePtrOrNull()) 
  if (if (const auto* D = Type.getTagDecl())
    addToken(D->getLocation(), D);

81 ↗

(On Diff #208416)

I don't really know what you mean with this comment after the move RecordDecl around (or rather where to put the comment and what to put in it)

sorry for the confusion, I meant we group "class" cases together just as what your code does now (there are going to be many of kinds, it would be clear if we break them into group and sort them).

I wrote a comment but don't know if that's really helpful

The comment should comment something not obvious but important in the code, so here we don't have a if (isa<CXXDestructorDecl>(D))) case to handle the destructor, but the code does highlight destructors -- for destructors, we get a TagTypeLoc whose underlying decl is CXXRecordDecl in VisitTypeLoc), I think it deserves a comment here.

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

57 ↗

(On Diff #208622)

just realize this case, this seems weird to me, we are highlighting a keyword struct as a class type.

[jvikstrom]

No longer highlighting anonymous structs as types.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #208622)

So the Constructor TypeLoc does not have a TagTypeDecl and is not a TagTypeLoc. When we get the TypePtr of the constructor it's a "FunctionProtoType" and there is no way to distinguish it from other functions. Therefore we need to get the constructor decls as NamedDecls..

The comment was written badly though. This version should be better now I hope.

[jvikstrom]

Changed comment in addToken.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

52 ↗

(On Diff #208671)

if you move this to addToken (and change function parameter type to NamedDecl), then you don't need the check on Line 79.

73 ↗

(On Diff #208671)

nit: remove the debug dump.

77 ↗

(On Diff #208671)

Again, you can save one cost of TL.getTypePtr().

if (const auto* TypePtr = TL.getTypePtr())
  if (const auto* TD = TypePtr->getAsTagDecl())

87 ↗

(On Diff #208671)

how about?

We highlight class decls, constructor decls and destructor decls as `Class` type. The destructor decls are handled in `VisitTypeLoc` (we will visit a TypeLoc where the underlying Type is a CXXRecordDecl).

38 ↗

(On Diff #208622)

ah, I see, that make senses, thanks for the explanation.

[jvikstrom]

Addressed comments.

[sammccall]

(Thanks for addressing my comments earlier. I don't have more concerns, but haven't been following the last revisions in detail so will leave approval to Haojian)

[nridge]

@jvikstrom out of curiosity, are you testing these patches against a client-side implementation of semantic highlighting? If so, which one?

[jvikstrom]

In D64257#1577347, @nridge wrote:

@jvikstrom out of curiosity, are you testing these patches against a client-side implementation of semantic highlighting? If so, which one?

Yes, I am testing against Theia. Just modified the Theia c++ language client to include the semantic highlighting service.

[hokein]

thanks, looks good.

clang-tools-extra/clangd/SemanticHighlighting.cpp

83 ↗

(On Diff #208708)

this comment doesn't belong to the if statement below, should move to Line 87.

[hokein]

In D64257#1577347, @nridge wrote:

are you testing these patches against a client-side implementation of semantic highlighting? If so, which one?

Looks like Theia is the only LSP client supporting the semantic highlighting proposal, we use it to verify the behavior.

And we also have a hidden tweak (run clangd with -hidden-feature) to annotate all the highlighting tokens, we use it to verify whether the token is being highlighted in a correct TextMate scope (it is triggered via the codeAction in VSCode).

clang-tools-extra/trunk/clangd/SemanticHighlighting.h

 #include "Protocol.h"
 
 namespace clang {
 namespace clangd {
 
 enum class HighlightingKind {
   Variable = 0,
   Function,
+  Class,
+  Enum,
 
   NumKinds,
 };
 
 // Contains all information needed for the highlighting a token.
 struct HighlightingToken {
   HighlightingKind Kind;
   Range R;

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

 //===--- SemanticHighlighting.cpp - ------------------------- ---*- 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 "SemanticHighlighting.h"
 #include "Logger.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Collects all semantic tokens in an ASTContext.
 class HighlightingTokenCollector
     : public RecursiveASTVisitor<HighlightingTokenCollector> {
   std::vector<HighlightingToken> Tokens;
   ASTContext &Ctx;
   const SourceManager &SM;
 
 public:
   HighlightingTokenCollector(ParsedAST &AST)
       : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {}
 
   std::vector<HighlightingToken> collectTokens() {
     Tokens.clear();
     TraverseAST(Ctx);
     return Tokens;
   }
 
   bool VisitNamedDecl(NamedDecl *ND) {
-    // FIXME: (De)Constructors/operator need to be highlighted some other way.
-    if (ND->getDeclName().getNameKind() != DeclarationName::Identifier)
+    // Constructors' TypeLoc has a TypePtr that is a FunctionProtoType. It has
+    // no tag decl and therefore constructors must be gotten as NamedDecls
+    // instead.
+    if (ND->getDeclName().getNameKind() ==
+        DeclarationName::CXXConstructorName) {
+      addToken(ND->getLocation(), ND);
       return true;
+    }
 
-    if (ND->getDeclName().isEmpty())
-      // Don't add symbols that don't have any length.
+    if (ND->getDeclName().getNameKind() != DeclarationName::Identifier)
       return true;
+
     addToken(ND->getLocation(), ND);
     return true;
   }
 
   bool VisitDeclRefExpr(DeclRefExpr *Ref) {
     if (Ref->getNameInfo().getName().getNameKind() !=
         DeclarationName::Identifier)
       // Only want to highlight identifiers.
       return true;
 
     addToken(Ref->getLocation(), Ref->getDecl());
     return true;
   }
 
+  bool VisitTypeLoc(TypeLoc &TL) {
+    // This check is for not getting two entries when there are anonymous
+    // structs. It also makes us not highlight namespace qualifiers. For
+    // elaborated types the actual type is highlighted as an inner TypeLoc.
+    if (TL.getTypeLocClass() == TypeLoc::TypeLocClass::Elaborated)
+      return true;
+
+    if (const Type *TP = TL.getTypePtr())
+      if (const TagDecl *TD = TP->getAsTagDecl())
+          addToken(TL.getBeginLoc(), TD);
+    return true;
+  }
+
 private:
-  void addToken(SourceLocation Loc, const Decl *D) {
+  void addToken(SourceLocation Loc, const NamedDecl *D) {
+    if (D->getDeclName().isIdentifier() && D->getName().empty())
+      // Don't add symbols that don't have any length.
+      return;
+    // We highlight class decls, constructor decls and destructor decls as
+    // `Class` type. The destructor decls are handled in `VisitTypeLoc` (we will
+    // visit a TypeLoc where the underlying Type is a CXXRecordDecl).
+    if (isa<RecordDecl>(D)) {
+      addToken(Loc, HighlightingKind::Class);
+      return;
+    }
+    if (isa<CXXConstructorDecl>(D)) {
+      addToken(Loc, HighlightingKind::Class);
+      return;
+    }
+    if (isa<EnumDecl>(D)) {
+      addToken(Loc, HighlightingKind::Enum);
+      return;
+    }
     if (isa<VarDecl>(D)) {
       addToken(Loc, HighlightingKind::Variable);
       return;
     }
     if (isa<FunctionDecl>(D)) {
       addToken(Loc, HighlightingKind::Function);
       return;
     }

 
 llvm::StringRef toTextMateScope(HighlightingKind Kind) {
   // FIXME: Add scopes for C and Objective C.
   switch (Kind) {
   case HighlightingKind::Function:
     return "entity.name.function.cpp";
   case HighlightingKind::Variable:
     return "variable.cpp";
+  case HighlightingKind::Class:
+    return "entity.name.type.class.cpp";
+  case HighlightingKind::Enum:
+    return "entity.name.type.enum.cpp";
   case HighlightingKind::NumKinds:
     llvm_unreachable("must not pass NumKinds to the function");
   }
   llvm_unreachable("unhandled HighlightingKind");
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/test/semantic-highlighting.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
 ---
 #      CHECK:  "id": 0,
 # CHECK:      "semanticHighlighting": {
 # CHECK-NEXT:        "scopes": [
 # CHECK-NEXT:          [
 # CHECK-NEXT:            "variable.cpp"
 # CHECK-NEXT:          ],
 # CHECK-NEXT:          [
 # CHECK-NEXT:            "entity.name.function.cpp"
 # CHECK-NEXT:          ]
+# CHECK-NEXT:          [
+# CHECK-NEXT:            "entity.name.type.class.cpp"
+# CHECK-NEXT:          ],
+# CHECK-NEXT:          [
+# CHECK-NEXT:            "entity.name.type.enum.cpp"
+# CHECK-NEXT:          ]
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int x = 2;"}}}
 #      CHECK:  "method": "textDocument/semanticHighlighting",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "lines": [
 # CHECK-NEXT:      {

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

makeHighlightingTokens(llvm::ArrayRef<Range> Ranges, HighlightingKind Kind) {
   return Tokens;
 }
 
 void checkHighlightings(llvm::StringRef Code) {
   Annotations Test(Code);
   auto AST = TestTU::withCode(Test.code()).build();
   static const std::map<HighlightingKind, std::string> KindToString{
       {HighlightingKind::Variable, "Variable"},
-      {HighlightingKind::Function, "Function"}};
+      {HighlightingKind::Function, "Function"},
+      {HighlightingKind::Class, "Class"},
+      {HighlightingKind::Enum, "Enum"}};
   std::vector<HighlightingToken> ExpectedTokens;
   for (const auto &KindString : KindToString) {
-    std::vector<HighlightingToken> Toks =
-        makeHighlightingTokens(Test.ranges(KindString.second), KindString.first);
+    std::vector<HighlightingToken> Toks = makeHighlightingTokens(
+        Test.ranges(KindString.second), KindString.first);
     ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
     R"cpp(
-      struct AS {
+      struct $Class[[AS]] {
         double SomeMember;
       };
       struct {
       } $Variable[[S]];
-      void $Function[[foo]](int $Variable[[A]]) {
+      void $Function[[foo]](int $Variable[[A]], $Class[[AS]] $Variable[[As]]) {
         auto $Variable[[VeryLongVariableName]] = 12312;
-        AS     $Variable[[AA]];
+        $Class[[AS]]     $Variable[[AA]];
         auto $Variable[[L]] = $Variable[[AA]].SomeMember + $Variable[[A]];
         auto $Variable[[FN]] = [ $Variable[[AA]]](int $Variable[[A]]) -> void {};
         $Variable[[FN]](12312);
       }
     )cpp",
     R"cpp(
       void $Function[[foo]](int);
       void $Function[[Gah]]();
       void $Function[[foo]]() {
         auto $Variable[[Bou]] = $Function[[Gah]];
       }
+      struct $Class[[A]] {
+        void $Function[[abc]]();
+      };
     )cpp",
     R"cpp(
-      struct A {
-        A();
-        ~A();
-        void $Function[[abc]]();
-        void operator<<(int);
+      namespace abc {
+        template<typename T>
+        struct $Class[[A]] {
+          T t;
+        };
+      }
+      template<typename T>
+      struct $Class[[C]] : abc::A<T> {
+        typename T::A* D;
+      };
+      abc::$Class[[A]]<int> $Variable[[AA]];
+      typedef abc::$Class[[A]]<int> AAA;
+      struct $Class[[B]] {
+        $Class[[B]]();
+        ~$Class[[B]]();
+        void operator<<($Class[[B]]);
+        $Class[[AAA]] AA;
+      };
+      $Class[[B]]::$Class[[B]]() {}
+      $Class[[B]]::~$Class[[B]]() {}
+      void $Function[[f]] () {
+        $Class[[B]] $Variable[[BB]] = $Class[[B]]();
+        $Variable[[BB]].~$Class[[B]]();
+        $Class[[B]]();
+      }
+    )cpp",
+    R"cpp(
+      enum class $Enum[[E]] {};
+      enum $Enum[[EE]] {};
+      struct $Class[[A]] {
+        $Enum[[E]] EEE;
+        $Enum[[EE]] EEEE;
       };
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 }
 
 TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {