This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Clang tidy is now alias aware
AbandonedPublic

Authored by njames93 on Jan 12 2020, 4:40 AM.

Details

Summary

Clang-tidy has had an on growing issue with blindly adding all checks from a module using the -checks=somemodule-* option. If somemodule alias' another enabled check. the check will get ran twice giving duplicated warning and fix options. This can lead to fixes overlapping preventing them from being applied or worse still applying twice resulting in malformed code. There are other issues with ConfigOptions being different in the 2 different instances so one instance of the check may be running a different set of options to the other.

This patch solves these issue by making clang-tidy only run 1 instance of an aliased check even if it has been turned on with multiple names. Warning messages are appended with the original check name if it has been enabled(via cmd line, or config file). If the original check hasn't been enabled then the name will be the first enabled alias that was registered.

Config options are also handled in a similar fashion, see example

[{ key: MyAliasedName.Option1, value: 0 }.
{ key: OriginalCheckName.Option1, value: 1 }]

If the check original-check-name was turned on, then Option1 will be parsed as being 1, otherwise it ignores OriginalCheckName.Option1 key and falls back to the first enabled alias check that declares Option1

To declare a check as an alias when you register then check, just add the check this alias' to as a second parameter

CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
    "hicpp-braces-around-statements");
// Update to
CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
    "hicpp-braces-around-statements", "readability-braces-around-statements");

I haven't added in all the alias declarations in just yet (If you want me to just ask), nor have I added test cases. However once all the alias declarations are added in, the tests fall nicely in line with the current test infrastructure.
Adding this line to the readability-braces-around-statements checks causes no errors, no multiple fix attempts or warning messages.

// RUN: %check_clang_tidy %s readability-braces-around-statements,hicpp-readability-braces-around-statements %t

Likewise changing it to

// RUN: %check_clang_tidy %s hicpp-readability-braces-around-statements %t

Also runs without a hitch (obviously warning messages are appended with [hicpp-readability-braces-around-statements] instead of [readability-braces-around-statements] as above.

Diff Detail

Event Timeline

njames93 created this revision.Jan 12 2020, 4:40 AM

Unit tests: pass. 61774 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

should warning be printed with all the enabled checks, or just the primary name?

warning: statement should be inside braces [readability-braces-around-statements]

vs

warning: statement should be inside braces [readability-braces-around-statements, hicpp-braces-around-statements]
warning: statement should be inside braces [readability-braces-around-statements], [hicpp-braces-around-statements]

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

Also, there was previously some attempts at this:
https://lists.llvm.org/pipermail/cfe-dev/2017-September/055589.html
D25659
D38171
???

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

That's how it currently is for adding new alias definitions.
When you create an alias definition now you need the implementation files available.
However this approach doesn't need to modify the original module and appears mostly transparent to it.
Its also made in such a way that if you set a check as aliasing to a now existent name, it would just give you another way to invoke said check

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

That does sound like something I can get behind

There is one major nit I can think of and that is if you create an alias to the wrong check it would overwrite said check. I may look into adjusting that

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

That is not what i'm talking about.
I'm mainly worried about checks with custom config options:
It is likely that said options were specified only for one check,
not all of it's aliases too. So now the fact whether or not
these options will be used will depend on the registering order of checks.

To be noted, this will make adding of new aliases in non-up-stream modules more involved,
since now the aliases will no longer be confined to the new module, but to the original modules.

That's how it currently is for adding new alias definitions.
When you create an alias definition now you need the implementation files available.
However this approach doesn't need to modify the original module and appears mostly transparent to it.
Its also made in such a way that if you set a check as aliasing to a now existent name, it would just give you another way to invoke said check

If we proceed as-is, i think we also need one more config switch "CheckAliasesAreEnabled = 1".
And we should completely ignore aliases unless it's enabled.

That does sound like something I can get behind

There is one major nit I can think of and that is if you create an alias to the wrong check it would overwrite said check. I may look into adjusting that

I agree that the current alias situation is not ideal, though i'm not sure
how much we can fix it since it crosses user interface boundary
(i.e. what fixes won't lead to some regressions from the previous expected behavior)

If you're expecting a test to run twice usually something has gone wrong

I'm not certain I agree. Many of the aliases have different sets of default checking options from the primary check. Running the "same" check twice in these cases produces different output. How do you intend to handle situations like that? If the answer is "disable the aliases", then this has the potential to lose valuable coverage for some folks who are asking for all checks to be enabled.

How does this sound?
I'll put in a config option in there to disable the new alias behaviour.
Personally I'd say default to the new behaviour. Then if an organisation
relies on the old behaviour then can revert back with a command line option
or .clang-tidy file edit

Another potential solution is leave is as is by default. Then emit a warning if a check is ran twice. The warning could be suppressed by passing a option to say what behaviour you want?

I think the fact that this is a fourth (?) different incarnation of a patch
trying to solve the same *major* *ugly* problem, it may be an evidence that
perhaps this problem should not be approached from the 'let's hack it together'
approach, but with a concrete plan sent as an RFC to cfe-dev :/

I think the fact that this is a fourth (?) different incarnation of a patch
trying to solve the same *major* *ugly* problem, it may be an evidence that
perhaps this problem should not be approached from the 'let's hack it together'
approach, but with a concrete plan sent as an RFC to cfe-dev :/

+1

This is a pretty big part of the user interface for clang-tidy, so I think we need community buy-in on how to proceed. It's not that we think the current interface is perfect, it's more that there are complexities we need to better understand before deciding on a solution. I'd rather get a big picture idea of the design for aliases, how they interact with the command line, documentation, diagnostics, etc to make sure we're hitting all of the important bits. Also, we should make sure that whatever we do is at least someone comparable with the clang static analyzer (I don't know if they have aliases there or not) so that we don't design two totally different approaches to solving the same problem.

njames93 abandoned this revision.Feb 25 2020, 12:19 AM

I'm not certain I agree. Many of the aliases have different sets of default checking options from the primary check.

And that's precisely the problem. Aliases should be just that: aliases. If two checks check for the same thing but have different options, they should be merged into one.

Hi!

I've been following this issue about aliases in clang-tidy through different bug reports and commit attempts. I see that the author abandoned this commit without an explanation. What's the reason? Where can I follow the continuation of this discussion, is there a email list or something?

I believe making sure that clang-tidy only runs once instance of the checks is important, it can dramatically reduce the runtime. This can have a significant impact in CI working with large codebases. It also doesn't make sense that alias checks aren't identical and don't detect the same things. As it has been mentioned above, users should not carry the burden of knowing and maintaining what is aliasing what at any point in time, that's an internal implementation detail of clang-tidy.

I think this should be as simple as having an opt-in "DisableAllAliaseeChecks" option in .clang-tidy, that force-disables all the checks that are actually aliases and not proper checks.

Fully agree. Such an opt-in feature shouldn't break things. I'd be happy to implement it if people think it's a good idea! Would need some hand-holding though, never touched the LLVM codebase before :)