[clangd] Config: compile Fragment -> CompiledFragment -> Config

[sammccall]

Summary

https://reviews.llvm.org/D82335 shows how this fits in with related config work.

[hokein]

clang-tools-extra/clangd/CMakeLists.txt

41

we have a few config-related files now, I wonder would it make sense to create a config/ subdir?

clang-tools-extra/clangd/ConfigCompile.cpp

48

this is only used in method compileRegex, looks like can be moved to a local variable? is there life-time issue?

97

this member seems unused.

clang-tools-extra/clangd/ConfigProvider.h

33

nit: I find the name Params is too general, it implies less information. When I read the code using Params, I have to go-to-definition to see what it means.

if we only have a single member in this struct, maybe just use it directly.

41

I might miss the context, where is the provider, not added yet?

48

Would it be more nature to have a function compile(or so) to do the actual compile (Fragment -> CompiledFragment) rather than doing it in a constructor?

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

41

my first impression was that each // XXX is a separated test, but it turns out not, the tests here seem to be stateful -- Frag keeps being modified. I think it is intentional, but it is hard for readers to track given that the code is long.

[sammccall]

clang-tools-extra/clangd/CMakeLists.txt

41

It seems marginal to me - could go either way. We have a small handful (2 headers and 3 impl files). There may be 1-2 more, but I'm not sure yet. (config-files-on-disk, and json->fragment).
So 6-7 files... smaller than the other subdirs, but not tiny.

There's also the fact that config.h/cpp is not like the others: all features (not just infra) depend on this, it's not in the config:: namespace etc. So should it really be in the subdir?

Mind if we hold off on this until at least the scope of D82335 has landed? I think it would be awkward to do this move halfway through this patchset, but fairly easy to do at any point that things are stable.

clang-tools-extra/clangd/ConfigCompile.cpp

97

it's used on line 52 diag(Error, ...).

I wouldn't bother extracting it except there should be a bunch more places where errors are emitted, and keeping them readable is worthwhile IME.

clang-tools-extra/clangd/ConfigProvider.h

33

Agree with the vague name.
In most contexts, the name is config::Params which is at least a bit more specific.

if we only have a single member in this struct, maybe just use it directly.

I don't like this, though - there are likely to be more things here in the future (e.g. platform), and passing around a string is much less clear as to intent. So any idea about a name for this struct?

What about config::Environment or config::Env? still too vague?

41

Right, D82335 has a rought draft of everything together (it's called ConfigProvider in that patch).

I think it's less churn-y to add the documentation as the classes are added, even if it means temporarily referring to something that doesn't exist yet.

48

I'm not sure. The idea that the input/result is Fragment/CompiledFragment, and that the compilation cannot fail, suggests to me that a constructor is OK here.
On the other hand, the DiangosticCallback param (which we don't keep a reference to) is a bit weird.

So I don't really feel strongly about it one way or the other, happy to change it if you do think it would be significantly better.

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

41

Agree. Made this test reset fragment state after each group of assertions.

[sammccall]

Address some review comments

[hokein]

clang-tools-extra/clangd/CMakeLists.txt

41

it seems marginal to me - could go either way. We have a small handful (2 headers and 3 impl files). There may be 1-2 more, but I'm not sure yet. (config-files-on-disk, and json->fragment).
So 6-7 files... smaller than the other subdirs, but not tiny.

if we end up with 6-7 files, I'd prefer to create a subdirectory, which make our source structure a bit tidy, our clangd/ dir already has a lot of source files.

Mind if we hold off on this until at least the scope of D82335 has landed? I think it would be awkward to do this move halfway through this patchset, but fairly easy to do at any point that things are stable.

sure, I didn't mean we should do this in this patch.

clang-tools-extra/clangd/ConfigCompile.cpp

66

I think if this case happened, we might just bail out directly -- adding the regex conditions seems unnecessary, we never execute them on Line 127 (but we might still need computing the regex for diagnostics).

and I'm not sure the motivation of using vector for Conditions and Apply, it looks like Apply is just 1 element (if we do the bailout for conditions, Conditions will be at most 1 element).

I feel like using a single unique_function for Conditions and Apply might be a better fit with Fragment. There is a comment in ConfigFragment.h:

Each separate condition must match (combined with AND).
When one condition has multiple values, any may match (combined with OR).

so my reading is that:

• Fragment and CompiledFragment is a 1:1 mapping
• A Fragment::Condition represents a single condition, so the AND match applies for different Fragments, which is in ConfigProvider?
• A single condition can have multiple values (PathMatch), we use OR match

Is that correct?

85

nit: use early return.

97

Ah, I see. I must have missed that.

btw, could you spell out the auto type for Error? I believe it is DiagKind, and it would be more clear that it is used in the first parameter of diag (given that they are close).

clang-tools-extra/clangd/ConfigProvider.h

33

not better idea. config::Env doesn't seem to be much better... and the signature apply(const Params &P, Config &C) seems neat. let's keep it as-is.

The comment of Path is a bit vague, it would be nice to clarify what kind of the file we're targeting to? the config file or the .cpp source file?

48

You point is fair.
I'm not sure using compile is *significantly* better, but it is better IMO -- conceptually, the class name CompiledFragment implies that there should be a compile API (this was my impression when reading the code at the first time)

it might be ok to keep as-is.

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

32

nit: compileAndApply?

[sammccall]

clang-tools-extra/clangd/CMakeLists.txt

41

if we end up with 6-7 files, I'd prefer to create a subdirectory

SGTM. I'll carry on with this structure for now, and move files after - please remind me if I forget!

clang-tools-extra/clangd/ConfigCompile.cpp

66

I think if this case happened, we might just bail out directly -- adding the regex conditions seems unnecessary, we never execute them on Line 127 (but we might still need computing the regex for diagnostics).

and I'm not sure the motivation of using vector for Conditions and Apply, it looks like Apply is just 1 element (if we do the bailout for conditions, Conditions will be at most 1 element).

So the explanation for both of these things is that this file is going to grow a lot, and in particular these sections handling PathMatch etc are going to occur again and again, so we need a scalable pattern. I've deliberately limited the scope of the first patchset to only support one condition, one setting to apply etc, but you have to imagine there are likely to be 5 conditions and maybe 25 settings.

So with that in mind:

• yes, actually saving the conditions isn't necessary, but we do need to evaluate them for side-effects (diagnostics) and it's simpler not to keep track of them. Optimizing performance of broken configs is not a big win :-)
• it's marginally simpler to have only one Apply function, but much simpler if separate parts of compile() can push settings independently. Same goes for condition in fact...

Fragment and CompiledFragment is a 1:1 mapping

Yes. I think this is somewhere where the naming is helpful at least :-)

A Fragment::Condition represents a single condition, so the AND match applies for different Fragments, which is in ConfigProvider?

The AND match applies to the multiple Conditions in a single CompiledFragment. Each fragment only considers its own conditions.
So given:

If:
  Platform: win
  PathMatch: [foo1/.*, foo2/.*]
CompileFlags:
  Add: -foo
---
CompileFlags:
  Add: -bar

We end up with... one Provider, suppling two CompiledFragments (each compiled from a separate Fragment).

The first CompiledFragment has two Conditions, conceptually:

• Params.Platform == "win"
• Regex("foo1/.*").matches(Params.File) || Regex("foo2/.*").matches(Params.File)

The second CompiledFragment has no Conditions. Each fragment has one Apply function.

The CompiledFragments are applied independently. They each consider their own Conditions, ANDing them together.

So CompiledFragment #1 checks both conditions. If any fails, it bails out. Otherwise it runs its Apply function, adding -foo.

Then CompiledFragment #2 runs. It has no conditions, so never bails out. It adds -bar.

85

Early return isn't a pattern that's compatible with lots of optional pieces - it doesn't scale as this file grows.

clang-tools-extra/clangd/ConfigProvider.h

48

Argh, you convinced me this would be better, but I can't come up with a form I like.

• static CompiledFragment::compile repeats itself in a weird way. But any other option requires messing around with friend functions, which feels off
• Fragment::compile is maybe the most natural, but the dependencies are backwards (needs the CompiledFragment type, it gets implemented in the wrong file...).
• standalone compile() is maybe the most palatable, but seems a little undiscoverable
• having a function return CompiledFragment means that putting it into a shared_ptr is annoying work, and breaks our pointer-logging trick, but having the factory function return a pointer also seems odd.

I'm starting to think maybe the CompiledFragment class shouldn't be public at all, and we should just using CompiledFragment = unique_function<bool(const Params&, Config&) const>. WDYT?

[sammccall]

address comments

[sammccall]

clang-tools-extra/clangd/ConfigProvider.h

48

I'm starting to think maybe the CompiledFragment class shouldn't be public at all

Thinking more about this:

• making it std::function<...> instead lets us wrap the shared_ptr inside which makes a few APIs a bit cleaner
• as its an interface type, this leaves config providers free to use other ways of manipulating config rather than going via fragments, which is potentially nice for embedders
• the main downside is we're committing to not exposing any more structure (e.g. querying a fragment for which directory it applies to)

starting to quite like this idea.

[sammccall]

CompiledFragment becomes a std::function, compile interface moved to Fragment.h

[hokein]

clang-tools-extra/clangd/ConfigCompile.cpp

41

nit: I'd use a more verbose name, CompiledFragmentImpl.

47

maybe Impl::applying config ... to allow better debug logging?

this reminds us: since the config can easily become a monster, think about multiple config files from different sources (user config file, project config file, LSP, flags) are applied together, we might need a tracing mechanism (at least some useful logs) to help us diagnose a final complicated config. no need to do anything in this patch, just something we should keep in mind.

66

oh, thanks for the explanation, this helps me a lot to understand the code.

so the member Condition in Fragment will be enhanced to something like std::vector<XXX> Conditions in the future? If so, can we defer that for CompiledFragment as well?

Since Fragment and CompiledFragment is 1:1 mapping, I think it is important to keep their implementation aligned, it'd help a lot for readers to understand the code. The condition logic (AND, OR) is subtle and complicated, I was reading this part of code back and forth, still inferred it in an incorrect way.

127

nit: maybe name it ConfigFile, SourceFile sounds like a .cpp file.

clang-tools-extra/clangd/ConfigFragment.h

35

the dependence ( ConfigFragment.h depends on ConfigProvider.h) seems a bit counterintuitive...

81

this API is neat, thanks!

clang-tools-extra/clangd/ConfigProvider.h

48

SG

[sammccall]

Address comments

clang-tools-extra/clangd/ConfigCompile.cpp

47

I've changed these from config::Fragment to "Config fragment" to be less misleading. I'm not sure sprinkling in internal class names helps much vs having them be clear sentences, I always end up searching for the log string to see exactly where it comes from anyway.

This is dlog, it would be nice to make it vlog but I think it's just way too spammy, we'll get several of these for every file that's opened and background-indexed...

we might need a tracing mechanism (at least some useful logs)

My secret plan is to have a clangd flag like clangd --check <filename> that will *not* run the language server, but instead dump a bunch of information about how clangd would process that file (stringing together CDB, config, query-driver, diagnostics...). This would miss config-over-LSP but in every other respect seems much simpler than trying to find ways to extract this stuff live and get it in front of the user somehow.

66

so the member Condition in Fragment will be enhanced to something like std::vector<XXX> Conditions in the future?

Ah, not quite...
Fragment is supposed to closely follow the YAML format, which is:
If: { PathMatch: foo, Platform: bar }
not
If: [ {PathMatch: foo}, {Platform: bar} ]
That is, the partitioning of parts of the If block into Conditions is implicit rather than explicit. (It's basically partitioning on the map keys, but maybe there'll be exceptions).

The subtle difference between Fragment::Condition and CompiledFragmentImpl::Condition is confusing though. I think renaming the former to Fragment::If is much clearer (not sure why I didn't name it that in the first place).

Since Fragment and CompiledFragment is 1:1 mapping, I think it is important to keep their implementation aligned

I think the fragment and the fragmentcompiler are pretty well aligned, but I don't think duplicating the fragment structure in the final CompiledFragmentImpl is a good idea.

The reason is captured pretty well in the essay "Parse, don't validate". Through compilation here we want to examine the config and establish that we can apply this configuration efficiently, and without any more errors. The type-driven way to do that is to emit a function that applies the configuration and whose signature doesn't allow for errors.

https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/

clang-tools-extra/clangd/ConfigFragment.h

35

Agree, it seems backwards - this is the one thing that seemed cleaner about the old factoring.
But it does appear that everything is in the right file, the dependency is needed, and a (circular) dependency in the other direction won't be needed so... maybe we can live with it?

The way I think of it is that ConfigProvider is defining an entirely abstract interface that ConfigFragment is implementing.

[hokein]

Thanks, looks good.

clang-tools-extra/clangd/ConfigCompile.cpp

47

that sounds good.

66

oh, I think I understand how the design is driven like this now. that makes sense.

I think it would be helpful to add some documentation (and give a concrete config example, the example you gave is good enough) for CompiledFragmentImpl::Conditions.

clang-tools-extra/clangd/ConfigFragment.h

100

I think this comment is important, but it is a bit vague (not quite friendly to readers without much config context), we might want to refine it -- would be nice to clearly state what's a condition, e.g. "PathMatch" is a condition, "Platform" is a condition.

maybe separate the rename to a separate patch, but up to you.

108

nit: maybe explicitly spell this is an OR match.

[sammccall]

Thanks a lot for the review, I think this is a lot better now.
And particularly on where docs are lacking - I'm happy to write comments but it's hard for me to know which bits are least clear.

clang-tools-extra/clangd/ConfigFragment.h

100

Updated comment to tie the concept of conditions to the contents of the If block, and added an example for the OR matching.

108

I'd prefer not to do this explicitly in each condition, as it's hard to get across in a couple of words and I think it'd hurt readability overall to repeat it so much.

clang-tools-extra/clangd/CMakeLists.txt

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

clang-tools-extra/clangd/ConfigCompile.cpp

+//===--- ConfigCompile.cpp - Translating Fragments into Config ------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Fragments are applied to Configs in two steps:
+//
+// 1. (When the fragment is first loaded)
+//    FragmentCompiler::compile() traverses the Fragment and creates
+//    function objects that know how to apply the configuration.
+// 2. (Every time a config is required)
+//    CompiledFragment() executes these functions to populate the Config.
+//
+// Work could be split between these steps in different ways. We try to
+// do as much work as possible in the first step. For example, regexes are
+// compiled in stage 1 and captured by the apply function. This is because:
+//
+//  - it's more efficient, as the work done in stage 1 must only be done once
+//  - problems can be reported in stage 1, in stage 2 we must silently recover
+//
+//===----------------------------------------------------------------------===//
+
+#include "Config.h"
+#include "ConfigFragment.h"
+#include "support/Logger.h"
+#include "support/Trace.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Regex.h"
+#include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+
+struct CompiledFragmentImpl {
+  std::vector<llvm::unique_function<bool(const Params &) const>> Conditions;
+  std::vector<llvm::unique_function<void(Config &) const>> Apply;
+  bool operator()(const Params &P, Config &C) const {
+    for (const auto &C : Conditions) {
+      if (!C(P)) {
+        dlog("Config fragment {0}: condition not met", this);
+        return false;
+      }
+    }
+    dlog("Config fragment {0}: applying {1} rules", this, Apply.size());
+    for (const auto &A : Apply)
+      A(C);
+    return true;
+  }
+};
+
+// Wrapper around condition compile() functions to reduce arg-passing.
+struct FragmentCompiler {
+  CompiledFragmentImpl &Out;
+  DiagnosticCallback Diagnostic;
+  llvm::SourceMgr *SourceMgr;
+
+  llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
+    std::string Anchored = "^(" + *Text + ")$";
+    llvm::Regex Result(Anchored);
+    std::string RegexError;
+    if (!Result.isValid(RegexError)) {
+      diag(Error, "Invalid regex " + Anchored + ": " + RegexError, Text.Range);
+      return llvm::None;
+    }
+    return Result;
+  }
+
+  void compile(Fragment &&F) {
+    compile(std::move(F.If));
+    compile(std::move(F.CompileFlags));
+  }
+
+  void compile(Fragment::IfBlock &&F) {
+    if (F.HasUnrecognizedCondition)
+      Out.Conditions.push_back([&](const Params &) { return false; });
+
+    auto PathMatch = std::make_unique<std::vector<llvm::Regex>>();
+    for (auto &Entry : F.PathMatch) {
+      if (auto RE = compileRegex(Entry))
+        PathMatch->push_back(std::move(*RE));
+    }
+    if (!PathMatch->empty()) {
+      Out.Conditions.push_back(
+          [PathMatch(std::move(PathMatch))](const Params &P) {
+            if (P.Path.empty())
+              return false;
+            return llvm::any_of(*PathMatch, [&](const llvm::Regex &RE) {
+              return RE.match(P.Path);
+            });
+          });
+    }
+  }
+
+  void compile(Fragment::CompileFlagsBlock &&F) {
+    if (!F.Add.empty()) {
+      std::vector<std::string> Add;
+      for (auto &A : F.Add)
+        Add.push_back(std::move(*A));
+      Out.Apply.push_back([Add(std::move(Add))](Config &C) {
+        C.CompileFlags.Edits.push_back([Add](std::vector<std::string> &Args) {
+          Args.insert(Args.end(), Add.begin(), Add.end());
+        });
+      });
+    }
+  }
+
+  constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
+  void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
+            llvm::SMRange Range) {
+    if (Range.isValid() && SourceMgr != nullptr)
+      Diagnostic(SourceMgr->GetMessage(Range.Start, Kind, Message, Range));
+    else
+      Diagnostic(llvm::SMDiagnostic("", Kind, Message));
+  }
+};
+
+} // namespace
+
+CompiledFragment Fragment::compile(DiagnosticCallback D) && {
+  llvm::StringRef ConfigFile = "<unknown>";
+  std::pair<unsigned, unsigned> LineCol = {0, 0};
+  if (auto *SM = Source.Manager.get()) {
+    unsigned BufID = SM->getMainFileID();
+    LineCol = SM->getLineAndColumn(Source.Location, BufID);
+    ConfigFile = SM->getBufferInfo(BufID).Buffer->getBufferIdentifier();
+  }
+  trace::Span Tracer("ConfigCompile");
+  SPAN_ATTACH(Tracer, "ConfigFile", ConfigFile);
+  auto Result = std::make_shared<CompiledFragmentImpl>();
+  vlog("Config fragment: compiling {0}:{1} -> {2}", ConfigFile, LineCol.first,
+       Result.get());
+
+  FragmentCompiler{*Result, D, Source.Manager.get()}.compile(std::move(*this));
+  // Return as cheaply-copyable wrapper.
+  return [Result(std::move(Result))](const Params &P, Config &C) {
+    return (*Result)(P, C);
+  };
+}
+
+} // namespace config
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/ConfigFragment.h

 //  - make use of the option inside clangd
 //  - document the new option (config.md in the llvm/clangd-www repository)
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H
 
+#include "ConfigProvider.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>
 
template <typename T> struct Located {
   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);
 
+  /// Analyzes and consumes this fragment, possibly yielding more diagnostics.
+  /// This always produces a usable result (errors are recovered).
+  ///
+  /// Typically, providers will compile a Fragment once when it's first loaded,
+  /// caching the result for reuse.
+  /// Like a compiled program, this is good for performance and also encourages
+  /// errors to be reported early and only once.
+  ///
+  /// The returned function is a cheap-copyable wrapper of refcounted internals.
+  CompiledFragment compile(DiagnosticCallback) &&;
+
   /// These fields are not part of the user-specified configuration, but
   /// instead are populated by the parser to describe the configuration source.
   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 should be 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;
 
   /// Conditions restrict when a Fragment applies.
   ///
   /// Each separate condition must match (combined with AND).
   /// When one condition has multiple values, any may match (combined with OR).
   ///
   /// Conditions based on a file's path use the following form:
   /// - if the fragment came from a project directory, the path is relative
   /// - if the fragment is global (e.g. user config), the path is absolute
   /// - paths always use forward-slashes (UNIX-style)
   /// If no file is being processed, these conditions will not match.
-  struct ConditionBlock {
+  struct IfBlock {
     /// The file being processed must fully match a regular expression.
     std::vector<Located<std::string>> PathMatch;
     /// An unrecognized key was found while parsing the condition.
     /// The condition will evaluate to false.
     bool HasUnrecognizedCondition = false;
   };
-  ConditionBlock Condition;
+  IfBlock If;
 
   struct CompileFlagsBlock {
     std::vector<Located<std::string>> Add;
   } CompileFlags;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigProvider.h

+//===--- ConfigProvider.h - Loading of user 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 structures used for this, that produce a Config.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
+
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+struct Config;
+namespace config {
+
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+  /// Absolute path to a source file we're applying the config to. Unix slashes.
+  /// Empty if not configuring a particular file.
+  llvm::StringRef Path;
+};
+
+/// 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 that has been fully analyzed and is ready to apply.
+/// Typically this is obtained from a Fragment by calling Fragment::compile().
+///
+/// Calling it updates the configuration to reflect settings from the fragment.
+/// Returns true if the condition was met and the settings were used.
+using CompiledFragment = std::function<bool(const Params &, Config &)>;
+
+} // namespace config
+} // namespace clangd
+} // namespace clang
+
+#endif

