Implementation of Document Highlights Request as described in LSP.
Details
Diff Detail
- Build Status
Buildable 12459 Build 12459: arc lint + arc unit
Event Timeline
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.
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.
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)
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 | |
1102 | This is duplicated code with getDeclarationLocation. It also doesn't use tryGetRealPathName | |
1199 | I don't think this is called? | |
1225 | This doesn't add or have a location in its signature so I think it should be named differently. | |
1266 | call takeDecls. TempDecls -> SelectedDecls? | |
1267 | remove? see comment below | |
1277 | DocumentHighlights. They are not Locations (so they are not confused with the struct in Protocol) | |
1279 | replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? then call takeHighlights. | |
1285 | I think we should remove macro handling for now, it doesn't highlight occurrences anyway. | |
1299 | 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 |
clangd/ClangdUnit.cpp | ||
---|---|---|
1279 | Also, use a range-based for loop | |
1281 | 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. |
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; } |
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!
clangd/ClangdServer.h | ||
---|---|---|
288 | Oops, yeah this should be a llvm::Expected |
Getting DocumentHighlightKind is now done in DocumentHighlightsFinder
Removed duplicated and unused code
Refactored method and variable names
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 |
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? |
Removed temporary test file
Updated test to account for read-access symbol verification
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. |
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 the first call to getLocForEnd in handleDeclOccurence will compute the position at |
Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test
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<>. if (!Highlights) { C.replyError(ErrorCode::InternalError, llvm::toString(Highlights.takeError())); return; } |
clangd/ClangdLSPServer.cpp | ||
---|---|---|
246 | Just to be clear, the error I see in the output is: It asserts when in ClangdLSPServer::onDocumentHighlight, referencing Highlights->Value |
Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit
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. | |
1116 | We should not return default-initialized Locations from this function. | |
1145 | 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==. | |
test/clangd/initialize-params-invalid.test | ||
23 | NIT: Misindent | |
test/clangd/initialize-params.test | ||
23 | NIT: Misindent |
Minor code cleanup getDeclarationLocation now returns llvm::Optional operator< for DocumentHighlight struct now properly compares the kind field
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 | ||
1107 | const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart)); if (!F) return llvm::None; | |
1117 | maybe move the check earlier? see comment above. | |
clangd/Protocol.h | ||
620 | Use /// like other structs |
clangd/ClangdServer.cpp | ||
---|---|---|
526 | missing return? llvm::make_error<llvm::StringError>( |
clangd/ClangdServer.cpp | ||
---|---|---|
528 | Invalid | |
530 | It trips here with assertions enabled because we haven't checked the previous "Value" for error before, sigh. 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 |
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; } |
Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp
Sorry I forgot to submit this additional comment in my last review pass :(
clangd/Protocol.cpp | ||
---|---|---|
1134 | static_cast<int>(DH.kind) |
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. | |
clangd/Protocol.h | ||
620 | NIT: remove this empty comment line and all the others. | |
639 | This comparison does not provide a total order. |
extra line