[llvm-branch-commits] [clang-tools-extra] 60cd75a - [clangd] Inject context provider rather than config into ClangdServer. NFC

Sam McCall via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Fri Jan 22 05:39:20 PST 2021


Author: Sam McCall
Date: 2021-01-22T14:34:30+01:00
New Revision: 60cd75a098d4f18d9c8903ddcb466b4e7deb0580

URL: https://github.com/llvm/llvm-project/commit/60cd75a098d4f18d9c8903ddcb466b4e7deb0580
DIFF: https://github.com/llvm/llvm-project/commit/60cd75a098d4f18d9c8903ddcb466b4e7deb0580.diff

LOG: [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

Added: 
    

Modified: 
    clang-tools-extra/clangd/ClangdLSPServer.cpp
    clang-tools-extra/clangd/ClangdLSPServer.h
    clang-tools-extra/clangd/ClangdServer.cpp
    clang-tools-extra/clangd/ClangdServer.h
    clang-tools-extra/clangd/unittests/ClangdTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 4e5d9f8bf0fa..24d3a3509ca8 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/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);

diff  --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index a41bc5666af3..3a46bd7b1bea 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/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;

diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 32e08e688f44..4f9ea0499077 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/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 
diff erent 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 
diff erent 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)) &&

diff  --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 926de39b507a..0a452daa79ed 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/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:

diff  --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index 06d761d4fabe..f10a994fca5c 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/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;


        


More information about the llvm-branch-commits mailing list