[clangd] Symbol search includes parameter types for (member) functions

[florianhumblot]

Summary

When using clangd as a language server in an editor such as vscode, it is currently hard to use the List Document Symbols feature with files where a function is overloaded many times.
Other language servers (such as the one provided by Microsoft) return more than just the symbol's name for the view, namely it also returns the parameters of the (member) functions in the file.
i.e. clang's current view:

versus Microsoft's current view:

This patch adds the parameter information to symbol listings of free functions and member functions, resulting in the following symbol navigation view:

This patch has been tested with vscode V1.80.2 and the vscode clangd extension version v0.1.24
No additional changes were necessary to either clang(d) or the vscode extension for the fix to work (other than specifying the custom clangd executable)

Resolves https://github.com/clangd/clangd/issues/1344

[florianhumblot]

Forgot to update the tests.

[sammccall]

LSP is fairly clear here and our behavior here seems to be pretty correct.

• name is "the name of this symbol",
• detail is "More detail for this symbol, e.g the signature of a function.".

We're providing the type as detail: void() (for functions just () might be better, or better still: () -> void).

Anyway we're providing the data, VSCode is choosing not to display it. It does show it in the hierarchical symbol view (if you are inside the symbol and click on its name in the breadcrumbs along the top), and also in the outline view.
It seems that the VSCode folks have decided that the since the ctrl-shift-O view shows results as a flat list, the "grey text" slot is used to show hierarchy rather than detail.
I'm afraid that's up to them, you can file feature requests to have them change it.

Indeed it looks like the MS cpptools extension works around this by putting the detail into the name, but I don't think we should be emulating this: it violates the spec intent, is likely to break things in other editors. (cpptools don't care about this, they only support vscode).

clang-tools-extra/clangd/Protocol.cpp

899

DocumentSymbol is already a structure modelling LSP, this function's job is to serialize it, not rearrange the fields.

Moreover, we shouldn't render the detail into a string and then go back later and attempt to parse it, but just produce the data we want in the first place (in FindSymbols.cpp, where it can be unittested).

[florianhumblot]

@sammccall I guess you're right, I'll look into getting a fix into vscode instead.

[nridge]

I understood https://github.com/clangd/clangd/issues/1344 as being about workspace/symbol rather than textDocument/documentSymbol, though I see now that both are affected.

The analysis is a bit different for the two, though: unlike DocumentSymbol, the result types for workspace/symbol (LSP specifies two options, SymbolInformation or WorkspaceSymbol) do not have a detail field. So, for workspace/symbol, the issue would require a fix on the clangd side (or a change to LSP, I guess).

@sammccall, would you support adding the signature to the symbol name for workspace/symbol?

clang-tools-extra/clangd/Protocol.cpp

 }
 
 llvm::json::Value toJSON(const DocumentSymbol &S) {
   llvm::json::Object Result{{"name", S.name},
                             {"kind", static_cast<int>(S.kind)},
                             {"range", S.range},
                             {"selectionRange", S.selectionRange}};
 
-  if (!S.detail.empty())
+  if (!S.detail.empty()) {
+    if (S.kind == SymbolKind::Method || S.kind == SymbolKind::Function) {
+      llvm::StringRef Detail{S.detail};
+      const auto Start = Detail.find_first_of('(');
+      const auto End = Detail.find_last_of(')');
+      const auto Distance = End - Start;
+      Result["name"] = S.name + Detail.substr(Start, Distance).str() + ')';
+    }
     Result["detail"] = S.detail;
+  }
   if (!S.children.empty())
     Result["children"] = S.children;
   if (S.deprecated)
     Result["deprecated"] = true;
   // FIXME: workaround for older gcc/clang
   return std::move(Result);
 }