[clangd] Handle queries without an originating file in ProjectAwareIndex

[kadircet]

Summary

This enables handling of queries like workspaceSymbols by the last used
external index, which might be null when the query comes from a file specific
context but that file doesn't have any ExternalIndex set (existing behaviour).

This also moves config::Params into Config, rather than just having a file path
inside the Config struct or deducing the environment from Context.

[kadircet]

Note that I don't feel strongly about making use of Context to figure out config::Params vs moving the Params into the Config. But I'd rather not only store Path in the Config since we might end up needing other environment variables in future.

I didn't go with the Context option as it is hard to find a place to fetch it. Maybe we can just forward declare it in Config.h and have a Config::Params similar to Config::Current. (but this would decouple the Config and Params that generated it :/)

[sammccall]

Sorry for lots of comments on a fairly small patch.

---

I want to mention one alternative we haven't considered. Ultimately the problem here is that we have actions that depend on config but we don't know what file to pull the config from.
This corresponds to passing Path="" to TUScheduler::run (3 calls) or runQuick (0 such calls).
Our decision is to effectively use the config from the last accessed file.
Why don't we put this policy in TUScheduler, rather than inferring it at the edges?
Because TUScheduler isn't threadsafe, I think this is as simple as adding a std::string LastFile member, and reading/writing to it in all the run* methods.

---

But I'd rather not only store Path in the Config since we might end up needing other environment variables in future.

Right. However this patch goes further, and says we must expose *all* other parameters, in the same way and with the same semantics (since we only get to document them once).
I don't find the argument totally convincing, because currently we have 2 params, and:

• fresh time shouldn't be exposed at all, unless i'm missing something
• the path is exposed for fairly subtle reasons, and we want to use it sparingly and so probably give it quite weak semantics (e.g. wouldn't be appropriate to grab "the current filename" for logging purposes, I think)

clang-tools-extra/clangd/Config.h

60

who owns the underlying string, with what lifetime?

The "root" that creates the config doesn't currently have any obligation to keep the params around for as long as the config might be active (which can be async via context). I think in config this should be a std::string

60

How do we expect this to be used?
If it's for determining the active project, then I'd expect us to give some weaker semantic like "representative file from the active project" so we can fill in e.g. the main file associated with a TU.

But actually we only use the boolean "is this set". Maybe we should just provide that?
"bool WasConfiguredForPath" or something? Generalizing past that might be premature.

69

try to avoid using multiple abbreviations of the same word :-)

clang-tools-extra/clangd/index/ProjectAware.cpp

152

This logic feels unnecessarily tricky: you check whether the spec is set both here and above.
Consider something longer but more direct

SymbolIndex *Specified = C.Index.External ? GetIndexFromSpec(*C.Index.External) : nullptr;
if (!Specified && C.Parm.Path.empty()) {
  // There was no index specified, but we're also not sure which path we're using.
  // Use the index from the last operation that had a valid path.
  return LastIndex.
}
// Cache the last-used index, even if it's null ("no external index" is a valid config!)
LastIndex = Specified;
return Specified;

154

this is racy: if there *is* a well-defined external index for the current config, then we should return it even if another thread is able to concurrently write to LastIndex.

[kadircet]

in favor of D103476

clang-tools-extra/clangd/ClangdServer.cpp

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;
+      Config::Params P;
       // 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);
+      P.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();
+        P.Path = PosixPath.str();
       }
 
       llvm::StringMap<std::vector<Diag>> ReportableDiagnostics;
-      Config C = Provider->getConfig(Params, [&](const llvm::SMDiagnostic &D) {
+      Config C = Provider->getConfig(P, [&](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.

clang-tools-extra/clangd/Config.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
 
 #include "support/Context.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
+#include <chrono>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// Settings that express user/project preferences and control clangd behavior.
 ///
 /// Generally, features should consume Config::current() and the caller is
 /// responsible for setting it appropriately. In practice these callers are
 /// ClangdServer, TUScheduler, and BackgroundQueue.
 struct Config {
   /// Returns the Config of the current Context, or an empty configuration.
   static const Config &current();
   /// Context key which can be used to set the current Config.
   static clangd::Key<Config> Key;
 
   Config() = default;
   Config(const Config &) = delete;
   Config &operator=(const Config &) = delete;
   Config(Config &&) = default;
   Config &operator=(Config &&) = default;
 
+  /// Describes the context used to evaluate configuration fragments.
+  struct Params {
+    /// Absolute path to a source file we're applying the config to. Unix
+    /// slashes. Empty if not configuring a particular file.
+    llvm::StringRef Path;
+    /// Hint that stale data is OK to improve performance (e.g. avoid IO).
+    /// FreshTime sets a bound for how old the data can be.
+    /// By default, providers should validate caches against the data source.
+    std::chrono::steady_clock::time_point FreshTime =
+        std::chrono::steady_clock::time_point::max();
+  };
+
+  /// Information about the environment used when generating the config.
+  Params Parm;
+
   struct CDBSearchSpec {
     enum { Ancestors, FixedDir, NoCDBSearch } Policy = Ancestors;
     // Absolute, native slashes, no trailing slash.
     llvm::Optional<std::string> FixedCDBPath;
   };
 
   /// Controls how the compile command for the current file is determined.
   struct {

clang-tools-extra/clangd/ConfigCompile.cpp

 #include "llvm/Support/SourceMgr.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 
+using Params = Config::Params;
+
 // Returns an empty stringref if Path is not under FragmentDir. Returns Path
 // as-is when FragmentDir is empty.
 llvm::StringRef configRelative(llvm::StringRef Path,
                                llvm::StringRef FragmentDir) {
   if (FragmentDir.empty())
     return Path;
   if (!Path.consume_front(FragmentDir))
     return llvm::StringRef();

clang-tools-extra/clangd/ConfigProvider.h

 //  - specifying different settings for different files
 // This file defines the structures used for this, that produce a Config.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
 
+#include "Config.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #include <chrono>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
-struct Config;
 class ThreadsafeFS;
 namespace config {
 
-/// Describes the context used to evaluate configuration fragments.
-struct Params {
-  /// Absolute path to a source file we're applying the config to. Unix slashes.
-  /// Empty if not configuring a particular file.
-  llvm::StringRef Path;
-  /// Hint that stale data is OK to improve performance (e.g. avoid IO).
-  /// FreshTime sets a bound for how old the data can be.
-  /// By default, providers should validate caches against the data source.
-  std::chrono::steady_clock::time_point FreshTime =
-      std::chrono::steady_clock::time_point::max();
-};
-
 /// Used to report problems in parsing or interpreting a config.
 /// Errors reflect structurally invalid config that should be user-visible.
 /// Warnings reflect e.g. unknown properties that are recoverable.
 /// Notes are used to report files and fragments.
 /// (This can be used to track when previous warnings/errors have been "fixed").
 using DiagnosticCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
 
 /// A chunk of configuration that has been fully analyzed and is ready to apply.
 /// Typically this is obtained from a Fragment by calling Fragment::compile().
 ///
 /// Calling it updates the configuration to reflect settings from the fragment.
 /// Returns true if the condition was met and the settings were used.
-using CompiledFragment = std::function<bool(const Params &, Config &)>;
+using CompiledFragment = std::function<bool(const Config::Params &, Config &)>;
 
 /// A source of configuration fragments.
 /// Generally these providers reflect a fixed policy for obtaining config,
 /// but return different concrete configuration over time.
 /// e.g. a provider that reads config from files is responsive to file changes.
 class Provider {
 public:
   virtual ~Provider() = default;
public:
   fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, const ThreadsafeFS &,
                                 bool Trusted = false);
 
   /// A provider that includes fragments from all the supplied providers.
   /// Order is preserved; later providers take precedence over earlier ones.
   static std::unique_ptr<Provider> combine(std::vector<const Provider *>);
 
   /// Build a config based on this provider.
-  Config getConfig(const Params &, DiagnosticCallback) const;
+  Config getConfig(const Config::Params &, DiagnosticCallback) const;
 
 private:
   /// Provide fragments that may be relevant to the file.
   /// The configuration provider is not responsible for testing conditions.
   ///
   /// Providers are expected to cache compiled fragments, and only
   /// reparse/recompile when the source data has changed.
   /// Despite the need for caching, this function must be threadsafe.
   ///
   /// When parsing/compiling, the DiagnosticCallback is used to report errors.
   virtual std::vector<CompiledFragment>
-  getFragments(const Params &, DiagnosticCallback) const = 0;
+  getFragments(const Config::Params &, DiagnosticCallback) const = 0;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigProvider.cpp

 #include <chrono>
 #include <mutex>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace config {
 
+using Params = Config::Params;
+
 // Threadsafe cache around reading a YAML config file from disk.
 class FileConfigCache : public FileCache {
   mutable llvm::SmallVector<CompiledFragment, 1> CachedValue;
   std::string Directory;
 
 public:
   FileConfigCache(llvm::StringRef Path, llvm::StringRef Directory)
       : FileCache(Path), Directory(Directory) {}

clang-tools-extra/clangd/index/ProjectAware.cpp

 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "support/Logger.h"
 #include "support/Threading.h"
 #include "support/Trace.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
+#include <atomic>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <tuple>
 
 namespace clang {
 namespace clangd {
 namespace {
private:
   SymbolIndex *getIndex() const;
 
   // Storage for all the external indexes.
   mutable std::mutex Mu;
   mutable llvm::DenseMap<Config::ExternalIndexSpec,
                          std::unique_ptr<SymbolIndex>>
       IndexForSpec;
   mutable std::unique_ptr<AsyncTaskRunner> Tasks;
+  // Points at the external index used to resolve last query. Might be null.
+  mutable std::atomic<SymbolIndex *> LastIndex = {};
 
   const IndexFactory Gen;
 };
 
 size_t ProjectAwareIndex::estimateMemoryUsage() const {
   size_t Total = 0;
   std::lock_guard<std::mutex> Lock(Mu);
   for (auto &Entry : IndexForSpec)
ProjectAwareIndex::indexedFiles() const {
   trace::Span Tracer("ProjectAwareIndex::indexedFiles");
   if (auto *Idx = getIndex())
     return Idx->indexedFiles();
   return [](llvm::StringRef) { return IndexContents::None; };
 }
 
 SymbolIndex *ProjectAwareIndex::getIndex() const {
   const auto &C = Config::current();
-  if (!C.Index.External)
+  auto GetIndexFromSpec =
+      [this](llvm::Optional<Config::ExternalIndexSpec> Spec) -> SymbolIndex * {
+    if (!Spec)
       return nullptr;
-  const auto &External = *C.Index.External;
+    const auto &External = *Spec;
     std::lock_guard<std::mutex> Lock(Mu);
     auto Entry = IndexForSpec.try_emplace(External, nullptr);
     if (Entry.second)
       Entry.first->getSecond() = Gen(External, Tasks.get());
     return Entry.first->second.get();
+  };
+
+  // Use the last index when request is not originating from a particular file.
+  // This enables features like workspaceSymbols to keep working.
+  // Note that this might set LastIndex to nullptr when the user operates on a
+  // file without an external index. This is deliberate to support users with
+  // files open from projects with and without an external index.
+  if (C.Index.External || !C.Parm.Path.empty())
+    LastIndex = GetIndexFromSpec(C.Index.External);
+  return LastIndex;
 }
 } // namespace
 
 std::unique_ptr<SymbolIndex> createProjectAwareIndex(IndexFactory Gen,
                                                      bool Sync) {
   assert(Gen);
   return std::make_unique<ProjectAwareIndex>(std::move(Gen), Sync);
 }
 } // namespace clangd
 } // namespace clang

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

loadExternalIndex(const Config::ExternalIndexSpec &External,
   llvm_unreachable("Invalid ExternalIndexKind.");
 }
 
 class FlagsConfigProvider : public config::Provider {
 private:
   config::CompiledFragment Frag;
 
   std::vector<config::CompiledFragment>
-  getFragments(const config::Params &,
+  getFragments(const Config::Params &,
                config::DiagnosticCallback) const override {
     return {Frag};
   }
 
 public:
   FlagsConfigProvider() {
     llvm::Optional<Config::CDBSearchSpec> CDBSearch;
     llvm::Optional<Config::ExternalIndexSpec> IndexSpec;
if (!RemoteIndexAddress.empty()) {
       IndexSpec = std::move(Spec);
       BGPolicy = Config::BackgroundPolicy::Skip;
     }
 #endif
     if (!EnableBackgroundIndex) {
       BGPolicy = Config::BackgroundPolicy::Skip;
     }
 
-    Frag = [=](const config::Params &, Config &C) {
+    Frag = [=](const Config::Params &, Config &C) {
       if (CDBSearch)
         C.CompileFlags.CDBSearch = *CDBSearch;
       if (IndexSpec)
         C.Index.External = *IndexSpec;
       if (BGPolicy)
         C.Index.Background = *BGPolicy;
       if (AllScopesCompletion.getNumOccurrences())
         C.Completion.AllScopes = AllScopesCompletion;

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

TEST(ClangdServerTest, RespectsConfig) {
   #else
   int x;
   #endif
   int y = ^x;
   )cpp");
   // Provide conditional config that defines FOO for foo.cc.
   class ConfigProvider : public config::Provider {
     std::vector<config::CompiledFragment>
-    getFragments(const config::Params &,
+    getFragments(const Config::Params &,
                  config::DiagnosticCallback DC) const override {
       config::Fragment F;
       F.If.PathMatch.emplace_back(".*foo.cc");
       F.CompileFlags.Add.emplace_back("-DFOO=1");
       return {std::move(F).compile(DC)};
     }
   } CfgProvider;
 

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

 using ::testing::StartsWith;
 using ::testing::UnorderedElementsAre;
 
 class ConfigCompileTests : public ::testing::Test {
 protected:
   CapturedDiags Diags;
   Config Conf;
   Fragment Frag;
-  Params Parm;
+  Config::Params Parm;
 
   bool compileAndApply() {
     Conf = Config();
     Diags.Diagnostics.clear();
     auto Compiled = std::move(Frag).compile(Diags.callback());
     return Compiled(Parm, Conf);
   }
 };

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

 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 
+using Params = Config::Params;
+
 // Provider that appends an arg to compile flags.
 // The arg is prefix<N>, where N is the times getFragments() was called.
 // It also yields a diagnostic each time it's called.
 class FakeProvider : public Provider {
   std::string Prefix;
   mutable std::atomic<unsigned> Index = {0};
 
   std::vector<CompiledFragment>

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

TEST(ProjectAware, CreatedOnce) {
   match(*Idx, Req);
   // Now it should be created.
   EXPECT_EQ(InvocationCount, 1U);
   match(*Idx, Req);
   // It is cached afterwards.
   EXPECT_EQ(InvocationCount, 1U);
   return;
 }
+
+TEST(ProjectAware, LastIndex) {
+  IndexFactory Gen = [&](const Config::ExternalIndexSpec &Spec,
+                         AsyncTaskRunner *) { return createIndex(); };
+
+  auto Idx = createProjectAwareIndex(std::move(Gen), true);
+  FuzzyFindRequest Req;
+  Req.Query = "1";
+  Req.AnyScope = true;
+
+  auto QueryWithoutPath = [&] {
+    Config C;
+    WithContextValue With(Config::Key, std::move(C));
+    return match(*Idx, Req);
+  };
+
+  // Empty without any external index before hand.
+  EXPECT_THAT(QueryWithoutPath(), IsEmpty());
+  // Now issue a request that will dispatch to an external index.
+  {
+    Config C;
+    C.Index.External.emplace();
+    C.Index.External->Location = "test";
+    WithContextValue With(Config::Key, std::move(C));
+    EXPECT_THAT(match(*Idx, Req), ElementsAre("1"));
+  }
+  // Should use the same external index.
+  EXPECT_THAT(QueryWithoutPath(), ElementsAre("1"));
+  // Issue a query without an external index.
+  {
+    Config C;
+    C.Parm.Path = "path";
+    WithContextValue With(Config::Key, std::move(C));
+    EXPECT_THAT(match(*Idx, Req), IsEmpty());
+  }
+  // Should use the last index, which was none.
+  EXPECT_THAT(QueryWithoutPath(), IsEmpty());
+  return;
+}
 } // namespace clangd
 } // namespace clang