[clangd] Introduce config parsing for External blocks

[kadircet]

Summary

Enable configuration of remote and static indexes through config files
in addition to command line arguments.

[sammccall]

LG (comment nits) thanks!
(Meta-point: not sure how useful splitting this patch out from the compile step is...)

clang-tools-extra/clangd/ConfigFragment.h

165

I'd move the file-take-precedence-over-server part to server.

Or even just say only one source (file/server) should be configured, leave the behavior unspecified, and report an error when compiling the fragment.

165

"Configuration information for" is redundant here.

maybe just

/ An external index uses data source outside of clangd itself.
/ This is usually prepared using clangd-indexer.

170

nit: I'm not sure "monolithic" is a useful qualifier to the audience here.
Path to an index file generated by clangd-indexer?

171

Generalize this as "Relative paths may be used, if the config fragment is associated with a directory."?

173

Should we mention slashes here too? :-\

Maybe we should just lift "all paths use forward-slashes" to a top-level comment.

174

nit: for a clangd-index-server

176

Again: "Default is the directory associated with the config frament".

(Repeating this makes me wonder if we should have defined a more specific name like "Home"...)

clang-tools-extra/clangd/ConfigYAML.cpp

127

nit: this sort of validation would normally live in ConfigCompile, since it's not to do with serialization

[kadircet]

• Update comments
• Drop the warning for specifying server on non-grpc builds

[kadircet]

(Meta-point: not sure how useful splitting this patch out from the compile step is...)

I wanted to keep them separate especially to ensure testing isn't mixed up. As these things tend to get big easily :/

clang-tools-extra/clangd/ConfigFragment.h

173

This is literally used for pointing at a file on disk though. So it doesn't really matter if this is forward or back slashes?

176

not sure where else we repeated this ?

clang-tools-extra/clangd/ConfigYAML.cpp

127

moving into compile logic.

[kadircet]

(sorry looks like i forgot to hit submit :( )

[kadircet]

• Preserve location of external block

clang-tools-extra/clangd/ConfigFragment.h

struct Fragment {
   /// Controls how clangd understands code outside the current file.
   /// clangd's indexes provide information about symbols that isn't available
   /// to clang's parser, such as incoming references.
   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 {
+      /// 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
+      /// and relative otherwise. Should always use forward-slashes.
+      llvm::Optional<Located<std::string>> MountPoint;
+    };
+    llvm::Optional<Located<ExternalBlock>> External;
   };
   IndexBlock Index;
 
   // Describes the style of the codebase, beyond formatting.
   struct StyleBlock {
     // 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.

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 <system_error>
void parse(Fragment::ClangTidyBlock &F, Node &N) {
     });
     Dict.parse(N);
   }
 
   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;
+      parse(External, N);
+      F.External.emplace(std::move(External));
+      F.External->Range = N.getSourceRange();
+    });
+    Dict.parse(N);
+  }
+
+  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);
   }
 
   // Helper for parsing mapping nodes (dictionaries).
   // We don't use YamlIO as we want to control over unknown keys.
   class DictParser {
     llvm::StringRef Description;
     std::vector<std::pair<llvm::StringRef, std::function<void(Node &)>>> Keys;

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

 //===-- ConfigYAMLTests.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 "Annotations.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
 #include "Protocol.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
- 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, ExternalBlock) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+Index:
+  External:
+    File: "foo"
+    Server: ^"bar"
+    MountPoint: "baz"
+  )yaml");
+  auto Results =
+      Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_EQ(Results.size(), 1u);
+  ASSERT_TRUE(Results[0].Index.External);
+  EXPECT_THAT(*Results[0].Index.External.getValue()->File, Val("foo"));
+  EXPECT_THAT(*Results[0].Index.External.getValue()->MountPoint, Val("baz"));
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(*Results[0].Index.External.getValue()->Server, Val("bar"));
+}
+
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang