[clangd] Inject context provider rather than config into ClangdServer. NFC

This is a step towards allowing CDB behavior to being configurable.

Previously ClangdServer itself created the configs and installed them into
contexts. This was natural as it knows how to deal with resulting diagnostics.

However this prevents config being used in CDB, which must be created before
ClangdServer. So we extract the context provider (config loader) as a separate
object, which publishes diagnostics to a ClangdServer::Callbacks itself.

Now initialization looks like:
 - First create the config::Provider
 - Then create the ClangdLSPServer, passing config provider
 - Next, create the context provider, passing config provider + diagnostic callbacks
 - now create the CDB, passing context provider
 - finally create ClangdServer, passing CDB, context provider, and diagnostic callbacks

Differential Revision: https://reviews.llvm.org/D95087

Author: sam-mccall

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -1469,6 +1469,12 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
       MsgHandler(new MessageHandler(*this)), TFS(TFS),
       SupportedSymbolKinds(defaultSymbolKinds()),
       SupportedCompletionItemKinds(defaultCompletionItemKinds()), Opts(Opts) {
+  if (Opts.ConfigProvider) {
+    assert(!Opts.ContextProvider &&
+           "Only one of ConfigProvider and ContextProvider allowed!");
+    this->Opts.ContextProvider = ClangdServer::createConfiguredContextProvider(
+        Opts.ConfigProvider, this);
+  }
 
   // clang-format off
   MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize);

clang-tools-extra/clangd/ClangdLSPServer.h

@@ -41,6 +41,8 @@ class SymbolIndex;
 class ClangdLSPServer : private ClangdServer::Callbacks {
 public:
   struct Options : ClangdServer::Options {
+    /// Supplies configuration (overrides ClangdServer::ContextProvider).
+    config::Provider *ConfigProvider = nullptr;
     /// Look for compilation databases, rather than using compile commands
     /// set via LSP (extensions) only.
     bool UseDirBasedCDB = true;

clang-tools-extra/clangd/ClangdServer.cpp

@@ -133,14 +133,14 @@ ClangdServer::Options::operator TUScheduler::Options() const {
   Opts.StorePreamblesInMemory = StorePreamblesInMemory;
   Opts.UpdateDebounce = UpdateDebounce;
   Opts.AsyncPreambleBuilds = AsyncPreambleBuilds;
+  Opts.ContextProvider = ContextProvider;
   return Opts;
 }
 
 ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
                            const ThreadsafeFS &TFS, const Options &Opts,
                            Callbacks *Callbacks)
-    : ConfigProvider(Opts.ConfigProvider), CDB(CDB), TFS(TFS),
-      ServerCallbacks(Callbacks),
+    : CDB(CDB), TFS(TFS),
       DynamicIdx(Opts.BuildDynamicSymbolIndex
                      ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex,
                                      Opts.CollectMainFileRefs)
@@ -153,14 +153,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
       // FIXME(ioeric): this can be slow and we may be able to index on less
       // critical paths.
       WorkScheduler(
-          CDB,
-          [&, this] {
-            TUScheduler::Options O(Opts);
-            O.ContextProvider = [this](PathRef P) {
-              return createProcessingContext(P);
-            };
-            return O;
-          }(),
+          CDB, TUScheduler::Options(Opts),
           std::make_unique<UpdateIndexCallbacks>(
               DynamicIdx.get(), Callbacks, Opts.TheiaSemanticHighlighting)) {
   // Adds an index to the stack, at higher priority than existing indexes.
@@ -181,9 +174,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
       if (Callbacks)
         Callbacks->onBackgroundIndexProgress(S);
     };
-    BGOpts.ContextProvider = [this](PathRef P) {
-      return createProcessingContext(P);
-    };
+    BGOpts.ContextProvider = Opts.ContextProvider;
     BGOpts.CollectMainFileRefs = Opts.CollectMainFileRefs;
     BackgroundIdx = std::make_unique<BackgroundIndex>(
         TFS, CDB,
@@ -216,6 +207,83 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
     BackgroundIdx->boostRelated(File);
 }
 
+std::function<Context(PathRef)>
+ClangdServer::createConfiguredContextProvider(const config::Provider *Provider,
+                                              Callbacks *Publish) {
+  if (!Provider)
+    return [](llvm::StringRef) { return Context::current().clone(); };
+
+  struct Impl {
+    const config::Provider *Provider;
+    ClangdServer::Callbacks *Publish;
+    std::mutex PublishMu;
+
+    Impl(const config::Provider *Provider, ClangdServer::Callbacks *Publish)
+        : Provider(Provider), Publish(Publish) {}
+
+    Context operator()(llvm::StringRef File) {
+      config::Params Params;
+      // Don't reread config files excessively often.
+      // FIXME: when we see a config file change event, use the event timestamp?
+      Params.FreshTime =
+          std::chrono::steady_clock::now() - std::chrono::seconds(5);
+      llvm::SmallString<256> PosixPath;
+      if (!File.empty()) {
+        assert(llvm::sys::path::is_absolute(File));
+        llvm::sys::path::native(File, PosixPath, llvm::sys::path::Style::posix);
+        Params.Path = PosixPath.str();
+      }
+
+      llvm::StringMap<std::vector<Diag>> ReportableDiagnostics;
+      Config C = Provider->getConfig(Params, [&](const llvm::SMDiagnostic &D) {
+        // Create the map entry even for note diagnostics we don't report.
+        // This means that when the file is parsed with no warnings, we
+        // publish an empty set of diagnostics, clearing any the client has.
+        handleDiagnostic(D, !Publish || D.getFilename().empty()
+                                ? nullptr
+                                : &ReportableDiagnostics[D.getFilename()]);
+      });
+      // Blindly publish diagnostics for the (unopened) parsed config files.
+      // We must avoid reporting diagnostics for *the same file* concurrently.
+      // Source diags are published elsewhere, but those are different files.
+      if (!ReportableDiagnostics.empty()) {
+        std::lock_guard<std::mutex> Lock(PublishMu);
+        for (auto &Entry : ReportableDiagnostics)
+          Publish->onDiagnosticsReady(Entry.first(), /*Version=*/"",
+                                      std::move(Entry.second));
+      }
+      return Context::current().derive(Config::Key, std::move(C));
+    }
+
+    void handleDiagnostic(const llvm::SMDiagnostic &D,
+                          std::vector<Diag> *ClientDiagnostics) {
+      switch (D.getKind()) {
+      case llvm::SourceMgr::DK_Error:
+        elog("config error at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
+             D.getColumnNo(), D.getMessage());
+        break;
+      case llvm::SourceMgr::DK_Warning:
+        log("config warning at {0}:{1}:{2}: {3}", D.getFilename(),
+            D.getLineNo(), D.getColumnNo(), D.getMessage());
+        break;
+      case llvm::SourceMgr::DK_Note:
+      case llvm::SourceMgr::DK_Remark:
+        vlog("config note at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
+             D.getColumnNo(), D.getMessage());
+        ClientDiagnostics = nullptr; // Don't emit notes as LSP diagnostics.
+        break;
+      }
+      if (ClientDiagnostics)
+        ClientDiagnostics->push_back(toDiag(D, Diag::ClangdConfig));
+    }
+  };
+
+  // Copyable wrapper.
+  return [I(std::make_shared<Impl>(Provider, Publish))](llvm::StringRef Path) {
+    return (*I)(Path);
+  };
+}
+
 void ClangdServer::removeDocument(PathRef File) { WorkScheduler.remove(File); }
 
 void ClangdServer::codeComplete(PathRef File, Position Pos,
@@ -802,62 +870,6 @@ llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
   return WorkScheduler.fileStats();
 }
 
-Context ClangdServer::createProcessingContext(PathRef File) const {
-  if (!ConfigProvider)
-    return Context::current().clone();
-
-  config::Params Params;
-  // Don't reread config files excessively often.
-  // FIXME: when we see a config file change event, use the event timestamp.
-  Params.FreshTime = std::chrono::steady_clock::now() - std::chrono::seconds(5);
-  llvm::SmallString<256> PosixPath;
-  if (!File.empty()) {
-    assert(llvm::sys::path::is_absolute(File));
-    llvm::sys::path::native(File, PosixPath, llvm::sys::path::Style::posix);
-    Params.Path = PosixPath.str();
-  }
-
-  llvm::StringMap<std::vector<Diag>> ReportableDiagnostics;
-  auto ConfigDiagnosticHandler = [&](const llvm::SMDiagnostic &D) {
-    // Ensure we create the map entry even for note diagnostics we don't report.
-    // This means that when the file is parsed with no warnings, we'll
-    // publish an empty set of diagnostics, clearing any the client has.
-    auto *Reportable = D.getFilename().empty()
-                           ? nullptr
-                           : &ReportableDiagnostics[D.getFilename()];
-    switch (D.getKind()) {
-    case llvm::SourceMgr::DK_Error:
-      elog("config error at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
-           D.getColumnNo(), D.getMessage());
-      if (Reportable)
-        Reportable->push_back(toDiag(D, Diag::ClangdConfig));
-      break;
-    case llvm::SourceMgr::DK_Warning:
-      log("config warning at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
-          D.getColumnNo(), D.getMessage());
-      if (Reportable)
-        Reportable->push_back(toDiag(D, Diag::ClangdConfig));
-      break;
-    case llvm::SourceMgr::DK_Note:
-    case llvm::SourceMgr::DK_Remark:
-      vlog("config note at {0}:{1}:{2}: {3}", D.getFilename(), D.getLineNo(),
-           D.getColumnNo(), D.getMessage());
-      break;
-    }
-  };
-  Config C = ConfigProvider->getConfig(Params, ConfigDiagnosticHandler);
-  // Blindly publish diagnostics for the (unopened) parsed config files.
-  // We must avoid reporting diagnostics for *the same file* concurrently.
-  // Source file diags are published elsewhere, but those are different files.
-  if (!ReportableDiagnostics.empty()) {
-    std::lock_guard<std::mutex> Lock(ConfigDiagnosticsMu);
-    for (auto &Entry : ReportableDiagnostics)
-      ServerCallbacks->onDiagnosticsReady(Entry.first(), /*Version=*/"",
-                                          std::move(Entry.second));
-  }
-  return Context::current().derive(Config::Key, std::move(C));
-}
-
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)) &&

