[clangd] Make in-memory CDB always available as an overlay, refactor.

[sammccall]

Summary

The new implementation is a GlobalCompilationDatabase that overlays a base.
Normally this is the directory-based CDB.
To preserve the behavior of compile_args_from=LSP, the base may be null.

The OverlayCDB is always present, and so the extensions to populate it
are always supported.

It also allows overriding the flags of the fallback command. This is
just unit-tested for now, but the plan is to expose this as an extension
on the initialize message. This addresses use cases like
https://github.com/thomasjo/atom-ide-cpp/issues/16

[ilya-biryukov]

FWIW, the old implementation of the CDB looked simpler (which makes sense, since it only allowed the in-memory compile commands, but the new implementation also falls back to the base CDB, i.e. it's now doing two things).
However, if we can't avoid this protocol extension, the change LG and the model behind it should hopefully make sense from the user's perspective.
Still not sure whether overriding the normal command (not the fallback command) when we also read from the CDB is a useful feature.

clangd/ClangdLSPServer.cpp

670 ↗

(On Diff #171034)

Is this an optimization to not trigger compile command changes?
Can we perform it on the CDB level to make sure we hit it in the future if more code calling setCompileCommand is added?

clangd/ClangdLSPServer.h

42 ↗

(On Diff #171034)

NIT: the negative variable names might cause confusion (i.e. the double negations are hard to read). Maybe use the name of the corresponding field (UseDirectoryCDB)?

139 ↗

(On Diff #171034)

Maybe add a comment on how the two compilation databases are combined?

clangd/GlobalCompilationDatabase.h

79 ↗

(On Diff #171034)

The name does not seem to fully capture what this class does, i.e. allowing to override compile commands for some files.
Maybe use OverridenCDB or something similar?

[sammccall]

In D53687#1275580, @ilya-biryukov wrote:

FWIW, the old implementation of the CDB looked simpler (which makes sense, since it only allowed the in-memory compile commands, but the new implementation also falls back to the base CDB, i.e. it's now doing two things).

Yeah, this is more complicated than the old InMemoryCompilationDatabase, but simpler than InMemoryCompilationDatabase + ClangdLSPServer::CompilationDb together, which it's replacing.

clangd/ClangdLSPServer.cpp

670 ↗

(On Diff #171034)

Is this an optimization to not trigger compile command changes?

This is my attempt to preserve the optimization from rL338597, without having to keep the more complex interface to setCompileCommand.
(And fix a bug in it: if there was an old command and you change it, you should refresh).

The optimisation seems kind of suspect to me to be honest (if it's worth doing, it's probably worth doing right - i.e. per-file, not globally), but it's easy enough to preserve for now.

Can we perform it on the CDB level to make sure we hit it in the future if more code calling setCompileCommand is added?

Not quite sure what you mean: ClangdLSPServer needs to check whether there were changes in order to decide whether to invalidate. Having CompilationDatabase *also* aware of changes seems more complex/coupled and no less error-prone.

clangd/ClangdLSPServer.h

42 ↗

(On Diff #171034)

I was reluctant to replace a bool constructor parameter with a bool that does the exact opposite, but we do only have one callsite...

clangd/GlobalCompilationDatabase.h

79 ↗

(On Diff #171034)

Hmm, I feel the opposite way about these names - overlaying is exactly selective overriding of some elements, and overriding is more vague.

I did try to think of some other names, I almost like "mutable" but it's not actually essential, e.g. we don't allow the fallback flags to be mutated.

[sammccall]

Switch boolean parameters to positive sense, rebase.

[ilya-biryukov]

LGTM

clangd/ClangdLSPServer.cpp

670 ↗

(On Diff #171034)

Reparsing the files if compile commands did not change is (almost) a no-op now, so not sure it is worth it either.
SG to keep it for now, though.

Not quite sure what you mean: ClangdLSPServer needs to check whether there were changes in order to decide whether to invalidate. Having CompilationDatabase *also* aware of changes seems more complex/coupled and no less error-prone.

I initially thought this patch also adds watches for changes to compile commands (we discussed those yesterday), but the assumption was wrong, please ignore the comment.

clangd/GlobalCompilationDatabase.h

79 ↗

(On Diff #171034)

Well, the comment is there so it's probably fine either way.
My reasoning behind "Overlay" is that it's usually a thing that combines two or more "layers", calling one after another, without introducing any extra functionality on its own.

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

 /// MessageHandler binds the implemented LSP methods (e.g. onInitialize) to
 /// corresponding JSON-RPC methods ("initialize").
 /// The server also supports $/cancelRequest (MessageHandler provides this).
 class ClangdLSPServer : private DiagnosticsConsumer {
 public:
   /// If \p CompileCommandsDir has a value, compile_commands.json will be
   /// loaded only from \p CompileCommandsDir. Otherwise, clangd will look
   /// for compile_commands.json in all parent directories of each file.
+  /// If UseDirBasedCDB is false, compile commands are not read from disk.
+  // FIXME: Clean up signature around CDBs.
   ClangdLSPServer(Transport &Transp, const clangd::CodeCompleteOptions &CCOpts,
-                  llvm::Optional<Path> CompileCommandsDir,
-                  bool ShouldUseInMemoryCDB, const ClangdServer::Options &Opts);
+                  llvm::Optional<Path> CompileCommandsDir, bool UseDirBasedCDB,
+                  const ClangdServer::Options &Opts);
   ~ClangdLSPServer();
 
   /// Run LSP server loop, communicating with the Transport provided in the
   /// constructor. This method must not be executed more than once.
   ///
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
private:
   bool ShutdownRequestReceived = false;
 
   std::mutex FixItsMutex;
   typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
   /// Caches FixIts per file and diagnostics
   llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
 
-  /// Encapsulates the directory-based or the in-memory compilation database
-  /// that's used by the LSP server.
-  class CompilationDB {
-  public:
-    static CompilationDB makeInMemory();
-    static CompilationDB
-    makeDirectoryBased(llvm::Optional<Path> CompileCommandsDir);
-
-    /// Sets the compilation command for a particular file.
-    /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
-    ///
-    /// \returns True if the File had no compilation command before.
-    bool
-    setCompilationCommandForFile(PathRef File,
-                                 tooling::CompileCommand CompilationCommand);
-
-    /// Returns a CDB that should be used to get compile commands for the
-    /// current instance of ClangdLSPServer.
-    GlobalCompilationDatabase &getCDB() { return *CDB; }
-
-  private:
-    CompilationDB(std::unique_ptr<GlobalCompilationDatabase> CDB,
-                  bool IsDirectoryBased)
-        : CDB(std::move(CDB)), IsDirectoryBased(IsDirectoryBased) {}
-
-    // if IsDirectoryBased is true, an instance of InMemoryCDB.
-    // If IsDirectoryBased is false, an instance of DirectoryBasedCDB.
-    std::unique_ptr<GlobalCompilationDatabase> CDB;
-    bool IsDirectoryBased;
-  };
-
   // Most code should not deal with Transport directly.
   // MessageHandler deals with incoming messages, use call() etc for outgoing.
   clangd::Transport &Transp;
   class MessageHandler;
   std::unique_ptr<MessageHandler> MsgHandler;
   std::atomic<int> NextCallID = {0};
   std::mutex TranspWriter;
   void call(StringRef Method, llvm::json::Value Params);
private:
   CompletionItemKindBitset SupportedCompletionItemKinds;
   // Whether the client supports CodeAction response objects.
   bool SupportsCodeAction = false;
 
   // Store of the current versions of the open documents.
   DraftStore DraftMgr;
 
   // The CDB is created by the "initialize" LSP method.
-  bool UseInMemoryCDB; // FIXME: make this a capability.
+  bool UseDirBasedCDB;                     // FIXME: make this a capability.
   llvm::Optional<Path> CompileCommandsDir; // FIXME: merge with capability?
-  llvm::Optional<CompilationDB> CDB;
+  std::unique_ptr<GlobalCompilationDatabase> BaseCDB;
+  // CDB is BaseCDB plus any comands overridden via LSP extensions.
+  llvm::Optional<OverlayCDB> CDB;
   // 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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDLSPSERVER_H

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

if (Params.rootUri && *Params.rootUri)
     ClangdServerOpts.WorkspaceRoot = Params.rootUri->file();
   else if (Params.rootPath && !Params.rootPath->empty())
     ClangdServerOpts.WorkspaceRoot = *Params.rootPath;
   if (Server)
     return Reply(make_error<LSPError>("server already initialized",
                                       ErrorCode::InvalidRequest));
   if (const auto &Dir = Params.initializationOptions.compilationDatabasePath)
     CompileCommandsDir = Dir;
-  CDB.emplace(UseInMemoryCDB
-                  ? CompilationDB::makeInMemory()
-                  : CompilationDB::makeDirectoryBased(CompileCommandsDir));
-  Server.emplace(CDB->getCDB(), FSProvider,
-                 static_cast<DiagnosticsConsumer &>(*this), ClangdServerOpts);
+  if (UseDirBasedCDB)
+    BaseCDB = llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(
+        CompileCommandsDir);
+  CDB.emplace(BaseCDB.get());
+  Server.emplace(*CDB, FSProvider, static_cast<DiagnosticsConsumer &>(*this),
+                 ClangdServerOpts);
   applyConfiguration(Params.initializationOptions.ConfigSettings);
 
   CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets;
   DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes;
   DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory;
   if (Params.capabilities.WorkspaceSymbolKinds)
     SupportedSymbolKinds |= *Params.capabilities.WorkspaceSymbolKinds;
   if (Params.capabilities.CompletionItemKinds)

 void ClangdLSPServer::applyConfiguration(
     const ConfigurationSettings &Settings) {
   // Per-file update to the compilation database.
   bool ShouldReparseOpenFiles = false;
   for (auto &Entry : Settings.compilationDatabaseChanges) {
     /// The opened files need to be reparsed only when some existing
     /// entries are changed.
     PathRef File = Entry.first;
-    if (!CDB->setCompilationCommandForFile(
-            File, tooling::CompileCommand(
-                      std::move(Entry.second.workingDirectory), File,
+    auto Old = CDB->getCompileCommand(File);
+    auto New =
+        tooling::CompileCommand(std::move(Entry.second.workingDirectory), File,
                                 std::move(Entry.second.compilationCommand),
-                      /*Output=*/"")))
-      ShouldReparseOpenFiles = true;
+                                /*Output=*/"");
+    if (Old != New)
+      CDB->setCompileCommand(File, std::move(New));
   }
   if (ShouldReparseOpenFiles)
     reparseOpenedFiles();
 }
 
 // FIXME: This function needs to be properly tested.
 void ClangdLSPServer::onChangeConfiguration(
     const DidChangeConfigurationParams &Params) {
   applyConfiguration(Params.settings);
 }
 
 void ClangdLSPServer::onReference(const ReferenceParams &Params,
                                   Callback<std::vector<Location>> Reply) {
   Server->findReferences(Params.textDocument.uri.file(), Params.position,
                          std::move(Reply));
 }
 
 ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  Optional<Path> CompileCommandsDir,
-                                 bool ShouldUseInMemoryCDB,
+                                 bool UseDirBasedCDB,
                                  const ClangdServer::Options &Opts)
     : Transp(Transp), MsgHandler(new MessageHandler(*this)), CCOpts(CCOpts),
       SupportedSymbolKinds(defaultSymbolKinds()),
       SupportedCompletionItemKinds(defaultCompletionItemKinds()),
-      UseInMemoryCDB(ShouldUseInMemoryCDB),
+      UseDirBasedCDB(UseDirBasedCDB),
       CompileCommandsDir(std::move(CompileCommandsDir)),
       ClangdServerOpts(Opts) {
   // clang-format off
   MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize);
   MsgHandler->bind("shutdown", &ClangdLSPServer::onShutdown);
   MsgHandler->bind("textDocument/rangeFormatting", &ClangdLSPServer::onDocumentRangeFormatting);
   MsgHandler->bind("textDocument/onTypeFormatting", &ClangdLSPServer::onDocumentOnTypeFormatting);
   MsgHandler->bind("textDocument/formatting", &ClangdLSPServer::onDocumentFormatting);

 }
 
 void ClangdLSPServer::reparseOpenedFiles() {
   for (const Path &FilePath : DraftMgr.getActiveFiles())
     Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                         WantDiagnostics::Auto);
 }
 
-ClangdLSPServer::CompilationDB ClangdLSPServer::CompilationDB::makeInMemory() {
-  return CompilationDB(llvm::make_unique<InMemoryCompilationDb>(),
-                       /*IsDirectoryBased=*/false);
-}
-
-ClangdLSPServer::CompilationDB
-ClangdLSPServer::CompilationDB::makeDirectoryBased(
-    Optional<Path> CompileCommandsDir) {
-  auto CDB = llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(
-      std::move(CompileCommandsDir));
-  return CompilationDB(std::move(CDB),
-                       /*IsDirectoryBased=*/true);
-}
-
-bool ClangdLSPServer::CompilationDB::setCompilationCommandForFile(
-    PathRef File, tooling::CompileCommand CompilationCommand) {
-  if (IsDirectoryBased) {
-    elog("Trying to set compile command for {0} while using directory-based "
-         "compilation database",
-         File);
-    return false;
-  }
-  return static_cast<InMemoryCompilationDb *>(CDB.get())
-      ->setCompilationCommandForFile(File, std::move(CompilationCommand));
-}
-
 } // namespace clangd
 } // namespace clang

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

private:
   mutable llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
 
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 
-/// Gets compile args from an in-memory mapping based on a filepath. Typically
-/// used by clients who provide the compile commands themselves.
-class InMemoryCompilationDb : public GlobalCompilationDatabase {
+/// Wraps another compilation database, and supports overriding the commands
+/// using an in-memory mapping.
+class OverlayCDB : public GlobalCompilationDatabase {
 public:
-  /// Gets compile command for \p File from the stored mapping.
+  // Base may be null, in which case no entries are inherited.
+  // FallbackFlags are added to the fallback compile command.
+  OverlayCDB(const GlobalCompilationDatabase *Base,
+             std::vector<std::string> FallbackFlags = {})
+      : Base(Base), FallbackFlags(std::move(FallbackFlags)) {}
+
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
+  tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
-  /// Sets the compilation command for a particular file.
-  ///
-  /// \returns True if the File had no compilation command before.
-  bool setCompilationCommandForFile(PathRef File,
-                                    tooling::CompileCommand CompilationCommand);
+  /// Sets or clears the compilation command for a particular file.
+  void
+  setCompileCommand(PathRef File,
+                    llvm::Optional<tooling::CompileCommand> CompilationCommand);
 
 private:
   mutable std::mutex Mutex;
   llvm::StringMap<tooling::CompileCommand> Commands; /* GUARDED_BY(Mut) */
+  const GlobalCompilationDatabase *Base;
+  std::vector<std::string> FallbackFlags;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H

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

DirectoryBasedGlobalCompilationDatabase::getCDBForFile(PathRef File) const {
   for (auto Path = path::parent_path(File); !Path.empty();
        Path = path::parent_path(Path))
     if (auto CDB = getCDBInDirLocked(Path))
       return CDB;
   return nullptr;
 }
 
 Optional<tooling::CompileCommand>
-InMemoryCompilationDb::getCompileCommand(PathRef File) const {
+OverlayCDB::getCompileCommand(PathRef File) const {
+  {
     std::lock_guard<std::mutex> Lock(Mutex);
     auto It = Commands.find(File);
-  if (It == Commands.end())
-    return None;
+    if (It != Commands.end())
       return It->second;
   }
+  return Base ? Base->getCompileCommand(File) : None;
+}
+
+tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
+  auto Cmd = Base ? Base->getFallbackCommand(File)
+                  : GlobalCompilationDatabase::getFallbackCommand(File);
+  std::lock_guard<std::mutex> Lock(Mutex);
+  Cmd.CommandLine.insert(Cmd.CommandLine.end(), FallbackFlags.begin(),
+                         FallbackFlags.end());
+  return Cmd;
+}
 
-bool InMemoryCompilationDb::setCompilationCommandForFile(
-    PathRef File, tooling::CompileCommand CompilationCommand) {
+void OverlayCDB::setCompileCommand(
+    PathRef File, llvm::Optional<tooling::CompileCommand> Cmd) {
   std::unique_lock<std::mutex> Lock(Mutex);
-  auto ItInserted = Commands.insert(std::make_pair(File, CompilationCommand));
-  if (ItInserted.second)
-    return true;
-  ItInserted.first->setValue(std::move(CompilationCommand));
-  return false;
+  if (Cmd)
+    Commands[File] = std::move(*Cmd);
+  else
+    Commands.erase(File);
 }
 
 } // namespace clangd
 } // namespace clang

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

int main(int argc, char *argv[]) {
   // Change stdin to binary to not lose \r\n on windows.
   sys::ChangeStdinToBinary();
   auto Transport = newJSONTransport(
       stdin, outs(),
       InputMirrorStream ? InputMirrorStream.getPointer() : nullptr, PrettyPrint,
       InputStyle);
   ClangdLSPServer LSPServer(
       *Transport, CCOpts, CompileCommandsDirPath,
-      /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
+      /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs, Opts);
   constexpr int NoShutdownRequestErrorCode = 1;
   set_thread_name("clangd.main");
   return LSPServer.run() ? 0 : NoShutdownRequestErrorCode;
 }

clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp

TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   EXPECT_EQ(Cmd.Output, "");
 
   // .h files have unknown language, so they are parsed liberally as obj-c++.
   Cmd = DB.getFallbackCommand(testPath("foo/bar.h"));
   EXPECT_THAT(Cmd.CommandLine, ElementsAre("clang", "-xobjective-c++-header",
                                            testPath("foo/bar.h")));
 }
 
+static tooling::CompileCommand cmd(StringRef File, StringRef Arg) {
+  return tooling::CompileCommand(testRoot(), File, {"clang", Arg, File}, "");
+}
+
+class OverlayCDBTest : public ::testing::Test {
+  class BaseCDB : public GlobalCompilationDatabase {
+  public:
+    Optional<tooling::CompileCommand>
+    getCompileCommand(StringRef File) const override {
+      if (File == testPath("foo.cc"))
+        return cmd(File, "-DA=1");
+      return None;
+    }
+
+    tooling::CompileCommand getFallbackCommand(StringRef File) const override {
+      return cmd(File, "-DA=2");
+    }
+  };
+
+protected:
+  OverlayCDBTest() : Base(llvm::make_unique<BaseCDB>()) {}
+  std::unique_ptr<GlobalCompilationDatabase> Base;
+};
+
+TEST_F(OverlayCDBTest, GetCompileCommand) {
+  OverlayCDB CDB(Base.get());
+  EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")),
+            Base->getCompileCommand(testPath("foo.cc")));
+  EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
+
+  auto Override = cmd(testPath("foo.cc"), "-DA=3");
+  CDB.setCompileCommand(testPath("foo.cc"), Override);
+  EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")), Override);
+  EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
+  CDB.setCompileCommand(testPath("missing.cc"), Override);
+  EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), Override);
+}
+
+TEST_F(OverlayCDBTest, GetFallbackCommand) {
+  OverlayCDB CDB(Base.get(), {"-DA=4"});
+  EXPECT_THAT(CDB.getFallbackCommand(testPath("bar.cc")).CommandLine,
+              ElementsAre("clang", "-DA=2", testPath("bar.cc"), "-DA=4"));
+}
+
+TEST_F(OverlayCDBTest, NoBase) {
+  OverlayCDB CDB(nullptr, {"-DA=6"});
+  EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), None);
+  auto Override = cmd(testPath("bar.cc"), "-DA=5");
+  CDB.setCompileCommand(testPath("bar.cc"), Override);
+  EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), Override);
+
+  EXPECT_THAT(CDB.getFallbackCommand(testPath("foo.cc")).CommandLine,
+              ElementsAre("clang", testPath("foo.cc"), "-DA=6"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang