[clangd] Added functionality for getting semantic highlights for variable and function declarations

[jvikstrom]

Summary

Adds functionality for getting semantic highlights

[jvikstrom]

Adds CMakeLists changes

[jvikstrom]

Renamed SemanticSymbol to SemanticToken

[Eugene.Zelenko]

clang-tools-extra/clangd/SemanticHighlight.cpp

2

Header is missing.

33

Please don't use auto if type is not spelled explicitly.

35

Please don't use auto if type is not spelled explicitly.

100

May be std::array is better way to do this? Same below.

136

Please don't use auto if type is not spelled explicitly.

clang-tools-extra/clangd/SemanticHighlight.h

3

Please merge with previous line. If it doesn't fit, remove some - and =.

15

Please separate with empty line.

36

Please use = default;

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

3

Please merge with previous line. If it doesn't fit, remove some - and =.

[hokein]

Thanks, I think we can simplify the interface further, see my comments inline.

The boundary of this patch seems unclear, currently it contains C++ APIs, and some implementations for semantic highlighting LSP. I think we should merely focus on the C++ APIs in this patch.

clang-tools-extra/clangd/SemanticHighlight.cpp

33

Looks like the code doesn't handle the case where D is expanded from a macro (Loc is a macro location).

130

This is Textmate-specific, I think we should lift it to a new File (TextMate.cpp). We can do it in a separate patch.

I'd use a map to explicitly express the relationship between SemanticScope and TextMate scope, the current implementation is not obvious.

{SemanticScope::Function, "entity.name.function"},
{SemanticScope::Variable, "variable.other"},

134

We should only collect the highlights from the main AST, I think we should set traversal scope only to localTopDecls (use ParsedAST as parameter would help here)

ASTCtx.setTraversalScope(MainAST.getLocalTopLevelDecls());

clang-tools-extra/clangd/SemanticHighlight.h

15

nit: the name of header guard doesn't reflect the file name.

22

please clean-up your #includes:

• Headers.h is unused
• we use RecursiveASTVisitor, Lexer in the .cpp file, we should include these headers in the .cpp file rather the .h file.

27

The comment seems not very related to this structure.

We'd use this enum class to find a corresponding TextMate scope in the future, but this is implementation detail, the comment here should mention the high-level things about this structure.

29

TextMate is using the term scope for different tokens, but the scope has different meaning in C++ world (usually the namespace "ns::" of a symbol). I'd avoid using this word in the C++ interface.

Just SemanticHighlightingKind?

30

not exactly sure whether we should distinguish these cases at the moment (function declaration vs function calls, variable declaration vs variable references).

I think we could just use Variable, Function at the beginning, and add more kinds afterwards.

40

instead of using start position + length, I'd use Range R;

62

I'd drop the AST word, how about naming it "getSemanticHighlights"?

I think we can just return "vector<HighlightingToken>", and we could we could group them by line when returning a LSP result.

The function parameter should be ParsedAST.

[jvikstrom]

Removed LSP specific stuff. Changed a bunch of types and names. Added fixme for macro expansion.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlight.cpp

130

Will do this in CL when adding C++ ClangdServer api calls

[hokein]

clang-tools-extra/clangd/SemanticHighlight.cpp

40

We can check the name of NamedDecl is empty rather than detecting the length.

Could you add a test for this?

76

let's move the above lines into SemanticSymbolASTCollector, we can define a new method "collectTokens()".

clang-tools-extra/clangd/SemanticHighlight.h

10

this comment doesn't provide much information IMO, let's remove it.

[hokein]

clang-tools-extra/clangd/SemanticHighlight.cpp

12

The comment is out of date now.

clang-tools-extra/clangd/SemanticHighlight.h

30

We can get rid of the default constructor and the constructor below.

39

The comment is out of date.

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

32

Thinking more about the test, I think we could improve the test to make it more scalable and less verbose code.

How about associating the range name (e.g. $vlan) in the annotation with the name in SemanticHighlightKind? For example, in this test case, we could use annotation like

