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

[rapgenic]

Summary

On some editors (namely VSCode) documentLinks are rendered with an
underline, which might be distracting to see. This patch adds a flag to
disable such request, as it's used only for includes, and the open include
feature can be already fulfilled by the definition request

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

[njames93]

Can you apply the clang-format patch suggested by the pre-merge bot. Alternatively, if you have a recent version of clang-format and the appropriate git-hooks you should be able to run git clang-format for the same effect.

[rapgenic]

Fix formatting

[sammccall]

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).

clang-tools-extra/clangd/ClangdLSPServer.cpp

639

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.

1516

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.

clang-tools-extra/clangd/ClangdLSPServer.h

55

this belongs below in the list of per-feature options.

clang-tools-extra/clangd/test/xrefs.test

4 ↗

(On Diff #313667)

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).

[rapgenic]

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.

I think it's already like that, in that specific case, and it uses goToDefinition instead: if I ctrl+click on a variable/class/function name in a comment i can actually go to the definition, if I am not wrong.

As for the naming of the option, maybe the title of the patch is not too accurate, however from the option description it's quite clear that it's only related to include files, so any additional feature using documentLinks should be easy to add while keeping this option too.

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).

In my opinion (and also by looking at the already implemented options in .clangd config file) the config file seems to be related to project specific configurations (compilation flags, indexing, etc.), whereas the option that this patch implements is more related to the "editing experience", which is something one should not change very often. So I think that it might not be worth putting it in the config file.

clang-tools-extra/clangd/test/xrefs.test

4 ↗

(On Diff #313667)

I'm moving the test, as it is related to document links it seems pertinent to put it there, if you prefer we can leave it out of course

clang-tools-extra/clangd/ClangdLSPServer.h

struct Options : ClangdServer::Options {
     /// A fixed directory to search for a compilation database in.
     /// If not set, we search upward from the source file.
     llvm::Optional<Path> CompileCommandsDir;
     /// The offset-encoding to use, or None to negotiate it over LSP.
     llvm::Optional<OffsetEncoding> Encoding;
     /// If set, periodically called to release memory.
     /// Consider malloc_trim(3)
     std::function<void()> MemoryCleanup = nullptr;
 
     /// Per-feature options. Generally ClangdServer lets these vary
     /// per-request, but LSP allows limited/no customizations.
     clangd::CodeCompleteOptions CodeComplete;
     clangd::RenameOptions Rename;
     /// Returns true if the tweak should be enabled.
     std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
       return !T.hidden(); // only enable non-hidden tweaks.
     };
+    /// If set, include files in #include directives are exposed to the clients
+    /// as documentLinks. If disabled include files can still be opened with the
+    /// go to definition feature.
+    bool IncludesAsLinks = true;
   };
 
   ClangdLSPServer(Transport &Transp, const ThreadsafeFS &TFS,
                   const ClangdLSPServer::Options &Opts);
   /// The destructor blocks on any outstanding background tasks.
   ~ClangdLSPServer();
 
   /// Run LSP server loop, communicating with the Transport provided in the

clang-tools-extra/clangd/ClangdLSPServer.cpp

llvm::json::Object Result{
             {"signatureHelpProvider",
              llvm::json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"declarationProvider", true},
             {"definitionProvider", true},
             {"implementationProvider", true},
             {"documentHighlightProvider", true},
-            {"documentLinkProvider",
-             llvm::json::Object{
-                 {"resolveProvider", false},
-             }},
             {"hoverProvider", true},
             {"renameProvider", std::move(RenameProvider)},
             {"selectionRangeProvider", true},
             {"documentSymbolProvider", true},
             {"workspaceSymbolProvider", true},
             {"referencesProvider", true},
             {"astProvider", true},
             {"executeCommandProvider",
if (Opts.Encoding)
     Result["offsetEncoding"] = *Opts.Encoding;
   if (Opts.TheiaSemanticHighlighting)
     Result.getObject("capabilities")
         ->insert(
             {"semanticHighlighting",
              llvm::json::Object{{"scopes", buildHighlightScopeLookupTable()}}});
   if (Opts.FoldingRanges)
     Result.getObject("capabilities")->insert({"foldingRangeProvider", true});
+  if (Opts.IncludesAsLinks)
+    // Currently we only provide documentLink for #included headers.
+    // If that's turned off, let clients avoid sending the request altogether.
+    Result.getObject("capabilities")
+        ->insert({"documentLinkProvider",
+                  llvm::json::Object{{"resolveProvider", false}}});
   Reply(std::move(Result));
 }
 
 void ClangdLSPServer::onInitialized(const InitializedParams &Params) {}
 
 void ClangdLSPServer::onShutdown(const ShutdownParams &Params,
                                  Callback<std::nullptr_t> Reply) {
   // Do essentially nothing, just say we're ready to exit.
ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
   MsgHandler->bind("callHierarchy/outgoingCalls", &ClangdLSPServer::onCallHierarchyOutgoingCalls);
   MsgHandler->bind("textDocument/selectionRange", &ClangdLSPServer::onSelectionRange);
   MsgHandler->bind("textDocument/documentLink", &ClangdLSPServer::onDocumentLink);
   MsgHandler->bind("textDocument/semanticTokens/full", &ClangdLSPServer::onSemanticTokens);
   MsgHandler->bind("textDocument/semanticTokens/full/delta", &ClangdLSPServer::onSemanticTokensDelta);
   MsgHandler->bind("$/memoryUsage", &ClangdLSPServer::onMemoryUsage);
   if (Opts.FoldingRanges)
     MsgHandler->bind("textDocument/foldingRange", &ClangdLSPServer::onFoldingRange);
   // clang-format on
 }
 
 ClangdLSPServer::~ClangdLSPServer() {
   IsBeingDestroyed = true;
   // Explicitly destroy ClangdServer first, blocking on threads it owns.
   // This ensures they don't access any other members.
   Server.reset();
 }

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

 # CHECK-NEXT:          "character": 9,
 # CHECK-NEXT:          "line": 1
 # CHECK-NEXT:        }
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "target": "file://{{.*}}/stddef.h"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 # CHECK-NEXT:}
+---
+# Go to definition on include (necessary when documentLinks are disabled)
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":15}}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "range": {
+# CHECK-NEXT:        "end": {
+# CHECK-NEXT:          "character": 0,
+# CHECK-NEXT:          "line": 0
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "start": {
+# CHECK-NEXT:          "character": 0,
+# CHECK-NEXT:          "line": 0
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "uri": "file://{{.*}}/{{([A-Z]:/)?}}stdint.h"
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
 
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/clangd/tool/ClangdMain.cpp

 opt<bool> FoldingRanges{
     "folding-ranges",
     cat(Features),
     desc("Enable preview of FoldingRanges feature"),
     init(false),
     Hidden,
 };
 
+opt<bool> IncludesAsLinks{
+    "includes-as-links",
+    cat(Features),
+    desc("Provide a document link to the files included with #include "
+         "directive. If set to false, include files can still be opened with "
+         "go to definition feature"),
+    init(true),
+};
+
 opt<unsigned> WorkerThreadsCount{
     "j",
     cat(Misc),
     desc("Number of async workers used by clangd. Background index also "
          "uses this many workers."),
     init(getDefaultAsyncThreadsCount()),
 };
 
#endif
   } else {
     Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
   Opts.MemoryCleanup = getMemoryCleanupFunction();
+  Opts.IncludesAsLinks = IncludesAsLinks;
 
   Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
   Opts.CodeComplete.Limit = LimitResults;
   if (CompletionStyle.getNumOccurrences())
     Opts.CodeComplete.BundleOverloads = CompletionStyle != Detailed;
   Opts.CodeComplete.ShowOrigins = ShowOrigins;
   Opts.CodeComplete.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {