[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

75

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

79

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

80

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?

104

nit: clang-format

215

entity.name.type.class.cpp?

clang-tools-extra/clangd/SemanticHighlighting.h

29

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

75

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

67

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?

68

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"

75

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

76

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

79

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

104

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

104

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

[jvikstrom]

Addressed comments.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

80

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

[jvikstrom]

Clang formatted.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

80

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

73

nit: you could simplify the code like

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

83

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

89

could we split the enum case into a separate testcase?

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

103

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

89

(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

83

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–45

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

73

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

83

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

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–45

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

38–45

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

49

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

70

nit: remove the debug dump.

74

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

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

83

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

[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

85

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/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/clangd/SemanticHighlighting.cpp

public:
 
   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.
+    // Constructors have a TypePtr TagDecl that is a Function, therefore we need
+    // to get constructors as a NamedDecl instead.
+    if (ND->getDeclName().getNameKind() ==
+        DeclarationName::CXXConstructorName) {
+      addToken(ND->getLocation(), ND);
+      return true;
+    }
+
     if (ND->getDeclName().getNameKind() != DeclarationName::Identifier)
       return true;
 
     if (ND->getDeclName().isEmpty())
       // Don't add symbols that don't have any length.
       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 (!TL.getTypePtr())
+      return true;
+
+    if (TagDecl *D = TL.getTypePtr()->getAsTagDecl())
+      addToken(TL.getBeginLoc(), D);
+    return true;
+  }
+
 private:
   void addToken(SourceLocation Loc, const Decl *D) {
+    // Classes, Constructors and Destructors all get highlighted as a "Class".
+    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;
     }
   }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
     if (Loc.isMacroID())
       // FIXME: skip tokens inside macros for now.
       return;
 
     auto R = getTokenRange(SM, Ctx.getLangOpts(), Loc);
     if (!R) {

 
 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/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/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 {
+      $Class[[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) {