[clang-tidy][RFC] Run each check only once

Hi!

I’m writing to this list as suggested in this bug:
https://reviews.llvm.org/D72566

Background

Hi!

Are there any opinions on this topic? As said, there has been several commit attempts to fix this, but people thought it would be better to first discuss it here before hacking some code. So that’s why I’m bringing this up :slight_smile:

Thanks!
/Carlos

Hi!

I'm writing to this list as suggested in this bug:
⚙ D72566 [clang-tidy] Clang tidy is now alias aware

Background

clang-tidy has implemented a number of checks, some of which are aliases of "real" checks. Currently if a user specifies that they want to run both the "real" and the "alias" check, clang-tidy will run the same check twice. Or N times if there are N aliases.

When applying clang-tidy to large codebases, this can have a severe impact in runtime in Continuous Integration checks. The only solution so far is to manually disable alias checks, but this generates noise in the config file, and is error-prone and cumbersome to maintain in the future, as one always need to check the LLVM docs to see what is aliasing what.

I have read in different bugs that one tricky problem with this is that "some alias checks run with different defaults, therefore leading to different results". I would be interested to know which checks are these and where this different default configuration is documented. I would argue that if 2 checks run different things and produce different results, then they are not aliases. They may share the same implementation, but they are not identical (therefore cannot be called "aliases".

Traditionally, we've always considered two checks to be aliases when
they share the same implementation and only differ via configuration.
We have a lot of aliased checks, see the bottom of this page:
clang-tidy - Clang-Tidy Checks — Extra Clang Tools 16.0.0git documentation As for the
configuration differences, those are documented with the individual
checks.

I have also read the argument "people should not enable all checks". I believe that should be up to users to decide. Plus, as we've seen, there are aliases even within the same category, e.g. "cert-*", so the problem still remains.

I strongly agree with the argument that people should not enable all
checks. For example, we have checks that sometimes give conflicting
diagnostics where the user cannot win. But more importantly, given
that many clang-tidy checks are for specific coding standards, I
believe it is very unlikely to realistically enable all checks anyway
because you rarely want to conform to every coding standard (more
often, you want to conform to one or two). Coding standards comprise
the majority of clang-tidy checks. abseil, altera, android, boost,
cert, cppcoreguidelines, darwin, fuschia, google, hicpp, linuxkernel,
llvm, llvmlibc, mpi, objc, openmp, and zircon are the coding standard
modules, while bugprone, majority of clang-analyzer, concurrency,
misc, modernize, performance, portability, and readability are the
general-purpose modules.

Proposal

- Find which checks are "fake aliases", if any - share the same implementation but run with different defaults and produce different results. Change these checks to not be considered as "aliases".

- Add an opt-in feature (command line, config) to enable "the same check should be run only once". Diagnostics should come only from the "main" check (if it's active) or from the "alias" check (if the "main" check is not active).

Personally, I'm unmotivated by the idea of running all checks in
clang-tidy and I don't think it's a feature we want to encourage.
There's not a lot of reason to believe that it's valuable to enable
the LLVM style guide checks at the same time as the Linux Kernel
checks at the same time as the android checks given how unrelated
these modules are to one another (and other such mildly-nonsensical
combinations exist).

That said, aliases between one of the coding standard modules and one
of the general-purpose modules happen quite frequently and combining
these kinds of modules (or general-purpose checks with other
general-purpose checks in another module) is very reasonable to want
to disable exact aliases for. So my thinking is: if the checks have
*no* configuration differences, there could be value in allowing the
user to run the check only once. I have no idea how many checks would
qualify though. Also, I think we'd still want to list all the enabled
checks which trigger the diagnostic as we currently do. e.g. if cert-*
and misc-* are enabled, we still want both of them listed in the text
inside the in: `warning: found assert() that could be replaced by
static_assert() [cert-dcl03-c,misc-static-assert]`, otherwise behavior
with NOLINT suppression tags could get interesting.

~Aaron

While i don't really have an opinion as to what should happen if
someone enables multiple aliased checks with different configs,
i'm starting to strongly believe that this whole concept of
aliased checks is broken, and there must be an explicit opt-out,
that completely disabled all the aliasee checks,
while not affecting the non-aliasee checks.

I would like to see that change happen, and i would try to help review
such a patch.

Roman

While i don't really have an opinion as to what should happen if
someone enables multiple aliased checks with different configs,
i'm starting to strongly believe that this whole concept of
aliased checks is broken, and there must be an explicit opt-out,
that completely disabled all the aliasee checks,
while not affecting the non-aliasee checks.

Can you explain why? My inclination is to be opposed to such a thing
because the design of clang-tidy has been to encourage aliases to be
able to share implementations while being able to expose significantly
different checks between modules. So while I can understand a
rationale for a flag to disable *identical* checks, I think we set the
wrong expectations by surfacing a feature to disable *all* aliases
because that implies the aliases are not useful in general. (Note, if
the discussion is instead about whether we should deprecate alias
checks entirely, my opinions may differ. My point is that if we are
continuing to encourage aliases, they should continue to be surfaced
as first-class checks otherwise users have to worry more about what
check is an alias and what check is not, which doesn't seem like a
useful distinction to force users to reason about.)

~Aaron

I realized I didn’t click on “Reply all” :facepalm:, sending again:

Thanks a lot for the feedback!

As for the
configuration differences, those are documented with the individual
checks.

I have gone through your link, checking one by one each of the alias checks. There’s not a single one of them that describes any configuration, or where it says that its configuration differs from the “main” check. The docs only say "this is an alias check, please check for documentation. Am I looking at the wrong place?

For example, we have checks that sometimes give conflicting
diagnostics where the user cannot win.

It’s very simple to win - the user disables either of the checks which is giving conflict diagnostics.

We are running clang-tidy with all checks enabled plus a handful disabled on a large codebase and it’s working just fine. The checks that we disable are not due to conflicts, but simply because we don’t want them. Having a list of the checks that are disabled visualizes more clearly what is it extra that we could potentially check, but don’t want to for reasons.

you rarely want to conform to every coding standard

It’s not about conformance, it’s about “writing code using best practices”. Coding guidelines are there to propose best practices in their fields, and usually most are in the same line. Aggregating all checks from all coding guidelines allows one to be exposed to best practices from all different domains. If a particular check doesn’t apply to your domain, you just disable it, but still get a chance to get a diagnostic and read about the pitfalls the check is preventing you from.

In other words, the use case is “use clang-tidy to learn about best C++ coding practices”, instead of “conform to X guideline”. Whether this makes sense or not it’s up to the user I believe. It really depends on what everyone wants to achieve and I don’t see why clang-tidy should have a say on that.

we still want both of them listed

A bit annoying but I could live with that, I can see quite a pain to implement the different edge cases - let’s keep it simple. The biggest gain is decrease in runtime for the tool in CI.

As Roman says, what would be the problem if this was an opt-out feature?

Thanks!

/Carlos

I want to also emphasize that, as user of clang-tidy not really knowing its internals, I find extremely confusing that there are “two types of aliases”, ones that are “identical” and ones that are “not identical, have different (undocumented?) configuration”. People coming from C++ to use clang-tidy typically refer to “alias” when doing e.g. a typedef / using declaration. They think of an alias as “something identical to something else, interchangeable”. If 2 things are not interchangeable, they are not 2 aliases, they are 2 independent things (that may share a lot in common). A class template Foo can be used like Foo and Foo. But they are not the same thing. I cannot replace one with the other in my code - they are not aliases. I can however do “using FooInt = Foo” and use the alias interchangeably in my code. So I think a lot of people can get confused by this an perhaps I agree with Roman that the whole concept of aliasing is at the very least misleading. People at my company were greatly surprised to get to know about this just recently.

Since I’m an outsider - may I ask “what is the purpose of aliases”? Obviously there’s code reuse, but why does that need to be exposed to the user? Isn’t that an implementation detail of clang-tidy? Users only need to care that when they ask clang-tidy to run checks A, B and C, clang-tidy does that and catches bugs in their code. They don’t need to know how they are implemented. And if A and B are identical (as per the existing documentation), then my proposal is that clang-tidy caches the results of A when being asked to run B (assuming that checks are deterministic, of course), so that there’s no need to spend large amounts of time running identical checks.

PS: it’s not even possible to properly read the documentation of alias checks. After 5-10 seconds, the browser (Chrome) automatically redirects to the “main check”. So I’d be surprised if some “alias-specific configuration” could be documented there.

/Carlos

I realized I didn't click on "Reply all" :facepalm:, sending again:

Thanks a lot for the feedback!

> As for the
configuration differences, those are documented with the individual
checks.

I have gone through your link, checking one by one each of the alias checks. There's not a single one of them that describes any configuration, or where it says that its configuration differs from the "main" check. The docs only say "this is an alias check, please check <main check> for documentation. Am I looking at the wrong place?

The primary check documents the configuration options. e.g.,
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html.
However, I am betting there are inconsistencies in documentation that
could be improved and you raise a valid point of it being hard to know
*how* these checks differ (if they differ at all).

> For example, we have checks that sometimes give conflicting
diagnostics where the user cannot win.

It's very simple to win - the user disables either of the checks which is giving conflict diagnostics.

Our definitions of "simple" differ -- I consider any changes to a
configuration file (or adding pragmas) to silence a diagnostic to be
strictly "harder" than locally modifying the code so that the
diagnostic isn't triggered in the first place.

We are running clang-tidy with all checks enabled plus a handful disabled on a large codebase and it's working just fine. The checks that we disable are not due to conflicts, but simply because we don't want them. Having a list of the checks that are disabled visualizes more clearly what is it extra that we could potentially check, but don't want to for reasons.

> you rarely want to conform to every coding standard

It's not about conformance, it's about "writing code using best practices". Coding guidelines are there to propose best practices in their fields, and usually most are in the same line. Aggregating all checks from all coding guidelines allows one to be exposed to best practices from all different domains. If a particular check doesn't apply to your domain, you just disable it, but still get a chance to get a diagnostic and read about the pitfalls the check is preventing you from.

In other words, the use case is "use clang-tidy to learn about best C++ coding practices", instead of "conform to X guideline". Whether this makes sense or not it's up to the user I believe. It really depends on what everyone wants to achieve and I don't see why clang-tidy should have a say on that.

My point is: these modules are not designed for "writing code using
best practices" on the whole, so enabling all of them is basically not
a useful operation for most people. The Linux kernel module checks are
highly specific to the Linux kernel and have nothing to do with code
quality outside of that project. The LLVM module checks are highly
specific to LLVM in the same way. The LLVM libc module checks are
highly specific to LLVM's libc in the same way. Etc. Making it easier
for users to enable all of the checks at once requires check authors
to consider *all* interactions between modules, and to date that isn't
a price we've been willing to make check authors pay. Instead, we
typically view modules as being orthogonal sets of functionality that
may or may not play well together.

> we still want both of them listed
A bit annoying but I could live with that, I can see quite a pain to implement the different edge cases - let's keep it simple. The biggest gain is decrease in runtime for the tool in CI.

As Roman says, what would be the problem if this was an opt-out feature?

To date, aliases have been surfaced to the user as a first-class check
(while the documentation typically redirects to the primary check for
simplicity of writing check aliases, there's no requirement that they
do so; other than that, aliases should behave the same as any other
check as far as the user is concerned.) You're proposing to make
aliases be sort of second-class checks that are more closely tied to
the primary check by providing an option that says aliases are
disabled as a blanket operation rather than a case-by-case basis. I am
not convinced this is a good approach. We may already have checks for
which this is a problem (where the aliased check and the primary check
are checking different things), and this closes off the design space
for such checks in the future (and possibly encourages less code reuse
in the process).

Btw, I'm not saying "no way", I'm saying "let's make sure we're not
regressing functionality or painting ourselves into a corner."

I'd be curious to hear from Alex on the topic as well, as this is not
the first discussion about alias behaviors.

~Aaron

Also, to date I believe we've never really cared which module has the
primary check and which one has the alias check (it's fine for the
primary to be in cert- and the alias to be in bugprone- or vice
versa), but with this sort of an option, that implementation detail is
leaked out to the user in a more obvious way. If we go this route, we
might have to move some checks around so there's some consistent rule
to aid users (like, the general-purpose module holds the primary and
the coding style guidelines get the alias), but it may not work in
every situation (I can imagine a check living in two style guides but
not a general purpose module, or two general purpose modules but not a
style guide).

~Aaron

Making it easier
for users to enable all of the checks at once requires check authors
to consider all interactions between modules, and to date that isn’t
a price we’ve been willing to make check authors pay. Instead, we
typically view modules as being orthogonal sets of functionality that
may or may not play well together.

Fully agree to that, checks should absolutely be independent, granular and focused on doing one single thing without depending on one another. And they may conflict with each other. My point is that it should be the user making the decision as to how to handle conflicts, enabling and disabling whichever checks they want. Why is that a problem? We are not requiring extra work on LLVM developers. Why should they have a say on how the user configures the tool? If developers have exposed this public CLI/config interface, users are free to use it as they see fit (and pay the price of their choices, of course).

You’re proposing to make aliases be sort of second-class checks

Hmm, not exactly, I agree with you that all publicly documented checks should be regarded as “first-class” checks. What I’m proposing here could be considered more as an “optimization” - don’t run the same check twice, just re-use previous results. If you know that you are going to run 2 checks that are identical, just run one of them.

we’ve never really cared which module has the primary check and which one has the alias check
I actually think this is one of the problems that you were concerned about above. Today, not all checks are first-class. There are “primary” checks and “alias” checks, and a decision has to be made as to where to put which, which causes the very issues you are describing. Not all checks from cppcoreguidelines are “primary”, some come from “misc”, so what? They aren’t any less valuable.

I refer to my question above - why do we even need this distinction between “primary” and “alias”, i.e. what problem is this solving? There’s 3 checks checking for C style arrays? That’s great, why not just link them to the same documentation, and internally (not leaked to the user) use the same implementation?

The only use case I can think of to publicly document that there are “alias” checks is to let the user know that they can “skip redundant checks” by manually disabling them. If there’s any other use case I’d be very happy to know!

/Carlos

> Making it easier
for users to enable all of the checks at once requires check authors
to consider *all* interactions between modules, and to date that isn't
a price we've been willing to make check authors pay. Instead, we
typically view modules as being orthogonal sets of functionality that
may or may not play well together.

Fully agree to that, checks should absolutely be independent, granular and focused on doing one single thing without depending on one another. And they may conflict with each other. My point is that it should be the user making the decision as to how to handle conflicts, enabling and disabling whichever checks they want. Why is that a problem?

I am not convinced that it's the right granularity. We've developed
aliases with the idea that as far as the user is concerned, which is
primary and which is aliased should not matter. Now we want to change
that with a blanket switch that impacts all aliases. This isn't a
problem per se, but I'm not convinced we're ready to add a switch to
disable all aliases yet.

We are not requiring extra work on LLVM developers.

We certainly are requiring extra work for check developers because now
they have to consider whether to make a check an alias or not because
that now matters to how their check is surfaced to users.

Why should they have a say on how the user configures the tool? If developers have exposed this public CLI/config interface, users are free to use it as they see fit (and pay the price of their choices, of course).

> You're proposing to make aliases be sort of second-class checks

Hmm, not exactly, I agree with you that all publicly documented checks should be regarded as "first-class" checks. What I'm proposing here could be considered more as an "optimization" - don't run the same check twice, just re-use previous results. If you know that you are going to run 2 checks that are identical, just run one of them.

I think that functionality is a good first step. If it's limited to
only checks that are *identical*, then my concerns largely go away.
But I had the impression that the desired scope was larger -- to avoid
running checks that are identical when ignoring the default
configurations of the checks -- and that difference is the bit that
worries me.

> we've never really cared which module has the primary check and which one has the alias check
I actually think this is one of the problems that you were concerned about above. Today, not all checks are first-class. There are "primary" checks and "alias" checks, and a decision has to be made as to where to put which, which causes the very issues you are describing. Not all checks from cppcoreguidelines are "primary", some come from "misc", so what? They aren't any less valuable.

Today, all checks are first-class as far as the user running the tool
is concerned. We mention aliases in public so users are aware that
they exist and to ease documentation burdens (maybe that was a
mistake), but users don't have to care which one is primary and which
ones are secondary when they run the tool. So long as we keep that
property, I'm content.

I refer to my question above - why do we even need this distinction between "primary" and "alias", i.e. what problem is this solving? There's 3 checks checking for C style arrays? That's great, why not just link them to the same documentation, and internally (not leaked to the user) use the same implementation?

That's the whole goal of aliases. You bring up a good point on whether
we need users to know about them at all.

The only use case I can think of to publicly document that there are "alias" checks is to let the user know that they can "skip redundant checks" by manually disabling them. If there's any other use case I'd be very happy to know!

They're a convenience for developing a check once but varying its
behavior across modules, as this use case comes up somewhat frequently
with coding standards. So the primary check gets all the options that
users can tweak, and then coding style checks set those tweakable
options as they see fit. However, this possibly doesn't require the
user to know about the check being an alias. And there's no benefit to
running a check twice when it's configuration is identical to a
previous check that I can tell.

~Aaron

I think we agree in the goals of this change, perhaps it gets confusing when using the term “alias” with different meanings, so let me summarize what I believe is the requirements of what we want to have, without entering in technicalities:

  • All publicly documented checks should be “first-class checks”, meaning users can be enable/disable them independently of each other. Not the scope of this change to deprecate the concept/name of “alias”, perhaps in the future.

  • Users don’t need to know/care how these checks are implemented. If they reuse code or not, that’s not their concern. They just need to care that their code will get checked properly when the check is enabled, so that they can fix bugs, improve code quality or be compliant with a given guideline.

  • Check developers should keep their exact same development flow, and be able to reuse code if possible as they do today.

  • Identical checks should run only once. The definition of “identical” is: a) they have the same implementation and b) they have the same configuration. Same implementation but different configuration is not identical checks, so they would all still be run. So the scope is smaller than what it might have looked like in my first post, I hope it’s clear now.

  • Two additional points of discussion: diagnostics and profiling. IMO implementing this change would be easier if we just skipped diagnostics and profiling for the not-run “identical” checks. We have a list of all checks, then remove the identical ones and run the updated list of checks. Keeping diagnostics and profiling info would probably complicate the design of the tool, but maybe someone has clever ideas. From a user perspective, I think it’s neat to avoid diagnostics from identical checks, since it’s less noisy to NOLINT them in code.

Would you agree on the above?

So the primary check gets all the options that users can tweak, and then coding style checks set those tweakable options as they see fit

Thanks, this helps me understand the design. Makes sense to have a “core” implementation of the check with knobs that can be tweaked via config for each guideline. I have some thoughts about build system and where to put such “common” checks that are re-usable, but let’s leave that for another time :slight_smile:

I think we agree in the goals of this change, perhaps it gets confusing when using the term "alias" with different meanings, so let me summarize what I believe is the requirements of what we want to have, without entering in technicalities:

* All publicly documented checks should be "first-class checks", meaning users can be enable/disable them independently of each other. Not the scope of this change to deprecate the concept/name of "alias", perhaps in the future.

+1. To be super clear, I consider checks which redirect their
documentation to still count as "publicly documented checks".

* Users don't need to know/care how these checks are implemented. If they reuse code or not, that's not their concern. They just need to care that their code will get checked properly when the check is enabled, so that they can fix bugs, improve code quality or be compliant with a given guideline.

+1

* Check developers should keep their exact same development flow, and be able to reuse code if possible as they do today.

+1

* Identical checks should run only once. The definition of "identical" is: a) they have the same implementation and b) they have the same configuration. Same implementation but different configuration is not identical checks, so they would all still be run. So the scope is smaller than what it might have looked like in my first post, I hope it's clear now.

+1

* Two additional points of discussion: diagnostics and profiling. IMO implementing this change would be easier if we just skipped diagnostics and profiling for the not-run "identical" checks. We have a list of all checks, then remove the identical ones and run the updated list of checks. Keeping diagnostics and profiling info would probably complicate the design of the tool, but maybe someone has clever ideas. From a user perspective, I think it's neat to avoid diagnostics from identical checks, since it's less noisy to NOLINT them in code.

Regarding diagnostics, what I'm hoping we can do is keep the
diagnostic behavior we already have. e.g.,
Compiler Explorer where the diagnostic says:

warning: found assert() that could be replaced by static_assert()
[cert-dcl03-c,misc-static-assert]

and lists both of the ways in which the diagnostic can be triggered
but does not show hicpp-static-assert which is a third way to trigger
the diagnostic (but was never enabled). I think this is useful because
not listing the identical checks can potentially cause some pain for
users depending on how they fix the diagnostic or migrate their code.
While this one would be most sensible to just change the code, it's
not unthinkable that a developer will add a `// NOLINT` comment to
disable the diagnostic. If they're only shown one check name in the
diagnostic, the user may elect to write `// NOLINT(cert-dcl03-c)` to
disable *only* the check name they see. They'll be rather surprised if
they decide to drop checking the CERT module and they start getting
diagnostics on that line about misc-static-assert when the code was
previously suppressing the warning. Seeing multiple checks that
trigger will hopefully help the user to know to write `// NOLINT`
instead of a specific check, or may help gently guide them towards
fixing their code instead of suppressing the diagnostic.