clang-tools-extra/clangd/ConfigYAML.cpp

 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) { return parse(F.Condition, N); });
+    Dict.handle("If", [&](Node &N) { return parse(F.If, N); });
     Dict.handle("CompileFlags",
                 [&](Node &N) { return parse(F.CompileFlags, N); });
     return Dict.parse(N);
   }
 
 private:
-  bool parse(Fragment::ConditionBlock &F, Node &N) {
-    DictParser Dict("Condition", this);
+  bool parse(Fragment::IfBlock &F, Node &N) {
+    DictParser Dict("If", this);
     Dict.unrecognized(
         [&](llvm::StringRef) { F.HasUnrecognizedCondition = true; });
     Dict.handle("PathMatch", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.PathMatch = std::move(*Values);
       return !N.failed();
     });
     return Dict.parse(N);

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
+  ConfigCompileTests.cpp
   ConfigYAMLTests.cpp
   DexTests.cpp
   DiagnosticsTests.cpp
   DraftStoreTests.cpp
   ExpectedTypeTest.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp

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 "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::SizeIs;
+using ::testing::StartsWith;
+
+class ConfigCompileTests : public ::testing::Test {
+protected:
+  CapturedDiags Diags;
+  Config Conf;
+  Fragment Frag;
+  Params Parm;
+
+  bool compileAndApply() {
+    Conf = Config();
+    Diags.Diagnostics.clear();
+    auto Compiled = std::move(Frag).compile(Diags.callback());
+    return Compiled(Parm, Conf);
+  }
+};
+
+TEST_F(ConfigCompileTests, Condition) {
+  // No condition.
+  Frag = {};
+  Frag.CompileFlags.Add.emplace_back("X");
+  EXPECT_TRUE(compileAndApply()) << "Empty config";
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(1));
+
+  // Regex with no file.
+  Frag = {};
+  Frag.If.PathMatch.emplace_back("fo*");
+  EXPECT_FALSE(compileAndApply());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(0));
+
+  // Following tests have a file path set.
+  Parm.Path = "bar";
+
+  // Non-matching regex.
+  Frag = {};
+  Frag.If.PathMatch.emplace_back("fo*");
+  EXPECT_FALSE(compileAndApply());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+
+  // Matching regex.
+  Frag = {};
+  Frag.If.PathMatch.emplace_back("fo*");
+  Frag.If.PathMatch.emplace_back("ba*r");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+
+  // Invalid regex.
+  Frag = {};
+  Frag.If.PathMatch.emplace_back("**]@theu");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_THAT(Diags.Diagnostics, SizeIs(1));
+  EXPECT_THAT(Diags.Diagnostics.front().Message, StartsWith("Invalid regex"));
+
+  // Valid regex and unknown key.
+  Frag = {};
+  Frag.If.HasUnrecognizedCondition = true;
+  Frag.If.PathMatch.emplace_back("ba*r");
+  EXPECT_FALSE(compileAndApply());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+}
+
+TEST_F(ConfigCompileTests, CompileCommands) {
+  Frag.CompileFlags.Add.emplace_back("-foo");
+  std::vector<std::string> Argv = {"clang", "a.cc"};
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(1));
+  Conf.CompileFlags.Edits.front()(Argv);
+  EXPECT_THAT(Argv, ElementsAre("clang", "a.cc", "-foo"));
+}
+
+} // namespace
+} // namespace config
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/ConfigTesting.h

+//===-- ConfigTesting.h - Helpers for configuration tests -------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+
+#include "Protocol.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include <functional>
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Provides a DiagnosticsCallback that records diganostics.
+// Unlike just pushing them into a vector, underlying storage need not survive.
+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.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
+          << D.Message << "@" << llvm::to_string(D.Pos);
+    }
+  };
+  std::vector<Diag> Diagnostics;
+};
+
+MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
+MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
+MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
+MATCHER_P(DiagRange, R, "") { return arg.Range == R; }
+
+inline 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;
+}
+
+inline Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
+  return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
+}
+
+} // namespace config
+} // namespace clangd
+} // namespace clang
+
+#endif

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 "ConfigTesting.h"
 #include "Protocol.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/ScopedPrinter.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::AllOf;
 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.Rng.emplace();
-        Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-        Out.Rng->start.character = R.first;
-        Out.Rng->end.character = R.second;
-      }
-    };
-  }
-  struct Diag {
-    std::string Message;
-    llvm::SourceMgr::DiagKind Kind;
-    Position Pos;
-    llvm::Optional<Range> Rng;
-
-    friend void PrintTo(const Diag &D, std::ostream *OS) {
-      *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
-          << D.Message << "@" << llvm::to_string(D.Pos);
-    }
-  };
-  std::vector<Diag> Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
 TEST(ParseYAML, SyntacticForms) {
   CapturedDiags Diags;
   const char *YAML = R"yaml(
 If:
   PathMatch:
     - 'abc'
 CompileFlags: { Add: [foo, bar] }
 ---
 CompileFlags:
   Add: |
     b
     az
   )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.HasUnrecognizedCondition);
-  EXPECT_THAT(Results.front().Condition.PathMatch, ElementsAre(Val("abc")));
+  EXPECT_FALSE(Results.front().If.HasUnrecognizedCondition);
+  EXPECT_THAT(Results.front().If.PathMatch, ElementsAre(Val("abc")));
   EXPECT_THAT(Results.front().CompileFlags.Add,
               ElementsAre(Val("foo"), Val("bar")));
 
   EXPECT_THAT(Results.back().CompileFlags.Add, ElementsAre(Val("b\naz\n")));
 }
 
 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,
+  EXPECT_EQ(toRange(Results.front().If.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(AllOf(DiagMessage("Unknown Condition key UnknownCondition"),
+      ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"),
                         DiagKind(llvm::SourceMgr::DK_Warning),
                         DiagPos(YAML.range().start), DiagRange(YAML.range())),
                   AllOf(DiagMessage("Unexpected token. Expected Key, Flow "
                                     "Entry, or Flow Mapping End."),
                         DiagKind(llvm::SourceMgr::DK_Error),
                         DiagPos(YAML.point()), DiagRange(llvm::None))));
 
   ASSERT_EQ(Results.size(), 2u);
   EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
-  EXPECT_TRUE(Results.front().Condition.HasUnrecognizedCondition);
+  EXPECT_TRUE(Results.front().If.HasUnrecognizedCondition);
   EXPECT_THAT(Results.back().CompileFlags.Add, IsEmpty());
 }
 
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang