This is an archive of the discontinued LLVM Phabricator instance.

[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

jvikstrom created this revision.Jun 19 2019, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 8:27 AM
jvikstrom updated this revision to Diff 205608.Jun 19 2019, 8:29 AM

Adds CMakeLists changes

Renamed SemanticSymbol to SemanticToken

Eugene.Zelenko added inline comments.
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 =.

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 updated this revision to Diff 206178.Jun 24 2019, 2:55 AM
jvikstrom marked 18 inline comments as done.

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

jvikstrom marked 2 inline comments as done.Jun 24 2019, 2:56 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/SemanticHighlight.cpp
130

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

hokein added inline comments.Jun 24 2019, 7:28 AM
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 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
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 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
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?

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

Via Conduit