[clangd] Drop optional on ExternalIndexSpec

[sammccall]

Sorry about delay.

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

555 ↗

(On Diff #336841)

I think you hit llvm_unreachable here - is this an invariant enforced by the caller, or did you mean to return nullptr?

[kadircet]

Add comment to IndexFactory about pre-condition of spec never being none.

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

555 ↗

(On Diff #336841)

it is enforced by the caller (i.e. ProjectAwareIndex::getIndex) hence fallback to unreachable was deliberate.

It was more explicit before as Generator takes a reference, also semantically it feels wrong for generator to be called with a None spec.

Even though we are lucky and can indicate the error by returning null in this case, I'd rather keep it as an assertion failure, similar to before. Gonna update the documentation on the IndexFactory to mention that spec is never None though.

[kadircet]

Rebase

clang-tools-extra/clangd/Config.h

std::vector<llvm::unique_function<void(std::vector<std::string> &) const>>
         Edits;
     /// Where to search for compilation databases for this file's flags.
     CDBSearchSpec CDBSearch = {CDBSearchSpec::Ancestors, llvm::None};
   } CompileFlags;
 
   enum class BackgroundPolicy { Build, Skip };
   /// Describes an external index configuration.
   struct ExternalIndexSpec {
-    enum { None, File, Server } Kind;
+    enum { None, File, Server } Kind = None;
     /// This is one of:
     /// - Address of a clangd-index-server, in the form of "ip:port".
     /// - Absolute path to an index produced by clangd-indexer.
     std::string Location;
     /// Absolute path to source root this index is associated with, uses
     /// forward-slashes.
     std::string MountPoint;
   };
   /// Controls background-index behavior.
   struct {
     /// Whether this TU should be indexed.
     BackgroundPolicy Background = BackgroundPolicy::Build;
-    llvm::Optional<ExternalIndexSpec> External;
+    ExternalIndexSpec External;
   } Index;
 
   /// Controls warnings and errors when parsing code.
   struct {
     bool SuppressAll = false;
     llvm::StringSet<> Suppress;
 
     /// Configures what clang-tidy checks to run and options to use with them.

clang-tools-extra/clangd/ConfigCompile.cpp

if (Spec.Kind != Config::ExternalIndexSpec::None) {
       auto AbsPath = makeAbsolute(std::move(*External.MountPoint), "MountPoint",
                                   llvm::sys::path::Style::posix);
       if (!AbsPath)
         return;
       Spec.MountPoint = std::move(*AbsPath);
     }
     Out.Apply.push_back([Spec(std::move(Spec))](const Params &P, Config &C) {
       if (Spec.Kind == Config::ExternalIndexSpec::None) {
-        C.Index.External.reset();
+        C.Index.External = Spec;
         return;
       }
       if (P.Path.empty() || !pathStartsWith(Spec.MountPoint, P.Path,
                                             llvm::sys::path::Style::posix))
         return;
       C.Index.External = Spec;
       // Disable background indexing for the files under the mountpoint.
       // Note that this will overwrite statements in any previous fragments

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

 #include <functional>
 #include <memory>
 namespace clang {
 namespace clangd {
 
 /// A functor to create an index for an external index specification. Functor
 /// should perform any high latency operation in a separate thread through
 /// AsyncTaskRunner, if set.
+/// Spec is never `None`.
 using IndexFactory = std::function<std::unique_ptr<SymbolIndex>(
     const Config::ExternalIndexSpec &, AsyncTaskRunner *)>;
 
 /// Returns an index that answers queries using external indices. IndexFactory
 /// specifies how to generate an index from an external source. If \p Sync is
 /// set, index won't own any asnyc task runner.
 /// IndexFactory must be injected because this code cannot depend on the remote
 /// index client.
 std::unique_ptr<SymbolIndex> createProjectAwareIndex(IndexFactory, bool Sync);
 } // namespace clangd
 } // namespace clang
 
 #endif

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

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)
+  if (C.Index.External.Kind == Config::ExternalIndexSpec::None)
     return nullptr;
-  const auto &External = *C.Index.External;
+  const auto &External = C.Index.External;
   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();
 }
 } // 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/unittests/ConfigCompileTests.cpp

TEST_F(ConfigCompileTests, ExternalServerNeedsTrusted) {
   External.Server.emplace("xxx");
   Frag.Index.External = std::move(External);
   compileAndApply();
   EXPECT_THAT(
       Diags.Diagnostics,
       ElementsAre(DiagMessage(
           "Remote index may not be specified by untrusted configuration. "
           "Copy this into user config to use it.")));
-  EXPECT_FALSE(Conf.Index.External.hasValue());
+  EXPECT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
   Frag.Source.Trusted = true;
   Fragment::IndexBlock::ExternalBlock External;
   External.File.emplace("");
   External.Server.emplace("");
   Frag.Index.External = std::move(External);
   compileAndApply();
 #ifdef CLANGD_ENABLE_REMOTE
   EXPECT_THAT(
       Diags.Diagnostics,
       Contains(
           AllOf(DiagMessage("Exactly one of File, Server or None must be set."),
                 DiagKind(llvm::SourceMgr::DK_Error))));
 #else
   ASSERT_TRUE(Conf.Index.External.hasValue());
   EXPECT_EQ(Conf.Index.External->Kind, Config::ExternalIndexSpec::File);
 #endif
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockDisableWithNone) {
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
+
   Fragment::IndexBlock::ExternalBlock External;
   External.IsNone = true;
   Frag.Index.External = std::move(External);
   compileAndApply();
-  EXPECT_FALSE(Conf.Index.External.hasValue());
+  EXPECT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) {
   Frag.Index.External.emplace(Fragment::IndexBlock::ExternalBlock{});
   compileAndApply();
   EXPECT_THAT(
       Diags.Diagnostics,
       Contains(
TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
   Frag = GetFrag("", "foo");
   compileAndApply();
   ASSERT_THAT(
       Diags.Diagnostics,
       ElementsAre(
           AllOf(DiagMessage("MountPoint must be an absolute path, because this "
                             "fragment is not associated with any directory."),
                 DiagKind(llvm::SourceMgr::DK_Error))));
-  ASSERT_FALSE(Conf.Index.External);
+  EXPECT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 
   auto FooPath = testPath("foo/", llvm::sys::path::Style::posix);
   FooPath = llvm::sys::path::convert_to_slash(FooPath);
   // Ok when relative.
   Frag = GetFrag(testRoot(), "foo/");
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_TRUE(Conf.Index.External);
-  EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::File);
+  EXPECT_THAT(Conf.Index.External.MountPoint, FooPath);
 
   // None defaults to ".".
   Frag = GetFrag(FooPath, llvm::None);
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_TRUE(Conf.Index.External);
-  EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::File);
+  EXPECT_THAT(Conf.Index.External.MountPoint, FooPath);
 
   // Without a file, external index is empty.
   Parm.Path = "";
   Frag = GetFrag("", FooPath.c_str());
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_FALSE(Conf.Index.External);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 
   // File outside MountPoint, no index.
   auto BazPath = testPath("bar/baz.h", llvm::sys::path::Style::posix);
   BazPath = llvm::sys::path::convert_to_slash(BazPath);
   Parm.Path = BazPath;
   Frag = GetFrag("", FooPath.c_str());
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_FALSE(Conf.Index.External);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 
   // File under MountPoint, index should be set.
   BazPath = testPath("foo/baz.h", llvm::sys::path::Style::posix);
   BazPath = llvm::sys::path::convert_to_slash(BazPath);
   Parm.Path = BazPath;
   Frag = GetFrag("", FooPath.c_str());
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_TRUE(Conf.Index.External);
-  EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::File);
+  EXPECT_THAT(Conf.Index.External.MountPoint, FooPath);
 
   // Only matches case-insensitively.
   BazPath = testPath("fOo/baz.h", llvm::sys::path::Style::posix);
   BazPath = llvm::sys::path::convert_to_slash(BazPath);
   Parm.Path = BazPath;
 
   FooPath = testPath("FOO/", llvm::sys::path::Style::posix);
   FooPath = llvm::sys::path::convert_to_slash(FooPath);
   Frag = GetFrag("", FooPath.c_str());
   compileAndApply();
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
 #ifdef CLANGD_PATH_CASE_INSENSITIVE
-  ASSERT_TRUE(Conf.Index.External);
-  EXPECT_THAT(Conf.Index.External->MountPoint, FooPath);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::File);
+  EXPECT_THAT(Conf.Index.External.MountPoint, FooPath);
 #else
-  ASSERT_FALSE(Conf.Index.External);
+  ASSERT_EQ(Conf.Index.External.Kind, Config::ExternalIndexSpec::None);
 #endif
 }
 
 TEST_F(ConfigCompileTests, AllScopes) {
   // Defaults to true.
   EXPECT_TRUE(compileAndApply());
   EXPECT_TRUE(Conf.Completion.AllScopes);
 

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

TEST(ProjectAware, Test) {
   auto Idx = createProjectAwareIndex(std::move(Gen), true);
   FuzzyFindRequest Req;
   Req.Query = "1";
   Req.AnyScope = true;
 
   EXPECT_THAT(match(*Idx, Req), IsEmpty());
 
   Config C;
-  C.Index.External.emplace();
-  C.Index.External->Location = "test";
+  C.Index.External.Kind = Config::ExternalIndexSpec::File;
+  C.Index.External.Location = "test";
   WithContextValue With(Config::Key, std::move(C));
   EXPECT_THAT(match(*Idx, Req), ElementsAre("1"));
   return;
 }
 
 TEST(ProjectAware, CreatedOnce) {
   unsigned InvocationCount = 0;
   IndexFactory Gen = [&](const Config::ExternalIndexSpec &, AsyncTaskRunner *) {
     ++InvocationCount;
     return createIndex();
   };
 
   auto Idx = createProjectAwareIndex(std::move(Gen), true);
   // No invocation at start.
   EXPECT_EQ(InvocationCount, 0U);
   FuzzyFindRequest Req;
   Req.Query = "1";
   Req.AnyScope = true;
 
   // Cannot invoke without proper config.
   match(*Idx, Req);
   EXPECT_EQ(InvocationCount, 0U);
 
   Config C;
-  C.Index.External.emplace();
-  C.Index.External->Location = "test";
+  C.Index.External.Kind = Config::ExternalIndexSpec::File;
+  C.Index.External.Location = "test";
   WithContextValue With(Config::Key, std::move(C));
   match(*Idx, Req);
   // Now it should be created.
   EXPECT_EQ(InvocationCount, 1U);
   match(*Idx, Req);
   // It is cached afterwards.
   EXPECT_EQ(InvocationCount, 1U);
   return;
 }
 } // namespace clangd
 } // namespace clang