void $Function[foo](int $Variable[[a]]) {
   auto $Variable[[A]] = 222;
}

We have a common function like checkHighlights which verifies that all Functions and Variable ranges.

void checkHighlights(... annotation) {
  auto ActualHighlights = getHighlights(annotation.code());
  EXPECT_THAT(annotations.ranges('Function'), unorderedElemenetsAre(filter(ActualHighlights, SemanticHighlightKind::Function));
  EXPECT_THAT(annotations.ranges('Variable'), unorderedElemenetsAre(filter(ActualHighlights, SemanticHighlightKind::Variable)) 
}

[Eugene.Zelenko]

clang-tools-extra/clangd/SemanticHighlight.cpp

2

License header was not added despite comment was marked as done.

clang-tools-extra/clangd/SemanticHighlight.h

14

Separator line was not added despite comment was marked as done.

[jvikstrom]

Fixed tests and edge case with function declarations without names for parameters.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlight.cpp

76

Should I expose the entire class or keep the getSemanticHighlights function? (I'm just thinking that RecursiveASTVisitor contains a lot of public functions which makes it not super obvious which function to call to get semantic highlight things when looking at autocomplete)

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlight.cpp

76

Actually I can just do private inheritance and declare RecursiveASTVisitor<SemanticSymbolASTCollector> to be a friend.

[jvikstrom]

Made SemanticTokenCollector visible and removed getSemanticHighlights function.

[jvikstrom]

Added header and empty line

[hokein]

clang-tools-extra/clangd/SemanticHighlight.cpp

44

we can reuse the getTokenRange() function (in SourceCode.h), you need to sync your branch to master.

47

I'd move this line at the beginning of the method.

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

58

checkHighlights(R"cpp(void $Function[[Foo]]))cpp");

checkHighlights(...) {
   ...
   ExpectedTokens = annotation.ranges(toString(SemanticHighlightKind::Variable));
   ExpectedTokens = annotation.ranges(toString(SemanticHighlightKind::Function));
   EXPECT_THAT(ExpectedTokens, UnorderedElementsAreArray(ActualTokens));
}

[jvikstrom]

Changed tests

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

58

Had to add some extra logic to merge the different token types

[hokein]

the test looks better now, another round of reviews, most are nits.

clang-tools-extra/clangd/SemanticHighlight.cpp

20

nit: public.

21

do we need friend declaration now?

29

I'd move this line to collectTokens as they are related.

As discussed, setTraversalScope doesn't guarantee that we wouldnot visit Decl* outside of the main file (some decls are still reachable), so we may get tokens outside of the main file. If you don't address it in this patch, that's fine, leave a FIXME here.

33

add assume(Tokens.empty())?, if we call this method twice, we will accumulate the Tokens.

46

addSymbol => addToken;
NamedDecl *D => const NamedDecl* D

48

nit: FIXME: skip tokens inside macros for now.

52

use getDeclName().isEmpty().

57

How about pulling out a function llvm::Optional<SemanticToken> makeSemanticToken(..)?

60

this would crash clangd if we meet this case, I think we could just emit a log.

clang-tools-extra/clangd/SemanticHighlight.h

15

nit: remove the unused include.

20

Maybe just

enum SemanticHighlightKind {
   Variable,
   Function
};

39

add a new line.

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

12

not used #include

16

is this #include used?

25

we are passing a copy here, use llvm::ArrayRef.

28

nit: ++I

36

nit: llvm::StringRef

40

static const

44

nit: const auto&

54

nit: update the test name.

66

I think we can group these two testcases like

const char* TestCases[] =  {
   {},
   {}, 
};

for (...) {
}

67

maybe use another obvious case:

struct {
} name;

[jvikstrom]

Made SemanticTokenCollector skip Decls not in the main file.

[jvikstrom]

Fixed a bunch of small comments.

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlight.cpp

29

Will fix in a separate patch for topLevelDecls. Don't really know what FIXME to add here. Added one to getLocalTopLevelDecls. Don't really have a good understanding of the reason as to what happens yet so the FIXME is very general (as you can probably tell from it)...

57

Don't understand what you mean.

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

25

I changed to pass a const vector & instead. Is that alright as well?

[hokein]

much clearer now, a few more nits.

clang-tools-extra/clangd/ClangdUnit.cpp

69 ↗

(On Diff #206593)

not need to add this in this patch as you are also working on a fix in a separate patch.

clang-tools-extra/clangd/SemanticHighlight.cpp

52

nit: remove the {} if the body contains only one statement, the same to other places.

57

I meant we could move lines 56 ~ 66 to a new function, the current implementation is fine (as addToken is small).

58

nit: just "if (!R)" would work.

65

maybe just Tokens.emplace_back(Kind, *R).

76

we probably don't need the != operator in this patch.

clang-tools-extra/clangd/SemanticHighlight.h

14

the RecursiveASTVisitor.h should be in the .cpp file.

33

needs high-level comments.

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

25

yeah, that works as well, llvm prefers llvm::ArrayRef, but up to you.

48

the code seems not clang-format, could you run clang-format on your patch?

[jvikstrom]

• [clangd] Added functionality for getting semantic highlights for variable and function declarations

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

48

It has been clang-formated. The spaces are part of the test (need to check that the range does not include spaces even if there are multiple ones)

[hokein]

thanks mostly good. Thinking more about the name, we should align with the LSP proposal, see my comments inline.

clang-tools-extra/clangd/SemanticHighlight.cpp

21

HighlightingTokenCollector.

clang-tools-extra/clangd/SemanticHighlight.h

18

LSP proposal is using Highlighting rather than Highlight, let's align with the LSP proposal, using Highlighting in our names (comments, function, class, and files).

The name seems too verbose, let's drop the Semantic, just use HighlightingKind.

24

here use HighlightingToken.

33

getSemanticHighlightings.

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

47

nit: SemanticTokenCollector => SemanticHighlighting

[jvikstrom]

clang-tools-extra/clangd/SemanticHighlight.h

18

There is also a DocumentHighlightingKind enum though. Is there not a possibility of confusing people with HighlightingKind and DocumentHighlightingKind?

[hokein]

clang-tools-extra/clangd/SemanticHighlight.h

18

We have a DocumentHighlightKind (without ing), but that is in the LSP layer.

Yeah, it would not be super clear, but if we are in the related context, HighlightingKind is clear -- ClangdServer will use getSemanticHighlights to get all highlight tokens, so I'd prefer shorter names.

[jvikstrom]

Renamed types to follow LSP. Also renamed files.

[hokein]

Thanks, looks good.

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

1 ↗

(On Diff #206630)

nit: SemanticHighlightingTest.cpp => SemanticHighlightingTests.cpp

18 ↗

(On Diff #206630)

nit: clang-format

[hokein]

the patch was landed in rL364421, not sure why it was not associated with this review.

clang-tools-extra/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   Headers.cpp
   IncludeFixer.cpp
   JSONTransport.cpp
   Logger.cpp
   Protocol.cpp
   Quality.cpp
   RIFF.cpp
   Selection.cpp
+  SemanticHighlight.cpp
   SourceCode.cpp
   Threading.cpp
   Trace.cpp
   TUScheduler.cpp
   URI.cpp
   XRefs.cpp
 
   index/Background.cpp

clang-tools-extra/clangd/SemanticHighlight.h

+//==-- SemanticHighlight.h - Generating highlights from the AST--*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken &Lhs, const SemanticToken &Rhs);
+bool operator!=(const SemanticToken &Lhs, const SemanticToken &Rhs);
+
+std::vector<SemanticToken> getSemanticHighlights(ParsedAST &AST);
+
+} // namespace clangd
+} // namespace clang
+
+#endif
 No newline at end of file

clang-tools-extra/clangd/SemanticHighlight.cpp

+//===--- SemanticHighlight.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 "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// Collects all semantic tokens in an ASTContext.
+class SemanticTokenCollector
+    : private RecursiveASTVisitor<SemanticTokenCollector> {
+  friend class RecursiveASTVisitor<SemanticTokenCollector>;
+  std::vector<SemanticToken> Tokens;
+  ASTContext &Ctx;
+  const SourceManager &SM;
+
+public:
+  SemanticTokenCollector(ParsedAST &AST)
+      : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {
+    auto TopDecls = AST.getLocalTopLevelDecls();
+    std::vector<Decl *> FilteredTopDecls;
+    // If a top level decl is not part of the main file it can't be highlighted.
+    for (auto Decl : TopDecls) {
+      if (AST.getSourceManager().getFileID(Decl->getLocation()) ==
+          AST.getSourceManager().getMainFileID()) {
+        FilteredTopDecls.push_back(Decl);
+      }
+    }
+
+    Ctx.setTraversalScope(FilteredTopDecls);
+  }
+
+  std::vector<SemanticToken> collectTokens() {
+    TraverseAST(Ctx);
+    return Tokens;
+  }
+
+private:
+  bool VisitVarDecl(VarDecl *Var) {
+    addSymbol(Var, SemanticHighlightKind::Variable);
+    return true;
+  }
+  bool VisitFunctionDecl(FunctionDecl *Func) {
+    addSymbol(Func, SemanticHighlightKind::Function);
+    return true;
+  }
+
+  void addSymbol(NamedDecl *D, SemanticHighlightKind Kind) {
+    if (D->getLocation().isMacroID()) {
+      // FIXME; This is inside a macro. For now skip the token
+      return;
+    }
+
+    if (D->getName().size() == 0) {
+      // Don't add symbols that don't have any length.
+      return;
+    }
+
+    auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+    if (!R.hasValue()) {
+      // R should always have a value, if it doesn't something is very wrong.
+      llvm_unreachable("Tried to add semantic token with an invalid range");
+    }
+
+    SemanticToken S;
+    S.R = R.getValue();
+    S.Kind = Kind;
+    Tokens.push_back(S);
+  }
+};
+
+} // namespace
+
+bool operator==(const SemanticToken &Lhs, const SemanticToken &Rhs) {
+  return Lhs.Kind == Rhs.Kind && Lhs.R == Rhs.R;
+}
+bool operator!=(const SemanticToken &Lhs, const SemanticToken &Rhs) {
+  return !(Lhs == Rhs);
+}
+
+std::vector<SemanticToken> getSemanticHighlights(ParsedAST &AST) {
+  return SemanticTokenCollector(AST).collectTokens();
+}
+
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   IndexActionTests.cpp
   IndexTests.cpp
   JSONTransportTests.cpp
   PrintASTTests.cpp
   QualityTests.cpp
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
   SymbolInfoTests.cpp
   SyncAPI.cpp
   TUSchedulerTests.cpp
   TestFS.cpp
   TestIndex.cpp

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

+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- 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 "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector<SemanticToken> makeSemanticTokens(std::vector<Range> Ranges,
+                                             SemanticHighlightKind Kind) {
+  std::vector<SemanticToken> Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+    Tokens[I].R = Ranges[I];
+    Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(std::string Code) {
+
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map<SemanticHighlightKind, std::string> KindToString{
+      {SemanticHighlightKind::Variable, "Variable"},
+      {SemanticHighlightKind::Function, "Function"}};
+  std::vector<SemanticToken> ExpectedTokens;
+  for (auto KindString : KindToString) {
+    std::vector<SemanticToken> Toks =
+        makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+    ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  checkHighlights(R"cpp(
+    struct A {
+      double SomeMember;
+    };
+    void $Function[[foo]](int $Variable[[a]]) {
+      auto $Variable[[VeryLongVariableName]] = 12312;
+      A     $Variable[[aa]];
+    }
+  )cpp");
+}
+
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
+    void $Function[[foo]](int);
+  )cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang