[clangd] Introduce a CommandLineConfigProvider

[kadircet]

Summary

This enables unifying command line flags with config options in clangd
internals. This patch changes behaviour in 2 places:

• BackgroundIndex was previously disabled when -remote-index was

provided. After this patch, it will be enabled but all files will have
bkgindex policy set to Skip.

• -index-file was loaded at startup (at least load was initiated), now

the load will happen through ProjectAwareIndex with first index query.

Unfortunately this doesn't simplify any options initially, as

• CompileCommandsDir is also used by clangd --check workflow, which

doesn't use configs.

• EnableBackgroundIndex option controls whether the component will be

created at all, which implies creation of extra threads registering a
listener for compilation database discoveries.

[sammccall]

Both side-effects seem fine to me.

EnableBackgroundIndex option controls whether the component will be created at all

Do you think we should eventually switch to always setting ClangdServer::Options::BackgroundIndex to true in ClangdServerMain (or removing the option?)
If not, I'm not sure if there's much benefit in also setting the config flag...

CompileCommandsDir is also used by clangd --check workflow, which doesn't use configs

We can/should fix this though, right?

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

555

(This seems a little weird, get passed in a task runner and maybe ignore it. Seems like the asyncprojectindex should know whether it's supposed to be sync or not, and either pass in a null runner or it should handle the asynchrony itself... not something for this patch)

568

or just FlagsConfigProvider?

572

Hmm, it seems natural to move assembling some of these objects into the constructor, I'm not sure it matters much, at least for now, but putting the actual flag-inspection on the hot-ish path seems constraining.

(Lowest-ceremony I can come up with is just making them e.g. Optional<ExternalIndexSpec> members of the provider, and then having a single fragment which capture the provider this and keep the conditionals inside it)

775

If we fix this, we can move the rest of the logic into the flags config provider constructor.

[kadircet]

• Address comments

[kadircet]

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

555

adding a fixme, will follow-up.

775

yes, i was unsure about whether we would like to propagate config into --check mode, (but you didn't oppose the fixme, so doing that now :D)

clang-tools-extra/clangd/ClangdLSPServer.h

class ClangdLSPServer : private ClangdServer::Callbacks,
                         private LSPBinder::RawOutgoing {
 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;
-    /// A fixed directory to search for a compilation database in.
-    /// If not set, we search upward from the source file.
-    llvm::Optional<Path> CompileCommandsDir;
     /// The offset-encoding to use, or None to negotiate it over LSP.
     llvm::Optional<OffsetEncoding> Encoding;
     /// If set, periodically called to release memory.
     /// Consider malloc_trim(3)
     std::function<void()> MemoryCleanup = nullptr;
 
     /// Per-feature options. Generally ClangdServer lets these vary
     /// per-request, but LSP allows limited/no customizations.

clang-tools-extra/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onInitialize(const InitializeParams &Params,
 
   if (Params.rootUri && *Params.rootUri)
     Opts.WorkspaceRoot = std::string(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
     Opts.WorkspaceRoot = *Params.rootPath;
   if (Server)
     return Reply(llvm::make_error<LSPError>("server already initialized",
                                             ErrorCode::InvalidRequest));
-  if (const auto &Dir = Params.initializationOptions.compilationDatabasePath)
-    Opts.CompileCommandsDir = Dir;
   if (Opts.UseDirBasedCDB) {
     DirectoryBasedGlobalCompilationDatabase::Options CDBOpts(TFS);
-    CDBOpts.CompileCommandsDir = Opts.CompileCommandsDir;
+    if (const auto &Dir = Params.initializationOptions.compilationDatabasePath)
+      CDBOpts.CompileCommandsDir = Dir;
     CDBOpts.ContextProvider = Opts.ContextProvider;
     BaseCDB =
         std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
     BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
                                      std::move(BaseCDB));
   }
   auto Mangler = CommandMangler::detect();
   if (Opts.ResourceDir)

clang-tools-extra/clangd/test/log.test

-# RUN: env CLANGD_FLAGS=-index-file=no-such-index not clangd -lit-test </dev/null 2>&1 >/dev/null | FileCheck %s
+# RUN: env CLANGD_FLAGS=-compile-commands-dir=no-such-dir not clangd -lit-test </dev/null 2>&1 >/dev/null | FileCheck %s
 CHECK: I[{{.*}}]{{.*}} clangd version {{.*}}
 CHECK: Working directory: {{.*}}
 CHECK: argv[0]: clangd
 CHECK: argv[1]: -lit-test
-CHECK: CLANGD_FLAGS: -index-file=no-such-index
-CHECK: E[{{.*}}] Can't open no-such-index
+CHECK: CLANGD_FLAGS: -compile-commands-dir=no-such-dir
+CHECK: E[{{.*}}] Path specified by --compile-commands-dir does not exist.
 CHECK: Starting LSP over stdin/stdout
 

clang-tools-extra/clangd/tool/Check.cpp

 // Currently it adds some basic logging of progress and results.
 // We should consider extending it to also recognize common symptoms and
 // recommend solutions (e.g. standard library installation issues).
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
+#include "Config.h"
 #include "GlobalCompilationDatabase.h"
 #include "Hover.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "XRefs.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
public:
 
   Checker(llvm::StringRef File, const ClangdLSPServer::Options &Opts)
       : File(File), Opts(Opts) {}
 
   // Read compilation database and choose a compile command for the file.
   bool buildCommand(const ThreadsafeFS &TFS) {
     log("Loading compilation database...");
     DirectoryBasedGlobalCompilationDatabase::Options CDBOpts(TFS);
-    CDBOpts.CompileCommandsDir = Opts.CompileCommandsDir;
+    CDBOpts.CompileCommandsDir =
+        Config::current().CompileFlags.CDBSearch.FixedCDBPath;
     std::unique_ptr<GlobalCompilationDatabase> BaseCDB =
         std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
     BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
                                      std::move(BaseCDB));
     auto Mangler = CommandMangler::detect();
     if (Opts.ResourceDir)
       Mangler.ResourceDir = *Opts.ResourceDir;
     auto CDB = std::make_unique<OverlayCDB>(
Contents = R"cpp(
       #include <string>
 
       size_t N = 50;
       auto xxx = std::string(N, 'x');
     )cpp";
   }
   log("Testing on source file {0}", File);
 
+  auto ContextProvider = ClangdServer::createConfiguredContextProvider(
+      Opts.ConfigProvider, nullptr);
+  WithContext Ctx(ContextProvider(""));
   Checker C(File, Opts);
   if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents) ||
       !C.buildAST())
     return false;
   C.testLocationFeatures();
 
   log("All checks completed, {0} errors", C.ErrCount);
   return C.ErrCount == 0;
 }
 
 } // namespace clangd
 } // namespace clang

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

 //===--- ClangdMain.cpp - clangd server loop ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
+#include "Config.h"
+#include "ConfigProvider.h"
 #include "Features.inc"
 #include "PathMapping.h"
 #include "Protocol.h"
 #include "TidyProvider.h"
 #include "Transport.h"
 #include "index/Background.h"
 #include "index/Index.h"
 #include "index/Merge.h"

 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include <chrono>
 #include <cstdlib>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <thread>
+#include <utility>
 #include <vector>
 
 #ifndef _WIN32
 #include <unistd.h>
 #endif
 
 #ifdef __GLIBC__
 #include <malloc.h>
loadExternalIndex(const Config::ExternalIndexSpec &External,
   case Config::ExternalIndexSpec::Server:
     log("Associating {0} with remote index at {1}.", External.MountPoint,
         External.Location);
     return remote::getClient(External.Location, External.MountPoint);
   case Config::ExternalIndexSpec::File:
     log("Associating {0} with monolithic index at {1}.", External.MountPoint,
         External.Location);
     auto NewIndex = std::make_unique<SwapIndex>(std::make_unique<MemIndex>());
-    Tasks.runAsync("Load-index:" + External.Location,
-                   [File = External.Location, PlaceHolder = NewIndex.get()] {
-                     if (auto Idx = loadIndex(File, /*UseDex=*/true))
-                       PlaceHolder->reset(std::move(Idx));
-                   });
+    auto IndexLoadTask = [File = External.Location,
+                          PlaceHolder = NewIndex.get()] {
+    };
+    // FIXME: The signature should contain a null-able TaskRunner istead, and
+    // the task should be scheduled accordingly.
+    if (Sync) {
+      IndexLoadTask();
+    } else {
+      Tasks.runAsync("Load-index:" + External.Location,
+                     std::move(IndexLoadTask));
+    }
     return std::move(NewIndex);
   }
   llvm_unreachable("Invalid ExternalIndexKind.");
 }
+
+class FlagsConfigProvider : public config::Provider {
+private:
+  config::CompiledFragment Frag;
+
+  std::vector<config::CompiledFragment>
+  getFragments(const config::Params &,
+               config::DiagnosticCallback) const override {
+    return {Frag};
+  }
+
+public:
+  FlagsConfigProvider() {
+    llvm::Optional<Config::CDBSearchSpec> CDBSearch;
+    llvm::Optional<Config::ExternalIndexSpec> IndexSpec;
+    llvm::Optional<Config::BackgroundPolicy> BGPolicy;
+
+    // If --compile-commands-dir arg was invoked, check value and override
+    // default path.
+    if (!CompileCommandsDir.empty()) {
+      if (llvm::sys::fs::exists(CompileCommandsDir)) {
+        // We support passing both relative and absolute paths to the
+        // --compile-commands-dir argument, but we assume the path is absolute
+        // in the rest of clangd so we make sure the path is absolute before
+        // continuing.
+        llvm::SmallString<128> Path(CompileCommandsDir);
+        if (std::error_code EC = llvm::sys::fs::make_absolute(Path)) {
+          elog("Error while converting the relative path specified by "
+               "--compile-commands-dir to an absolute path: {0}. The argument "
+               "will be ignored.",
+               EC.message());
+        } else {
+          CDBSearch = {Config::CDBSearchSpec::FixedDir, Path.str().str()};
+        }
+      } else {
+        elog("Path specified by --compile-commands-dir does not exist. The "
+             "argument will be ignored.");
+      }
+    }
+    if (!IndexFile.empty()) {
+      Config::ExternalIndexSpec Spec;
+      Spec.Kind = Spec.File;
+      Spec.Location = IndexFile;
+      IndexSpec = std::move(Spec);
+    }
+    if (!RemoteIndexAddress.empty()) {
+      assert(!ProjectRoot.empty() && IndexFile.empty());
+      Config::ExternalIndexSpec Spec;
+      Spec.Kind = Spec.Server;
+      Spec.Location = RemoteIndexAddress;
+      Spec.MountPoint = ProjectRoot;
+      IndexSpec = std::move(Spec);
+      BGPolicy = Config::BackgroundPolicy::Skip;
+    }
+    if (!EnableBackgroundIndex) {
+      BGPolicy = Config::BackgroundPolicy::Skip;
+    }
+    Frag = [=](const config::Params &, Config &C) {
+      if (CDBSearch)
+        C.CompileFlags.CDBSearch = *CDBSearch;
+      if (IndexSpec)
+        C.Index.External = *IndexSpec;
+      if (BGPolicy)
+        C.Index.Background = *BGPolicy;
+      return true;
+    };
+  }
+};
 } // namespace
 } // namespace clangd
 } // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
   CantRunAsXPCService = 2,
   CheckFailed = 3
)";
   for (int I = 0; I < argc; ++I)
     log("argv[{0}]: {1}", I, argv[I]);
   if (auto EnvFlags = llvm::sys::Process::GetEnv(FlagsEnvVar))
     log("{0}: {1}", FlagsEnvVar, *EnvFlags);
 
   ClangdLSPServer::Options Opts;
   Opts.UseDirBasedCDB = (CompileArgsFrom == FilesystemCompileArgs);
 
-  // If --compile-commands-dir arg was invoked, check value and override default
-  // path.
-  if (!CompileCommandsDir.empty()) {
-    if (llvm::sys::fs::exists(CompileCommandsDir)) {
-      // We support passing both relative and absolute paths to the
-      // --compile-commands-dir argument, but we assume the path is absolute in
-      // the rest of clangd so we make sure the path is absolute before
-      // continuing.
-      llvm::SmallString<128> Path(CompileCommandsDir);
-      if (std::error_code EC = llvm::sys::fs::make_absolute(Path)) {
-        elog("Error while converting the relative path specified by "
-             "--compile-commands-dir to an absolute path: {0}. The argument "
-             "will be ignored.",
-             EC.message());
-      } else {
-        Opts.CompileCommandsDir = std::string(Path.str());
-      }
-    } else {
-      elog("Path specified by --compile-commands-dir does not exist. The "
-           "argument will be ignored.");
-    }
-  }
-
   switch (PCHStorage) {
   case PCHStorageFlag::Memory:
     Opts.StorePreamblesInMemory = true;
     break;
   case PCHStorageFlag::Disk:
     Opts.StorePreamblesInMemory = false;
     break;
   }
   if (!ResourceDir.empty())
     Opts.ResourceDir = ResourceDir;
   Opts.BuildDynamicSymbolIndex = true;
   std::vector<std::unique_ptr<SymbolIndex>> IdxStack;
   std::unique_ptr<SymbolIndex> StaticIdx;
-  std::future<void> AsyncIndexLoad; // Block exit while loading the index.
-  if (!IndexFile.empty()) {
-    // Load the index asynchronously. Meanwhile SwapIndex returns no results.
-    SwapIndex *Placeholder;
-    StaticIdx.reset(Placeholder = new SwapIndex(std::make_unique<MemIndex>()));
-    AsyncIndexLoad = runAsync<void>([Placeholder] {
-      if (auto Idx = loadIndex(IndexFile, /*UseDex=*/true))
-        Placeholder->reset(std::move(Idx));
-    });
-    if (Sync)
-      AsyncIndexLoad.wait();
-  }
 #if CLANGD_ENABLE_REMOTE
   if (RemoteIndexAddress.empty() != ProjectRoot.empty()) {
     llvm::errs() << "remote-index-address and project-path have to be "
                     "specified at the same time.";
     return 1;
   }
   if (!RemoteIndexAddress.empty()) {
     if (IndexFile.empty()) {
       log("Connecting to remote index at {0}", RemoteIndexAddress);
-      StaticIdx = remote::getClient(RemoteIndexAddress, ProjectRoot);
-      EnableBackgroundIndex = false;
     } else {
       elog("When enabling remote index, IndexFile should not be specified. "
            "Only one can be used at time. Remote index will ignored.");
     }
   }
 #endif
   Opts.BackgroundIndex = EnableBackgroundIndex;
   auto PAI = createProjectAwareIndex(loadExternalIndex);
if (EnableConfig) {
     if (llvm::sys::path::user_config_directory(UserConfig)) {
       llvm::sys::path::append(UserConfig, "clangd", "config.yaml");
       vlog("User config file is {0}", UserConfig);
       ProviderStack.push_back(
           config::Provider::fromYAMLFile(UserConfig, /*Directory=*/"", TFS));
     } else {
       elog("Couldn't determine user config file, not loading");
     }
-    std::vector<const config::Provider *> ProviderPointers;
-    for (const auto &P : ProviderStack)
-      ProviderPointers.push_back(P.get());
-    Config = config::Provider::combine(std::move(ProviderPointers));
-    Opts.ConfigProvider = Config.get();
-  }
+  }
+  ProviderStack.push_back(std::make_unique<FlagsConfigProvider>());
 
   // Create an empty clang-tidy option.
   TidyProvider ClangTidyOptProvider;
   if (EnableClangTidy) {
     std::vector<TidyProvider> Providers;
     Providers.reserve(4 + EnableConfig);
     Providers.push_back(provideEnvironment());
     Providers.push_back(provideClangTidyFiles(TFS));