[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Seems fair enough to do something about this, though it's a bit sad we're doing it just because VSCode has bad UI.

A couple of open design questions:

1. In the future it may make sense to have other documentLinks too. (e.g. imagine a comment `Configures the Frobnicator` - we should linkify Frobnicator to point to a class, once documentLink supports target ranges). So we should decide if this feature is "disable documentLink" or "don't include included headers in documentLink". This affects e.g. naming of the flag, which is awkward to change later.
2. Do we really want to add command-line flags for this or does it rather belong as a config option? (In which case we'd want to change the behavior of the method, not turn the method itself on/off, to allow the config option to vary across files in the usual way).



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:639
     Result.getObject("capabilities")->insert({"foldingRangeProvider", true});
+  if (Opts.IncludesAsLinks)
+    Result.getObject("capabilities")
----------------
this is worth a comment:

Currently we only provide documentLink for #included headers. If that's turned off, let clients avoid sending the request altogether.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1513
     MsgHandler->bind("textDocument/foldingRange", &ClangdLSPServer::onFoldingRange);
+  if (Opts.IncludesAsLinks)
+    MsgHandler->bind("textDocument/documentLink", &ClangdLSPServer::onDocumentLink);
----------------
It seems simpler for debugging and tests to leave the method bound regardless, and only associate the *capability* with the option.

This is what we usually do, though foldingRange wasn't done this way for some reason. (Maybe because we know it's *crashy* so we're being extra cautious). In any case, no need for it in this case.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:55
     std::function<void()> MemoryCleanup = nullptr;
+    /// If set, include files in #include directives are exposed to the clients
+    /// as documentLinks. If disabled include files can still be opened with the
----------------
this belongs below in the list of per-feature options.


================
Comment at: clang-tools-extra/clangd/test/xrefs.test:4
 ---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include <stdint.h>\nextern int x;\nint x = 0;\nint y = x;"}}}
 ---
----------------
Having tests depend on external headers is a bit of a pain downstream - it means they need to run in an environment where the headers are available.

We do this by necessity in `document-link.test` - maybe we should move this test here?
(Personally I'm comfortable with this functionality only being covered by unit-tests, as it is now though).


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

https://reviews.llvm.org/D93763