[clangd] Introduce config compilation for External blocks

[kadircet]

Summary

Compilation logic for External blocks. A few of the high level points:

• Requires exactly one-of File/Server at a time:
  • Server is ignored in case of both, with a warning.
  • Having none is an error, would render ExternalBlock void.
• Ensures mountpoint is an absolute path:
  • Interprets it as relative to FragmentDirectory.
  • Defaults to FragmentDirectory when empty.
• Marks Background as Skip.

Depends on D90748.

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

283

My math friends would call this "case bashing" :-)
In particular I'm cringing at the thought of adding a third option here (it's not likely, but I think the code would be easier to understand if it was clear that two is an arbitrary number).

What about:

unsigned SourceCount = External.File.hasValue() + External.Server.hasValue();
if (SourceCount != 1)
  return diag(Error, "Exactly one of File and Server must be set");

(I don't think separate error messages for missing/multiple, or any particular behavior for multiple, is particularly useful)

285

these missing ranges are potentially pretty difficult for users - they won't know e.g. which file the error occurred in.

Do you think we can Located<> the whole ExternalBlock?

299

somewhere we should be converting the paths to native slashes, I think.

312

pull out a method like Optional<string> makeAbsolute(StringRef Path, StringRef Description)?

312

would also be good to include "Because this configuration is not associated with a directory"

329

we do all this validation and wind up writing back into the same data structure that can't represent it.
We don't actually have to validate again later, because now our invariants are rules, not suggestions. Still, it seems fragile, and violates a maxim that got stuck in my head: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/

Can we have an type for this spec in config, and have only a single slot for it?
It could be as simple as:

struct ExternalIndexSpec { 
  enum {File, Remote} Kind;
  std::string Location; // specific to kind, hack this up later if need be
  std::string MountPoint;
};

I think the single biggest improvement is that we'd model "replacing the index for this config" as a single assignment rather than 5 coordinated ones. So if you want something less different, putting the Optional around the config struct instead of individual fields would be fine too. (though that would require it to be named)

334

hang on, you say "under the mountpoint", but... that's not what's happening.

If we want that behavior, we need to use SourceInfo::Directory as the mountpoint (and so probably give users a way to set that in the global config fragment).

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

285

I was doing that initially, but they felt similarly annoying, as they point at value corresponding to External block rather than the whole ,value range. In addition to that it only covers the first line :/

Happy to do that if you think that's still better than an empty/invalid range.

334

hang on, you say "under the mountpoint", but... that's not what's happening.

Umm, why? We bail-out at the beginning of apply if filepath doesn't start with mountpoint ?

[kadircet]

• Define an ExternalIndexSpec structure, and populate that during compile.

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

279

consistent with other helpers, can we avoid passing errors around, and instead issue the diagnostic here and return optional?

Signature seems overly general: the Path argument is always a Located<string>, the Base is always the fragment directory. These seem likely to always hold.

285

I think it's better than the empty range, even if it just gives us a location for the block, that tells us which file and which fragment.

(sorry, not sure what ",value range" is...)

292

why not path::make_absolute?

319

comment echoes the code, which is not hard to read - remove?

334

Sorry, I was confused...

[kadircet]

• Use ExternalBlock's range for diags.
• Move diagnostic emitting to makeAbsolute.

[thakis]

This breaks tests on Windows: http://45.33.8.238/win/28408/step_9.txt

Please take a look, and revert for now if it takes a while to fix.

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 <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// Settings that express user/project preferences and control clangd behavior.
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 };
+  /// Describes an external index configuration.
+  struct ExternalIndexSpec {
+    enum { 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;
 
   /// Style of the codebase.
   struct {
     // Namespaces that should always be fully qualified, meaning no "using"
     // declarations, always spell out the whole name (with or without leading
     // ::). All nested namespaces are affected as well.
     std::vector<std::string> FullyQualifiedNamespaces;

clang-tools-extra/clangd/ConfigCompile.cpp

 //  - problems can be reported in stage 1, in stage 2 we must silently recover
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigProvider.h"
+#include "Features.inc"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
     std::string RegexError;
     if (!Result.isValid(RegexError)) {
       diag(Error, "Invalid regex " + Anchored + ": " + RegexError, Text.Range);
       return llvm::None;
     }
     return Result;
   }
 
+  llvm::Optional<std::string> makeAbsolute(Located<std::string> Path,
+                                           llvm::StringLiteral Description,
+                                           llvm::sys::path::Style Style) {
+    if (llvm::sys::path::is_absolute(*Path))
+      return *Path;
+    if (FragmentDirectory.empty()) {
+      diag(Error,
+           llvm::formatv(
+               "{0} must be an absolute path, because this fragment is not "
+               "associated with any directory.",
+               Description)
+               .str(),
+           Path.Range);
+      return llvm::None;
+    }
+    llvm::SmallString<256> AbsPath = llvm::StringRef(*Path);
+    llvm::sys::fs::make_absolute(FragmentDirectory, AbsPath);
+    llvm::sys::path::native(AbsPath, Style);
+    return AbsPath.str().str();
+  }
+
   // Helper with similar API to StringSwitch, for parsing enum values.
   template <typename T> class EnumSwitch {
     FragmentCompiler &Outer;
     llvm::StringRef EnumName;
     const Located<std::string> &Input;
     llvm::Optional<T> Result;
     llvm::SmallVector<llvm::StringLiteral, 8> ValidValues;
 
if (F.Background) {
       if (auto Val = compileEnum<Config::BackgroundPolicy>("Background",
                                                            **F.Background)
                          .map("Build", Config::BackgroundPolicy::Build)
                          .map("Skip", Config::BackgroundPolicy::Skip)
                          .value())
         Out.Apply.push_back(
             [Val](const Params &, Config &C) { C.Index.Background = *Val; });
     }
+    if (F.External)
+      compile(std::move(**F.External), F.External->Range);
+  }
+
+  void compile(Fragment::IndexBlock::ExternalBlock &&External,
+               llvm::SMRange BlockRange) {
+#ifndef CLANGD_ENABLE_REMOTE
+    if (External.Server) {
+      diag(Error, "Clangd isn't compiled with remote index support, ignoring "
+                  "Server." External.Server->Range);
+      External.Server.reset();
+    }
+#endif
+    // Make sure exactly one of the Sources is set.
+    unsigned SourceCount =
+        External.File.hasValue() + External.Server.hasValue();
+    if (SourceCount != 1) {
+      diag(Error, "Exactly one of File or Server 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);
+    }
+    // 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 (!P.Path.startswith(Spec.MountPoint))
+        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;
+    });
   }
 
   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 ::
         StringRef Namespace(*N);
         Namespace.consume_front("::");
         Namespace.consume_back("::");
         FullyQualifiedNamespaces.push_back(Namespace.str());
       }
       Out.Apply.push_back([FullyQualifiedNamespaces(
                               std::move(FullyQualifiedNamespaces))](
                               const Params &, Config &C) {

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

 //===-- ConfigCompileTests.cpp --------------------------------------------===//
 //
 // 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 "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
+#include "Features.inc"
 #include "TestFS.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SizeIs;
 using ::testing::StartsWith;
 
 class ConfigCompileTests : public ::testing::Test {
 protected:
   CapturedDiags Diags;
EXPECT_EQ(
       Conf.ClangTidy.Checks,
       "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
   EXPECT_EQ(Conf.ClangTidy.CheckOptions.size(), 2U);
   EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("StrictMode"), "true");
   EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("example-check.ExampleOption"),
             "0");
 }
 
+TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("");
+  External.Server.emplace("");
+  Frag.Index.External = std::move(External);
+  compileAndApply();
+  llvm::StringLiteral ExpectedDiag =
+#ifdef CLANGD_ENABLE_REMOTE
+      "Exactly one of File or Server must be set.";
+#else
+      "Clangd isn't compiled with remote index support, ignoring Server.";
+#endif
+  EXPECT_THAT(Diags.Diagnostics,
+              Contains(AllOf(DiagMessage(ExpectedDiag),
+                             DiagKind(llvm::SourceMgr::DK_Error))));
+}
+
+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))));
+}
+
+TEST_F(ConfigCompileTests, ExternalBlockDisablesBackgroundIndex) {
+  Parm.Path = "/foo/bar/baz.h";
+  Frag.Index.Background.emplace("Build");
+  Fragment::IndexBlock::ExternalBlock External;
+  External.File.emplace("/foo");
+  External.MountPoint.emplace("/foo/bar");
+  Frag.Index.External = std::move(External);
+  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;
+    Frag.Source.Directory = Directory.str();
+    Fragment::IndexBlock::ExternalBlock External;
+    External.File.emplace("/foo");
+    if (MountPoint)
+      External.MountPoint.emplace(*MountPoint);
+    Frag.Index.External = std::move(External);
+    return Frag;
+  };
+
+  Parm.Path = "/foo/bar.h";
+  // Non-absolute MountPoint without a directory raises an error.
+  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);
+
+  // Ok when relative.
+  Frag = GetFrag("/", "foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+
+  // None defaults to ".".
+  Frag = GetFrag("/", llvm::None);
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/");
+
+  // Without a file, external index is empty.
+  Parm.Path = "";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File outside MountPoint, no index.
+  Parm.Path = "/bar/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_FALSE(Conf.Index.External);
+
+  // File under MountPoint, index should be set.
+  Parm.Path = "/foo/baz.h";
+  Frag = GetFrag("", "/foo");
+  compileAndApply();
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_TRUE(Conf.Index.External);
+  EXPECT_THAT(Conf.Index.External->MountPoint, "/foo");
+}
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang