[clang-tidy] `readability-implicit-bool-conversion` is too noisy by default

[firewave]

Out-of-the-box the readability-implicit-bool-conversion is way too noisy to be usable.

AllowIntegerConditions and AllowPointerConditions should be set to true to be begin with as those are common patterns.

With those patterns allowed I would expect that I only get assignments where an implicit conversion is allowed (which might actually indicate an unintentional behavior so that pattern rather belongs into misc-* or bugprone-*). But usages in return statements will still produce warnings.

bool f2(int i, int* i1, int& i2)
{
    if (i) {
        return i;
    }
    if (i1) {
        return *i1;
    }
    return i2;
}

<source>:4:16: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    4 |         return i;
      |                ^
      |                  != 0
<source>:7:16: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    7 |         return *i1;
      |                ^  
      |                    != 0
<source>:9:12: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    9 |     return i2;
      |            ^ 
      |               != 0

https://godbolt.org/z/jnWjbea13

This is also a very common pattern but it is not a condition and more akin to an assignment. So maybe that case should get additional (off by default) options. This is already tracked in #79447.

It is possible that there are more cases but it is hard to tell with the amount of warnings the check currently produces. There is also more related issues:
#63450
#36323

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Oliver Stöneberg (firewave)

Out-of-the-box the `readability-implicit-bool-conversion` is way too noisy to be usable.

AllowIntegerConditions and AllowPointerConditions should be set to true to be begin with as those are common patterns.

With those patterns allowed I would expect that I only get assignments where an implicit conversion is allowed (which might actually indicate an unintentional behavior so that pattern rather belongs into misc-* or bugprone-*). But usages in return statements will still produce warnings.

bool f2(int i, int* i1, int& i2)
{
    if (i) {
        return i;
    }
    if (i1) {
        return *i1;
    }
    return i2;
}

<source>:4:16: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    4 |         return i;
      |                ^
      |                  != 0
<source>:7:16: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    7 |         return *i1;
      |                ^  
      |                    != 0
<source>:9:12: warning: implicit conversion 'int' -> 'bool' [readability-implicit-bool-conversion]
    9 |     return i2;
      |            ^ 
      |               != 0

https://godbolt.org/z/jnWjbea13

This is also a very common pattern but it is not a condition and more akin to an assignment. So maybe that case should get additional (off by default) options. This is already tracked in #79447.

It is possible that there are more cases but it is hard to tell with the amount of warnings the check currently produces. There is also more related issues:
#63450
#36323

[firewave]

CC'ing some people involved in the existing tickets.
@adesitter @chrchr-github @aeubanks @4m4n-x-B4w4ne

[HerrCai0907]

It is hard to say too noisy or too relax. As I know there are some coding guidelines explicitly forbidden the pattern you mentioned in ticket, e.g. in auto industry.
The default options are either too strict or too lenient, and it's difficult to satisfy everyone. In my personal opinion, when there are demands on both ends of the scale, it's not advisable to change the default configuration easily. Instead, maintaining stability between different versions is more important. Perhaps we could seek the opinions of others. @5chmidti @vbvictor @AaronBallman

[LegalizeAdulthood]

Would it suffice to add more documentation on the check about the strictness of the check and provide example configurations for different tastes?

[carlosgalvezp]

Personally I think it's better to be stricter by default. You get to see warnings in your code, and decide if they make sense for your project or not. If not, then you investigate which options are available to tune it to your liking. Once you are satisfied, you enable the check for the project and fix warnings.

Otherwise, one typically doesn't immediately think "hmm, I'd expect the check to warn here, why doesn't it?" It has to pass some time to come across such a case, and then you find the option that would make tidy stricter. However, enough time has passed to make technical debt creep in and it's harder to fix for the whole project.

Yes, some guidelines (e.g. MISRA C++2023) are super strict on exactly this check. But they do allow contextual pointer to bool conversions (AllowPointerConversions), so I could be OK with turning that into true as default.

[firewave]

Thanks for clarifying. I wasn't aware of that. The amount of warnings you get still feels overwhelming compared to other checks.

I still think it needs tunables for the return case. Not sure about the "assignment" one because that would cause it to report nothing. Unless there is another category I did not think of. I will look over the warnings I see if there is another one than "condition", "assignment" and "return".

And I think the documentation is fine - maybe even a bit too excessive and could probably need some streamlining. Having tunables for the categories would be doubling for what it reports so only limitations/exception need to be pointed out.

[HerrCai0907]

In javascript world, eslint also very complex, but it simplify the complexity by composable set of predefined rules. The user only need to use npm install <some preset rules package> to obtain the static check rule configuration that meets the requirements of your project. It only needs 1 second.
For C++, we don't have convenient package manager, but similar, could we maintain several preset configurations in e.g. documents for different scenarios?
The advantage is that we don't have to argue about the default configuration of checks for different scenarios. It also helps new users quickly get started and avoid to waste lots of time to find suitable check list and corresponded configuration.

[LegalizeAdulthood]

For C++, we don't have convenient package manager,

We do, it's called vcpkg, but it's used for dependencies not for installing a set of lint rules. npm's idea of a "package" is much broader and just means "install some files into my tree".

I see no problem with communities defining their own collectively agreed upon clang-tidy config files and sharing them. It's as simple as making a github gist for the single config file.

[firewave]

For C++, we don't have convenient package manager, but similar, could we maintain several preset configurations in e.g. documents for different scenarios?

Isn't that just aliased checks with fixed configurations? Those already exist within clang-tidy.

Still you need the knobs to adjust the checks. Might be worth also having a look at how granular the compiler warnings for implicit number conversions are.

Also mind my earlier comment:

With those patterns allowed I would expect that I only get assignments where an implicit conversion is allowed (which might actually indicate an unintentional behavior so that pattern rather belongs into misc-* or bugprone-*).

[HerrCai0907]

For C++, we don't have convenient package manager, but similar, could we maintain several preset configurations in e.g. documents for different scenarios?

Isn't that just aliased checks with fixed configurations? Those already exist within clang-tidy.

But we cannot introduce suitable alias for each case in clang-tidy source code. This can affect binary size, compilation speed, and even runtime speed if not configured correctly.

Configuring alias for check itself alone does not help this problem. What I expected here is clang-tidy configuration available out of the box. Users only need to start with a configuration of corresponding strictness and make some fine-tuning according to their needs, rather than starting with a unified, hardcoded default configuration.

[vbvictor]

Personally I think it's better to be stricter by default. You get to see warnings in your code, and decide if they make sense for your project or not. If not, then you investigate which options are available to tune it to your liking.

I generally agree with making strict by default because It's easy to miss valuable options that are off-by-default.

could we maintain several preset configurations in e.g. documents for different scenarios?

I think we already have cert/cppcoreguidelines checks for this perpose. They were enough strict/useful to be in the list created by a committee. Otherwise I don't know how we can objectively measure "strictness" of check s. To me, if users don't want potential bugs in code, enable bugprone-* (with little adjustments) and so on. Maybe we shouldn't create a default config, but rather write a guide on how to better work with the tool.