This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by carlosgalvezp on Apr 9 2023, 2:15 AM.

Details

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

Diff Detail

Event Timeline

carlosgalvezp created this revision.Apr 9 2023, 2:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
carlosgalvezp requested review of this revision.Apr 9 2023, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2023, 2:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
carlosgalvezp retitled this revision from [WIP][clang-tidy] Support introducing checks as a list in the config file to [WIP][clang-tidy] Support specifying Checks as a list in the config file.

Update commit message

  • Simplify code with llvm:join.
  • Add documentation.
carlosgalvezp retitled this revision from [WIP][clang-tidy] Support specifying Checks as a list in the config file to [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 edited the summary of this revision. (Show Details)

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 retitled this revision from [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... to [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....
carlosgalvezp edited the summary of this revision. (Show Details)

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...

carlosgalvezp retitled this revision from [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... to [clang-tidy] Support introducing checks as a list in the config file.Apr 9 2023, 1:57 PM
carlosgalvezp edited the summary of this revision. (Show Details)
Eugene.Zelenko accepted this revision.Apr 9 2023, 2:51 PM

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

This revision is now accepted and ready to land.Apr 9 2023, 2:51 PM
njames93 requested changes to this revision.Apr 9 2023, 4:54 PM

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

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

128–140

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

131–132
clang-tools-extra/docs/ReleaseNotes.rst
310–311

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

This revision now requires changes to proceed.Apr 9 2023, 4:54 PM
carlosgalvezp marked an inline comment as done.Apr 9 2023, 11:30 PM

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

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

131–132

Neat, thank you!

carlosgalvezp marked 2 inline comments as done.
carlosgalvezp retitled this revision from [clang-tidy] Support introducing checks as a list in the config file to [clang-tidy] Support specifying checks as a list in the config file.
carlosgalvezp edited the summary of this revision. (Show Details)

Revert unrelated changes

carlosgalvezp marked an inline comment as done.Apr 10 2023, 4:24 AM
carlosgalvezp added inline comments.
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
128–140

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 added inline comments.Apr 10 2023, 4:41 AM
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
128–140

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

njames93 accepted this revision.Apr 10 2023, 5:02 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
128–140

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

This revision is now accepted and ready to land.Apr 10 2023, 5:02 AM

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 :)

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

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?

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.

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?