Page MenuHomePhabricator

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

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

Details

Summary

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
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 added inline comments.Jun 24 2019, 7:57 AM
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 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
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 marked an inline comment as done.Jun 25 2019, 12:15 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/SemanticHighlight.cpp
76

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

Changed tests

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

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

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

clang-tools-extra/clangd/SemanticHighlight.cpp
19

nit: public.

20

do we need friend declaration now?

28

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

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

45

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

47

nit: FIXME: skip tokens inside macros for now.

51

use getDeclName().isEmpty().

56

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

59

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

clang-tools-extra/clangd/SemanticHighlight.h
14

nit: remove the unused include.

19

Maybe just

enum SemanticHighlightKind {
   Variable,
   Function
};
39

add a new line.

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

not used #include

15

is this #include used?

24

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

27

nit: ++I

35

nit: llvm::StringRef

39

static const

43

nit: const auto&

53

nit: update the test name.

65

I think we can group these two testcases like

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

for (...) {
}
66

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
clang-tools-extra/clangd/SemanticHighlight.cpp
28

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

Don't understand what you mean.

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

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

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.

56

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
24

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

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 marked an inline comment as done.Jun 26 2019, 3:40 AM
jvikstrom added inline comments.
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 added inline comments.Jun 26 2019, 4:47 AM
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 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.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
1 ↗(On Diff #206630)

nit: SemanticHighlightingTest.cpp => SemanticHighlightingTests.cpp

18 ↗(On Diff #206630)

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.