[clangd] Allow observation of changes to global CDBs.

[sammccall]

Summary

Currently, changes *within* CDBs are not tracked (CDB has no facility to do so).
However, discovery of new CDBs are tracked (all files are marked as modified).
Also, files whose compilation commands are explicitly set are marked modified.

The intent is to use this for auto-index. Newly discovered files will be indexed
with low priority.

[ilya-biryukov]

clangd/Function.h

90

I assume the Event is supposed to be used only with non-reference and non-const qualified types.
Maybe add a static assert for that? Something like:
static_assert(std::is_same_v<std::decay_t<T>, T>)

135

As discussed offline, running the callbacks under the lock might cause deadlocks.
We probably won't hit this case in near future, but it would be nice to find a way to diagnose this situation. Not sure what we can do non-intrusively, though.

clangd/GlobalCompilationDatabase.h

72

Maybe add a /*Cached*/ comment here too?

unittests/clangd/GlobalCompilationDatabaseTests.cpp

98

Maybe add an actual test?

[sammccall]

Address comments.
Add missing OverlayCDB->Base watching (oops!)

[ilya-biryukov]

LGTM

clangd/Function.h

147

NIT: Maybe move this static_assert to the top of the class?
I'd argue this is part of the public interface as it puts constraints on the template parameters.

clangd/GlobalCompilationDatabase.cpp

73

NIT: maybe initialize Cached to a specific value as a precaution against introducing UB with some future changes? The current behavior itself seems correct, it's never accessed when uninitialized, albeit it takes some time to figure it out.

[ilya-biryukov]

clangd/Function.h

108

We need to unsubscribe this in the assignment operator before overwriting Parent.
Currently we won't delete this from the parent's listeners.

Maybe add a test for that too?

[sammccall]

Address comments, add test for Event machinery.

[sammccall]

clangd/Function.h

147

On balance I don't think I agree here.

I'm not sure about having the static assert at all, unless we have reason to believe it will compile but do the wrong thing. Happy to compromise by including it, but I don't think it belongs up front.
It's not very human-readable, and computers are just as happy with it in the private section. It's consistent with some other ways of disallowing compilation, like declaring members private.

[ilya-biryukov]

LGTM

clangd/Function.h

147

Reference collapsing rules can bite us here, e.g. I don't think making Event<int&> and Event<int&&> valid makes sense, even though both would compile.

It's consistent with some other ways of disallowing compilation, like declaring members private.

Would also argue those private members should be declared at the top of the class for the same reasons.

But we just have different opinions here, it's not terribly important.

clangd/Function.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUNCTION_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUNCTION_H
 
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/Support/Error.h"
+#include <mutex>
 #include <tuple>
 #include <utility>
 
 namespace clang {
 namespace clangd {
 
 /// A Callback<T> is a void function that accepts Expected<T>.
 /// This is accepted by ClangdServer functions that logically return T.

 /// The returned object must be called no more than once, as \p As are
 /// std::forwarded'ed (therefore can be moved) into \p F during the call.
 template <class Func, class... Args>
 ForwardBinder<Func, Args...> Bind(Func F, Args &&... As) {
   return ForwardBinder<Func, Args...>(
       std::make_tuple(std::forward<Func>(F), std::forward<Args>(As)...));
 }
 
+/// An Event<T> allows events of type T to be broadcast to listeners.
+template <typename T>
+class Event {
+public:
+  // A Listener is the callback through which events are delivered.
+  using Listener = std::function<void(const T&)>;
+
+  // A subscription defines the scope of when a listener should receive events.
+  // After destroying the subscription, no more events are received.
+  class Subscription {
+    Event *Parent;
+    unsigned ListenerID;
+
+    Subscription(Event *Parent, unsigned ListenerID)
+        : Parent(Parent), ListenerID(ListenerID) {}
+    friend Event;
+
+  public:
+    Subscription(Subscription &&Other) { *this = std::move(Other); }
+    Subscription &operator=(Subscription &&Other) {
+      std::tie(Parent, ListenerID) = std::tie(Other.Parent, Other.ListenerID);
+      Other.Parent = nullptr;
+      return *this;
+    }
+    // Destroying a subscription may block if an event is being broadcast.
+    ~Subscription() {
+      if (!Parent) return;
+      std::lock_guard<std::mutex>(Parent->ListenersMu);
+      llvm::erase_if(Parent->Listeners,
+                     [&](const std::pair<Listener, unsigned> &P) {
+                       return P.second == ListenerID;
+                     });
+    }
+  };
+
+  // Adds a listener that will observe all future events until the returned
+  // subscription is destroyed.
+  // May block if an event is currently being broadcast.
+  Subscription observe(Listener L) {
+    std::lock_guard<std::mutex> Lock(ListenersMu);
+    Listeners.push_back({std::move(L), ++ListenerCount});
+    return Subscription(this, ListenerCount);;
+  }
+
+  // Synchronously sends an event to all registered listeners.
+  void broadcast(const T& V) {
+    std::lock_guard<std::mutex> Lock(ListenersMu);
+    for (const auto &L : Listeners)
+      L.first(V);
+  }
+
+  ~Event() {
+    std::lock_guard<std::mutex> Lock(ListenersMu);
+    assert(Listeners.empty());
+  }
+
+private:
+  std::mutex ListenersMu;
+  std::vector<std::pair<Listener, unsigned>> Listeners;
+  unsigned ListenerCount = 0;
+};
+
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/GlobalCompilationDatabase.h

 //===--- GlobalCompilationDatabase.h -----------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H
 
 #include "Path.h"
+#include "Function.h"
 #include "llvm/ADT/StringMap.h"
 #include <memory>
 #include <mutex>
 #include <vector>
 
 namespace clang {
 
 namespace tooling {
public:
   virtual llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const = 0;
 
   /// Makes a guess at how to build a file.
   /// The default implementation just runs clang on the file.
   /// Clangd should treat the results as unreliable.
   virtual tooling::CompileCommand getFallbackCommand(PathRef File) const;
 
-  /// FIXME(ibiryukov): add facilities to track changes to compilation flags of
-  /// existing targets.
+  using CommandChanged = Event<std::vector<std::string>>;
+  /// The callback is notified when files may have new compile commands.
+  /// The argument is a list of full file paths.
+  CommandChanged::Subscription watch(CommandChanged::Listener L) {
+    return OnCommandChanged.observe(std::move(L));
+  }
+
+protected:
+  mutable CommandChanged OnCommandChanged;
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
 /// directories.
 class DirectoryBasedGlobalCompilationDatabase
     : public GlobalCompilationDatabase {
 public:
   DirectoryBasedGlobalCompilationDatabase(
       llvm::Optional<Path> CompileCommandsDir);
   ~DirectoryBasedGlobalCompilationDatabase() override;
 
   /// Scans File's parents looking for compilation databases.
   /// Any extra flags will be added.
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
-  tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
+  std::pair<tooling::CompilationDatabase *, bool>
+  getCDBInDirLocked(PathRef File) const;
 
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
   mutable llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
 
   /// Used for command argument pointing to folder where compile_commands.json

clangd/GlobalCompilationDatabase.cpp

if (auto CDB = getCDBForFile(File)) {
     if (!Candidates.empty())
       return std::move(Candidates.front());
   } else {
     log("Failed to find compilation database for {0}", File);
   }
   return None;
 }
 
-tooling::CompilationDatabase *
+std::pair<tooling::CompilationDatabase *, /*Cached*/ bool>
 DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   // FIXME(ibiryukov): Invalidate cached compilation databases on changes
   auto CachedIt = CompilationDatabases.find(Dir);
   if (CachedIt != CompilationDatabases.end())
-    return CachedIt->second.get();
+    return {CachedIt->second.get(), true};
   std::string Error = "";
   auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
   auto Result = CDB.get();
   CompilationDatabases.insert(std::make_pair(Dir, std::move(CDB)));
-  return Result;
+  return {Result, false};
 }
 
 tooling::CompilationDatabase *
 DirectoryBasedGlobalCompilationDatabase::getCDBForFile(PathRef File) const {
   namespace path = sys::path;
   assert((path::is_absolute(File, path::Style::posix) ||
           path::is_absolute(File, path::Style::windows)) &&
          "path must be absolute");
 
+  tooling::CompilationDatabase * CDB = nullptr;
+  bool Cached;
   std::lock_guard<std::mutex> Lock(Mutex);
-  if (CompileCommandsDir)
-    return getCDBInDirLocked(*CompileCommandsDir);
-  for (auto Path = path::parent_path(File); !Path.empty();
-       Path = path::parent_path(Path))
-    if (auto CDB = getCDBInDirLocked(Path))
-      return CDB;
-  return nullptr;
+  if (CompileCommandsDir) {
+    std::tie(CDB, Cached) = getCDBInDirLocked(*CompileCommandsDir);
+  } else {
+    for (auto Path = path::parent_path(File); !CDB && !Path.empty();
+         Path = path::parent_path(Path)) {
+      std::tie(CDB, Cached) = getCDBInDirLocked(Path);
+    }
+  }
+  if (CDB && !Cached)
+    OnCommandChanged.broadcast(CDB->getAllFiles());
 }
 
 Optional<tooling::CompileCommand>
 OverlayCDB::getCompileCommand(PathRef File) const {
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     auto It = Commands.find(File);
     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;
 }
 
 void OverlayCDB::setCompileCommand(
     PathRef File, llvm::Optional<tooling::CompileCommand> Cmd) {
+  {
     std::unique_lock<std::mutex> Lock(Mutex);
     if (Cmd)
       Commands[File] = std::move(*Cmd);
     else
       Commands.erase(File);
   }
+  OnCommandChanged.broadcast({File});
+}
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/GlobalCompilationDatabaseTests.cpp

TEST_F(OverlayCDBTest, NoBase) {
   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"));
 }
 
+TEST_F(OverlayCDBTest, Watch) {
+  OverlayCDB Inner(nullptr);
+  OverlayCDB Outer(&Inner);
+
+  std::vector<std::vector<std::string>> Changes;
+  Outer.watch([&](const std::vector<std::string> &ChangedFiles) {
+    Changes.push_back(ChangedFiles);
+  });
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang