[clangd] Add clang-tidy options to config

[njames93]

Summary

First step of implementing clang-tidy configuration into clangd config.
This is just adding support for reading and verifying the clang tidy options from the config fragments.
No support is added for actually using the options within clang-tidy yet.

That will be added in a follow up as its a little more involved.

[njames93]

clang-tools-extra/clangd/ConfigCompile.cpp

273–274

Is this the direction we want to go, the first draft paper on this seems to suggest so.

clang-tools-extra/clangd/ConfigTidyProvider.cpp

38–41 ↗

(On Diff #302090)

This definitely isn't right, but I'm unsure of the correct way to get the config for the specified file.
Is it better to not have this class share the interface of ClangTidyOptionsProvider and instead have it take an interface that takes a Config and a Filename and works from there instead?

[njames93]

Added unittests for reading and compiling config and fixed the bugs causing said unittests to fail.

[njames93]

Rename Tidy to ClangTidy in Fragment
Extract CheckOptions logic into a new class DynamicDictParser, this should help if config is extended to load other dictionaries where keys are unknown strings.e

[njames93]

Removed support for using clang-tidy that will happen in a follow up as its a more involved change.

[njames93]

Rebase and make apply take reference to Params.
Fix fragments checks being applied on top of each other instead of overwriting the current config checks.

[sammccall]

Thank you!

This design looks really good, just have some nits on comments, possible simplifications etc.

(Sorry about the delay getting to these, on it now!)

clang-tools-extra/clangd/ConfigCompile.cpp

268

nit: method name is confusing because it uses "check" as a noun where you'd expect it to be a verb (this method validates Adjuster)

Maybe appendTidyCheckSpec?

272

nit: I think this code is going to too much trouble to produce a precise error message

• not sure "Add check item can't start with '-'" is clearer than without the word "add". Errors show the error location/range.
• "check name" would be clearer than "check item" (even if wildcards are allowed, IMO)
• it feels a bit weird to be really specific about - and , - these are references to a fixed list of checks, so all that really matters is that it's not the name of a check. I'd consider just returning fixed "Invalid clang-tidy check name" for both cases

273–274

Yeah it looks like a good solution to me. It's not as powerful as an ordered list of globs, but it's simple and consistent with the other settings we have. .clang-tidy is still the right thing for full control.

275

Str.startswith("-") - we haven't checked that it's empty (and shouldn't bother IMO)

301

nit: I'd suggest *including" the leading comma in these
e.g. Checks is ",foo,bar". (Maybe call it ChecksSuffix)

Then when applying,
C.ClangTidy.Checks.append(Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/1 : 0);

advantages:

• marginally simplifies checkAdjuster (no need to conditionally add the comma)
• the Apply step (which is somewhat performance-sensitive) is at most a single allocation (which is awkward to achieve otherwise)

clang-tools-extra/clangd/ConfigFragment.h

178

nit: drop "this section" and add trailing period, consistent with other sections.

I think "code" would be clearer than codebase.

183

I wonder if it's worth having this when we support Remove: *.

technically this does something slightly different:

• it still runs the machinery just with no actual checks
• you can enable: true later to override it, without losing the previously configured list of checks

Is one of these important? Is there some other benefit?
(I'm not opposed to having this if you want it, but I'm curious)

184

nit: we should document each separately.

e.g. "/// List of checks to enable - can be wildcards like "readability-*".

185

nit: "get ran" is confusing - this doesn't affect the order in which checks *run*, just which overrrides which.
And this text/example could probably be a little more compact.

I'd suggest:

List of checks to disable.
Takes precedence over Add. To enable all llvm checks except include order:
  Add: llvm-*
  Remove: llvm-include-onder

188

should this be llvm-*?

clang-tools-extra/clangd/ConfigYAML.cpp

188

instead of adding a second class for this, can we reuse DictParser, and change unrecognized() so:

• the callback gets access to the key location and the value
• the callback is responsible for emitting the "unknown key" error if it wants one

i.e. the default unrecognized handler is:

[this](Located<std::string> Key, Node &Value) { Outer->warning("Unknown " + Description + " key " + *Key); }

and we replace it for parsing CheckOptions

235

can we implement this on top of scalarValue? seems like it would avoid a bunch of repetition and the efficiency doesn't seem that important

239

not sure why we're trimming here

246

Technically we're probably supposed to accept
y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF
(https://yaml.org/type/bool.html)

I'd be happy accepting y/n/yes/no/true/false/on/off case-insensitively though :-)

251

conversely, not sure why we'd accept a number here

[njames93]

clang-tools-extra/clangd/ConfigFragment.h

183

I'm not 100% sure what you are asking here.

clang-tools-extra/clangd/ConfigYAML.cpp

188

Not sure I'm a huge fan of that approach. I can't imagine a use case where the 2 dictionary modes will be used at the same time so there isn't a need for it to support both.
By that I mean you wont have a dictionary where its expected to have both known and unknown keys.

DictParser could well be implemented as a a specialisation of DynamicDictParser but not much would be gained from that.

[njames93]

Address comments

[sammccall]

Thanks! Just some simplifications and doc nits left, then please go ahead and land

clang-tools-extra/clangd/Config.h

74

nit: we're using triple slash comments for these...

77

Enable is trivial enough to go without documentation, but the format of Checks certainly needs to be documented.

78

I think this should be a StringMap<string>

It makes sense to use a vector-of-pairs in ConfigFragment to preserve the Located information for keys, but we don't need to do that in Config.

clang-tools-extra/clangd/ConfigFragment.h

183

I'm asking whether we really need Enable, or whether we should remove it and recommend Remove: * to disable the checks.

If there isn't a clear reason we need it, my preference is to omit it for simplicity (we can add more setting later, but it's harder to remove them).

I don't feel strongly though, this is up to you.

clang-tools-extra/clangd/ConfigYAML.cpp

188

Yes, they're being used for different purposes, so using the same class isn't the most expressive.
But this isn't a public API, it's a local helper. The implementation is nontrivial and almost identical. This is just about reducing the implementation complexity.

235

You've extracted a common getScalar function here instead, I think to avoid scalarValue copying the string?
But this isn't a hot path, and all the legal values for this string are in SSO range anyway - can we avoid this extra complexity?

[njames93]

clang-tools-extra/clangd/Config.h

78

If we do use a StringMap<string>, whats the policy when multiple fragments specify the same option.
Currently at the Fragment level there is logic that ignores duplicated keys, but when compiling the fragments, should we give priority to keys declared in earlier or later fragments. In any case would it also be wise to warn about that.

clang-tools-extra/clangd/ConfigFragment.h

183

If we use Remove: * when it comes to actually implementing this, it would be wise to disable running clang-tidy if there are no checks enabled. But yes, I do agree that it could be removed at first, then added at a later date if needs must.

clang-tools-extra/clangd/ConfigYAML.cpp

188

Ok I'll update it.

235

It's not about that, I'd prefer the warning message to explicitly say it needs a boolean even if they pass something that's not a scalar. Reusing scalarValue will give a warning saying SomeKey should be scalar. What we actually want is a warning saying SomeKey should be a boolean

[sammccall]

clang-tools-extra/clangd/Config.h

78

We merge with the later fragments taking priority. So in this case each fragment writes its keys into the single stringmap, replacing whatever is there.

(Most common case of later fragments is subdirectory overriding parent, or user config overriding in-tree).

I don't think we need to warn for this - does clang-tidy warn if there are multiple options sources configured and one overrides a setting from another? This is the same thing.

clang-tools-extra/clangd/ConfigFragment.h

183

This sounds like a good idea to me. This would happen in ParsedAST::build.
If the loop through CTChecks never actually manages to call registerPPCallbacks/Matchers, then we can skip the matchAST call.

I'll send a patch.

[njames93]

Removed Enable.
Removed the scalarBool parser as it was only needed by Enable.
Removed DynamicDictParser in favour of reworking DictParser to get better control of how to handle unknown keys.
Use a StringMap to store the CheckOptions in the Config.

[njames93]

Fix the unit tests.

clang-tools-extra/clangd/Config.h

struct Config {
 
   /// 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;
   } Style;
+
+  // Configures what clang-tidy checks to run and options to use with them.
+  struct {
+    bool Enable;
+    std::string Checks;
+    std::vector<std::pair<std::string, std::string>> CheckOptions;
+  } ClangTidy;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigCompile.cpp

void compile(Fragment &&F) {
     if (!F.Source.Directory.empty()) {
       FragmentDirectory = llvm::sys::path::convert_to_slash(F.Source.Directory);
       if (FragmentDirectory.back() != '/')
         FragmentDirectory += '/';
     }
     compile(std::move(F.If));
     compile(std::move(F.CompileFlags));
     compile(std::move(F.Index));
+    compile(std::move(F.ClangTidy));
   }
 
   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 (!F.FullyQualifiedNamespaces.empty()) {
                               const Params &, Config &C) {
         C.Style.FullyQualifiedNamespaces.insert(
             C.Style.FullyQualifiedNamespaces.begin(),
             FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
       });
     }
   }
 
+  void checkAdjuster(std::string &Adjuster, const Located<std::string> &Arg,
+                     bool IsPositive) {
+    SmallString<64> DiagBuffer;
+    StringRef Str = *Arg;
+    StringRef Type(IsPositive ? "Add" : "Remove");
+    // Don't support negating here, its handled if the item is in the Add or
+    // Remove list.
+    if (Str[0] == '-') {
+      diag(Error,
+           (Type + " check item can't start with '-'").toStringRef(DiagBuffer),
+           Arg.Range);
+      return;
+    }
+    if (Str.contains(',')) {
+      diag(Error,
+           ("Invalid character ',' found in " + Type + " check item")
+               .toStringRef(DiagBuffer),
+           Arg.Range);
+      return;
+    }
+    if (!Adjuster.empty())
+      Adjuster += ',';
+    if (!IsPositive)
+      Adjuster += '-';
+    Adjuster += Str;
+  }
+
+  void compile(Fragment::ClangTidyBlock &&F) {
+    if (F.Enable)
+      Out.Apply.push_back([Enable = **F.Enable](const Params &, Config &C) {
+        C.ClangTidy.Enable = Enable;
+      });
+
+    std::string Checks;
+    for (auto &CheckGlob : F.Add)
+      checkAdjuster(Checks, CheckGlob, true);
+
+    for (auto &CheckGlob : F.Remove)
+      checkAdjuster(Checks, CheckGlob, false);
+
+    if (!Checks.empty())
+      Out.Apply.push_back(
+          [Checks = std::move(Checks)](const Params &, Config &C) {
+            if (C.ClangTidy.Checks.empty())
+              C.ClangTidy.Checks = Checks;
+            else
+              C.ClangTidy.Checks += ',' + Checks;
+          });
+    if (!F.CheckOptions.empty()) {
+      std::vector<std::pair<std::string, std::string>> CheckOptions;
+      for (auto &Opt : F.CheckOptions)
+        CheckOptions.emplace_back(std::move(*Opt.first),
+                                  std::move(*Opt.second));
+      Out.Apply.push_back(
+          [CheckOptions = std::move(CheckOptions)](const Params &, Config &C) {
+            C.ClangTidy.CheckOptions.insert(C.ClangTidy.CheckOptions.end(),
+                                            CheckOptions.begin(),
+                                            CheckOptions.end());
+          });
+    }
+  }
+
   constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
   constexpr static llvm::SourceMgr::DiagKind Warning =
       llvm::SourceMgr::DK_Warning;
   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

clang-tools-extra/clangd/ConfigFragment.h

struct Fragment {
   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.
     // Affects availability of the AddUsing tweak.
     std::vector<Located<std::string>> FullyQualifiedNamespaces;
   };
   StyleBlock Style;
+
+  /// This section controls how clang-tidy will run over the code base
+  ///
+  /// The settings are merged with any settings found in .clang-tidy
+  /// configiration files with these ones taking precedence.
+  struct ClangTidyBlock {
+    llvm::Optional<Located<bool>> Enable;
+    /// List of checks to enable or disable, can use wildcards.
+    /// \ref Add checks get ran first, then \ref Remove checks. This lets you
+    /// enable all checks from a module apart from a few specific checks,
+    /// Example:
+    ///   Add: llvm-
+    ///   Remove: llvm-include-order
+    /// This will enable all checks from the llvm module apart from
+    /// llvm-include-order.
+    std::vector<Located<std::string>> Add;
+    std::vector<Located<std::string>> Remove;
+
+    /// A Key-Value pair list of options to pass to clang-tidy checks
+    /// These take precedence over options specified in clang-tidy configuration
+    /// files. Example:
+    ///   CheckOptions:
+    ///     readability-braces-around-statements.ShortStatementLines: 2
+    std::vector<std::pair<Located<std::string>, Located<std::string>>>
+        CheckOptions;
+  };
+  ClangTidyBlock ClangTidy;
 };
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigYAML.cpp

public:
   // Tries to parse N into F, returning false if it failed and we couldn't
   // meaningfully recover (YAML syntax error, or hard semantic error).
   bool parse(Fragment &F, Node &N) {
     DictParser Dict("Config", this);
     Dict.handle("If", [&](Node &N) { parse(F.If, N); });
     Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); });
     Dict.handle("Index", [&](Node &N) { parse(F.Index, N); });
     Dict.handle("Style", [&](Node &N) { parse(F.Style, N); });
