[clang-tidy] Support specifying checks as a list in the config file

[carlosgalvezp]

Summary

Specifying checks as a string is convenient for quickly using
clang-tidy to run a handful of checks. However it is not
suitable for projects that have a long list of enabled or
disabled checks. It is specially troublesome in case one
wants to interleave comments with the checks, to explain
why they are enabled or disabled.

Currently this can be achieved via multiline strings in YAML,
but it's error-prone. For example, comments must end with a
comma for clang-tidy to continue processing the list of globs;
a missing comma will make clang-tidy silently ignore the rest
of the list.

Instead, enable passing a native YAML list to the "Checks"
option in the config file. The implementation is done such
that the old behavior is kept: a user can pass a string
or a list. We can consider deprecating passing the checks
as a string altogether in a future release, to simplify
the internal logic of the YAML parser.

Fixes https://github.com/llvm/llvm-project/issues/51428

[carlosgalvezp]

Add tests.

[carlosgalvezp]

Update commit message

[carlosgalvezp]

• Simplify code with llvm:join.
• Add documentation.

[carlosgalvezp]

Update commit message

Updating D147876: [clang-tidy] Support introducing checks as a list in the config file

Specifying checks as a string is convenient for quickly using
clang-tidy to run a handful of checks. However it is not
suitable for projects that have a long list of enabled or...

[carlosgalvezp]

Fix commit message title

Updating D147876: [clang-tidy] Support introducing checks as a list in the config file

Specifying checks as a string is convenient for quickly using
clang-tidy to run a handful of checks. However it is not
suitable for projects that have a long list of enabled...

[Eugene.Zelenko]

Looks OK for me but will be good idea to wait for other reviewers.

[njames93]

From an architectural point of view, is there any reason we don't change the backend to treat checks internally as a vector?

clang-tools-extra/docs/clang-tidy/index.rst also needs updating to specify this new behaviour.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

73–74

Any reason for this line changing, this formatting change looks unrelated.

129–141

All this code can just be inlined into the function below and this function can just be removed

132–133

clang-tools-extra/docs/ReleaseNotes.rst

304 ↗

(On Diff #512038)

This change is unrelated and should be pushed as an NFC commit

[carlosgalvezp]

In D147876#4254451, @njames93 wrote:

From an architectural point of view, is there any reason we don't change the backend to treat checks internally as a vector?

clang-tools-extra/docs/clang-tidy/index.rst also needs updating to specify this new behaviour.

I believe that makes sense, just wanted to keep changes as minimal as possible! Our reliance on Checks being a string is spread into quite a few files, but I'm happy to change to a vector on a separate patch. I posted an RFC on discourse with a couple more follow-up questions, would be great if you could take a look:
https://discourse.llvm.org/t/rfc-support-specifying-checks-as-a-list-in-the-config-file/69856/1

Do note that the index.rst file is indeed updated with the new behavior.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

73–74

clang-format applied the change. Can fix in a separate patch.

132–133

Neat, thank you!

[carlosgalvezp]

Revert unrelated changes

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

129–141

Can you elaborate on how to do it? I cannot call mapOptional("Checks" twice, one for string and one for vector, since it will print an error message on either case.

The function yamlize does not exp

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

129–141

... The function yamlize does not allow one to specify the Checks key as far as I can tell either.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

129–141

I think you're right, I was getting confused with a different serialisation library that does support that kind of logic

[carlosgalvezp]

Thanks for the review @njames93 ! Do you think it makes sense that we deprecate the string format, so that we only support the list format? To be fully removed in clang-tidy 19. Only for config file, for --checks we can still support string. This would allow us to get rid of all the code I wrote and simply pass the vector to IO.mapOptional :)

[njames93]

In D147876#4256155, @carlosgalvezp wrote:

Thanks for the review @njames93 ! Do you think it makes sense that we deprecate the string format, so that we only support the list format? To be fully removed in clang-tidy 19. Only for config file, for --checks we can still support string. This would allow us to get rid of all the code I wrote and simply pass the vector to IO.mapOptional :)

Deprecating options from the config is a right pain to do and discourages upgrading clang-tidy version with checked in configuration files

[carlosgalvezp]

In D147876#4256182, @njames93 wrote:

In D147876#4256155, @carlosgalvezp wrote:

Thanks for the review @njames93 ! Do you think it makes sense that we deprecate the string format, so that we only support the list format? To be fully removed in clang-tidy 19. Only for config file, for --checks we can still support string. This would allow us to get rid of all the code I wrote and simply pass the vector to IO.mapOptional :)

Deprecating options from the config is a right pain to do and discourages upgrading clang-tidy version with checked in configuration files

Just because it's a pain it shouldn't stop us from working on it if we believe it's the right direction and will make the tool easier to use and maintain - I'm happy to do it :)

with checked in configuration files

I'm not sure I understand what you mean by "checked in", could you clarify?

If it's a pain for people to update their .clang-tidy files we could potentially create some Python script that does the change for them?

[njames93]

In D147876#4257197, @carlosgalvezp wrote:

In D147876#4256182, @njames93 wrote:

with checked in configuration files

I'm not sure I understand what you mean by "checked in", could you clarify?

When the .clang-tidy file is checked into the source control(git). It means that anyone who contributes to the project will need to ensure that they have a version of clang-tidy that will be able to read the file.
This can cause problems as binaries of clang-tidy aren't always provided for specific versions on certain platforms.

[carlosgalvezp]

When the .clang-tidy file is checked into the source control(git). It means that anyone who contributes to the project will need to ensure that they have a version of clang-tidy that will be able to read the file.
This can cause problems as binaries of clang-tidy aren't always provided for specific versions on certain platforms.

Isn't that expected though? All projects have expectations on the required dependencies to build their code. For example LLVM requires a compiler with C++17 support and a modern CMake that needs to be downloaded from the CMake website on older Ubuntu versions. Granted, those are backwards-compatible constraints, but nevertheless require that the user sets them up. It doesn't come out of the box in a regular distribution. Same applies to other C++ libraries, Python pip packages, etc. That's why typically projects should set up a sandboxed environment with pinned versions of dependencies to ensure they can always be built and linted at any point in time.

Take also for example Clang compiler, I can see in the release notes that they have removed 2 user-facing compiler flags:
https://clang.llvm.org/docs/ReleaseNotes.html#removed-compiler-flags

This means this compiler can no longer be used to compile code that has a checked-in CMakeLists.txt (or similar) that uses those compiler flags. How much of a problem that is? Should those projects really prevent Clang from cleaning technical debt and be user-friendly for the general public?

I do acknowledge that the proposed deprecation is much bigger since it will affect essentially all projects. I'm thinking perhaps we can simply extend the notice period so everyone has time to update, together with printing some warning?

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

template <> struct MappingTraits<ClangTidyOptions::StringPair> {
   }
 };
 
 struct NOptionMap {
   NOptionMap(IO &) {}
   NOptionMap(IO &, const ClangTidyOptions::OptionMap &OptionMap) {
     Options.reserve(OptionMap.size());
     for (const auto &KeyValue : OptionMap)
-      Options.emplace_back(std::string(KeyValue.getKey()), KeyValue.getValue().Value);
+      Options.emplace_back(std::string(KeyValue.getKey()),
+                           KeyValue.getValue().Value);
   }
   ClangTidyOptions::OptionMap denormalize(IO &) {
     ClangTidyOptions::OptionMap Map;
     for (const auto &KeyValue : Options)
       Map[KeyValue.first] = ClangTidyOptions::ClangTidyValue(KeyValue.second);
     return Map;
   }
   std::vector<ClangTidyOptions::StringPair> Options;
if (isa<SequenceNode>(I.getCurrentNode())) {
       }
       IO.endMapping();
     } else {
       IO.setError("expected a sequence or map");
     }
   }
 }
 
