Page MenuHomePhabricator

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

Authored by jvikstrom on Jun 19 2019, 8:27 AM.



Adds functionality for getting semantic highlights

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hokein added inline comments.Jun 24 2019, 7:28 AM
11 ↗(On Diff #206178)

The comment is out of date now.

29 ↗(On Diff #206178)

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

38 ↗(On Diff #206178)

The comment is out of date.

31 ↗(On Diff #206178)

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 added inline comments.Jun 24 2019, 7:57 AM
1 ↗(On Diff #206178)

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

13 ↗(On Diff #206178)

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

jvikstrom updated this revision to Diff 206247.Jun 24 2019, 9:53 AM
jvikstrom marked 7 inline comments as done.

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

jvikstrom added inline comments.Jun 24 2019, 9:53 AM
75 ↗(On Diff #206178)

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 marked an inline comment as done.Jun 25 2019, 12:15 AM
jvikstrom added inline comments.
75 ↗(On Diff #206178)

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

jvikstrom retitled this revision from [clang-tidy] Added functionality for getting semantic highlights for variable and function declarations to [clangd] Added functionality for getting semantic highlights for variable and function declarations.Jun 25 2019, 12:23 AM

Made SemanticTokenCollector visible and removed getSemanticHighlights function.

jvikstrom marked 3 inline comments as done.

Added header and empty line

hokein added inline comments.Jun 25 2019, 1:36 AM
43 ↗(On Diff #206376)

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

46 ↗(On Diff #206376)

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

57 ↗(On Diff #206376)
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 updated this revision to Diff 206393.Jun 25 2019, 2:36 AM
jvikstrom marked 4 inline comments as done.

Changed tests

57 ↗(On Diff #206376)

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

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

19 ↗(On Diff #206393)

nit: public.

20 ↗(On Diff #206393)

do we need friend declaration now?

28 ↗(On Diff #206393)

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.

32 ↗(On Diff #206393)

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

45 ↗(On Diff #206393)

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

47 ↗(On Diff #206393)

nit: FIXME: skip tokens inside macros for now.

51 ↗(On Diff #206393)

use getDeclName().isEmpty().

56 ↗(On Diff #206393)

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

59 ↗(On Diff #206393)

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

14 ↗(On Diff #206393)

nit: remove the unused include.

19 ↗(On Diff #206393)

Maybe just

enum SemanticHighlightKind {
39 ↗(On Diff #206393)

add a new line.

11 ↗(On Diff #206393)

not used #include

15 ↗(On Diff #206393)

is this #include used?

24 ↗(On Diff #206393)

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

27 ↗(On Diff #206393)

nit: ++I

35 ↗(On Diff #206393)

nit: llvm::StringRef

39 ↗(On Diff #206393)

static const

43 ↗(On Diff #206393)

nit: const auto&

53 ↗(On Diff #206393)

nit: update the test name.

65 ↗(On Diff #206393)

I think we can group these two testcases like

const char* TestCases[] =  {

for (...) {
66 ↗(On Diff #206393)

maybe use another obvious case:

struct {
} name;
jvikstrom updated this revision to Diff 206450.Jun 25 2019, 7:56 AM

Made SemanticTokenCollector skip Decls not in the main file.

jvikstrom marked 24 inline comments as done.

Fixed a bunch of small comments.

jvikstrom added inline comments.Jun 26 2019, 1:03 AM
28 ↗(On Diff #206393)

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

56 ↗(On Diff #206393)

Don't understand what you mean.

24 ↗(On Diff #206393)

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

much clearer now, a few more nits.

69 ↗(On Diff #206593)

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

56 ↗(On Diff #206393)

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

51 ↗(On Diff #206593)

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

57 ↗(On Diff #206593)

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

64 ↗(On Diff #206593)

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

75 ↗(On Diff #206593)

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

13 ↗(On Diff #206593)

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

32 ↗(On Diff #206593)

needs high-level comments.

24 ↗(On Diff #206393)

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

47 ↗(On Diff #206593)

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

jvikstrom updated this revision to Diff 206618.Jun 26 2019, 2:35 AM
jvikstrom marked 11 inline comments as done.
  • [clangd] Added functionality for getting semantic highlights for variable and function declarations
47 ↗(On Diff #206593)

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)

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

20 ↗(On Diff #206618)


17 ↗(On Diff #206618)

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.

23 ↗(On Diff #206618)

here use HighlightingToken.

32 ↗(On Diff #206618)


46 ↗(On Diff #206618)

nit: SemanticTokenCollector => SemanticHighlighting

jvikstrom marked an inline comment as done.Jun 26 2019, 3:40 AM
jvikstrom added inline comments.
17 ↗(On Diff #206618)

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

hokein added inline comments.Jun 26 2019, 4:47 AM
17 ↗(On Diff #206618)

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 updated this revision to Diff 206630.EditedJun 26 2019, 5:01 AM
jvikstrom marked 6 inline comments as done.

Renamed types to follow LSP. Also renamed files.

hokein accepted this revision.Jun 26 2019, 5:24 AM

Thanks, looks good.


nit: SemanticHighlightingTest.cpp => SemanticHighlightingTests.cpp


nit: clang-format

This revision is now accepted and ready to land.Jun 26 2019, 5:24 AM
hokein closed this revision.Jun 27 2019, 5:40 AM

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