This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Document highlights for clangd
ClosedPublic

Authored by Nebiroth on Sep 29 2017, 1:28 PM.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

This updated patch still does not handle highlighting macro references correctly. I will make another patch at a later time for this issue.

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

I'll take a look at it later. Thanks!

Hi William, can you quickly go over the diff before I do a review? There are a few more-obvious things that can be addressed, i.e. commented lines, extra new/deleted lines, temporary code, lower case variable names, etc.

malaperle requested changes to this revision.Nov 20 2017, 6:18 AM
This revision now requires changes to proceed.Nov 20 2017, 6:18 AM
Nebiroth updated this revision to Diff 123848.Nov 21 2017, 1:59 PM
Nebiroth edited edge metadata.

Removed some commented lines and temporary code
Streamlined and removed some code that overlaps/conflicts with code hover patch so it's easier to merge (patch D35894)

malaperle requested changes to this revision.Nov 23 2017, 8:33 AM
malaperle added inline comments.
clangd/ClangdLSPServer.cpp
67

extra line

242

Items -> Highlights?

clangd/ClangdServer.h
288

can this thing fail? Should it be Expected<Tagged... ??

clangd/ClangdUnit.cpp
936–937

I think this class should be exactly the same as the code hover patch, so changes from there and the comments on that patch should apply here too.

938

just Decls?

939

just MacroInfos?

940

remove, see comment below.

954

Last

962

This comment needs to be updated (like the code hover patch)

964

Last

973

extra line

974

I think you need to do this in DocumentHighlightsFinder not here. Each occurrence can have its kind, not just the user selected location.

978

I don't think you need any of those getters. Only use the "takes"

1014

It's not "a given FileID and file", the finder is given a list of Decls

1086

This is duplicated code with getDeclarationLocation. It also doesn't use tryGetRealPathName
I also don't think this is called??

1183

I don't think this is called?

1209

This doesn't add or have a location in its signature so I think it should be named differently.
DocumentHighligh getDocumentHighlight(Decl*) ? Or addDocumentHighlight(Decl*) if you move it in DocumentHighlightsFinder.

1250

call takeDecls. TempDecls -> SelectedDecls?

1251

remove? see comment below

1261

DocumentHighlights. They are not Locations (so they are not confused with the struct in Protocol)

1263

replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? then call takeHighlights.

1269

I think we should remove macro handling for now, it doesn't highlight occurrences anyway.

1283

remove std::move, returning such local object will use copy elision

clangd/ClangdUnit.h
314

remove?

clangd/Protocol.h
637

remove

639

needed?

clangd/ProtocolHandlers.cpp
49

revert change

clangd/ProtocolHandlers.h
34

revert change

This revision now requires changes to proceed.Nov 23 2017, 8:33 AM
malaperle added inline comments.Nov 23 2017, 9:00 AM
clangd/ClangdUnit.cpp
1263

Also, use a range-based for loop

1265

The vector returned by getKinds() doesn't match the size of getSourceRanges(). This is because the kinds vector is computed in DeclarationLocationsFinder which only computes the kind of the occurrence the user has selected, not the kinds of all occurrences for a given Decl. Once you move the code to create the highlights in DocumentHighlightsFinder, you won't need a vector of kinds and this won't be a problem.

malaperle added inline comments.Nov 23 2017, 10:26 AM
clangd/ClangdUnit.cpp
975

With this code, I always get "text" kind. It's because index::SymbolRoleSet is a bitfield so you have to check the write, read bits. Something like:

DocumentHighlightKind Kind = DocumentHighlightKind::Text;
if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Write) & Roles) {
  Kind = DocumentHighlightKind::Write;
} else if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Read) & Roles) {
  Kind = DocumentHighlightKind::Read;
}

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

This looks interesting. It does solve the problem that currently we traverse the whole tree. However, I don't think it has support for macros (they are not covered by USRs I believe) and it looks like it only can return one NamedDecl* at a location, and it's possible to have multiple, non-named Decls. Perhaps we should look at improving this!

Nebiroth marked 31 inline comments as done.Nov 24 2017, 9:28 AM
Nebiroth added inline comments.
clangd/ClangdServer.h
288

Oops, yeah this should be a llvm::Expected

Nebiroth updated this revision to Diff 124224.Nov 24 2017, 9:41 AM
Nebiroth edited edge metadata.
Nebiroth marked an inline comment as done.

Getting DocumentHighlightKind is now done in DocumentHighlightsFinder
Removed duplicated and unused code
Refactored method and variable names

malaperle requested changes to this revision.Nov 24 2017, 10:03 AM
malaperle added inline comments.
clangd/ClangdLSPServer.cpp
242

Items -> Highlights?

clangd/ClangdServer.h
288

can this thing fail? Should it be Expected<Tagged... ??

clangd/ClangdUnit.cpp
956

std::move

965

std::move

1017

remove

1018

remove

1028

remove

1037

remove

1039

remove

1079

either this should be named getDocumentHighlight or it should do the push back and return void

This revision now requires changes to proceed.Nov 24 2017, 10:03 AM
Nebiroth updated this revision to Diff 124230.Nov 24 2017, 10:21 AM
Nebiroth edited edge metadata.
Nebiroth marked 6 inline comments as done.

Fixed a few outstanding issues that were reported as completed

malaperle requested changes to this revision.Nov 24 2017, 12:25 PM
malaperle added inline comments.
test/clangd/documenthighlight.test
1

we should test that we can get kind == 1, 2, 3

16

kind 0 is invalid

23

kind 216? 220?

29

kind 0 is invalid

This revision now requires changes to proceed.Nov 24 2017, 12:25 PM
Nebiroth updated this revision to Diff 124238.Nov 24 2017, 1:04 PM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

Fixed test checking for values from an incorrect bit shift

malaperle requested changes to this revision.Nov 27 2017, 6:09 AM

There were some things I missed, sorry about that!

clangd/main.cpp
1 ↗(On Diff #124238)

This files needs to be removed

test/clangd/documenthighlight.test
11

I think we should cover "kind":2 as well. It's probably not a big change.

16

This comment should be updated, it was copy/pasted probably?

23

This comment should be updated, it was copy/pasted probably?

30

This comment should be updated, it was copy/pasted probably?

This revision now requires changes to proceed.Nov 27 2017, 6:09 AM
Nebiroth updated this revision to Diff 124391.Nov 27 2017, 8:37 AM
Nebiroth edited edge metadata.
Nebiroth marked an inline comment as done.

Removed temporary test file
Updated test to account for read-access symbol verification

malaperle requested changes to this revision.Nov 27 2017, 12:50 PM

I tested the patch and it works quite well! I think those are the last comments from me. Sorry, I should have bundled them together a bit more :(

clangd/ClangdUnit.cpp
1019

Not used.

malaperle added inline comments.Nov 27 2017, 12:50 PM
clangd/ClangdUnit.cpp
1029

Last

1069

Don't call getLocForEndOfToken here again. This is the reason why sometimes it includes an extra character. Just to explain a bit, supposed we have
int Foo;

the first call to getLocForEnd in handleDeclOccurence will compute the position at
int Foo|;
Then if you call is again, it will go at the end of the ';' token so it will now be
int Foo;|

This revision now requires changes to proceed.Nov 27 2017, 12:50 PM
Nebiroth updated this revision to Diff 124458.Nov 27 2017, 1:33 PM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test

malaperle added inline comments.Nov 27 2017, 1:34 PM
clangd/ClangdLSPServer.cpp
246

I get a test failure here because there is an assertion that the Expected<> needs to be checked. I can't really think of any failure case right now where we wouldn't just return an empty array of highlights. But I think it's better for consistency and future-proofing to keep the Expected<>.
I think you can just do like in onRename for now

if (!Highlights) {
  C.replyError(ErrorCode::InternalError,
               llvm::toString(Highlights.takeError()));
  return;
}
malaperle added inline comments.Nov 28 2017, 1:32 PM
clangd/ClangdLSPServer.cpp
246

Just to be clear, the error I see in the output is:
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

It asserts when in ClangdLSPServer::onDocumentHighlight, referencing Highlights->Value

Nebiroth updated this revision to Diff 124810.Nov 29 2017, 1:44 PM

Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit

Nebiroth updated this revision to Diff 124834.Nov 29 2017, 3:30 PM

Fixed wrong content header making the test fail

ilya-biryukov requested changes to this revision.Nov 30 2017, 1:31 AM
ilya-biryukov added inline comments.
clangd/ClangdLSPServer.cpp
67

Still there

clangd/ClangdServer.cpp
513

Other functions don't crash in this case anymore, we should follow the same pattern here (see findDefinitions for an example)

524

We should return an error this case.

clangd/ClangdUnit.cpp
1062

This function should be private.

1100

We should not return default-initialized Locations from this function.
Return llvm::Optional<Location> and handle the case when getFileEntryForID returns null by returning llvm::None.

1169

Let's add a comment that we don't currently handle macros. It's ok for the first iteration, having documentHighlights for Decls is useful enough to get it in.

clangd/Protocol.h
640

Please also compare kind here, to make this operator consistent with operator==.
std::tie(LHS.range, LHS.kind) < std::tie(RHS.range, RHS.kind) should work. If it doesn't, converting enums to int should help: std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind))

test/clangd/initialize-params-invalid.test
23

NIT: Misindent

test/clangd/initialize-params.test
23

NIT: Misindent

This revision now requires changes to proceed.Nov 30 2017, 1:31 AM
Nebiroth updated this revision to Diff 124951.Nov 30 2017, 8:51 AM
Nebiroth edited edge metadata.
Nebiroth marked 6 inline comments as done.
Minor code cleanup
getDeclarationLocation now returns llvm::Optional
operator< for DocumentHighlight struct now properly compares the kind field
malaperle requested changes to this revision.Nov 30 2017, 12:08 PM

very minor comments

clangd/ClangdServer.h
288

"for a symbol hovered on."

It doesn't have to be a symbol and the user doesn't have to hover on it. So maybe just "for a given position"

clangd/ClangdUnit.cpp
1091
const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
if (!F)
  return llvm::None;
1101

maybe move the check earlier? see comment above.

clangd/Protocol.h
620

Use /// like other structs

This revision now requires changes to proceed.Nov 30 2017, 12:08 PM
malaperle added inline comments.Nov 30 2017, 12:53 PM
clangd/ClangdServer.cpp
526

missing return?
I get a warning that looks legit:
./tools/clang/tools/extra/clangd/ClangdServer.cpp:526:7: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]

llvm::make_error<llvm::StringError>(
Nebiroth updated this revision to Diff 125228.Dec 1 2017, 2:54 PM
Nebiroth edited edge metadata.

Minor code cleanup
unparse and parse methods for JSON are updated

malaperle requested changes to this revision.Dec 3 2017, 9:08 PM
malaperle added inline comments.
clangd/ClangdServer.cpp
528

Invalid

530

It trips here with assertions enabled because we haven't checked the previous "Value" for error before, sigh.
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

How about this:

std::vector<DocumentHighlight> Result;
llvm::Optional<llvm::Error> Err;
Resources->getAST().get()->runUnderLock([Pos, &Result, &Err,
                                         this](ParsedAST *AST) {
  if (!AST)
    Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                              llvm::errc::invalid_argument);
  Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
});

if (Err)
  return std::move(*Err);

return make_tagged(Result, TaggedFS.Tag);

The tests pass for me with this.

clangd/Protocol.cpp
1132

Too many new lines. Only keep one.

clangd/Protocol.h
626

Use /// like other places

633

Use /// like other places

This revision now requires changes to proceed.Dec 3 2017, 9:08 PM
Nebiroth updated this revision to Diff 125346.Dec 4 2017, 8:11 AM
Nebiroth edited edge metadata.

Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are enabled

Can you also reformat the code too with clang-format? I think there's a few things in ClangdUnit.h/.cpp

clangd/ClangdServer.cpp
521

We should do like findDefinitions here:

if (!Resources)
  return llvm::make_error<llvm::StringError>(
      "findDocumentHighlights called on non-added file",
      llvm::errc::invalid_argument);
528

Woops, I forgot the return here if it's an error, so it should be:

if (!AST) {
  Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                            llvm::errc::invalid_argument);
  return;
}
malaperle requested changes to this revision.Dec 4 2017, 8:47 PM
This revision now requires changes to proceed.Dec 4 2017, 8:47 PM
Nebiroth updated this revision to Diff 125522.Dec 5 2017, 6:58 AM
Nebiroth edited edge metadata.

Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp

malaperle requested changes to this revision.Dec 5 2017, 8:27 AM

Sorry I forgot to submit this additional comment in my last review pass :(

clangd/Protocol.cpp
1134

static_cast<int>(DH.kind)

This revision now requires changes to proceed.Dec 5 2017, 8:27 AM
Nebiroth updated this revision to Diff 125619.Dec 5 2017, 2:44 PM
Nebiroth edited edge metadata.

Added static_cast when unparsing

malaperle accepted this revision.Dec 5 2017, 8:58 PM
malaperle added a subscriber: sammccall.

This looks good to me. @ilya-biryukov, @sammccall Any objections?
I think we can do further iterations later to handle macros and other specific cases (multiple Decls at the position, etc) . It's already very useful functionality.

Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.

clangd/ClangdUnit.cpp
961

Mentioning AST in this comment is weird, macros have nothing to do with the AST. We should remove/rephrase the comment.
I'm not sure if multiple occurences of MacroInfo are even possible here, but we could leave the code as is anyway.

clangd/Protocol.h
620

NIT: remove this empty comment line and all the others.

639

This comparison does not provide a total order.
Please use std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind)) instead.

ilya-biryukov requested changes to this revision.Dec 8 2017, 4:19 AM
This revision now requires changes to proceed.Dec 8 2017, 4:19 AM
Nebiroth updated this revision to Diff 126371.Dec 11 2017, 8:19 AM
Nebiroth edited edge metadata.
Nebiroth marked 3 inline comments as done.

Merged with latest llvm/clang
Minor code cleanup

@ilya-biryukov Need someone to land this.

ilya-biryukov accepted this revision.EditedDec 12 2017, 4:24 AM

LGTM module a slight tweak to fix compilation. I'll land this today.

This revision is now accepted and ready to land.Dec 12 2017, 4:24 AM
This revision was automatically updated to reflect the committed changes.