+struct ChecksVariant {
+  std::optional<std::string> AsString;
+  std::optional<std::vector<std::string>> AsVector;
+};
+
+template <>
+void yamlize(IO &IO, ChecksVariant &Checks, bool, EmptyContext &Ctx) {
+  if (!IO.outputting()) {
+    // Special case for reading from YAML
+    // Must support reading from both a string or a list
+    Input &I = reinterpret_cast<Input &>(IO);
+
+    if (isa<ScalarNode>(I.getCurrentNode()) ||
+        isa<BlockScalarNode>(I.getCurrentNode())) {
+      Checks.AsString = std::string();
+      yamlize(IO, *Checks.AsString, true, Ctx);
+    } else if (isa<SequenceNode>(I.getCurrentNode())) {
+      Checks.AsVector = std::vector<std::string>();
+      yamlize(IO, *Checks.AsVector, true, Ctx);
+    } else {
+      IO.setError("expected string or sequence");
+    }
+  }
+}
+
+static void mapChecks(IO &IO, std::optional<std::string> &Checks) {
+  if (IO.outputting()) {
+    // Output as a string
+    IO.mapOptional("Checks", Checks);
+  } else {
+    // Input as either a string or a list
+    ChecksVariant ChecksAsVariant;
+    IO.mapOptional("Checks", ChecksAsVariant);
+
+    // If we got the input as string, assign directly
+    if (ChecksAsVariant.AsString) {
+      Checks = ChecksAsVariant.AsString;
+    }
+    // If we got the input as a list, concatenate the items to create string
+    else if (ChecksAsVariant.AsVector) {
+      Checks = std::string();
+      for (std::string const &check : *ChecksAsVariant.AsVector) {
+        *Checks += check + ",";
+      }
+    }
+  }
+}
+
 template <> struct MappingTraits<ClangTidyOptions> {
   static void mapping(IO &IO, ClangTidyOptions &Options) {
     bool Ignored = false;
-    IO.mapOptional("Checks", Options.Checks);
+    mapChecks(IO, Options.Checks);
     IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
     IO.mapOptional("HeaderFileExtensions", Options.HeaderFileExtensions);
     IO.mapOptional("ImplementationFileExtensions",
                    Options.ImplementationFileExtensions);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Ignored); // deprecated
     IO.mapOptional("FormatStyle", Options.FormatStyle);
     IO.mapOptional("User", Options.User);

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file-list-bracket

+Checks: [
+  "-*",
+  "hicpp-uppercase-literal-suffix",
+  "hicpp-use-auto",
+  "hicpp-use-emplace",
+]

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file-list-dash

+Checks:
+  - "-*"
+  - "hicpp-uppercase-literal-suffix"
+  - "hicpp-use-auto"
+  - "hicpp-use-emplace"

clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp

 // RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config -- | FileCheck %s -check-prefix=CHECK-BASE
 // CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
 // RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file-spaces --list-checks -- | FileCheck %s -check-prefix=CHECK-SPACES
 // CHECK-SPACES: Enabled checks:
 // CHECK-SPACES-NEXT: hicpp-uppercase-literal-suffix
 // CHECK-SPACES-NEXT: hicpp-use-auto
 // CHECK-SPACES-NEXT: hicpp-use-emplace
 // CHECK-SPACES-EMPTY:
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file-list-dash --list-checks -- | FileCheck %s -check-prefix=CHECK-LIST-DASH
+// CHECK-LIST-DASH: Enabled checks:
+// CHECK-LIST-DASH-NEXT: hicpp-uppercase-literal-suffix
+// CHECK-LIST-DASH-NEXT: hicpp-use-auto
+// CHECK-LIST-DASH-NEXT: hicpp-use-emplace
+// CHECK-LIST-DASH-EMPTY:
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file-list-bracket --list-checks -- | FileCheck %s -check-prefix=CHECK-LIST-BRACKET
+// CHECK-LIST-BRACKET: Enabled checks:
+// CHECK-LIST-BRACKET-NEXT: hicpp-uppercase-literal-suffix
+// CHECK-LIST-BRACKET-NEXT: hicpp-use-auto
+// CHECK-LIST-BRACKET-NEXT: hicpp-use-emplace
+// CHECK-LIST-BRACKET-EMPTY: