[clangd] Receive compilationDatabasePath in 'initialize' request

[simark]

Summary

That way, as soon as the "initialize" is received by the server, it can start
parsing/indexing with a valid compilation database and not have to wait for a
an initial 'didChangeConfiguration' that might or might not happen.
Then, when the user changes configuration, a didChangeConfiguration can be sent.

Signed-off-by: Marc-Andre Laperle <[e-mail]>

[ilya-biryukov]

Not strictly opposed to this change, but is there any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

[malaperle]

In D49833#1176337, @ilya-biryukov wrote:

Not strictly opposed to this change, but is there any reason why the clients can't guarantee they'll send didChangeConfiguration right after clangd is initialized?

LSP:

Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server.

So the client is free to send "didOpen" or any request after the "initialize", which means we could end up parsing files with wrong commands before we get the first "didChangeConfiguration". In fact, in our indexing prototype we start indexing as soon as the "initialize" is processed and we do not know whether or not there will be a didChangeConfiguration soon after.
Setting the CDB path as part of the initialize seems more natural and less error-prone.

[malaperle]

Was there any objection to this patch? It would have been nice to have this before the branching.

[ilya-biryukov]

In D49833#1181651, @malaperle wrote:

Was there any objection to this patch? It would have been nice to have this before the branching.

Sorry for the delay, somehow missed this update in my inbox.

In D49833#1177230, @malaperle wrote:

LSP:

Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server.

So the client is free to send "didOpen" or any request after the "initialize", which means we could end up parsing files with wrong commands before we get the first "didChangeConfiguration". In fact, in our indexing prototype we start indexing as soon as the "initialize" is processed and we do not know whether or not there will be a didChangeConfiguration soon after.
Setting the CDB path as part of the initialize seems more natural and less error-prone.

Thanks for clarifying the use-case, that does seem like the best option if you want to start indexing as soon as you initialize.

Overall LG, just a small suggestion on how to rearrange the options.

clangd/Protocol.h

362 ↗

(On Diff #157420)

Maybe add another level of indirection from the start to make sure we'll be able to add more init options later without breaking the existing clients?

/// Clangd-specific initialization options that can be passed on the initial initialization request.
struct ClangdInitOptions {
  llvm::Optional<ClangdConfigurationParamsChange> initialConfiguration; 
  // can add more options here in a backwards-compatible manner
};

struct InitializeParams {
  /// ....
  llvm::Optional<ClangdConfigurationParamsChange> initializationOptions;
};

[malaperle]

@simark would you mind finishing this patch and committing it? I won't be able to finish it given that I started it at a different company, etc.

[simark]

In D49833#1182615, @malaperle wrote:

@simark would you mind finishing this patch and committing it? I won't be able to finish it given that I started it at a different company, etc.

Yes, I'll take it over, thanks for getting it started.

[simark]

Address Ilya's comment (add an indirection for initializationOptions).

[ilya-biryukov]

LGTM with a small nit. Thanks for the change!

clangd/ClangdLSPServer.cpp

419 ↗

(On Diff #158366)

NIT: replace with applyConfiguration(Params.settings) to get rid of the extra local var

[simark]

Thanks, done.

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

private:
   void onChangeConfiguration(DidChangeConfigurationParams &Params) override;
 
   std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
 
   /// Forces a reparse of all currently opened files.  As a result, this method
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
   bool ShutdownRequestReceived = false;
 
   /// Used to indicate that the 'exit' notification was received from the
   /// Language Server client.

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

for (size_t I = SymbolKindMin; I <= static_cast<size_t>(SymbolKind::Array);
        ++I)
     Defaults.set(I);
   return Defaults;
 }
 
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
+  if (Params.initializationOptions)
+    applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
     Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
     Server.setRootPath(*Params.rootPath);
 
   CCOpts.EnableSnippets =
       Params.capabilities.textDocument.completion.completionItem.snippetSupport;
 
Server.findHover(Params.textDocument.uri.file(), Params.position,
                                   llvm::toString(H.takeError()));
                        return;
                      }
 
                      reply(*H);
                    });
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-    DidChangeConfigurationParams &Params) {
-  ClangdConfigurationParamsChange &Settings = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+    const ClangdConfigurationParamsChange &Settings) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
     NonCachedCDB.setCompileCommandsDir(
         Settings.compilationDatabasePath.getValue());
     CDB.clear();
 
     reparseOpenedFiles();
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+    DidChangeConfigurationParams &Params) {
+  applyConfiguration(Params.settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<Path> CompileCommandsDir,
                                  const ClangdServer::Options &Opts)
     : Out(Out), NonCachedCDB(std::move(CompileCommandsDir)), CDB(NonCachedCDB),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
       Server(CDB, FSProvider, /*DiagConsumer=*/*this, Opts) {}
 

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

struct ClientCapabilities {
   llvm::Optional<WorkspaceClientCapabilities> workspace;
 
   // Text document specific client capabilities.
   TextDocumentClientCapabilities textDocument;
 };
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional<std::string> compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+
 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
struct InitializeParams {
   // User provided initialization options.
   // initializationOptions?: any;
 
   /// The capabilities provided by the client (editor or tool)
   ClientCapabilities capabilities;
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional<TraceLevel> trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional<ClangdInitializationOptions> initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
 struct DidOpenTextDocumentParams {
   /// The document that was opened.
   TextDocumentItem textDocument;
 
   /// Extension storing per-file metadata, such as compilation flags.

 bool fromJSON(const llvm::json::Value &, FileEvent &);
 
 struct DidChangeWatchedFilesParams {
   /// The actual file events.
   std::vector<FileEvent> changes;
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional<std::string> compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
   ClangdConfigurationParamsChange settings;
 };
 bool fromJSON(const llvm::json::Value &, DidChangeConfigurationParams &);
 
 struct FormattingOptions {

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

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
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
 bool fromJSON(const json::Value &Params, DidOpenTextDocumentParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("metadata", R.metadata);
 }