+    Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
     Dict.parse(N);
     return !(N.failed() || HadError);
   }
 
 private:
   void parse(Fragment::IfBlock &F, Node &N) {
     DictParser Dict("If", this);
     Dict.unrecognized(
void parse(Fragment::StyleBlock &F, Node &N) {
     DictParser Dict("Style", this);
     Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.FullyQualifiedNamespaces = std::move(*Values);
     });
     Dict.parse(N);
   }
 
+  void parse(Fragment::ClangTidyBlock &F, Node &N) {
+    DictParser Dict("ClangTidy", this);
+    Dict.handle("Enable", [&](Node &N) {
+      if (auto Value = scalarBool(N, "Enable"))
+        F.Enable = *Value;
+    });
+    Dict.handle("Add", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.Add = std::move(*Values);
+    });
+    Dict.handle("Remove", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.Remove = std::move(*Values);
+    });
+    Dict.handle("CheckOptions", [&](Node &N) {
+      DynamicDictParser DynamicDict(
+          "CheckOptions",
+          [&](Located<std::string> &&Key, Node &Val) {
+            if (auto Value = scalarValue(Val, *Key))
+              F.CheckOptions.emplace_back(std::move(Key), std::move(*Value));
+          },
+          this);
+      DynamicDict.parse(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.parse(N);
   }
 
   // Helper for parsing mapping nodes (dictionaries).
void parse(Node &N) const {
           Outer->warning("Unknown " + Description + " key " + **Key, *K);
           if (Unknown)
             Unknown(**Key);
         }
       }
     }
   };
 
+  class DynamicDictParser {
+  public:
+    using KeyValueHandler =
+        std::function<void(Located<std::string> &&, Node &)>;
+
+  private:
+    llvm::StringRef Description;
+    KeyValueHandler Handler;
+    Parser *Outer;
+
+  public:
+    DynamicDictParser(llvm::StringRef Description, KeyValueHandler Handler,
+                      Parser *Parent)
+        : Description(Description), Handler(Handler), Outer(Parent) {}
+
+    void parse(Node &N) {
+      if (N.getType() != Node::NK_Mapping) {
+        Outer->error(Description + " should be a dictionary", N);
+        return;
+      }
+      llvm::SmallString<64> MsgDesc({Description, " key"});
+      llvm::SmallSet<std::string, 8> Seen;
+
+      for (auto &KV : llvm::cast<MappingNode>(N)) {
+        auto *K = KV.getKey();
+        if (!K) // YAMLParser emitted an error.
+          continue;
+        auto Key = Outer->scalarValue(*K, MsgDesc);
+        if (!Key)
+          continue;
+        if (!Seen.insert(**Key).second) {
+          Outer->warning("Duplicate " + Description + " key '" + **Key +
+                             "' is ignored",
+                         *K);
+          if (auto *V = KV.getValue())
+            V->skip();
+          continue;
+        }
+        auto *V = KV.getValue();
+        if (!V) // YAMLParser emitted an error.
+          continue;
+        Handler(std::move(*Key), *V);
+      }
+    }
+  };
+
+  // Try to parse a single boolean value from the node, warn on failure.
+  llvm::Optional<Located<bool>> scalarBool(Node &N, llvm::StringRef Desc) {
+    llvm::SmallString<256> Buf;
+    llvm::StringRef Str;
+    if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
+      Str = S->getValue(Buf).trim();
+    else if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
+      Str = BS->getValue().trim();
+    else {
+      warning(Desc + " should be a boolean", N);
+      return llvm::None;
+    }
+    if (Str.equals_lower("true"))
+      return Located<bool>(true, N.getSourceRange());
+    if (Str.equals_lower("false"))
+      return Located<bool>(false, N.getSourceRange());
+    bool Result;
+    if (!Str.getAsInteger(10, Result))
+      return Located<bool>(Result, N.getSourceRange());
+    warning("Couldn't parse '" + Str + "' as a boolean for " + Desc, N);
+    return llvm::None;
+  }
+
   // Try to parse a single scalar value from the node, warn on failure.
   llvm::Optional<Located<std::string>> scalarValue(Node &N,
                                                    llvm::StringRef Desc) {
     llvm::SmallString<256> Buf;
     if (auto *S = llvm::dyn_cast<ScalarNode>(&N))
       return Located<std::string>(S->getValue(Buf).str(), N.getSourceRange());
     if (auto *BS = llvm::dyn_cast<BlockScalarNode>(&N))
       return Located<std::string>(BS->getValue().str(), N.getSourceRange());

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

for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.PathSpec);
     Frag = {};
     Frag.If.PathExclude.emplace_back(Case.PathSpec);
     Frag.Source.Directory = Case.Directory;
     EXPECT_NE(compileAndApply(), Case.ShouldMatch);
     ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   }
 }
+
+TEST_F(ConfigCompileTests, Tidy) {
+  Frag.ClangTidy.Add.emplace_back("bugprone-use-after-move");
+  Frag.ClangTidy.Add.emplace_back("llvm-*");
+  Frag.ClangTidy.Remove.emplace_back("llvm-include-order");
+  Frag.ClangTidy.Remove.emplace_back("readability-*");
+  Frag.ClangTidy.CheckOptions.emplace_back(
+      std::make_pair(std::string("StrictMode"), std::string("true")));
+  Frag.ClangTidy.CheckOptions.emplace_back(std::make_pair(
+      std::string("example-check.ExampleOption"), std::string("0")));
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(
+      Conf.ClangTidy.Checks,
+      "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
+  EXPECT_THAT(Conf.ClangTidy.CheckOptions,
+              ElementsAre(std::make_pair("StrictMode", "true"),
+                          std::make_pair("example-check.ExampleOption", "0")));
+}
+
 } // namespace
 } // namespace config
 } // namespace clangd
 } // namespace clang

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

 
 MATCHER_P(Val, Value, "") {
   if (*arg == Value)
     return true;
   *result_listener << "value is " << *arg;
   return false;
 }
 
+MATCHER_P2(PairVal, Value1, Value2, "") {
+  if (*arg.first == Value1 && *arg.second == Value2)
+    return true;
+  *result_listener << "values are [" << *arg.first << ", " << *arg.second
+                   << "]";
+  return false;
+}
+
 TEST(ParseYAML, SyntacticForms) {
   CapturedDiags Diags;
   const char *YAML = R"yaml(
 If:
   PathMatch:
     - 'abc'
 CompileFlags: { Add: [foo, bar] }
 ---
 CompileFlags:
   Add: |
     b
     az
 ---
 Index:
   Background: Skip
+---
+ClangTidy: { Enable: true }
+---
+ClangTidy: 
+  Enable: 0
+  CheckOptions: 
+    IgnoreMacros: true
+    example-check.ExampleOption: 0
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
-  ASSERT_EQ(Results.size(), 3u);
+  ASSERT_EQ(Results.size(), 5u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
   EXPECT_THAT(Results[0].CompileFlags.Add, ElementsAre(Val("foo"), Val("bar")));
 
   EXPECT_THAT(Results[1].CompileFlags.Add, ElementsAre(Val("b\naz\n")));
 
   ASSERT_TRUE(Results[2].Index.Background);
   EXPECT_EQ("Skip", *Results[2].Index.Background.getValue());
+  ASSERT_TRUE(Results[3].ClangTidy.Enable);
+  EXPECT_TRUE(**Results[3].ClangTidy.Enable);
+  ASSERT_TRUE(Results[4].ClangTidy.Enable);
+  EXPECT_FALSE(**Results[4].ClangTidy.Enable);
+  EXPECT_THAT(Results[4].ClangTidy.CheckOptions,
+              ElementsAre(PairVal("IgnoreMacros", "true"),
+                          PairVal("example-check.ExampleOption", "0")));
 }
 
 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().If.PathMatch.front().Range,
                     *Results.front().Source.Manager),
             YAML.range());
 }
 
 TEST(ParseYAML, Diagnostics) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
 If:
-  [[UnknownCondition]]: "foo"
+  $unknown[[UnknownCondition]]: "foo"
 CompileFlags:
   Add: 'first'
 ---
-CompileFlags: {^
+ClangTidy:
+  Enable: $notBool[[NotABool]]
+---
+CompileFlags: {$unexpected^
 )yaml");
   auto Results =
       Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
 
   ASSERT_THAT(
       Diags.Diagnostics,
-      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))));
+      ElementsAre(
+          AllOf(DiagMessage("Unknown If key UnknownCondition"),
+                DiagKind(llvm::SourceMgr::DK_Warning),
+                DiagPos(YAML.range("unknown").start),
+                DiagRange(YAML.range("unknown"))),
+          AllOf(
+              DiagMessage("Couldn't parse 'NotABool' as a boolean for Enable"),
+              DiagPos(YAML.range("notBool").start),
+              DiagRange(YAML.range("notBool"))),
+                DiagPos(YAML.point("unexpected")), DiagRange(llvm::None))));
 
-  ASSERT_EQ(Results.size(), 1u); // invalid fragment discarded.
+  ASSERT_EQ(Results.size(), 2u); // invalid fragment discarded.
   EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
   EXPECT_TRUE(Results.front().If.HasUnrecognizedCondition);
 }
 
 TEST(ParseYAML, Invalid) {
   CapturedDiags Diags;
   const char *YAML = R"yaml(
 If: