[PATCH] D70872: [clangd] Implement "textDocument/documentLink" protocol support

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1204
 
+void ClangdLSPServer::onDocumentLink(
+    const DocumentLinkParams &Params,
----------------
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.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1263
+///
+/// TODO(forster): For now only file URIs are supported.
+struct DocumentLink {
----------------
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)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:181
+    if (!Inc.Resolved.empty()) {
+      Result.emplace_back(DocumentLink(
+          {Inc.R, URIForFile::canonicalize(Inc.Resolved, *MainFilePath)}));
----------------
nit `push_back(DocumentLink{...})`


================
Comment at: clang-tools-extra/clangd/test/document-link.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
----------------
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.


================
Comment at: clang-tools-extra/clangd/test/document-link.test:21
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "target": "file://{{.*}}/iostream"
+# CHECK-NEXT:    },
----------------
MForster wrote:
> 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.
> 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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70872/new/

https://reviews.llvm.org/D70872