[clangd] Provide a way to disable external index

[kadircet]

Summary

Users can reset any external index set by previous fragments by
putting a None for the index location, e.g:

Index:
  External:
    Server: None

Note that this will reset both File and Server, and will be a no-op when user
puts Server: None but they have a clangd without remote-index support.

[kadircet]

Note that this will reset both File and Server, and will be a no-op when user
puts Server: None but they have a clangd without remote-index support.

This feels like a nasty consequence, but I am not sure if it is important enough to justify some other syntax like:

Index:
  External: None

but happy to go this way if you think that's more user friendly.

[sammccall]

Using server: None for this seems a bit weird and irregular (why not file: none?)

What about no key inside External?
Like just empty External: or maybe it has to be External: {} (I don't know YAML well...)

[kadircet]

• As discussed offline, support External: None instead.

[sammccall]

clang-tools-extra/clangd/Config.h

86

Hmm, we could now make this non-optional, slight yak-shave, up to you.

[kadircet]

clang-tools-extra/clangd/Config.h

86

i'll do that in a follow-up, since we want this to be cherry-picked i'd rather keep the change to a minimum.

[saugustine]

This change breaks llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:570:11: error: enumeration value 'None' not handled in switch [-Werror,-Wswitch]. I would fix forward, but the solution is not obvious to me.

Reverting shortly.

[crr0004]

I am very new here so I will very likely get this wrong. From what I can tell around the warning, the case missing in ClangdMain.cpp std::unique_ptr<SymbolIndex> loadExternalIndex(const Config::ExternalIndexSpec &External, AsyncTaskRunner *Tasks) shouldn't be getting hit with a None in the External.Kind field. Rather it should return an error.

To prevent the error from being thrown, at the ProjectAwareIndex.cpp SymbolIndex *ProjectAwareIndex::getIndex() const call, it should return a nullptr when the External.Field is None.

There would also need to be a test case to ensure that works as intended.

[kadircet]

thanks for the insights @crr0004 ! i've actually relanded this in rGecc6965b2342397a215b00e8e476d8d37d080322 with the fix to clangdmain. since we always reset the externalindexspec when the kind is None in ConfigCompile.cpp:370, loadExternalIndex should never be called with a spec of None kind. Hence we are just falling to llvm_unreachable.

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 { File, Server } Kind;
+    enum { None, File, Server } Kind;
     /// 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;
   } 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

#ifndef CLANGD_ENABLE_REMOTE
     if (External.Server) {
       elog("Clangd isn't compiled with remote index support, ignoring Server: "
            "{0}",
            *External.Server);
       External.Server.reset();
     }
 #endif
     // Make sure exactly one of the Sources is set.
-    unsigned SourceCount =
-        External.File.hasValue() + External.Server.hasValue();
+    unsigned SourceCount = External.File.hasValue() +
+                           External.Server.hasValue() + *External.IsNone;
     if (SourceCount != 1) {
-      diag(Error, "Exactly one of File or Server must be set.", BlockRange);
+      diag(Error, "Exactly one of File, Server or None must be set.",
+           BlockRange);
       return;
     }
     Config::ExternalIndexSpec Spec;
     if (External.Server) {
       Spec.Kind = Config::ExternalIndexSpec::Server;
       Spec.Location = std::move(**External.Server);
     } else if (External.File) {
       Spec.Kind = Config::ExternalIndexSpec::File;
       auto AbsPath = makeAbsolute(std::move(*External.File), "File",
                                   llvm::sys::path::Style::native);
       if (!AbsPath)
         return;
       Spec.Location = std::move(*AbsPath);
+    } else {
+      assert(*External.IsNone);
+      Spec.Kind = Config::ExternalIndexSpec::None;
     }
+    if (Spec.Kind != Config::ExternalIndexSpec::None) {
       // Make sure MountPoint is an absolute path with forward slashes.
       if (!External.MountPoint)
         External.MountPoint.emplace(FragmentDirectory);
       if ((**External.MountPoint).empty()) {
         diag(Error, "A mountpoint is required.", BlockRange);
         return;
       }
       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();
+        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
       // (including the current one).
       C.Index.Background = Config::BackgroundPolicy::Skip;

clang-tools-extra/clangd/ConfigFragment.h

struct IndexBlock {
     /// Whether files are built in the background to produce a project index.
     /// This is checked for translation units only, not headers they include.
     /// Legal values are "Build" or "Skip".
     llvm::Optional<Located<std::string>> Background;
     /// An external index uses data source outside of clangd itself. This is
     /// usually prepared using clangd-indexer.
     /// Exactly one source (File/Server) should be configured.
     struct ExternalBlock {
+      /// Whether the block is explicitly set to `None`. Can be used to clear
+      /// any external index specified before.
+      Located<bool> IsNone = false;
       /// Path to an index file generated by clangd-indexer. Relative paths may
       /// be used, if config fragment is associated with a directory.
       llvm::Optional<Located<std::string>> File;
       /// Address and port number for a clangd-index-server. e.g.
       /// `123.1.1.1:13337`.
       llvm::Optional<Located<std::string>> Server;
       /// Source root governed by this index. Default is the directory
       /// associated with the config fragment. Absolute in case of user config

clang-tools-extra/clangd/ConfigYAML.cpp

 //===--- ConfigYAML.cpp - Loading configuration fragments from YAML files -===//
 //
 // 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 "ConfigFragment.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/YAMLParser.h"
+#include <string>
 #include <system_error>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 using llvm::yaml::BlockScalarNode;
 using llvm::yaml::MappingNode;
private:
   }
 
   void parse(Fragment::IndexBlock &F, Node &N) {
     DictParser Dict("Index", this);
     Dict.handle("Background",
                 [&](Node &N) { F.Background = scalarValue(N, "Background"); });
     Dict.handle("External", [&](Node &N) {
       Fragment::IndexBlock::ExternalBlock External;
+      // External block can either be a mapping or a scalar value. Dispatch
+      // accordingly.
+      if (N.getType() == Node::NK_Mapping) {
         parse(External, N);
+      } else if (N.getType() == Node::NK_Scalar ||
+                 N.getType() == Node::NK_BlockScalar) {
+        parse(External, scalarValue(N, "External").getValue());
+      } else {
+        error("External must be either a scalar or a mapping.", N);
+        return;
+      }
       F.External.emplace(std::move(External));
       F.External->Range = N.getSourceRange();
     });
     Dict.parse(N);
   }
 
+  void parse(Fragment::IndexBlock::ExternalBlock &F,
+             Located<std::string> ExternalVal) {
+    if (!llvm::StringRef(*ExternalVal).equals_lower("none")) {
+      error("Only scalar value supported for External is 'None'",
+            ExternalVal.Range);
+      return;
+    }
+    F.IsNone = true;
+    F.IsNone.Range = ExternalVal.Range;
+  }
+
   void parse(Fragment::IndexBlock::ExternalBlock &F, Node &N) {
     DictParser Dict("External", this);
     Dict.handle("File", [&](Node &N) { F.File = scalarValue(N, "File"); });
     Dict.handle("Server",
                 [&](Node &N) { F.Server = scalarValue(N, "Server"); });
     Dict.handle("MountPoint",
                 [&](Node &N) { F.MountPoint = scalarValue(N, "MountPoint"); });
     Dict.parse(N);

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

TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
   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 or Server must be set."),
-                     DiagKind(llvm::SourceMgr::DK_Error))));
+      Contains(
+          AllOf(DiagMessage("Exactly one of File, Server or None must be set."),
 #else
   ASSERT_TRUE(Conf.Index.External.hasValue());
   EXPECT_EQ(Conf.Index.External->Kind, Config::ExternalIndexSpec::File);
 #endif
 }
 
+TEST_F(ConfigCompileTests, ExternalBlockDisableWithNone) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.IsNone = true;
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  EXPECT_FALSE(Conf.Index.External.hasValue());
+}
+
 TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) {
   Frag.Index.External.emplace(Fragment::IndexBlock::ExternalBlock{});
   compileAndApply();
   EXPECT_THAT(
       Diags.Diagnostics,
-      Contains(AllOf(DiagMessage("Exactly one of File or Server must be set."),
-                     DiagKind(llvm::SourceMgr::DK_Error))));
+      Contains(
+          AllOf(DiagMessage("Exactly one of File, Server or None must be set."),
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
   auto BazPath = testPath("foo/bar/baz.h", llvm::sys::path::Style::posix);
   Parm.Path = BazPath;
   Frag.Index.Background.emplace("Build");
   Fragment::IndexBlock::ExternalBlock External;
   External.File.emplace(testPath("foo"));

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

- 1
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics,
               ElementsAre(DiagMessage("If should be a dictionary"),
                           DiagMessage("Config should be a dictionary")));
   ASSERT_THAT(Results, IsEmpty());
 }
 
+TEST(ParseYAML, ExternalBlockNone) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+Index:
+  External: None
+  )yaml");
+  auto Results =
+      Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  ASSERT_TRUE(Results[0].Index.External);
+  EXPECT_FALSE(Results[0].Index.External.getValue()->File.hasValue());
+  EXPECT_FALSE(Results[0].Index.External.getValue()->MountPoint.hasValue());
+  EXPECT_FALSE(Results[0].Index.External.getValue()->Server.hasValue());
+  EXPECT_THAT(*Results[0].Index.External.getValue()->IsNone, testing::Eq(true));
+}
+
 TEST(ParseYAML, ExternalBlock) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
 Index:
   External:
     File: "foo"
     Server: ^"bar"
     MountPoint: "baz"