[clangd] Added highlighting for variable references (declrefs)

[jvikstrom]

Summary

Added highlighting for variable references using VisitDeclRefExpr.

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

46 ↗

(On Diff #208006)

The DeclRefExpr is a very general expression, which can reference a variable, a function, an enum, etc. I think we want to distinguish with them (rather than putting all into Variable type).

And we are missing large majority of entities now, I think we could start collecting more entities (class, method, enum, etc).

[sammccall]

clang-tools-extra/clangd/SemanticHighlighting.cpp

46 ↗

(On Diff #208006)

IMO the way to ensure consistency is to make the highlight a function of the decl, regardless of whether you found it in the decl itself, a declrefexpr, etc.

so something like:

// (overloaded)
addToken(SourceLocation Loc, NamedDecl *D) {
  if (isa<VarDecl>(D))
    return addToken(Loc, HighlightingKind::Variable);
  // ... etc
}

bool VisitDeclRefExpr(DeclRefExpr *Ref) { 
  // bail out for operators here (if it's only *usages* you don't want to highlight)
  addToken(Ref->getLocation(), Ref->getDecl());
}

bool VisitNamedDecl(NamedDecl *ND) {
  addToken(ND->getLocation(), ND);
}

[jvikstrom]

Added overload for addToken and added more code to the test cases.

[jvikstrom]

Removed VisitVarDecl and VisitFuncDecl in favor of VisitNamedDecl.

[jvikstrom]

Removed debug prints from test.

[sammccall]

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #208030)

I think you might want to bail out (both here and in VisitDeclRefExpr) if the name kind isn't identifier.

Reason is you're only coloring the token at location, and most of the other name kinds can span multiple tokens or otherwise need special consideration.

61 ↗

(On Diff #208030)

note that methods, constructors, and destructors inherit from functiondecl, so if you want to exclude/distinguish those, order matters here

[jvikstrom]

Added testcae. Added another bailout from VisitNamedDecl.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #208030)

I must have missed the Identifier NameKind because I was first-hand looking for something like that.
Thanks.

Are you aware of any testcase I could add for this by the way?

61 ↗

(On Diff #208030)

I'm aware of that, but thanks for the heads up. Although should I add it in a comment somewhere in the method? Also added an additional testcase for classes and FIXMEs to the skip if statement in VisitNamedDecl.

[sammccall]

clang-tools-extra/clangd/SemanticHighlighting.cpp

38 ↗

(On Diff #208030)

Such a testcase would ensure you're not coloring any part of struct F { ~F(); } as a method, or operator << etc.

61 ↗

(On Diff #208030)

I don't think it needs a comment, especially if you're not actually highlighting them (because they have weird DeclarationNames)

FIXMEs to the skip if statement in VisitNamedDecl

I'm not actually sure there's anything to fix here - it's a bit hard to talk about constructor/destructor highlighting as distinct from type name highlighting in C++. If you want them highlighted as classes, then that should just start working when you start handling TypeLocs.

[sammccall]

(LG from my side)

[jvikstrom]

Added additional testcase.

[hokein]

the implementation looks good, a few comments on the test.

clang-tools-extra/clangd/SemanticHighlighting.cpp

61 ↗

(On Diff #208030)

I think constructor/destructor can be categorized in the function group, like entity.name.function.constructor, entity.name.function.destructor

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

69 ↗

(On Diff #208123)

nit: even for the test code, could we make the code style consistent (like follow the LLVM code style) here?

76 ↗

(On Diff #208123)

now we have 4 test cases, the purpose of each testcase is unclear to me (some of them are testing duplicated things), could we narrow the testcase so the each testcase just test one particular thing (e.g. one test for Variable and its references; one test for Function and its references;)?

I think the testcase here is to verify we don't highlighting the constructor/destructor, and operator, just

R"cpp(
struct Foo {
  Foo();
  ~Foo();
  void $Function[[foo]]();
}
)cpp"

[jvikstrom]

Made tests more readable.

[jvikstrom]

Separated into three testcases.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlighting.cpp

61 ↗

(On Diff #208030)

I'll have a look at constructors/destructors at the same time as I look at types

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

69 ↗

(On Diff #208123)

I think this should be consistent with LLVM code style.

[hokein]

thanks, looks good.

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

52 ↗

(On Diff #208143)

nit: I'd add 2-space to the code, like:

R"cpp(
    struct AS {}
    ...
)cpp,

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

HighlightingTokenCollector(ParsedAST &AST)
       : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {}
 
   std::vector<HighlightingToken> collectTokens() {
     Tokens.clear();
     TraverseAST(Ctx);
     return Tokens;
   }
 
-  bool VisitVarDecl(VarDecl *Var) {
-    addToken(Var, HighlightingKind::Variable);
+  bool VisitNamedDecl(NamedDecl *ND) {
+    // FIXME: (De)Constructors/operator need to be highlighted some other way.
+    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 VisitFunctionDecl(FunctionDecl *Func) {
-    addToken(Func, HighlightingKind::Function);
+
+  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;
   }
 
 private:
-  void addToken(const NamedDecl *D, HighlightingKind Kind) {
-    if (D->getLocation().isMacroID())
-      // FIXME: skip tokens inside macros for now.
+  void addToken(SourceLocation Loc, const Decl *D) {
+    if (isa<VarDecl>(D)) {
+      addToken(Loc, HighlightingKind::Variable);
+      return;
+    }
+    if (isa<FunctionDecl>(D)) {
+      addToken(Loc, HighlightingKind::Function);
       return;
+    }
+  }
 
-    if (D->getDeclName().isEmpty())
-      // Don't add symbols that don't have any length.
+  void addToken(SourceLocation Loc, HighlightingKind Kind) {
+    if (Loc.isMacroID())
+      // FIXME: skip tokens inside macros for now.
       return;
 
-    auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+    auto R = getTokenRange(SM, Ctx.getLangOpts(), Loc);
     if (!R) {
       // R should always have a value, if it doesn't something is very wrong.
       elog("Tried to add semantic token with an invalid range");
       return;
     }
 
     Tokens.push_back({Kind, R.getValue()});
   }

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

void checkHighlightings(llvm::StringRef Code) {
   }
 
   auto ActualTokens = getSemanticHighlightings(AST);
   EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
-      R"cpp(
-    struct A {
+      struct AS {
         double SomeMember;
       };
       struct {
-    }   $Variable[[HStruct]];
-    void $Function[[foo]](int $Variable[[a]]) {
+      } $Variable[[S]];
+      void $Function[[foo]](int $Variable[[A]]) {
         auto $Variable[[VeryLongVariableName]] = 12312;
-      A     $Variable[[aa]];
+        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]];
+      }
+    )cpp",
+    R"cpp(
+      struct A {
+        A();
+        ~A();
+        void $Function[[abc]]();
+        void operator<<(int);
+      };
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 }
 
 TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
   class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
   public: