[clangd] Enable snippet completion based on client capabilities.

[ilya-biryukov]

Summary

And remove -enable-snippets flag.

[ioeric]

lgtm

clangd/ClangdLSPServer.cpp

90 ↗

(On Diff #134016)

This was moved from the end to the top. Is this related to the snippet change here?

clangd/Protocol.h

184 ↗

(On Diff #134016)

Use default Optional? I think we would treat empty optional the same as false anyway?

clangd/tool/ClangdMain.cpp

60 ↗

(On Diff #134016)

Would we still have a way to disable snippets (e.g. for debugging) even if clients support them? Maybe make this a hidden option instead?

[ilya-biryukov]

• Remove initializers from Optional<> fields

clangd/ClangdLSPServer.cpp

90 ↗

(On Diff #134016)

Not really, I just thought it's nicer to reply after we process the request (e.g., if we crash for any reason it will happen before client receives the reply).
Can move it back down. WDYT?

clangd/Protocol.h

184 ↗

(On Diff #134016)

Thanks for spotting that, I forgot to update this.

clangd/tool/ClangdMain.cpp

60 ↗

(On Diff #134016)

I'm tempted to say in that case we could just recompile clangd so that it ignores the options (i.e. change onInitialized). The code that will take it into account would be a bit trickier (you can't just pass CodeCompleteOptions to ClangdLSPServer, we'll also have to pass Optional<bool> OverridenEnableSnippets).

[sammccall]

clangd/tool/ClangdMain.cpp

60 ↗

(On Diff #134016)

+1 to this - this is really a debugging option, and supporting it has a cost.

If we want debugging options to live forever (may be reasonable!) we need a more scalable way to define them than listing them all in ClangdMain.

test/clangd/completion-snippets-disabled.test

1 ↗

(On Diff #134405)

Please don't add three new lit tests for this. One should be certainly enough (off, assuming our normal completion test has this feature on).

But really I think this can be a unit test in CodeComplete tests instead, right?

If you really want to verify how missing snippetSupport parses, that's a unittest for protocol.h right? But it doesn't seem important.

[ilya-biryukov]

test/clangd/completion-snippets-disabled.test

1 ↗

(On Diff #134405)

Please don't add three new lit tests for this. One should be certainly enough (off, assuming our normal completion test has this feature on).

Our completion lit test include empty capabilities, we don't test it there at all.

But really I think this can be a unit test in CodeComplete tests instead, right?

We already have a test for that in CodeComplete, but I think we should have a small integration test that cover all three cases where snippetSupport is on, off and missing.
We could sneak in the "missing" case into the existing completion.test, but on/off tests are useful to have from my perspective.

If you really want to verify how missing snippetSupport parses, that's a unittest for protocol.h right? But it doesn't seem important.

I'm not sure checking the parsing code makes sense (it's close to being "correct by construction" from my point of view). Checking that LSP outputs the right thing seems useful, though.

[ilya-biryukov]

• Use default values for configuration instead of using Optional<> fields.
• Removed redundant tests.

[ilya-biryukov]

• Removed completion-snippets-missing.test

[ilya-biryukov]

As discussed offline with @sammccall, we don't really need to hold Optional<> fields for configuration entries, as we only consume them in clangd and therefore can just set the default values explicitly.
Makes the code simpler and requires only one extra test (with snippets enabled).

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

std::vector<TextEdit> replacementsToEdits(StringRef Code,
   for (const auto &R : Repls)
     Edits.push_back(replacementToEdit(Code, R));
   return Edits;
 }
 
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
+  if (Params.rootUri && !Params.rootUri->file.empty())
+    Server.setRootPath(Params.rootUri->file);
+  else if (Params.rootPath && !Params.rootPath->empty())
+    Server.setRootPath(*Params.rootPath);
+
+  CCOpts.EnableSnippets =
+      Params.capabilities.textDocument.completion.completionItem.snippetSupport;
+
   reply(json::obj{
       {{"capabilities",
         json::obj{
             {"textDocumentSync", 1},
             {"documentFormattingProvider", true},
             {"documentRangeFormattingProvider", true},
             {"documentOnTypeFormattingProvider",
              json::obj{
reply(json::obj{
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
             {"renameProvider", true},
             {"executeCommandProvider",
              json::obj{
                  {"commands", {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}},
              }},
         }}}});
-  if (Params.rootUri && !Params.rootUri->file.empty())
-    Server.setRootPath(Params.rootUri->file);
-  else if (Params.rootPath && !Params.rootPath->empty())
-    Server.setRootPath(*Params.rootPath);
 }
 
 void ClangdLSPServer::onShutdown(ShutdownParams &Params) {
   // Do essentially nothing, just say we're ready to exit.
   ShutdownRequestReceived = true;
   reply(nullptr);
 }
 

clang-tools-extra/trunk/clangd/Protocol.h

 };
 bool fromJSON(const json::Expr &E, TraceLevel &Out);
 
 struct NoParams {};
 inline bool fromJSON(const json::Expr &, NoParams &) { return true; }
 using ShutdownParams = NoParams;
 using ExitParams = NoParams;
 
+struct CompletionItemClientCapabilities {
+  /// Client supports snippets as insert text.
+  bool snippetSupport = false;
+  /// Client supports commit characters on a completion item.
+  bool commitCharacterSupport = false;
+  // Client supports the follow content formats for the documentation property.
+  // The order describes the preferred format of the client.
+  // NOTE: not used by clangd at the moment.
+  // std::vector<MarkupKind> documentationFormat;
+};
+bool fromJSON(const json::Expr &, CompletionItemClientCapabilities &);
+
+struct CompletionClientCapabilities {
+  /// Whether completion supports dynamic registration.
+  bool dynamicRegistration = false;
+  /// The client supports the following `CompletionItem` specific capabilities.
+  CompletionItemClientCapabilities completionItem;
+  // NOTE: not used by clangd at the moment.
+  // llvm::Optional<CompletionItemKindCapabilities> completionItemKind;
+
+  /// The client supports to send additional context information for a
+  /// `textDocument/completion` request.
+  bool contextSupport = false;
+};
+bool fromJSON(const json::Expr &, CompletionClientCapabilities &);
+
+// FIXME: most of the capabilities are missing from this struct. Only the ones
+// used by clangd are currently there.
+struct TextDocumentClientCapabilities {
+  /// Capabilities specific to the `textDocument/completion`
+  CompletionClientCapabilities completion;
+};
+bool fromJSON(const json::Expr &, TextDocumentClientCapabilities &);
+
+struct ClientCapabilities {
+  // Workspace specific client capabilities.
+  // NOTE: not used by clangd at the moment.
+  // WorkspaceClientCapabilities workspace;
+
+  // Text document specific client capabilities.
+  TextDocumentClientCapabilities textDocument;
+};
+
+bool fromJSON(const json::Expr &, ClientCapabilities &);
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
   /// process. If the parent process is not alive then the server should exit
   /// (see exit notification) its process.
   llvm::Optional<int> processId;
 
   /// The rootPath of the workspace. Is null
   /// if no folder is open.
   ///
   /// @deprecated in favour of rootUri.
   llvm::Optional<std::string> rootPath;
 
   /// The rootUri of the workspace. Is null if no
   /// folder is open. If both `rootPath` and `rootUri` are set
   /// `rootUri` wins.
   llvm::Optional<URIForFile> rootUri;
 
   // User provided initialization options.
   // initializationOptions?: any;
 
   /// The capabilities provided by the client (editor or tool)
-  /// Note: Not currently used by clangd
-  // ClientCapabilities capabilities;
+  ClientCapabilities capabilities;
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional<TraceLevel> trace;
 };
 bool fromJSON(const json::Expr &, InitializeParams &);
 
 struct DidOpenTextDocumentParams {
   /// The document that was opened.

clang-tools-extra/trunk/clangd/Protocol.cpp

if (auto S = E.asString()) {
     } else if (*S == "verbose") {
       Out = TraceLevel::Verbose;
       return true;
     }
   }
   return false;
 }
 
+bool fromJSON(const json::Expr &Params, CompletionItemClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+    return false;
+  O.map("snippetSupport", R.snippetSupport);
+  O.map("commitCharacterSupport", R.commitCharacterSupport);
+  return true;
+}
+
+bool fromJSON(const json::Expr &Params, CompletionClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+    return false;
+  O.map("dynamicRegistration", R.dynamicRegistration);
+  O.map("completionItem", R.completionItem);
+  O.map("contextSupport", R.contextSupport);
+  return true;
+}
+
+bool fromJSON(const json::Expr &Params, TextDocumentClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+    return false;
+  O.map("completion", R.completion);
+  return true;
+}
+
+bool fromJSON(const json::Expr &Params, ClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+    return false;
+  O.map("textDocument", R.textDocument);
+  return true;
+}
+
 bool fromJSON(const json::Expr &Params, InitializeParams &R) {
   json::ObjectMapper O(Params);
   if (!O)
     return false;
   // We deliberately don't fail if we can't parse individual fields.
   // Failing to handle a slightly malformed initialize would be a disaster.
   O.map("processId", R.processId);
   O.map("rootUri", R.rootUri);
   O.map("rootPath", R.rootPath);
+  O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
   // initializationOptions, capabilities unused
   return true;
 }
 
 bool fromJSON(const json::Expr &Params, DidOpenTextDocumentParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&

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

llvm::cl::desc("Specify a path to look for compile_commands.json. If path "
                    "is invalid, clangd will look in the current directory and "
                    "parent paths of each source file."));
 
 static llvm::cl::opt<unsigned>
     WorkerThreadsCount("j",
                        llvm::cl::desc("Number of async workers used by clangd"),
                        llvm::cl::init(getDefaultAsyncThreadsCount()));
 
-static llvm::cl::opt<bool> EnableSnippets(
-    "enable-snippets",
-    llvm::cl::desc(
-        "Present snippet completions instead of plaintext completions. "
-        "This also enables code pattern results." /* FIXME: should it? */),
-    llvm::cl::init(clangd::CodeCompleteOptions().EnableSnippets));
-
 // FIXME: Flags are the wrong mechanism for user preferences.
 // We should probably read a dotfile or similar.
 static llvm::cl::opt<bool> IncludeIneligibleResults(
     "include-ineligible-results",
     llvm::cl::desc(
         "Include ineligible completion results (e.g. private members)"),
     llvm::cl::init(clangd::CodeCompleteOptions().IncludeIneligibleResults),
     llvm::cl::Hidden);
int main(int argc, char *argv[]) {
 
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
 
   std::unique_ptr<SymbolIndex> StaticIdx;
   if (EnableIndexBasedCompletion && !YamlSymbolFile.empty())
     StaticIdx = BuildStaticIndex(YamlSymbolFile);
   clangd::CodeCompleteOptions CCOpts;
-  CCOpts.EnableSnippets = EnableSnippets;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitCompletionResult;
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(Out, WorkerThreadsCount, StorePreamblesInMemory,
                             CCOpts, ResourceDirRef, CompileCommandsDirPath,
                             EnableIndexBasedCompletion, StaticIdx.get());
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   return LSPServer.run(std::cin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
 }

clang-tools-extra/trunk/test/clangd/completion-snippets.test

+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s
+{
+  "jsonrpc": "2.0",
+  "id": 0,
+  "method": "initialize",
+  "params": {
+    "processId": 123,
+    "rootPath": "clangd",
+    "capabilities": {
+      "textDocument": {
+        "completion": {
+          "completionItem": {
+            "snippetSupport": true
+          }
+        }
+      }
+    },
+    "trace": "off"
+  }
+}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"int func_with_args(int a, int b);\nint main() {\nfunc_with\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":7}}}
+#      CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:    "isIncomplete": {{.*}}
+# CHECK-NEXT:    "items": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "detail": "int",
+# CHECK-NEXT:      "filterText": "func_with_args",
+# CHECK-NEXT:      "insertText": "func_with_args(${1:int a}, ${2:int b})",
+# CHECK-NEXT:      "insertTextFormat": 2,
+# CHECK-NEXT:      "kind": 3,
+# CHECK-NEXT:      "label": "func_with_args(int a, int b)",
+# CHECK-NEXT:      "sortText": "{{.*}}func_with_args"
+# CHECK-NEXT:    }
+# CHECK-NEXT:    ]
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}