Allow un-setting the compilation database path

[simark]

Summary

It is currently possible to tell clangd where to find the
compile_commands.json file through the initializationOptions or the
didChangeConfiguration message. However, it is not possible to tell
clangd to deselect any explicit compilation database path (i.e. go back
to the default).

This patch makes it possible by sending the value null:

params: {
  "settings": {
    "compilationDatabasePath": null
  }
}

Not including the compilationDatabasePath field doesn't change the value
(which means it doesn't deselect it if one is already set). I chose to
do it this way because the other possible field,
compilationDatabaseChanges, just contains a delta. So I think it makes
sense if compilationDatabasePath works the same way.

[simark]

Fix formatting.

[simark]

Not sure who should review this, please feel free to add anybody who would be appropriate.

[ilya-biryukov]

Wow, this is getting somewhat complicated.

Have you considered rerunning clangd whenever someone changes an option like that?
Would that be much more complicated on your side?

Not opposed to having an option too, just want to be aware of the costs involved on your end.

clangd/Protocol.h

368

Not a big fan or something like this, but maybe give special meaning to empty path instead of wrapping an optional into an optional?

Double optionals are a real pain to write and read.

[simark]

In D51725#1232092, @ilya-biryukov wrote:

Wow, this is getting somewhat complicated.

Have you considered rerunning clangd whenever someone changes an option like that?
Would that be much more complicated on your side?

Not opposed to having an option too, just want to be aware of the costs involved on your end.

Since we already support changing the compilation database path at runtime, I don't think it's significantly more complex to support resetting that value to the default of empty/not-used. If we can restart clangd when the users chooses to use no compilation database path, we might as well just restart it every time the user selects a new compilation database path.

I was assuming that restarting clangd might potentially be significantly more costly than just changing a setting, but maybe I'm wrong. The point of switching compilation database path is usually to point to a different build that has different flags, so all open files will get reparsed anyway... And just start clangd isn't particularly heavy.

I'll investigate how difficult it is to make our frontend (Theia) restart clangd when the user switches compilation database.

[ilya-biryukov]

If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)?

In D51725#1232171, @simark wrote:

I was assuming that restarting clangd might potentially be significantly more costly than just changing a setting, but maybe I'm wrong.

It's only cheaper if changes in the compilation database paths don't affect compile commands. If they do, everything will get rebuilt on next access, which is costly.

The point of switching compilation database path is usually to point to a different build that has different flags, so all open files will get reparsed anyway... And just start clangd isn't particularly heavy.

+1, since all compile commands change, we probably never get a performance win in practice.
One thing that (I believe) we're currently doing is using the old preamble for completion. This allow to code complete before reparse of preamble is finished.
I'm not sure this works when changing compile commands, though. Have not seen the breakages yet, but it's also neither something we've been testing nor something that pops up often in practice.

I'll investigate how difficult it is to make our frontend (Theia) restart clangd when the user switches compilation database.

If that is feasible, might allow removing some code from clangd. Though don't expect that amount to be too high.
One potential complication to restoring the state of the language server. It should be just the list of open files, but I may be missing something.

If you'll decide to go with an option to reset the path, see the comment about making empty path special. Optional<Optional<>> does not play nicely with json serialization code and hard to read (even though it does look like the right thing to do from the type system perspective).

[simark]

In D51725#1232748, @ilya-biryukov wrote:

If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)?

It is through a custom UI, that we name exactly that, build configurations. There is an example of the corresponding configuration here:

https://github.com/theia-ide/theia/blob/master/packages/cpp/README.md

I'll investigate how difficult it is to make our frontend (Theia) restart clangd when the user switches compilation database.

If that is feasible, might allow removing some code from clangd. Though don't expect that amount to be too high.
One potential complication to restoring the state of the language server. It should be just the list of open files, but I may be missing something.

I am not sure I understand correctly your last point. Of course, when restarting clangd, we need to send again a didOpen for each file the user has currently open in the editor. But that should be it?

If you'll decide to go with an option to reset the path, see the comment about making empty path special. Optional<Optional<>> does not play nicely with json serialization code and hard to read (even though it does look like the right thing to do from the type system perspective).

Yes, noted. I preferred to avoid giving a special meaning to an empty string because we could avoid it. But I wouldn't mind changing it if we go that route.

[kadircet]

clangd/Protocol.h

368

Sorry just passing by since I've seen a similar usage that already exists in codebase, wanted to point it out.

Some api like this could be easier, to make the decision of using empty path or anything else for an invalid path and making it an implementation detail.

https://github.com/llvm-mirror/clang/blob/master/include/clang/Sema/DeclSpec.h#L53

[ilya-biryukov]

In D51725#1233254, @simark wrote:

I am not sure I understand correctly your last point. Of course, when restarting clangd, we need to send again a didOpen for each file the user has currently open in the editor. But that should be it?

Yes, this and any initial configuration settings.
My personal experience is from VSCode, which does not call addDocument IIUC. This results in clangd producing errors about running actions in non-opened files.

If you'll decide to go with an option to reset the path, see the comment about making empty path special. Optional<Optional<>> does not play nicely with json serialization code and hard to read (even though it does look like the right thing to do from the type system perspective).

Yes, noted. I preferred to avoid giving a special meaning to an empty string because we could avoid it. But I wouldn't mind changing it if we go that route.

Also not a big fan of it, but Optional<Optional<>> looks terrible. Maybe I'm missing an obvious pattern to make it simpler here, though.
WDYT about deserializing null to empty string? It would mean that specifying both null and "" in json configuration yields the same results, but JSON that we send would look a bit nicer.

clangd/Protocol.h

368

It makes it less straight-forward, though.

[simark]

So I revisited this today. In the end, restarting clangd in our IDE works well. It should be merged anytime soon:

https://github.com/theia-ide/theia/pull/3017

But I am wondering what to do with the feature that allows clangd to change compilation database path using the didChangeConfiguration notification. In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged. Or, if we decide this is really not useful, then it should be removed. I don't think we should keep the feature in the current state, either we fix it or remove it.

Even if right now it's not very expensive to restart clangd, do you foresee a situation in the future where it could become more expensive?

Also, I tried to remove the double Optional and do as you suggested (use the empty string to mean to use no explicit compilation database path), and in the end I find it more confusing, because we need special cases at both ends. When we deserialize the JSON and the value is null, and when storing the value in DirectoryBasedGlobalCompilationDatabase, where we convert an empty string to an empty Optional.

[ilya-biryukov]

In D51725#1255695, @simark wrote:

But I am wondering what to do with the feature that allows clangd to change compilation database path using the didChangeConfiguration notification. In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged. Or, if we decide this is really not useful, then it should be removed. I don't think we should keep the feature in the current state, either we fix it or remove it.

Fair point. The original patch looked scary mostly because of Optional<Optional<...>>. Many thanks for clearing up your use-case, though. I thought Theia was exclusively using custom directories for compile_commands.json, I didn't know the user had an option to turn this behavior on/off and configure directories explicitly.
I would question the usefulness of the option if Theia can live without it, so I'd vouch for removing it. @sammccall, WDYT? Especially wrt to auto-indexing and it being a customizable build directory?

Even if right now it's not very expensive to restart clangd, do you foresee a situation in the future where it could become more expensive?

We should keep clangd restarts fast, I don't see any reason why they should be slow.
Changing compile_commands.json files would probably always require both reparsing all the files and reloading all the indexes, so there's little hope we can make clangd faster in those cases, unless we're willing to keep multiple versions of the AST/indexes in memory.

Also, I tried to remove the double Optional and do as you suggested (use the empty string to mean to use no explicit compilation database path), and in the end I find it more confusing, because we need special cases at both ends. When we deserialize the JSON and the value is null, and when storing the value in DirectoryBasedGlobalCompilationDatabase, where we convert an empty string to an empty Optional.

Yeah, moving this special meaning into all layers was a bad idea.
Maybe replace an empty optional with an empty string on deseraizliation? The rest of the code using Optional<Path> seems fine, it's just the ClangdConfigurationParamsChange definition that looks complicated.
The code for deserializing would look like this:

bool fromJSON(const json::Value &Params, ClangdConfigurationParamsChange &CCPC) {
  json::ObjectMapper O(Params);
  if (!O)
    return ...;

  if (const Value* CDBPath = Params->get("compilationDatabasePath")) {
    // Have to update the DB path, CCPC.compilationDatabasePath will have a value.
    llvm::Optional<std::string> NewDBPath;
    if (!fromJSON(CDBPath, NewDBPath))
      return false;
    CCPC.compilationDatabasePath = NewDBPath ? std::move(NewDBPath) : "";
  } else {
    // No need to update DB path, CCPC.compilationDatabasePath will be llvm::None.
  }
  ...
}

[sammccall]

In D51725#1261798, @ilya-biryukov wrote:

In D51725#1255695, @simark wrote:

But I am wondering what to do with the feature that allows clangd to change compilation database path using the didChangeConfiguration notification. In my opinion, it's a bug that it's not possible to switch back to use no explicit compilation database path, so I'd still like to get this patch merged. Or, if we decide this is really not useful, then it should be removed. I don't think we should keep the feature in the current state, either we fix it or remove it.

I would question the usefulness of the option if Theia can live without it, so I'd vouch for removing it. @sammccall, WDYT? Especially wrt to auto-indexing and it being a customizable build directory?

Agree with you both here. Not being able to reset the initial state is weird, but *being able to* is complicated. (Even more so than switching between CDBs).
And since switching between CDBs invalidates almost everything, restarting clangd seems like a great simplification if it's viable for you.
Supporting a single CDB that's available on startup (either a flag, or maybe more likely set during initialize()?) would be a very useful simplification. (Even if it's not a lot of code, I find myself getting tangled in it often).

Even if right now it's not very expensive to restart clangd, do you foresee a situation in the future where it could become more expensive?

We should keep clangd restarts fast, I don't see any reason why they should be slow.

Agree, it's important that clangd starts fast for other reasons. And auto-indexing should stop/resume fine.
Shutdown is a slightly trickier case: actually doing a clean shutdown may require waiting for e.g. any current AST parse to finish. But if this is a problem, I think you can just send SIGTERM.

Changing compile_commands.json files would probably always require both reparsing all the files and reloading all the indexes, so there's little hope we can make clangd faster in those cases, unless we're willing to keep multiple versions of the AST/indexes in memory.

The only thing I think we can gain is if you change CDBs, but actually many of the commands are *completely* identical. Then we could detect it and avoid invalidating ASTs etc for those files. But this seems like a really marginal edge-case.

[simark]

Thanks for your input. I have opened https://reviews.llvm.org/D53220, which removes that option.

clangd/ClangdLSPServer.h

setCompilationCommandForFile(PathRef File,
                                  tooling::CompileCommand CompilationCommand);
 
     /// Adds extra compilation flags to the compilation command for a particular
     /// file. Only valid for directory-based CDB, no-op and error log on
     /// InMemoryCDB;
     void setExtraFlagsForFile(PathRef File,
                               std::vector<std::string> ExtraFlags);
 
-    /// Set the compile commands directory to \p P.
+    /// Set the compile commands directory to \p P.  An empty Optional value
+    /// means to not use an explicit compile commands directory path.
     /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
-    void setCompileCommandsDir(Path P);
+    void setCompileCommandsDir(llvm::Optional<Path> P);
 
     /// Returns a CDB that should be used to get compile commands for the
     /// current instance of ClangdLSPServer.
     GlobalCompilationDatabase &getCDB();
 
   private:
     CompilationDB(std::unique_ptr<GlobalCompilationDatabase> CDB,
                   std::unique_ptr<CachingCompilationDb> CachingCDB,

clangd/ClangdLSPServer.cpp

elog("Trying to set extra flags for {0} while using in-memory compilation "
          File);
     return;
   }
   static_cast<DirectoryBasedGlobalCompilationDatabase *>(CDB.get())
       ->setExtraFlagsForFile(File, std::move(ExtraFlags));
   CachingCDB->invalidate(File);
 }
 
-void ClangdLSPServer::CompilationDB::setCompileCommandsDir(Path P) {
+void ClangdLSPServer::CompilationDB::setCompileCommandsDir(
+    llvm::Optional<Path> P) {
   if (!IsDirectoryBased) {
     elog("Trying to set compile commands dir while using in-memory compilation "
          "database");
     return;
   }
   static_cast<DirectoryBasedGlobalCompilationDatabase *>(CDB.get())
-      ->setCompileCommandsDir(P);
+      ->setCompileCommandsDir(std::move(P));
   CachingCDB->clear();
 }
 
 GlobalCompilationDatabase &ClangdLSPServer::CompilationDB::getCDB() {
   if (CachingCDB)
     return *CachingCDB;
   return *CDB;
 }

clangd/GlobalCompilationDatabase.h

public:
   /// Any extra flags will be added.
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
   /// Uses the default fallback command, adding any extra flags.
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
-  void setCompileCommandsDir(Path P);
+  void setCompileCommandsDir(llvm::Optional<Path> P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
   tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
   void addExtraFlags(PathRef File, tooling::CompileCommand &C) const;

clangd/GlobalCompilationDatabase.cpp

 tooling::CompileCommand
 DirectoryBasedGlobalCompilationDatabase::getFallbackCommand(
     PathRef File) const {
   auto C = GlobalCompilationDatabase::getFallbackCommand(File);
   addExtraFlags(File, C);
   return C;
 }
 
-void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(
+    llvm::Optional<Path> P) {
   std::lock_guard<std::mutex> Lock(Mutex);
   CompileCommandsDir = P;
   CompilationDatabases.clear();
 }
 
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
     PathRef File, std::vector<std::string> ExtraFlags) {
   std::lock_guard<std::mutex> Lock(Mutex);

clangd/Protocol.h

struct ClangdCompileCommand {
   std::vector<std::string> compilationCommand;
 };
 bool fromJSON(const llvm::json::Value &, ClangdCompileCommand &);
 
 /// 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;
+  /// Path to the directory containing the compile_commands.json file to use.
+  /// The outer Optional is empty if the compilationDatabasePath field was not
+  /// present at all in the request.
+  /// The inner Optional is empty if the compilationDatabasePath field was
+  /// preset, but the value was null.
+  /// If the value of the compilationDatabasePath in the request is a string,
+  /// both Optionals are instantiated.
+  llvm::Optional<llvm::Optional<std::string>> compilationDatabasePath;
 
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
   llvm::Optional<std::map<std::string, ClangdCompileCommand>>
       compilationDatabaseChanges;
 };
 bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
 

clangd/Protocol.cpp

 
 bool fromJSON(const llvm::json::Value &Params,
               ClangdCompileCommand &CDbUpdate) {
   json::ObjectMapper O(Params);
   return O && O.map("workingDirectory", CDbUpdate.workingDirectory) &&
          O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
-bool fromJSON(const json::Value &Params,
+bool fromJSON(const json::Value &Settings,
               ClangdConfigurationParamsChange &CCPC) {
-  json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+  json::ObjectMapper O(Settings);
+  const json::Value *P = Settings.getAsObject()->get("compilationDatabasePath");
+
+  if (P) {
+    // The field is present.
+    CCPC.compilationDatabasePath.emplace();
+    llvm::Optional<StringRef> S = P->getAsString();
+    if (S) {
+      // The field has a string value.
+      CCPC.compilationDatabasePath->emplace(*S);
+    }
+  }
+
+  return O &&
          O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
 bool fromJSON(const json::Value &Params, ReferenceParams &R) {
   TextDocumentPositionParams &Base = R;
   return fromJSON(Params, Base);
 }
 

test/clangd/compile-commands-path.test

 ---
 {"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT:     "diagnostics": [
 # CHECK-NEXT:       {
 # CHECK-NEXT:         "message": "MACRO is two",
 ---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":null}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT:     "diagnostics": [
+# CHECK-NEXT:       {
+# CHECK-NEXT:         "message": "MACRO is not defined",
+---
 {"jsonrpc":"2.0","id":10000,"method":"shutdown"}