[clangd] Config: Fragments and parsing from YAML

[sammccall]

Summary

This is a piece from the design in https://tinyurl.com/clangd-config
https://reviews.llvm.org/D82335 is a draft with all the bits linked together.
It doesn't do anything yet, it's just a library + tests.

It deliberately implements a minimal set of actual configuration options.

[kadircet]

clang-tools-extra/clangd/ConfigFragment.h

9

i think this paragraph belongs to Config.h

14

s/which is models/which models/

16

again i don't think these are relevant here.

maybe provide more details about any new config option should start propagating from here, as this is the closest layer to user written config,

61

what about fromYAML and returning vector<const Fragment> ?

65

why the bundling?

69

maybe invalid/absent rather than ignored (or change it to say should be ignored) ? I am assuming this is referring to locations of config parameters like Adds and conditions.

73

what is the use of this ?

75

why not make this const ? i don't think it makes sense to modify these after creation.

77

comments?

78

some comments? especially around fragment applies to file matching all (or any) of the pathmatches

81

HasUnrecognizedCondition ?

also default init to true maybe?

85

some comments.

I am not sure if putting Fragment suffix to all of these makes sense, as they already reside inside Fragment. Maybe section or block ? (same goes for ConditionFragment)

87

maybe make this an llvm:Optional too. Even though emptiness of Add would be OK in this case, for non-container "blocks" we might need to introduce an optional, and homogeneity would be nice when it happens.

[kadircet]

clang-tools-extra/clangd/ConfigYAML.cpp

29

a comment for this buf, especially the reason why it's a member rather than a function-local when needed. (I suppose to get rid of const/dest penalties?)

40

It is a little bit subtle that "at most once" execution of this handler is assured by DictParser. Can we put some comments explaining it to either DictParser itself or to handle member ?

74

maybe a comment for these two, suggesting the latter is used for unknown keys.

and also mention it is handlers responsibility emit any diagnostics in case of failures?

84

maybe assert on duplicate keys in debug builds?

100

i suppose yamlparser has already emitted a diagnostic by then ? can you add a comment about it if that's the case.

and if not, shouldn't we emit something about a broken stream?

same goes for KV.getValue() below.

105

maybe Ignoring duplicate key instead of just Duplicate key ?

136

s/S->getValue(Buf)/BS->getValue()/ (or change auto *BS to auto *S)

and a test

150

shouldn't we error/warn out if not ?

161

do we really want to return a boolean here (and not at the warning ?)

I can see that it's nice to return error("xxx") but it looks surprising as we are not returning the error but just false.
I would make this void, up to you though.

202

*sigh*

what a mess, Scanner in YamlParser already adds this buffer. It is unclear why though, as it adds a non-owning buffer and that buffer is never accessed. (well apart from SMLoc to Buffer conversions when emitting diags)
It rather makes use of pointers to the underlying data, obtained before adding that buffer. So it is the caller keeping the data alive anyways.

should we consider having an entry point that takes an SMLoc and a SourceMgr for yaml::Stream instead of this? That way we can add the owning entry to sourcemanager and start the stream at beginning of that buffer.

i am afraid this might bite us in the future, as SM is exposed publicly and one might try to iterate over buffers, which might come as a shock. we can postpone until that happens though, so feel free to leave it as it is.

[kadircet]

Thanks! Looks great, I've finally managed to finish all my iterations :D

Apart from the comments(IIRC which are mostly minor), it felt like there were a few lines that aren't clang-format'd. Please make sure to run clang-format on the files at some point.

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

58

is there an explanation to what these ranges are ?

All I could find is somewhere in the implementation making sure these are relative to the D.getLineNo and nothing else :/

73

why no print others ?

Kind: D.Message @ Pos - Range

133

i think having a full matcher(i.e with kind, pos and ranges) might be more expressive here, up to you though

[sammccall]

Address review comments

[sammccall]

