[clangd] Enforce rules around "initialize" request, and create ClangdServer lazily.

Summary:
LSP is a slightly awkward map to C++ object lifetimes: the initialize request
is part of the protocol and provides information that doesn't change over the
lifetime of the server.

Until now, we handled this by initializing ClangdServer and ClangdLSPServer
right away, and making anything that can be set in the "initialize" request
mutable.
With this patch, we create ClangdLSPServer immediately, but defer creating
ClangdServer until "initialize". This opens the door to passing the relevant
initialize params in the constructor and storing them immutably.
(That change isn't actually done in this patch).

To make this safe, we have the MessageDispatcher enforce that the "initialize"
method is called before any other (as required by LSP). That way each method
handler can assume Server is initialized, as today.

As usual, while implementing this I found places where our test cases violated
the protocol.

Reviewers: ioeric

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D53398

llvm-svn: 344741

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -87,6 +87,7 @@ CompletionItemKindBitset defaultCompletionItemKinds() {
 //  - logging of inbound messages
 //  - cancellation handling
 //  - basic call tracing
+// MessageHandler ensures that initialize() is called before any other handler.
 class ClangdLSPServer::MessageHandler : public Transport::MessageHandler {
 public:
   MessageHandler(ClangdLSPServer &Server) : Server(Server) {}
@@ -95,7 +96,9 @@ public:
     log("<-- {0}", Method);
     if (Method == "exit")
       return false;
-    if (Method == "$/cancelRequest")
+    if (!Server.Server)
+      elog("Notification {0} before initialization", Method);
+    else if (Method == "$/cancelRequest")
       onCancel(std::move(Params));
     else if (auto Handler = Notifications.lookup(Method))
       Handler(std::move(Params));
@@ -106,7 +109,11 @@ public:
 
   bool onCall(StringRef Method, json::Value Params, json::Value ID) override {
     log("<-- {0}({1})", Method, ID);
-    if (auto Handler = Calls.lookup(Method))
+    if (!Server.Server && Method != "initialize") {
+      elog("Call {0} before initialization.", Method);
+      Server.reply(ID, make_error<LSPError>("server not initialized",
+                                            ErrorCode::ServerNotInitialized));
+    } else if (auto Handler = Calls.lookup(Method))
       Handler(std::move(Params), std::move(ID));
     else
       Server.reply(ID, llvm::make_error<LSPError>("method not found",
@@ -254,6 +261,11 @@ void ClangdLSPServer::reply(llvm::json::Value ID,
 
 void ClangdLSPServer::onInitialize(const InitializeParams &Params,
                                    Callback<json::Value> Reply) {
+  if (Server)
+    return Reply(make_error<LSPError>("server already initialized",
+                                      ErrorCode::InvalidRequest));
+  Server.emplace(CDB.getCDB(), FSProvider,
+                 static_cast<DiagnosticsConsumer &>(*this), ClangdServerOpts);
   if (Params.initializationOptions) {
     const ClangdInitializationOptions &Opts = *Params.initializationOptions;
 
@@ -663,8 +675,7 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
                                      std::move(CompileCommandsDir))),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
       SupportedCompletionItemKinds(defaultCompletionItemKinds()),
-      Server(new ClangdServer(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this,
-                              Opts)) {
+      ClangdServerOpts(Opts) {
   // clang-format off
   MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize);
   MsgHandler->bind("shutdown", &ClangdLSPServer::onShutdown);
@@ -694,8 +705,6 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
 ClangdLSPServer::~ClangdLSPServer() = default;
 
 bool ClangdLSPServer::run() {
-  assert(Server);
-
   // Run the Language Server loop.
   bool CleanExit = true;
   if (auto Err = Transp.loop(*MsgHandler)) {
@@ -705,7 +714,6 @@ bool ClangdLSPServer::run() {
 
   // Destroy ClangdServer to ensure all worker threads finish.
   Server.reset();
-
   return CleanExit && ShutdownRequestReceived;
 }
 

clang-tools-extra/clangd/ClangdLSPServer.h

@@ -183,12 +183,10 @@ private:
   // Store of the current versions of the open documents.
   DraftStore DraftMgr;
 
-  // Server must be the last member of the class to allow its destructor to exit
-  // the worker thread that may otherwise run an async callback on partially
-  // destructed instance of ClangdLSPServer.
-  // Set in construtor and destroyed when run() finishes. To ensure all worker
-  // threads exit before run() returns.
-  std::unique_ptr<ClangdServer> Server;
+  // The ClangdServer is created by the "initialize" LSP method.
+  // It is destroyed before run() returns, to ensure worker threads exit.
+  ClangdServer::Options ClangdServerOpts;
+  llvm::Optional<ClangdServer> Server;
 };
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/ClangdServer.cpp

@@ -103,7 +103,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            const Options &Opts)
     : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
-      ResourceDir(Opts.ResourceDir ? Opts.ResourceDir->str()
+      ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
                                    : getStandardResourceDir()),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.URISchemes,

clang-tools-extra/clangd/ClangdServer.h

@@ -90,7 +90,7 @@ public:
     /// defaults and -resource-dir compiler flag).
     /// If None, ClangdServer calls CompilerInvocation::GetResourcePath() to
     /// obtain the standard resource directory.
-    llvm::Optional<StringRef> ResourceDir = llvm::None;
+    llvm::Optional<std::string> ResourceDir = llvm::None;
 
     /// Time to wait after a new file version before computing diagnostics.
     std::chrono::steady_clock::duration UpdateDebounce =

clang-tools-extra/test/clangd/exit-with-shutdown.test

@@ -1,4 +1,6 @@
 # RUN: clangd -lit-test < %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/test/clangd/exit-without-shutdown.test

@@ -1,2 +1,4 @@
 # RUN: not clangd -lit-test < %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
 {"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/test/clangd/initialize-sequence.test

@@ -0,0 +1,21 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+#      CHECK:  "error": {
+# CHECK-NEXT:    "code": -32002
+# CHECK-NEXT:    "message": "server not initialized"
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "id": 0,
+---
+{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","id":2,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#      CHECK:  "error": {
+# CHECK-NEXT:    "code": -32600
+# CHECK-NEXT:    "message": "server already initialized"
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "id": 2,
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
+