That said, if this turns out to be an implementation burden in
practice, I think the behavior is fine *so long as* the user who adds
`// NOLINT(cert-dcl03-c)` doesn't have the experience of silencing the
cert diagnostic while still emitting the misc diagnostic (which, if
we're not running the duplicate check in the first place, shouldn't
happen anyway).

Would you agree on the above?

I think we're in agreement, yes!

~Aaron

Hi again!

I’ve been looking into this for a while now and have some very crude proof-of-concept that seems to work without any impact for check developers and ensuring only “perfect aliases” are not run. The only things I can’t get around are diagnostics and profiling, so I’d need some help to figure that out (or drop the requirements). Should I push this proof-of-concept for review and continue the discussion there, or do you prefer to discuss here in the mailing list?

Best regards,
/Carlos

Hi again!

I've been looking into this for a while now and have some very crude proof-of-concept that seems to work without any impact for check developers and ensuring only "perfect aliases" are not run. The only things I can't get around are diagnostics and profiling, so I'd need some help to figure that out (or drop the requirements). Should I push this proof-of-concept for review and continue the discussion there, or do you prefer to discuss here in the mailing list?

If you've got a proof of concept, I think it's probably worth showing
at this point. (Just as an FYI for setting expectations, my review
time is pretty limited for the next while.)

~Aaron

Here it goes! https://reviews.llvm.org/D114317

As I wrote in the commit message, please take it with a grain of salt - the code is not pretty and many things are missing - I’ll take care of that if/when we are happy with the general direction. Looking forward to your feedback! If you have suggestions for the missing bits I’d be very happy to hear them.

/Carlos

Aaron,

Did you have a chance to look at the patch? I have tried to reach you via the comments section but haven’t got any reply yet.

Best regards,
Carlos

Aaron,

Did you have a chance to look at the patch? I have tried to reach you via the comments section but haven't got any reply yet.

Thank you for the ping, and sorry about the lack of response! I'll try
to get to it today.

~Aaron