[clang-tidy] Improved --verify-config when using literal style in config file (#85591)

felix642 · web-flow · commit f94ed6f79773 · 2024-04-22T20:05:32.000+02:00
Specifying checks using the literal style (|) in the clang-tidy config file is currently supported but was not implemented for the --verify-config options. This means that clang-tidy would work properly but, using the --verify-config option would raise an error due to some checks not being parsed properly. Fixes #53737
diff --git a/clang-tools-extra/clang-tidy/GlobList.cpp b/clang-tools-extra/clang-tidy/GlobList.cpp
@@ -19,12 +19,17 @@ static bool consumeNegativeIndicator(StringRef &GlobList) {
   return GlobList.consume_front("-");
 }
 
-// Converts first glob from the comma-separated list of globs to Regex and
-// removes it and the trailing comma from the GlobList.
-static llvm::Regex consumeGlob(StringRef &GlobList) {
+// Extracts the first glob from the comma-separated list of globs,
+// removes it and the trailing comma from the GlobList and
+// returns the extracted glob.
+static llvm::StringRef extractNextGlob(StringRef &GlobList) {
   StringRef UntrimmedGlob = GlobList.substr(0, GlobList.find_first_of(",\n"));
   StringRef Glob = UntrimmedGlob.trim();
   GlobList = GlobList.substr(UntrimmedGlob.size() + 1);
+  return Glob;
+}
+
+static llvm::Regex createRegexFromGlob(StringRef &Glob) {
   SmallString<128> RegexText("^");
   StringRef MetaChars("()^$|*+?.[]\\{}");
   for (char C : Glob) {
@@ -43,7 +48,8 @@ GlobList::GlobList(StringRef Globs, bool KeepNegativeGlobs /* =true */) {
   do {
     GlobListItem Item;
     Item.IsPositive = !consumeNegativeIndicator(Globs);
-    Item.Regex = consumeGlob(Globs);
+    Item.Text = extractNextGlob(Globs);
+    Item.Regex = createRegexFromGlob(Item.Text);
     if (Item.IsPositive || KeepNegativeGlobs)
       Items.push_back(std::move(Item));
   } while (!Globs.empty());
diff --git a/clang-tools-extra/clang-tidy/GlobList.h b/clang-tools-extra/clang-tidy/GlobList.h
@@ -44,8 +44,12 @@ class GlobList {
   struct GlobListItem {
     bool IsPositive;
     llvm::Regex Regex;
+    llvm::StringRef Text;
   };
   SmallVector<GlobListItem, 0> Items;
+
+public:
+  const SmallVectorImpl<GlobListItem> &getItems() const { return Items; };
 };
 
 /// A \p GlobList that caches search results, so that search is performed only
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -454,52 +454,27 @@ static constexpr StringLiteral VerifyConfigWarningEnd = " [-verify-config]\n";
 
 static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
                          StringRef Source) {
-  llvm::StringRef Cur, Rest;
+  GlobList Globs(CheckGlob);
   bool AnyInvalid = false;
-  for (std::tie(Cur, Rest) = CheckGlob.split(',');
-       !(Cur.empty() && Rest.empty()); std::tie(Cur, Rest) = Rest.split(',')) {
-    Cur = Cur.trim();
-    if (Cur.empty())
+  for (const auto &Item : Globs.getItems()) {
+    if (Item.Text.starts_with("clang-diagnostic"))
       continue;
-    Cur.consume_front("-");
-    if (Cur.starts_with("clang-diagnostic"))
-      continue;
-    if (Cur.contains('*')) {
-      SmallString<128> RegexText("^");
-      StringRef MetaChars("()^$|*+?.[]\\{}");
-      for (char C : Cur) {
-        if (C == '*')
-          RegexText.push_back('.');
-        else if (MetaChars.contains(C))
-          RegexText.push_back('\\');
-        RegexText.push_back(C);
-      }
-      RegexText.push_back('$');
-      llvm::Regex Glob(RegexText);
-      std::string Error;
-      if (!Glob.isValid(Error)) {
-        AnyInvalid = true;
-        llvm::WithColor::error(llvm::errs(), Source)
-            << "building check glob '" << Cur << "' " << Error << "'\n";
-        continue;
-      }
-      if (llvm::none_of(AllChecks.keys(),
-                        [&Glob](StringRef S) { return Glob.match(S); })) {
-        AnyInvalid = true;
+    if (llvm::none_of(AllChecks.keys(),
+                      [&Item](StringRef S) { return Item.Regex.match(S); })) {
+      AnyInvalid = true;
+      if (Item.Text.contains('*'))
         llvm::WithColor::warning(llvm::errs(), Source)
-            << "check glob '" << Cur << "' doesn't match any known check"
+            << "check glob '" << Item.Text << "' doesn't match any known check"
             << VerifyConfigWarningEnd;
+      else {
+        llvm::raw_ostream &Output =
+            llvm::WithColor::warning(llvm::errs(), Source)
+            << "unknown check '" << Item.Text << '\'';
+        llvm::StringRef Closest = closest(Item.Text, AllChecks);
+        if (!Closest.empty())
+          Output << "; did you mean '" << Closest << '\'';
+        Output << VerifyConfigWarningEnd;
       }
-    } else {
-      if (AllChecks.contains(Cur))
-        continue;
-      AnyInvalid = true;
-      llvm::raw_ostream &Output = llvm::WithColor::warning(llvm::errs(), Source)
-                                  << "unknown check '" << Cur << '\'';
-      llvm::StringRef Closest = closest(Cur, AllChecks);
-      if (!Closest.empty())
-        Output << "; did you mean '" << Closest << '\'';
-      Output << VerifyConfigWarningEnd;
     }
   }
   return AnyInvalid;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -103,6 +103,9 @@ Improvements to clang-tidy
 - Improved :program:`check_clang_tidy.py` script. Added argument `-export-fixes`
   to aid in clang-tidy and test development.
 
+- Fixed ``--verify-config`` option not properly parsing checks when using the 
+  literal operator in the ``.clang-tidy`` config.
+
 New checks
 ^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/verify-config.cpp
@@ -18,3 +18,15 @@
 // CHECK-VERIFY: command-line option '-checks': warning: check glob 'bad*glob' doesn't match any known check [-verify-config]
 // CHECK-VERIFY: command-line option '-checks': warning: unknown check 'llvm-includeorder'; did you mean 'llvm-include-order' [-verify-config]
 // CHECK-VERIFY: command-line option '-checks': warning: unknown check 'my-made-up-check' [-verify-config]
+
+// RUN: echo -e 'Checks: |\n bugprone-argument-comment\n bugprone-assert-side-effect,\n bugprone-bool-pointer-implicit-conversion\n readability-use-anyof*' > %T/MyClangTidyConfig
+// RUN: clang-tidy -verify-config \
+// RUN: --config-file=%T/MyClangTidyConfig | FileCheck %s -check-prefix=CHECK-VERIFY-BLOCK-OK
+// CHECK-VERIFY-BLOCK-OK: No config errors detected.
+
+// RUN: echo -e 'Checks: |\n bugprone-arguments-*\n bugprone-assert-side-effects\n bugprone-bool-pointer-implicit-conversion' > %T/MyClangTidyConfigBad
+// RUN: not clang-tidy -verify-config \
+// RUN: --config-file=%T/MyClangTidyConfigBad 2>&1 | FileCheck %s -check-prefix=CHECK-VERIFY-BLOCK-BAD
+// CHECK-VERIFY-BLOCK-BAD: command-line option '-config': warning: check glob 'bugprone-arguments-*' doesn't match any known check [-verify-config]
+// CHECK-VERIFY-BLOCK-BAD: command-line option '-config': warning: unknown check 'bugprone-assert-side-effects'; did you mean 'bugprone-assert-side-effect' [-verify-config]
+
