Page MenuHomePhabricator

[clang-tidy] Implement clang-tidy check aliases
Needs RevisionPublic

Authored by leanil on Sep 22 2017, 3:57 AM.

Details

Summary

Clang warnings can be mapped to virtual check names as aliases. The mappings can be set in the appropriate ClangTidyModules. If the virtual check is enabled, then

  • the corresponding warning options get passed to clang, and
  • the warning names are replaced with the check alias in the produced messages.

This is an implementation of the proposal here.

Diff Detail

Event Timeline

leanil created this revision.Sep 22 2017, 3:57 AM
aaron.ballman edited edge metadata.Sep 22 2017, 5:58 AM

Thank you for working on this -- it's very nice functionality!

clang-tidy/cert/CERTTidyModule.cpp
78

You should declare this as override.

79–81

Can you use try_emplace() instead of insert() -- this removes the need for the braced initializers to create the pair.

82

This check also needs a test case.

alexfh edited edge metadata.Sep 22 2017, 7:18 AM

András, that's definitely an interesting idea. However, it might be interesting to explore a more principled approach:

  1. Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.
  2. Make aliases first-class citizens (there was a proposal as well, but we didn't arrive to a consensus at that time). That would include the ability to configure an alias name for any check including clang-diagnostic- and clang-analyzer- checks.
  3. Use aliases to map clang-diagnostic- checks to check names under cert-, hicpp-, etc.

I didn't carefully consider all possible implications and there may be issues with any or all of the three parts of this, but I think it's worth exploring.

On a somewhat related note, since this is already talking about aliases

I feel like the current handling of the clang-tidy check aliases needs adjusting.
If one enables all the checks (Checks: '*'), and then disables some of them
on check-by-check basis, if the disabled check has aliases
(cppcoreguidelines-pro-type-vararg vs hicpp-vararg, hicpp-no-array-decay
vs cppcoreguidelines-pro-bounds-array-to-pointer-decay and so on)
each of the aliases must be explicitly be disabled too.

This is rather inconvenient.

If that is intentional, perhaps there could be a config option to either disable
creation/registration of the check aliases altogether, or an option to threat
aliases as a hard link, so anything happening to any of the aliases/base check
would happen to all of them.

(Also, if the check has parameters, and one specifies different parameters for the base check, and the aliases, what happens?)

xazax.hun edited edge metadata.Sep 25 2017, 5:50 AM

@leanil : Could you add a test case where the warnings are explicitly disabled in the compilation command and also one where only the aliased warning is explicitly disabled?

I feel like the current handling of the clang-tidy check aliases needs adjusting.
If one enables all the checks (Checks: '*'), and then disables some of them
on check-by-check basis, if the disabled check has aliases
(cppcoreguidelines-pro-type-vararg vs hicpp-vararg, hicpp-no-array-decay
vs cppcoreguidelines-pro-bounds-array-to-pointer-decay and so on)
each of the aliases must be explicitly be disabled too.

That is somewhat intentional I think. Lets imagined you are interested in cppcoreguidelines but not interested in high integrity cpp. It would be great to be able to use Checks: 'cppcoreguidelines*,-hicpp*' without having to worry about the aliases.

If that is intentional, perhaps there could be a config option to either disable
creation/registration of the check aliases altogether, or an option to threat
aliases as a hard link, so anything happening to any of the aliases/base check
would happen to all of them.

I think the source of the problems is that conceptually there are two distinct reasons to disable a check. One reason when the user is not interested in the results regardless what is the category or name of the check. In this case, all aliases should be disabled. The second is when a category of checks (e.g.: a guideline) is not applicable to the code. In this case aliases should not be disabled. So I think a global option would not solve this issue. A better solution might be a per glob notation. So - could mean do not disable aliases and -- could mean disable aliases as well but that is just an example.

All in all, I think this is not related strictly to this issue and I think we should discuss this separately on the mailing list.

(Also, if the check has parameters, and one specifies different parameters for the base check, and the aliases, what happens?)

I think this is a very good point, this needs to be documented somewhere.

András, that's definitely an interesting idea. However, it might be interesting to explore a more principled approach:

  1. Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.

I agree that this is a good idea. I think it could be done in this patch. But to be sure, could you elaborate on what do you mean by first-class citizen?

  1. Make aliases first-class citizens (there was a proposal as well, but we didn't arrive to a consensus at that time). That would include the ability to configure an alias name for any check including clang-diagnostic- and clang-analyzer- checks.

Do you have a link to the proposal? What do you mean by the ability to configure? As in having a config file or registering aliases in modules like now? If you mean the former, I think that is better addressed in a follow-up patch.

  1. Use aliases to map clang-diagnostic- checks to check names under cert-, hicpp-, etc.

    I didn't carefully consider all possible implications and there may be issues with any or all of the three parts of this, but I think it's worth exploring.

I feel like the current handling of the clang-tidy check aliases needs adjusting.
If one enables all the checks (Checks: '*'), and then disables some of them
on check-by-check basis, if the disabled check has aliases
(cppcoreguidelines-pro-type-vararg vs hicpp-vararg, hicpp-no-array-decay
vs cppcoreguidelines-pro-bounds-array-to-pointer-decay and so on)
each of the aliases must be explicitly be disabled too.

That is somewhat intentional I think. Lets imagined you are interested in cppcoreguidelines but not interested in high integrity cpp. It would be great to be able to use Checks: 'cppcoreguidelines*,-hicpp*' without having to worry about the aliases.

If that is intentional, perhaps there could be a config option to either disable
creation/registration of the check aliases altogether, or an option to threat
aliases as a hard link, so anything happening to any of the aliases/base check
would happen to all of them.

I think the source of the problems is that conceptually there are two distinct reasons to disable a check. One reason when the user is not interested in the results regardless what is the category or name of the check. In this case, all aliases should be disabled. The second is when a category of checks (e.g.: a guideline) is not applicable to the code. In this case aliases should not be disabled. So I think a global option would not solve this issue. A better solution might be a per glob notation. So - could mean do not disable aliases and -- could mean disable aliases as well but that is just an example.

All in all, I think this is not related strictly to this issue and I think we should discuss this separately on the mailing list.

(Also, if the check has parameters, and one specifies different parameters for the base check, and the aliases, what happens?)

I think this is a very good point, this needs to be documented somewhere.

A mail [0] posted by @JonasToth is about the similar problem, i think we can continue there.

I *think* he has a point, a some better way to setup aliases, without actually
creating aliased checks is the solution IMO. This way, there won't be any
need to disable all the aliases when completely disabling the checks, and
there won't be multiple config options for the each alias. The legacy support
is the question though.

[0] (https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html)

On a somewhat related note, since this is already talking about aliases

I feel like the current handling of the clang-tidy check aliases needs adjusting.
If one enables all the checks (Checks: '*'), and then disables some of them
on check-by-check basis,

Clang-tidy allows doing so, but I doubt this is a use case we want to encourage. At least, not for a day-to-day use. Most checks require a certain amount of work on the target code and sometimes fine tuning of the check options to be useful for any real codebase. Apart from that, there are checks that give duplicate warnings (some are aliases and some just cover intersecting set of issues), and there are checks that can give conflicting advice. These facts don't combine well with the "enable almost all checks" use case and limit its area of applicability to stress-testing clang-tidy and maybe to initial evaluation of clang-tidy checks on a certain codebase (though going from the other direction and determining the set of style rules for the codebase and then finding which of those are implemented in clang-tidy is a more fruitful approach).

if the disabled check has aliases
(cppcoreguidelines-pro-type-vararg vs hicpp-vararg, hicpp-no-array-decay
vs cppcoreguidelines-pro-bounds-array-to-pointer-decay and so on)
each of the aliases must be explicitly be disabled too.

It seems to be the least confusing of possible options. Gabor made a good point why this is useful as it is.

This is rather inconvenient.

If that is intentional, perhaps there could be a config option to either disable
creation/registration of the check aliases altogether, or an option to threat
aliases as a hard link, so anything happening to any of the aliases/base check
would happen to all of them.

(Also, if the check has parameters, and one specifies different parameters for the base check, and the aliases, what happens?)

A mail [0] posted by @JonasToth is about the similar problem, i think we can continue there.

Great! Could you summarize your points there as well? Thanks in advance.

A mail [0] posted by @JonasToth is about the similar problem, i think we can continue there.

Great! Could you summarize your points there as well? Thanks in advance.

Done.

On a somewhat related note, since this is already talking about aliases

I feel like the current handling of the clang-tidy check aliases needs adjusting.
If one enables all the checks (Checks: '*'), and then disables some of them
on check-by-check basis,

Clang-tidy allows doing so, but I doubt this is a use case we want to encourage.
At least, not for a day-to-day use.

It's much like clang's -Weverything, which is rather convenient, and is
inconveniently 'missing' in gcc, because the devs believe that there would
be complains that it results in new warnings each release,
which is kinda the point of that flag...

Most checks require a certain amount of work on the target code and sometimes
fine tuning of the check options to be useful for any real codebase. Apart from that,
there are checks that give duplicate warnings (some are aliases and some just cover
intersecting set of issues), and there are checks that can give conflicting advice.
These facts don't combine well with the "enable almost all checks" use case and limit
its area of applicability to stress-testing clang-tidy and maybe to initial evaluation of
clang-tidy checks on a certain codebase (though going from the other direction and
determining the set of style rules for the codebase and then finding which of those
are implemented in clang-tidy is a more fruitful approach).

Except the disagreeing checks, same could be said about -Weverything, clang-analyzer,
etc :) Yes, of course it does not just work, and of course needs some further tuning,
and of course the codebase that is being checked needs to be fixed not to warn.

I.e. the use-case is to always have all the warnings enabled, and each time a new check
is triggered, analyze whether it should be kept enabled, or disable it.

leanil updated this revision to Diff 116694.Sep 26 2017, 12:09 PM

Add more tests and minor fixes.

András, that's definitely an interesting idea. However, it might be interesting to explore a more principled approach:

  1. Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.

I agree that this is a good idea. I think it could be done in this patch. But to be sure, could you elaborate on what do you mean by first-class citizen?

  1. Make aliases first-class citizens (there was a proposal as well, but we didn't arrive to a consensus at that time). That would include the ability to configure an alias name for any check including clang-diagnostic- and clang-analyzer- checks.

Do you have a link to the proposal? What do you mean by the ability to configure? As in having a config file or registering aliases in modules like now? If you mean the former, I think that is better addressed in a follow-up patch.

I also think this sounds good, though I'm not quite sure it can be done in this patch. Anyway, we should definitely specify what we expect from first-class citizen checks. Please correct & extend the list:

  • can be enabled or disabled through a unique name
  • can have config options
  • can have aliases
  • inherits ClangTidyCheck

I see the last one as a requirement for handling everything uniformly (without workarounds like this patch), but also as a main point where an alias differs from a "real" check.

leanil marked 3 inline comments as done.Sep 28 2017, 7:40 AM

Let's also summarize what do we have now and what do we want.

I also think this sounds good, though I'm not quite sure it can be done in this patch. Anyway, we should definitely specify what we expect from first-class citizen checks. Please correct & extend the list:

  • can be enabled or disabled through a unique name

This is addressed by the current pathc.

  • can have config options

Do you know warnings that have config options? Does this make sense for this featre?

  • can have aliases

Does that make sense to be able to add aliases to warning aliases? I

  • inherits ClangTidyCheck

I think this is just technical and this does not really affect the users.

  1. Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.

I think this might be a good idea in general. @leanil could you address this point in this patch?

  1. Make aliases first-class citizens (there was a proposal as well, but we didn't arrive to a consensus at that time). That would include the ability to configure an alias name for any check including clang-diagnostic- and clang-analyzer- checks.

I am not sure that this makes sense, see my points above.

  1. Use aliases to map clang-diagnostic- checks to check names under cert-, hicpp-, etc.

I agree.

One problem to think about when we add all clang-diagnostic as "first or second" class citizen, checkes=* might now enable all the warnings which make no sense and might be surprising to the users. What do you think?

leanil updated this revision to Diff 120597.Oct 27 2017, 7:32 AM

Make clang-diagnostic-* checks first-class citizens and take full control of all diagnostics, i.e. disable all Clang diagnostics by default, and enable the ones that correspond to the enabled clang-diagnostic checks.

(As @alexfh suggested.)

Collecting all the diagnostics seems to be possible from multiple sources, I've decided to make DiagnosticIDs::getAllDiagnostics static.
I also had to replace explicit -W arguments with the new checks in some of the tests.

One problem to think about when we add all clang-diagnostic as "first or second" class citizen, checkes=* might now enable all the warnings which make no sense and might be surprising to the users. What do you think?

This is a good point. Should I insert ",-clang-diagnostic*" after any (positive) * ?

xazax.hun retitled this revision from Implement clang-tidy check aliases. to [clang-tidy] Implement clang-tidy check aliases.Oct 30 2017, 4:05 AM

One problem to think about when we add all clang-diagnostic as "first or second" class citizen, checkes=* might now enable all the warnings which make no sense and might be surprising to the users. What do you think?

This is a good point. Should I insert ",-clang-diagnostic*" after any (positive) * ?

@alexfh do you have some thoughts on this?

One problem to think about when we add all clang-diagnostic as "first or second" class citizen, checkes=* might now enable all the warnings which make no sense and might be surprising to the users. What do you think?

This is a good point. Should I insert ",-clang-diagnostic*" after any (positive) * ?

@alexfh do you have some thoughts on this?

I don't think this particular point is a large concern. As I mentioned multiple times, enabling all checks is almost never useful due to many checks overlapping or producing conflicting advice. The only place I can think of, where -checks=* is useful is in combination with -list-checks, where the presence of clang-diagnostic- entries would be desired anyway.

And, btw, sorry for the long delay. I've been on travelling / on vacation for the last few weeks.

The only place I can think of, where -checks=* is useful is in combination with -list-checks, where the presence of clang-diagnostic- entries would be desired anyway.

Can we replace -list-checks with -list-enabled-checks and -list-all-checks? The current behaviour is confusing and recently caused Chandler to abandon a clang-tidy demo.

And, btw, sorry for the long delay. I've been on travelling / on vacation for the last few weeks.

No problem. Will you have some time to think about the overall concept? Implementation and test wise it looks good to me.
I think this patch is a step in a good direction but of course, it is important for the community to agree on the approach.

test/clang-tidy/warning-check-aliases.cpp
11

There are some redundant empty lines.

xazax.hun added inline comments.Nov 28 2017, 4:47 AM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
263

I am wondering if this is still a good candidate to be a small vector with this size. Maybe a regular vector with a reserve might be better?

leanil updated this revision to Diff 127736.Dec 20 2017, 8:29 AM

Remove redundant empty lines.
Make list-clang-diagnostics test less strict.
Update getAllDiagnostics to use std::vector.

leanil marked 2 inline comments as done.Dec 20 2017, 8:32 AM

Does anyone have any more thoughts about this?

@alexfh did you have any chance to think about this change?

While i don't have a leg to stand on here, i'd be *much* more comfortable if this would be a proper RFC mail in cfe-dev,
that would explore all the possible options (this, and the one from cfe-dev thread clang-tidy and CppCoreGuidelines),
and *explain* why one is chosen, and not the other one.

I still feel like that proposal is more sound.

I apologize for completely ignoring this for a long time.

I'm totally fine taking full control of clang-diagnostic- "checks", i.e. automatically adding corresponding -W flags and removing all other -W flags (or prepending -Wno-everything).

However, could we split the aliases stuff to a separate patch? My concerns w.r.t. the current implementation:

  1. it doesn't allow more than one alias to each diagnostic
  2. it's not clear that clang diagnostic aliases need to be separate from aliases for native clang-tidy checks. It might be better to add a more high-level API to register aliases (e.g. registerCheckAlias(<alias name>, <check name>, <custom check options>)).
clang-tidy/ClangTidy.cpp
406

The "clang-diagnostic-" string is now repeated multiple times. Could you pull it out to a constant (or maybe llvm::StringRef getClangDiagnosticPrefix())?

clang-tidy/ClangTidyModule.h
96

Consider using llvm::StringMap.

I'm totally fine taking full control of clang-diagnostic- "checks", i.e. automatically adding corresponding -W flags and removing all other -W flags (or prepending -Wno-everything).

However, could we split the aliases stuff to a separate patch?

The other way round. It will be better to split the clang-diagnostic part to a separate patch, since this patch focuses on the aliases.

alexfh added inline comments.Mar 14 2018, 6:53 AM
test/clang-tidy/misc-suspicious-semicolon-fail.cpp
5

This reminds me that we should also filter out all -W(no-)error.* compiler arguments, if we're to take full control of clang diagnostics and make them behave closer to native checks.

test/clang-tidy/validate-check-names.cpp
2

Which clang diagnostic names fail this test?

alexfh requested changes to this revision.Mar 14 2018, 7:45 AM
This revision now requires changes to proceed.Mar 14 2018, 7:45 AM
jslap added a subscriber: jslap.Jan 2 2019, 8:59 PM