Thanks!

Apart from the comments(IIRC which are mostly minor), it felt like there were a few lines that aren't clang-format'd. Please make sure to run clang-format on the files at some point.

git-clang-format thinks it's clean, so please point out any examples you see!

clang-tools-extra/clangd/ConfigFragment.h

9

This first sentence does, and I'll put it there too, but I think one redundant sentence to establish the context is reasonable.

The next three lines belong in this file because they describe how users control config (which they do through ConfigFragment, not through Config).

16

again i don't think these are relevant here.

I think how this code relates to the rest of the system is a pretty important thing for documentation to cover! I wish docs in general did a better job of this.
Relying on xrefs in the code is pretty noisy and low-level, and guessing at filenames doesn't scale well.

maybe provide more details about any new config option should start propagating from here, as this is the closest layer to user written config,

Yeah - it depends whether you're looking at "here is a feature I'd like to make configurable" vs "here is some configuration syntax I want to support".
I've moved it here, but I'm not 100% sure it's the right call.

61

what about fromYAML

I can live with it if you like, but I prefer parse:

• Fragment::fromYAML reads funny as it returns multiple fragments in general
• parse describes the action more clearly and so hints toward thinking about error handling etc

returning vector<const Fragment> ?

why? the resulting vector is slower (has to copy instead of move on reallocation).
It prevents us from moving from the fragment when we compile it (which we do in some places)

65

Oops, this is so I could give them a collective comment, and I forgot to do it...

The idea is everything else represents the directly-user-specified "body" of the configuration, but these fields are different. (The associated directory also goes here, though wasn't needed in this patch).
I wanted to wall these off a bit as the outer struct will gain many fields and so this may become less obvious. (Also makes it easier to skip over if we're generating docs etc)

69

Changed to "should be ignored". guaranteeing that the SMRanges is stronger but not actually useful.

73

There may be multiple fragments in a file.

When tracking how fragments are used in the logs, there should be enough information to identify the particular fragment, without having to guess.

• during parsing, we can log in some source-specific way
• in the Fragment form, we can log this location to identify the fragment
• during compilation, we log this location and the pointer address of the new CompiledFragment
• subsequently we can just log the CompiledFragment address

75

const members are a pain, as they prevent moving the object (and vectors of them are doing copies, etc). For that reason I generally don't find "I don't semantically need to modify this" a good enough reason to mark a member const.

Adding a constructor muddles the idea of a dumb struct and presents an inconsistent API - the other fields *also* don't really change during the lifetime (parseYAML() is basically a big constructor)

77

Whoops, done. There's an argument to be made that we shouldn't maintain docs in two places, and the user docs should be canonical.
Something to think about later, maybe we can generate them.

I'm always unsure whether to put the doc for "blocks" like this on the struct or the field...

81

Renamed. I guess you meant false here? (an empty condition should not have this set)

87

I'd rather push this the other way: the presence of an empty block like this shouldn't have any semantic effect - I think we can be consistent about this and it helps the schema feel regular/extensible/predictable.

Condition was the only tricky case, but I don't think we need it, so I've removed the Optional on the condition instead. it's an AND, so an empty set of conditions evaluates to "unconditionally true" anyway.

clang-tools-extra/clangd/ConfigYAML.cpp

29

Moved it inline instead.

40

Ooh, hadn't noticed the subtlety with emplace :-)
Emplace is gone since condition is no longer optional.
Added a comment too.

74

maybe a comment for these two, suggesting the latter is used for unknown keys.

These are private fields, the public trivial setters are commented, I don't think commenting them again adds value.

and also mention it is handlers responsibility emit any diagnostics in case of failures?

Done.

136

Nice catch. This would be a good clang-tidy check I think... variable initialized in an if and then used in an else.

150

we do: scalarValue warns if it returns None and is documented to do so.

161

there's just one callsite, made void for now, can revisit with more callsites.

202

I can't think of any reason we'd iterate over the buffers, as there's only ever one real buffer and it's identifiable as the "main file" in the usual way.
So I'd rather take the horrible API if/when it actually becomes a problem.

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

58

These are ranges to highlight (e.g. in console output). SourceMgr/SMDiag is modeled after clang's SourceManager/Diagnostic, where this stuff is a bit better documented.

133

Done, though rather separate matchers as otherwise it's too much work to get decent error messages.

[kadircet]

LGTM with couple of nits.

Regarding clang-format, I was mostly confused by template <typename T> class Located { being on a single line, but apparently that's the style :D

clang-tools-extra/clangd/ConfigFragment.h

61

I can live with it if you like, but I prefer parse:

I was mainly unhappy about it because parse doesn't reflect the construction bit of the process, but it is clear from the signature and you are right about returning multiple fragments contradicting with the idea of from. I am more inclined towards from but I am fine with parse too if it feels more natural to you.

why? the resulting vector is slower (has to copy instead of move on reallocation).

It was to signal the fact that these correspond to some immutable input received, but I giving up on move semantics sounds like a nasty consequence. So let's keep it that way (unless you want to hide the members and provide only const accessors, which is some plumbing for a simple struct like this and might hinder readability)

73

right, I was thinking that we already have location information available for the start of the fragment during parsing and wasn't sure how useful it would be in later on stages as we know the exact location of the relevant entries (keys, scalars etc.) for logging.

I wasn't against the idea of storing it, just wanted to learn if it has any significant importance for the upcoming stages. thanks for the info!

75

this was in parallel with the idea of returning vector<const Fragment>. so agreed.

77

I suppose it makes sense to put the part starting with /// Conditions based on a file's path use the following form: above the field rather than the struct, especially if we are planning to generate docs.

also:
s/ConfigFragment/Fragment/

81

I was actually suggesting true to make sure this is explicitly set by the parser in case of success and not forgotten when something slips away. but not that important.

87

sure SG, I was rather afraid of introducing an int option one day, then we might need to signal it's existence with an Optional but one might also argue that we would likely have a "default" value for such options, hence instead of marking it as None we could just make use of the default in absence of that option.

clang-tools-extra/clangd/ConfigYAML.cpp

136

yup sounds good, also i would change the else if to an if, as we are returning anyways (which might be another tidy check)

150

right but this function still returns Result and not None

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

58

right, sorry i forgot to mention in the first comment. I was rather afraid of Range being relative to start of line, but being contained in another line. (due to the usage on start.character and end.character down below)

[sammccall]

Thanks for the careful review!

clang-tools-extra/clangd/ConfigFragment.h

73

Yeah, storing the start of the fragment given a slightly more correct location to represent the fragment itself, but more importantly we don't have to hunt around the config to find something that has a location.

77

Yeah - the counter-argument is that if you *are* reading the code then having the general documentation above the specific stuff reads better. And we can teach a doc generator/extractor to grab the right text easily enough. I'll leave it for now, I think.

s/ConfigFragment/Fragment/

Doh, I did update this everywhere, but apparently not in my brain, so it keeps coming back...

81

The problem is the only sensible implementation for a parser is to set it to false again, then set it to true once you hit an unknown key.
Particularly now that condition is not Optional. I think the invariant that a default-initialized Fragment corresponds to an empty document is more valuable.

87

Oh definitely I think we'll want Optional for singular leaf fields, like Optional<bool> BackgroundIndexBlock::Enabled or whatever.
You want a tristate of yes/no/inherit-existing and Optional is the best fit for that.

I meant specifically for *these "blocks" that act as namespaces*, we avoid giving side-effects to the presence/absence, so Optional isn't needed to encode some user intent.
(Not sure how to formalize this namespace idea, it's something struct-like that appears as a singular field in something struct-like)?

clang-tools-extra/clangd/ConfigYAML.cpp

150

Yeah, we ignore the illegal value and continue.
We could make this a hard error, but that is commitment that where we parse a scalar today we'll never accept anything else in future.
Whereas we might choose to allow some other structure in future (without picking a new name for the key). LSP is full of this...
On the other hand not seeing a dict where you expect one is a hard error, because there are fewer reasons to switch from a dict to something else.

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

58

Oh yeah that threw me for a loop too. I think I got the semantics here right, in any case it's only tests (we don't own any of the code *generating* the ranges, and we're not consuming it in production code yet).

clang-tools-extra/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   AST.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
   CollectMacros.cpp
   CompileCommands.cpp
   Compiler.cpp
+  ConfigYAML.cpp
   Diagnostics.cpp
   DraftStore.cpp
   ExpectedTypes.cpp
   FindSymbols.cpp
   FindTarget.cpp
   FileDistance.cpp
   Format.cpp
   FS.cpp

clang-tools-extra/clangd/ConfigFragment.h

+//===--- ConfigFragment.h - Unit of user-specified configuration -*- C++-*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// The configuration system allows users to control this:
+//  - in a user config file, a project config file, via LSP, or via flags
+//  - specifying different settings for different files
+//
+// This file defines the config::Fragment structure which is models one piece of
+// configuration as obtained from a source like a file.
+// This is distinct from how the config is interpreted (CompiledFragment),
+// combined (ConfigProvider) and exposed to the rest of clangd (Config).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_FRAGMENT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_FRAGMENT_H
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+/// An entity written in config along, with its optional location in the file.
+template <typename T> struct Located {
+  Located(T Value, llvm::SMRange Range = {})
+      : Range(Range), Value(std::move(Value)) {}
+
+  llvm::SMRange Range;
+  T &operator->() { return Value; }
+  const T &operator->() const { return Value; }
+  T &operator*() { return Value; }
+  const T &operator*() const { return Value; }
+
+private:
+  T Value;
+};
+
+/// Used to report problems in parsing or interpreting a config.
+/// Errors reflect structurally invalid config that should be user-visible.
+/// Warnings reflect e.g. unknown properties that are recoverable.
+using DiagnosticCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
+
+/// A chunk of configuration obtained from a config file, LSP, or elsewhere.
+struct Fragment {
+  /// Parses fragments from a YAML file (one from each --- delimited document).
+  /// Documents that contained fatal errors are omitted from the results.
+  /// BufferName is used for the SourceMgr and diagnostics.
+  static std::vector<Fragment> parseYAML(llvm::StringRef YAML,
+                                         llvm::StringRef BufferName,
+                                         DiagnosticCallback);
+
+  struct SourceInfo {
+    /// Retains a buffer of the original source this fragment was parsed from.
+    /// Locations within Located<T> objects point into this SourceMgr.
+    /// Shared because multiple fragments are often parsed from one (YAML) file.
+    /// May be null, then all locations are ignored.
+    std::shared_ptr<llvm::SourceMgr> Manager;
+    /// The start of the original source for this fragment.
+    /// Only valid if SourceManager is set.
+    llvm::SMLoc Location;
+  };
+  SourceInfo Source;
+
+  struct ConditionFragment {
+    std::vector<Located<std::string>> PathMatch;
+    /// An unrecognized key was found while parsing the condition.
+    /// The condition will evaluate to false.
+    bool UnrecognizedCondition;
+  };
+  llvm::Optional<ConditionFragment> Condition;
+
+  struct CompileFlagsFragment {
+    std::vector<Located<std::string>> Add;
+  } CompileFlags;
+};
+
+} // namespace config
+} // namespace clangd
+} // namespace clang
+
+#endif

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/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>
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using llvm::yaml::BlockScalarNode;
+using llvm::yaml::MappingNode;
+using llvm::yaml::Node;
+using llvm::yaml::ScalarNode;
+using llvm::yaml::SequenceNode;
+
+class Parser {
+  llvm::SourceMgr &SM;
+  llvm::SmallString<256> Buf;
+
+public:
+  Parser(llvm::SourceMgr &SM) : SM(SM) {}
+
+  // Tries to parse N into F, returning false if it failed and we couldn't
+  // meaningfully recover (e.g. YAML syntax error broke the stream).
+  // The private parse() helpers follow the same pattern.
+  bool parse(Fragment &F, Node &N) {
+    DictParser Dict("Config", this);
+    Dict.handle("If", [&](Node &N) {
+      F.Condition.emplace();
+      return parse(*F.Condition, N);
+    });
+    Dict.handle("CompileFlags",
+                [&](Node &N) { return parse(F.CompileFlags, N); });
+    return Dict.parse(N);
+  }
+
+private:
+  bool parse(Fragment::ConditionFragment &F, Node &N) {
+    DictParser Dict("Condition", this);
+    Dict.unrecognized([&](llvm::StringRef) { F.UnrecognizedCondition = true; });
+    Dict.handle("PathMatch", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.PathMatch = std::move(*Values);
+      return !N.failed();
+    });
+    return Dict.parse(N);
+  }
+
+  bool parse(Fragment::CompileFlagsFragment &F, Node &N) {
+    DictParser Dict("CompileFlags", this);
+    Dict.handle("Add", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.Add = std::move(*Values);
+      return !N.failed();
+    });
+    return 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<bool(Node &)>>> Keys;
+    std::function<void(llvm::StringRef)> Unknown;
+    Parser *Outer;
+
+  public:
+    DictParser(llvm::StringRef Description, Parser *Outer)
+        : Description(Description), Outer(Outer) {}
+
+    // Parse is called when Key is encountered.
+    void handle(llvm::StringLiteral Key, std::function<bool(Node &)> Parse) {
+      Keys.emplace_back(Key, std::move(Parse));
+    }
+
+    // Handle is called when a Key is not matched by any handle().
+    void unrecognized(std::function<void(llvm::StringRef)> Fallback) {
+      Unknown = std::move(Fallback);
+    }
+
+    // Process a mapping node and call handlers for each key/value pair.
+    bool parse(Node &N) const {
+      if (N.getType() != Node::NK_Mapping)
+        return Outer->error(Description + " should be a dictionary", N);
+      llvm::SmallSet<std::string, 8> Seen;
+      for (auto &KV : llvm::cast<MappingNode>(N)) {
+        auto *K = KV.getKey();
+        if (!K)
+          return false;
+        auto Key = Outer->scalarValue(*K, "Dictionary key");
+        if (!Key)
+          continue;
+        if (!Seen.insert(**Key).second) {
+          Outer->warning("Duplicate key " + **Key, *K);
+          continue;
+        }
+        auto *Value = KV.getValue();
+        if (!Value)
+          return false;
+        bool Matched = false;
+        for (const auto &Handler : Keys) {
+          if (Handler.first == **Key) {
+            if (!Handler.second(*Value))
+              return false;
+            Matched = true;
+            break;
+          }
+        }
+        if (!Matched) {
+          Outer->warning("Unknown " + Description + " key " + **Key, *K);
+          if (Unknown)
+            Unknown(**Key);
+        }
+      }
+      return true;
+    }
+  };
+
+  // Try to parse a single scalar value from the node, warn on failure.
+  llvm::Optional<Located<std::string>> scalarValue(Node &N,
+                                                   llvm::StringRef Desc) {
+    if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
+      return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
+    else if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
+      return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
+    warning(Desc + " should be scalar", N);
+    return llvm::None;
+  }
+
+  // Try to parse a list of single scalar values, or just a single value.
+  llvm::Optional<std::vector<Located<std::string>>> scalarValues(Node &N) {
+    std::vector<Located<std::string>> Result;
+    if (auto *S = llvm::dyn_cast<ScalarNode>(&N)) {
+      Result.emplace_back(S->getValue(Buf).str(), N.getSourceRange());
+    } else if (auto *S = llvm::dyn_cast<BlockScalarNode>(&N)) {
+      Result.emplace_back(S->getValue().str(), N.getSourceRange());
+    } else if (auto *S = llvm::dyn_cast<SequenceNode>(&N)) {
+      for (auto &Child : *S) {
+        if (auto Value = scalarValue(Child, "List item"))
+          Result.push_back(std::move(*Value));
+      }
+    } else {
+      warning("Expected scalar or list of scalars", N);
+      return llvm::None;
+    }
+    return Result;
+  }
+
+  // Report a "hard" error, reflecting a config file that can never be valid.
+  bool error(const llvm::Twine &Msg, const Node &N) {
+    SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Error, Msg,
+                    N.getSourceRange());
+    return false;
+  }
+
+  // Report a "soft" error that could be caused by e.g. version skew.
+  void warning(const llvm::Twine &Msg, const Node &N) {
+    SM.PrintMessage(N.getSourceRange().Start, llvm::SourceMgr::DK_Warning, Msg,
+                    N.getSourceRange());
+  }
+};
+
+} // namespace
+
+std::vector<Fragment> Fragment::parseYAML(llvm::StringRef YAML,
+                                          llvm::StringRef BufferName,
+                                          DiagnosticCallback Diags) {
+  // The YAML document may contain multiple conditional fragments.
+  // The SourceManager is shared for all of them.
+  auto SM = std::make_shared<llvm::SourceMgr>();
+  auto Buf = llvm::MemoryBuffer::getMemBufferCopy(YAML, BufferName);
+  // Adapt DiagnosticCallback to function-pointer interface.
+  // Callback receives both errors we emit and those from the YAML parser.
+  SM->setDiagHandler(
+      [](const llvm::SMDiagnostic &Diag, void *Ctx) {
+        (*reinterpret_cast<DiagnosticCallback *>(Ctx))(Diag);
+      },
+      &Diags);
+  std::vector<Fragment> Result;
+  for (auto &Doc : llvm::yaml::Stream(*Buf, *SM)) {
+    if (Node *N = Doc.parseBlockNode()) {
+      Fragment Fragment;
+      Fragment.Source.Manager = SM;
+      Fragment.Source.Location = N->getSourceRange().Start;
+      if (Parser(*SM).parse(Fragment, *N))
+        Result.push_back(std::move(Fragment));
+    }
+  }
+  // Hack: stash the buffer in the SourceMgr to keep it alive.
+  // SM has two entries: "main" non-owning buffer, and ignored owning buffer.
+  SM->AddNewSourceBuffer(std::move(Buf), llvm::SMLoc());
+  return Result;
+}
+
+} // namespace config
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   CanonicalIncludesTests.cpp
   ClangdTests.cpp
   ClangdLSPServerTests.cpp
   CodeCompleteTests.cpp
   CodeCompletionStringsTests.cpp
   CollectMacrosTests.cpp
   CompileCommandsTests.cpp
   CompilerTests.cpp
+  ConfigYAMLTests.cpp
   DexTests.cpp
   DiagnosticsTests.cpp
   DraftStoreTests.cpp
   ExpectedTypeTest.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp
   FindTargetTests.cpp

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 "Matchers.h"
+#include "Protocol.h"
+#include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "gtest/internal/gtest-internal.h"
+
+namespace clang {
+namespace clangd {
+namespace config {
+template <typename T> void PrintTo(const Located<T> &V, std::ostream *OS) {
+  *OS << ::testing::PrintToString(*V);
+}
+
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+MATCHER_P(Val, Value, "") {
+  if (*arg == Value)
+    return true;
+  *result_listener << "value is " << *arg;
+  return false;
+}
+
+Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
+  auto LineCol = SM.getLineAndColumn(L);
+  Position P;
+  P.line = LineCol.first - 1;
+  P.character = LineCol.second - 1;
+  return P;
+}
+
+Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
+  return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
+}
+
+struct CapturedDiags {
+  std::function<void(const llvm::SMDiagnostic &)> callback() {
+    return [this](const llvm::SMDiagnostic &D) {
+      Diagnostics.emplace_back();
+      Diag &Out = Diagnostics.back();
+      Out.Message = D.getMessage().str();
+      Out.Kind = D.getKind();
+      Out.Pos.line = D.getLineNo() - 1;
+      Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+      if (!D.getRanges().empty()) {
+        const auto &R = D.getRanges().front();
+        Out.Range.emplace();
+        Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
+        Out.Range->start.character = R.first;
+        Out.Range->end.character = R.second;
+      }
+    };
+  }
+  struct Diag {
+    std::string Message;
+    llvm::SourceMgr::DiagKind Kind;
+    Position Pos;
+    llvm::Optional<Range> Range;
+
+    friend void PrintTo(const Diag &D, std::ostream *OS) { *OS << D.Message; }
+  };
+  std::vector<Diag> Diagnostics;
+};
+
+MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
+
+TEST(ParseYAML, SemanticForms) {
+  CapturedDiags Diags;
+  const char *YAML = R"yaml(
+If:
+  PathMatch:
+    - 'abc'
+CompileFlags: { Add: [foo, bar] }
+---
+CompileFlags:
+  Add: baz
+  )yaml";
+  auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 2u);
+  EXPECT_FALSE(Results.front().Condition->UnrecognizedCondition);
+  EXPECT_THAT(Results.front().Condition->PathMatch, ElementsAre(Val("abc")));
+  EXPECT_THAT(Results.front().CompileFlags.Add,
+              ElementsAre(Val("foo"), Val("bar")));
+
+  EXPECT_FALSE(Results.back().Condition);
+  EXPECT_THAT(Results.back().CompileFlags.Add, ElementsAre(Val("baz")));
+}
+
+TEST(ParseYAML, Locations) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+If:
+  PathMatch: [['???bad***regex(((']]
+  )yaml");
+  auto Results =
+      Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  ASSERT_NE(Results.front().Source.Manager, nullptr);
+  EXPECT_EQ(toRange(Results.front().Condition->PathMatch.front().Range,
+                    *Results.front().Source.Manager),
+            YAML.range());
+}
+
+TEST(ParseYAML, Diagnostics) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+If:
+  [[UnknownCondition]]: "foo"
+CompileFlags:
+  Add: 'first'
+---
+CompileFlags: {^
+)yaml");
+  auto Results =
+      Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+
+  ASSERT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("Unknown Condition key UnknownCondition"),
+                          DiagMessage("Unexpected token. Expected Key, Flow "
+                                      "Entry, or Flow Mapping End.")));
+
+  EXPECT_EQ(Diags.Diagnostics.front().Kind, llvm::SourceMgr::DK_Warning);
+  EXPECT_EQ(Diags.Diagnostics.front().Pos, YAML.range().start);
+  EXPECT_THAT(Diags.Diagnostics.front().Range, HasValue(YAML.range()));
+
+  EXPECT_EQ(Diags.Diagnostics.back().Kind, llvm::SourceMgr::DK_Error);
+  EXPECT_EQ(Diags.Diagnostics.back().Pos, YAML.point());
+  EXPECT_EQ(Diags.Diagnostics.back().Range, llvm::None);
+
+  ASSERT_EQ(Results.size(), 2u);
+  EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
+  EXPECT_TRUE(Results.front().Condition->UnrecognizedCondition);
+  EXPECT_THAT(Results.back().CompileFlags.Add, IsEmpty());
+}
+
+} // namespace
+} // namespace config
+} // namespace clangd
+} // namespace clang