clang-tools-extra/clangd/ClangdServer.h

@@ -80,6 +80,12 @@ class ClangdServer {
     virtual void
     onBackgroundIndexProgress(const BackgroundQueue::Stats &Stats) {}
   };
+  /// Creates a context provider that loads and installs config.
+  /// Errors in loading config are reported as diagnostics via Callbacks.
+  /// (This is typically used as ClangdServer::Options::ContextProvider).
+  static std::function<Context(PathRef)>
+  createConfiguredContextProvider(const config::Provider *Provider,
+                                  ClangdServer::Callbacks *);
 
   struct Options {
     /// To process requests asynchronously, ClangdServer spawns worker threads.
@@ -111,8 +117,16 @@ class ClangdServer {
     /// If set, use this index to augment code completion results.
     SymbolIndex *StaticIndex = nullptr;
 
-    /// If set, queried to obtain the configuration to handle each request.
-    config::Provider *ConfigProvider = nullptr;
+    /// If set, queried to derive a processing context for some work.
+    /// Usually used to inject Config (see createConfiguredContextProvider).
+    ///
+    /// When the provider is called, the active context will be that inherited
+    /// from the request (e.g. addDocument()), or from the ClangdServer
+    /// constructor if there is no such request (e.g. background indexing).
+    ///
+    /// The path is an absolute path of the file being processed.
+    /// If there is no particular file (e.g. project loading) then it is empty.
+    std::function<Context(PathRef)> ContextProvider;
 
     /// The Options provider to use when running clang-tidy. If null, clang-tidy
     /// checks will be disabled.
@@ -343,19 +357,8 @@ class ClangdServer {
                   ArrayRef<tooling::Range> Ranges,
                   Callback<tooling::Replacements> CB);
 
-  /// Derives a context for a task processing the specified source file.
-  /// This includes the current configuration (see Options::ConfigProvider).
-  /// The empty string means no particular file is the target.
-  /// Rather than called by each feature, this is exposed to the components
-  /// that control worker threads, like TUScheduler and BackgroundIndex.
-  /// This means it's OK to do some IO here, and it cuts across all features.
-  Context createProcessingContext(PathRef) const;
-  config::Provider *ConfigProvider = nullptr;
-
   const GlobalCompilationDatabase &CDB;
   const ThreadsafeFS &TFS;
-  Callbacks *ServerCallbacks = nullptr;
-  mutable std::mutex ConfigDiagnosticsMu;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:

clang-tools-extra/clangd/unittests/ClangdTests.cpp

@@ -348,7 +348,8 @@ TEST(ClangdServerTest, RespectsConfig) {
   } CfgProvider;
 
   auto Opts = ClangdServer::optsForTest();
-  Opts.ConfigProvider = &CfgProvider;
+  Opts.ContextProvider =
+      ClangdServer::createConfiguredContextProvider(&CfgProvider, nullptr);
   OverlayCDB CDB(/*Base=*/nullptr, /*FallbackFlags=*/{},
                  tooling::ArgumentsAdjuster(CommandMangler::forTests()));
   MockFS FS;