[clangd] ExternalIndex turns off BackgroundIndex only if it isn't set

[kadircet]

Summary

This will enable users to have a Background: Build in their in-project
configs, while having an external index specification in their user
config file.

Depends on D90749.

[sammccall]

I'm not sure we should do this - it complicates the model substantially (I don't think I could explain it to a user), and it still leaves surprising cases.

I'm not sure config is the place for a lot of magic do-what-I-mean behavior - it's the place where the user overrides such behaviors.

(If you think the combinations are too confusing, we could consider *not* disabling background index implicitly for external indexes, and simply warning instead)

clang-tools-extra/clangd/ConfigCompile.cpp

306

This is vague and hard to specify more precisely.

308

doesn't this mean that if a user has explicit Background: Build at a higher scope, then a more narrowly scoped external index won't disable it?

This seems at least as confusing as the problems this is trying to solve.

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

308

doesn't this mean that if a user has explicit Background: Build at a higher scope, then a more narrowly scoped external index won't disable it?

Yes. The motivating case I had was something like this:

• Let's say user turns background-index on inside llvm-project/clang-tools-extra/clangd/.clangd.
• Defines an external index via user-config for llvm-project.

Since user-config is the inner-most provider, it will turn off the backgrond-indexing implicitly for the whole project, and user won't be able to turn it on for a subdirectory without specifying that in user-config directory too.

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

308

In that scenario, I'm not sure whether the user wants background indexing on or not. (e.g. more likely *someone else* turned it on in the project config, who wasn't aware of the user config).

Maybe there's a principle here that project config for an inner directory is actually more specific and therefore should take precedence over user config for an outer directory? That seems like it might be at least as useful as the current behavior, but we shouldn't be implementing it setting-by-setting.

clang-tools-extra/clangd/Config.h

struct Config {
 
   /// Controls how the compile command for the current file is determined.
   struct {
     // Edits to apply to the compile command, in sequence.
     std::vector<llvm::unique_function<void(std::vector<std::string> &) const>>
         Edits;
   } CompileFlags;
 
-  enum class BackgroundPolicy { Build, Skip };
+  enum class BackgroundPolicy { Auto, Build, Skip };
   /// Controls background-index behavior.
   struct {
     /// Whether this TU should be indexed.
-    BackgroundPolicy Background = BackgroundPolicy::Build;
+    BackgroundPolicy Background = BackgroundPolicy::Auto;
     /// Describes an external index configuration. At most one of Server or File
     /// will be set at a time.
     struct {
       /// Address of a remote-index-server, in the form of "ip:port".
       llvm::Optional<std::string> Server;
       /// Absolute path to a monolithic index.
       llvm::Optional<std::string> File;
       /// Absolute path to source root this index is associated with. Empty

clang-tools-extra/clangd/ConfigCompile.cpp

Out.Apply.push_back(
           C.Index.External.Server.reset();
 
           C.Index.External.MountPoint = std::move(**External.MountPoint);
           if (External.File)
             C.Index.External.File.emplace(std::move(**External.File));
           else
             C.Index.External.Server.emplace(std::move(**External.Server));
 
-          // Disable background indexing for the files under the mountpoint.
-          // Note that this will overwrite statements in any previous fragments
-          // (including the current one).
+          // Disable background indexing for the files under the mountpoint. If
+          // user didn't take any explicit actions.
+          if (C.Index.Background == Config::BackgroundPolicy::Auto)
             C.Index.Background = Config::BackgroundPolicy::Skip;
         });
   }
 
   void compile(Fragment::StyleBlock &&F) {
     if (!F.FullyQualifiedNamespaces.empty()) {
       std::vector<std::string> FullyQualifiedNamespaces;
       for (auto &N : F.FullyQualifiedNamespaces) {
         // Normalize the data by dropping both leading and trailing ::

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

 TEST_F(ConfigCompileTests, Index) {
   Frag.Index.Background.emplace("Skip");
   EXPECT_TRUE(compileAndApply());
   EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
 
   Frag = {};
   Frag.Index.Background.emplace("Foo");
   EXPECT_TRUE(compileAndApply());
-  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Build)
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Auto)
       << "by default";
   EXPECT_THAT(
       Diags.Diagnostics,
       ElementsAre(DiagMessage(
           "Invalid Background value 'Foo'. Valid values are Build, Skip.")));
 }
 
 TEST_F(ConfigCompileTests, PathSpecMatch) {
TEST_F(ConfigCompileTests, ExternalBlockErrOnNoSource) {
   compileAndApply();
   EXPECT_THAT(
       Diags.Diagnostics,
       Contains(AllOf(
           DiagMessage("At least one of File or Server must be specified."),
           DiagKind(llvm::SourceMgr::DK_Error))));
 }
 
-TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+TEST_F(ConfigCompileTests, ExternalBlockAndBackgroundIndex) {
   Parm.Path = "/foo/bar/baz.h";
-  Frag.Index.Background.emplace("Build");
+  auto CreateFragWithBackground = [](llvm::StringRef Background) {
     Fragment::IndexBlock::ExternalBlock External;
     External.File.emplace("/foo");
     External.MountPoint.emplace("/foo/bar");
+
+    Fragment Frag;
     Frag.Index.External = std::move(External);
+    if (!Background.empty())
+      Frag.Index.Background.emplace(Background.str());
+    return Frag;
+  };
+
+  // Preserves when explicitly mentioned.
+  Frag = CreateFragWithBackground("Build");
+  compileAndApply();
+  EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Build);
+
+  // Overwrites to Skip otherwise.
+  Frag = CreateFragWithBackground("");
   compileAndApply();
   EXPECT_EQ(Conf.Index.Background, Config::BackgroundPolicy::Skip);
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockMountPoint) {
   auto GetFrag = [](llvm::StringRef Directory,
                     llvm::Optional<const char *> MountPoint) {
     Fragment Frag;