[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 ↗

(On Diff #173853)

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 ↗

(On Diff #173853)

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 ↗

(On Diff #173853)

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

unittests/clangd/GlobalCompilationDatabaseTests.cpp

98 ↗

(On Diff #173853)

Maybe add an actual test?

[sammccall]

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

[ilya-biryukov]

LGTM

clangd/Function.h

147 ↗

(On Diff #174014)

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 ↗

(On Diff #174014)

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 ↗

(On Diff #174014)

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 Diff #174014)

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 ↗

(On Diff #174014)

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.

clang-tools-extra/trunk/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 LLVM_NODISCARD Subscription {
+    Event *Parent;
+    unsigned ListenerID;
+
+    Subscription(Event *Parent, unsigned ListenerID)
+        : Parent(Parent), ListenerID(ListenerID) {}
+    friend Event;
+
+  public:
+    Subscription() : Parent(nullptr) {}
+    Subscription(Subscription &&Other) : Parent(nullptr) {
+      *this = std::move(Other);
+    }
+    Subscription &operator=(Subscription &&Other) {
+      // If *this is active, unsubscribe.
+      if (Parent) {
+        std::lock_guard<std::recursive_mutex>(Parent->ListenersMu);
+        llvm::erase_if(Parent->Listeners,
+                       [&](const std::pair<Listener, unsigned> &P) {
+                         return P.second == ListenerID;
+                       });
+      }
+      // Take over the other subscription, and mark it inactive.
+      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)
+        *this = Subscription(); // Unsubscribe.
+    }
+  };
+
+  // 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::recursive_mutex> Lock(ListenersMu);
+    Listeners.push_back({std::move(L), ++ListenerCount});
+    return Subscription(this, ListenerCount);
+    ;
+  }
+
+  // Synchronously sends an event to all registered listeners.
+  // Must not be called from a listener to this event.
+  void broadcast(const T &V) {
+    // FIXME: it would be nice to dynamically check non-reentrancy here.
+    std::lock_guard<std::recursive_mutex> Lock(ListenersMu);
+    for (const auto &L : Listeners)
+      L.first(V);
+  }
+
+  ~Event() {
+    std::lock_guard<std::recursive_mutex> Lock(ListenersMu);
+    assert(Listeners.empty());
+  }
+
+private:
+  static_assert(std::is_same<typename std::decay<T>::type, T>::value,
+                "use a plain type: event values are always passed by const&");
+
+  std::recursive_mutex ListenersMu;
+  bool IsBroadcasting = false;
+  std::vector<std::pair<Listener, unsigned>> Listeners;
+  unsigned ListenerCount = 0;
+};
+
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/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 "Function.h"
 #include "Path.h"
 #include "llvm/ADT/StringMap.h"
 #include <memory>
 #include <mutex>
 #include <vector>
 
 namespace clang {
 
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) const {
+    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 *, /*Cached*/ 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
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 
 /// Wraps another compilation database, and supports overriding the commands
 /// using an in-memory mapping.
 class OverlayCDB : public GlobalCompilationDatabase {
 public:
   // 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)) {}
+             std::vector<std::string> FallbackFlags = {});
 
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// 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;
+  CommandChanged::Subscription BaseChanged;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H

clang-tools-extra/trunk/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 = false;
   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());
+}
+
+OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base,
+                       std::vector<std::string> FallbackFlags)
+    : Base(Base), FallbackFlags(std::move(FallbackFlags)) {
+  if (Base)
+    BaseChanged = Base->watch([this](const std::vector<std::string> Changes) {
+      OnCommandChanged.broadcast(Changes);
+    });
 }
 
 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

clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

add_extra_unittest(ClangdTests
   CodeCompletionStringsTests.cpp
   ContextTests.cpp
   DexTests.cpp
   DraftStoreTests.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp
   FSTests.cpp
+  FunctionTests.cpp
   FuzzyMatchTests.cpp
   GlobalCompilationDatabaseTests.cpp
   HeadersTests.cpp
   IndexTests.cpp
   JSONTransportTests.cpp
   QualityTests.cpp
   RIFFTests.cpp
   SerializationTests.cpp

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

+//===-- FunctionsTests.cpp ------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Function.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(EventTest, Subscriptions) {
+  Event<int> E;
+  int N = 0;
+  {
+    Event<int>::Subscription SubA;
+    // No subscriptions are active.
+    E.broadcast(42);
+    EXPECT_EQ(0, N);
+
+    Event<int>::Subscription SubB = E.observe([&](int) { ++N; });
+    // Now one is active.
+    E.broadcast(42);
+    EXPECT_EQ(1, N);
+
+    SubA = E.observe([&](int) { ++N; });
+    // Both are active.
+    EXPECT_EQ(1, N);
+    E.broadcast(42);
+    EXPECT_EQ(3, N);
+
+    SubA = std::move(SubB);
+    // One is active.
+    EXPECT_EQ(3, N);
+    E.broadcast(42);
+    EXPECT_EQ(4, N);
+  }
+  // None are active.
+  EXPECT_EQ(4, N);
+  E.broadcast(42);
+  EXPECT_EQ(4, N);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang

clang-tools-extra/trunk/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;
+  auto Sub = Outer.watch([&](const std::vector<std::string> &ChangedFiles) {
+    Changes.push_back(ChangedFiles);
+  });
+
+  Inner.setCompileCommand("A.cpp", tooling::CompileCommand());
+  Outer.setCompileCommand("B.cpp", tooling::CompileCommand());
+  Inner.setCompileCommand("A.cpp", llvm::None);
+  Outer.setCompileCommand("C.cpp", llvm::None);
+  EXPECT_THAT(Changes, ElementsAre(ElementsAre("A.cpp"), ElementsAre("B.cpp"),
+                                   ElementsAre("A.cpp"), ElementsAre("C.cpp")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang