[PATCH] D44882: [clangd] Implementation of workspace/symbol request

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry about the delay Marc-André - I got stuck on "how to test ClangdLSPServer" and then distracted!
But this looks great.



================
Comment at: clangd/ClangdLSPServer.cpp:101
+  // All clients should support those.
+  for (SymKindUnderlyingType I = SymbolKindMin;
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
----------------
malaperle wrote:
> I'd like to add some test to exercise the changes in ClangdLSPServer.cpp and Protocol.cpp but it's not straightforward to do nicely. Using a "lit" test, I cannot have a header and since symbols in the main file are not collected then it's not useful. If I make a gtest, I have to feed it the lsp server with stdin and check the stdout which is a bit messy, but doable.
Yeah, we don't have a good way to test ClangdLSPServer :-( I would like to fix that, but I don't know of any easy fixes.

This is kind of gross, but do standard library includes work from our lit tests? You could `#include <map>` and then test using some symbols you know are there...



================
Comment at: clangd/ClangdLSPServer.cpp:103
+       I <= static_cast<SymKindUnderlyingType>(SymbolKind::Array); ++I)
+    SupportedSymbolKinds.set(I);
+
----------------
I'd like to be slightly less hostile than this to (broken) clients that fail to call initialize.
As the patch stands they'll get an empty SupportedSymbolKinds, and we'll crash if they call documentSymbols.

Instead I'd suggest pulling out defaultSymbolKinds() and initializing to that in the constructor, and then overriding with either `SpecifiedSymbolKinds` or `SpecifiedSymbolKinds | defaultSymbolKinds()` here.


================
Comment at: clangd/ClangdServer.h:164
+  void workspaceSymbols(StringRef Query,
+                        const clangd::WorkspaceSymbolOptions &Opts,
+                        const DraftStore &DS,
----------------
I think it would probably be more consistent with other functions to just take `int limit` here. I'm not sure CodeCompletion is an example we want to emulate.

ClangdLSPServer might even just grab it from the code completion options, since that's the behavior we actually want.

Up to you, though.


================
Comment at: clangd/ClangdServer.h:165
+                        const clangd::WorkspaceSymbolOptions &Opts,
+                        const DraftStore &DS,
+                        Callback<std::vector<SymbolInformation>> CB);
----------------
maybe a short FIXME here too e.g. "remove param when the index has line/col"


================
Comment at: clangd/FindSymbols.cpp:42
+
+  if (!supportedSymbolKinds) {
+    // Provide some sensible default when all fails.
----------------
malaperle wrote:
> sammccall wrote:
> > This code shouldn't need to handle this case. The LSP specifies the default set of supported types if it's not provided, so ClangdLSPServer should enure we always have a valid set
> My thought is that if we start dealing with one of those:
> ```
>   Object = 19,
>   Key = 20,
>   Null = 21,
>   Event = 24,
>   Operator = 25,
>   TypeParameter = 26
> ```
> and it's an older client that only supports up to "Array = 18", then we can provide a fallback, otherwise the client my not handle the unknown kind gracefully. But we can add this later if necessary I guess.
Ah yes I agree, fallbacks are good. I just meant that ClangdLSPServer should ensure that we actually have the bitset populated with the right values. I think it looks good now.


================
Comment at: clangd/Protocol.cpp:209
+  default:
+    llvm_unreachable("Unexpected symbol kind");
+  }
----------------
This doesn't actually seem unreachable (or won't stay that way), maybe log and return `Null` or something like that? (Wow, there's really no catch-all option for this mandatory enum...)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44882