Page MenuHomePhabricator

[clangd] Implement "textDocument/documentLink" protocol support
ClosedPublic

Authored by MForster on Nov 30 2019, 1:58 PM.

Details

Summary

This adds an implementation for the "textDocument/documentLink" LSP request.

It returns links for all #include directives to the resolved target files.

Fixes https://github.com/clangd/clangd/issues/217.

Diff Detail

Event Timeline

MForster created this revision.Nov 30 2019, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2019, 1:58 PM
MForster retitled this revision from Advertise documentLink capability to [clangd] Implement "textDocument/documentLink" protocol support.Nov 30 2019, 2:01 PM
MForster edited the summary of this revision. (Show Details)
MForster added a reviewer: sammccall.
MForster marked an inline comment as done.Nov 30 2019, 2:19 PM
MForster added inline comments.
clang-tools-extra/clangd/test/document-link.test
22

Originally I tried to add a header file to the setup with a second didOpen request, but I didn't get this to work. Would I need to set this up like background-index.test, or is there a simpler way?

Anyway, I think the regular expression is probably good enough for the purpose of this test.

Thanks, looks nice!
It occurred to me we could compute (but not resolve) the ranges cheaply to speed up the UI. We don't need to do this now.
Only real thing to do is add a gunit test.

clang-tools-extra/clangd/ClangdLSPServer.cpp
1207

Eagerly resolving everything means that this request is going to block on the preamble/AST being built. This isn't terrible (clients should be sending their initial requests in parallel) but does mean a delay before the links show up. (And in updating if we insert an include and want to ctrl-click it, because we just invalidated the preamble).

There are a few ways we could improve this:
a) find the links using a quick pass (simple string matching or raw-lexer based), then resolve results from the MainFileIncludes structure (blocking on the AST at that point)
b) use a quick pass to serve results in the first place, e.g. a PreprocessorOnlyAction with SingleFileParseMode (doesn't descend into headers).
c) a combination of these.

I don't particularly think we need to do these at this point, but may be worth a comment.

clang-tools-extra/clangd/Protocol.h
1263

I don't think this TODO is needed - there's nothing to do unless we decide we want to emit other types of links.
(Which seems fairly unlikely: my understanding is that e.g. hyperlinks in comments should be resolved by editors generically rather than by language servers)

clang-tools-extra/clangd/XRefs.cpp
181

nit push_back(DocumentLink{...})

clang-tools-extra/clangd/test/document-link.test
2

Generally we smoke-test features in a lit test such as this one, and then do fine-grained testing as gunit tests (e.g. unittests/XRefTests.cpp). It's good to test end-to-end, but it's too hard to maintain lit tests for all cases as features get extended.

For this feature there's not much difference between the two, but you could drop one of the includes here and cover one more case in the unit tests:

#include "foo.h"
int end_of_preamble = 0;
#include "not_part_of_preamble.h"

(The non-preamble includes get into the data structures you're querying via a different path).

The unit tests are generally easier to set up:

  • you can use Annotations to write code with marked regions, and get the coordinates + unmarked code
  • you can use TestTU to add extra files "foo.h" to the VFS, and produce a ParsedAST
  • then just assert that the results match testPath("foo.h") + annotations.points()[0] etc.

TEST(LocateSymbol, WithIndex) in XRefsTests.cpp is a reasonable example.

22

Originally I tried to add a header file to the setup with a second didOpen request, but I didn't get this to work

Clangd's model is that the *current* file is always via LSP (didOpen), and all other files are read from disk. The setup would be slightly simpler than background-index.test because you don't need a compile command for the other file, but still a bit awkward.

I think the regex is OK, but would prefer to change to stdint.h/stddef.h (not the C++ versions). This is because they're builtin headers in clang, so even if we don't find a standard library they'll still exist. (clangd tests do formally depend on the builtins but not a stdlib, I believe).

Ping - how do you want to proceed here? I think the only thing that is really missing here is a unit test, I can add it if you don't have cycles.
I'll be gone after this week :-)

MForster updated this revision to Diff 233433.Dec 11 2019, 12:00 PM
MForster marked an inline comment as not done.

Adress review comments.

MForster marked 6 inline comments as done.Dec 11 2019, 12:02 PM

I think I addressed all review comments. PTAL.

sammccall accepted this revision.Dec 12 2019, 5:29 AM

Thanks! I'll land this for you.

This revision is now accepted and ready to land.Dec 12 2019, 5:29 AM

Thanks! I'll land this for you.

Thanks!

This revision was automatically updated to reflect the committed changes.

Sorry, I reverted this change in 079ef783dd5530b5f87beefe624b9179547ded7e. The tests depend on builtin headers, which is not intentionally supported in clangd tests; these tests are broken in some build environments.

which is intentionally not supported in clangd tests

Not intentionally as far as I'm aware :-)
Do you have a link or details for some failing environment? I'm OOO but I'd like to follow up and sort this out (ideally by making the builtins available)