1478840 - Should implicit conversions from bool be warnings/errors? clang-tidy readability-implicit-bool-cast

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I would like to think that nowadays, `bool`s are only used as just boolean values and should rarely be converted to integer values (as the C++98 standard allows, for historic reasons [0]).

The main use case I can think of is within debugging printf's, e.g.: `printf("b=%d", b);`.
But of course it's just a hunch, and I don't know if we have other valid uses for these conversions.

However I think it would be worth considering flagging these implicit conversions to avoid potential bugs, such as typos like `unsigned x = 1u < y;` where '<' should have been '<<' [1] -- which Rust would have found.
And wherever we really need to convert from bool, make these conversions explicit. E.g., the above printf would become `printf("b=%d", int(b))`. Or maybe `bool` is not the best type in some situations?

If writing a static analysis check for implicit conversions from bool is easy enough, I think it would be worth running it once, to find all cases in our code, and then decide if it's actually possible&useful to make it permanent and update our code accordingly.


[0] 11.7.2 at https://books.google.com.au/books?id=hS9mDwAAQBAJ&lpg=PT303&ots=xNjd4n5-Xh&dq=%22defining%20a%20Boolean%20enumeration%20would%20break%22&pg=PT302#v=onepage&q&f=false
[1] https://twitter.com/nnethercote/status/1022422804937863168

Comment 1

[Chris Peterson [:cpeterson]]

clang-tidy has a "readability-implicit-bool-conversion" check for implicit conversions of:

- integer expression/literal to boolean
- float expression/literal to boolean
- pointer/nullptr/NULL to boolean
- boolean expression/literal to integer
- boolean expression/literal to float

https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html

This check is probably very noisy but manageable because our clang-tidy review bot only reports new warnings. (IIUC, it diffs the list of warnings emitted before and after a review request's patches are applied.)

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thanks for the tip, Chris. I wish it had more fine-grained options.

It may be worth trying it locally (with the "AllowPointerConditions", as our style guide pretty much enforces the use of pointer->bool conversions), and see how much of each conversion we actually use.
I'll explore further if I find the time; if someone else is interested, please go ahead!

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Gerald Squelart [:gerald] from comment #2)
> It may be worth trying it locally (with the "AllowPointerConditions", as our
> style guide pretty much enforces the use of pointer->bool conversions), and
> see how much of each conversion we actually use.

Andi, is there a way to pass options to checks in clang-tidy/config.yaml, such as readability-implicit-bool-conversion's AllowPointerConditions option?

https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html#cmdoption-arg-allowpointerconditions

> I'll explore further if I find the time; if someone else is interested,
> please go ahead!

btw, our currently-vendored version of clang-tidy (5.0.1) uses the check name "readability-implicit-bool-cast" instead of "readability-implicit-bool-conversion":

~/.mozbuild/clang-tools/clang/bin/clang-tidy -list-checks -checks="*" | grep implicit-bool

Comment 4

[Andi [:andi]]

Chris, we don't have this feature just yet, I've had this idea for a long time now, but I didn't know if it would be useful or not. With you asking about this it seems we should implement it.
In the mean time there is another method when dealing with this we can create a .clang-tidy configuration file in the root of our repo and put there the parameters for each checker. I would use this as a temporary solution till we have the configuration file method in place since I don't want to have separate places where we activate the checkers and where we configure them.
I've created Bug 1479298 in order to track this.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Regarding checker parameters: It would be nice of course, but I think in this case the checker is still too broad, as we only want to catch implicit conversions *from* bool, and maybe even subsets of these (see below). So we may still need to write our own smaller checker, and that wouldn't require parameters.


Weekend exploration:
I managed to run a local clang-tidy build on Mac, with readability-implicit-bool-cast (no options). This generated 2.1GB of logs after 2h40m (a normal build takes 40m on that 2013 4c/8t Macbook Pro). Some numbers:
- 4.1M warnings
  - 225k unique warnings
    - 139k are implicit casts *from* bool (including 76k in xptdata.cpp alone!).

At a glance, here are some use cases I've noticed, in no particular order:
A. enum constants assigned with false/true, [0]
B. using 'char' to hold a boolean value for space reasons (as bool can be bigger on some platforms) [1]
C. Adding/subtracting a bool to/from a counter [2]
D. Assigning a bool to a uint:1 field (a comment says "N.B. These are booleans but need to be uint32_t to pack correctly on MSVC." -- Is this (still) true?) [3]
E. Creating a 0/1 bit in a bitwise expression [4]
F. Simply using a non-bool as a bool (not sure why -- C compat?) [5]
G. Typedef of bool-like type [6]
There may be more.

Anyway, it would be a very big and thankless task if we wanted to fix all these!
And even if introducing a new checker would only complain about new warnings after that, seeing how much these conversions are used means that a lot of people may encounter some inconvenience when they keep using the same data structures and patterns.

So... Is it all worth it, just to be able to catch potential typos like '<' instead of '<<'?


[0] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/js/src/vm/Stack.h#103
[1] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/mfbt/Maybe.h#172
[2] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/gfx/2d/Types.h#489
[3] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/js/src/vm/JSScript.h#2274
[4] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/js/src/jit/CompactBuffer.h#157
[5] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/js/src/vm/RegExpStatics.h#43
[6] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/plugins/base/npapi.h#112

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Gerald Squelart [:gerald] from comment #5)
> At a glance, here are some use cases I've noticed, in no particular order:
> A. enum constants assigned with false/true, [0]
> B. using 'char' to hold a boolean value for space reasons (as bool can be
> bigger on some platforms) [1]
> C. Adding/subtracting a bool to/from a counter [2]
> D. Assigning a bool to a uint:1 field (a comment says "N.B. These are
> booleans but need to be uint32_t to pack correctly on MSVC." -- Is this
> (still) true?) [3]
> E. Creating a 0/1 bit in a bitwise expression [4]
> F. Simply using a non-bool as a bool (not sure why -- C compat?) [5]
> G. Typedef of bool-like type [6]
...
> So... Is it all worth it, just to be able to catch potential typos like '<'
> instead of '<<'?

Interesting findings! Besides these unusual uses of bool, did readability-implicit-bool-cast find any actual bugs? If not, then it's probably not worth the effort to write a new checker.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

> did readability-implicit-bool-cast find any actual bugs?

Assessing this would require going through about ~63,000 warnings! (All conversions from bool, outside of xptdata.cpp.)
I'm afraid this is a tough ask, unless we can somehow automatically filter out not-buggy-for-sure cases to reduce the list to a more manageable size.

It's possible that spending a few days on some of these warnings could remove a lot of them; e.g., one non-bool member variable may generate many warnings for every time it's converted to/from bool, etc. But I'm not sure this would be enough to help us find real bugs.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I really think readability-implicit-bool-conversion should be disabled.

At the very least, it runs afoul of some of our style guide recommendations:
"When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr"
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

This is also not ok:
> Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]
>
> if (!largestDim)
>    ~^~~~~~~~~~~
>     == 0u

It's also complaining for non-short-circuit `bool(a) & bool(b)` type logic, which is more useful/common in some areas than others.

This is coming from the deluge of warnings I got from this patch:
https://phabricator.services.mozilla.com/D8325

Yes, we only notify warnings when the code changes, but this code is fine, and I haven't seen any case where this warning is particularly useful. Implicit conversion to bool is just generally part of normal C++, and implicit conversion *from* bool is easy to understand as well.

As mentioned above, if there are cases we want to warn about, I think we should do a more specific job of checking for them, rather than using this false-positive-heavy approximation.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1478840 - Disable readability-implicit-bool-conversion in clang-tidy. -

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I see bug 1253844 introduced readability-implicit-bool-conversion, and bug 1479298 removed warnings for integer and pointer conversions, leaving only conversions from bool, which I advocated for in this bug.
So this bug is effectively fixed.


(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> I really think readability-implicit-bool-conversion should be disabled.

I think you should open a separate bug blocking bug 1253844 and/or bug 1479298. ;-)

I see you've added a patch, but it goes against the whole bug (title & initial comments), so I think it shouldn't be attached here. Also, it applies to an older config (pre-bug 1479298), so it can't land as-is anyway.


> At the very least, it runs afoul of some of our style guide recommendations:
> "When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr
> or myPtr == nullptr"
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#CC_practices
> 
> This is also not ok:
> > Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]
> >
> > if (!largestDim)
> >    ~^~~~~~~~~~~
> >     == 0u

These should not cause warnings (anymore), thanks to bug 1479298.

> It's also complaining for non-short-circuit `bool(a) & bool(b)` type logic,
> which is more useful/common in some areas than others.

I guess in this case, `&` performs integral promotion of its arguments.
This could be solved by doing an explicit conversion, e.g.: `int(bool(a)) & int(bool(b))`. It's more verbose of course, but it removes the doubt that this could be an error and the operator should have been `&&`.

Looking at your review (linked below), at least this one seems to be a useful warning:
`if ((srcBase <= dstLevel) & (dstLevel <= srcLast)) {`
Why would you not want `&&` there?
Or is that some kind of optimization, because the processor can do first+second+and quicker than it could do first+jump+second? In which case, I'd still argue that making the conversions explicit removes this doubt that maybe you intended `&&`. And you could trust the compiler to do the right thing: A quick godbolt test seems to indicate that both `&` and `&&` produce the same code in clang -O1. ;-)

> This is coming from the deluge of warnings I got from this patch:
> https://phabricator.services.mozilla.com/D8325

It looks like you were caught between bug 1253844 (naked readability-implicit-bool-conversion) and bug 1479298 (restricted to bool conversions). Painful indeed.
Would it be possible to re-run this review, maybe by submitting a new patch that fixes other issues?

As I said above, if you still think readability-implicit-bool-conversion (with bool-only restrictions) is still unwanted, please open a new bug to make your case.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Please know that I totally get your frustration at all those sudden warnings, and I'm sorry I'm added to it.
I'm hoping bug 1479298 should alleviate most of the pain. But if you still feel strongly about it, I do think a new bug is the proper way to go (instead of making this one here do the reverse of its original intent!)
Thank you & good luck.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Oh, godbolt is new to me, thanks! It does indeed to the right thing at real optimization levels, at least for simple cases.
For less trivial cases, optimization quality seems to diverge, but it's probably good enough to rely on.

I'll get a refresh of analysis checks when I update the patch, so hopefully I just got caught in a noisy intermediate state!

Comment 13

[Andi [:andi]]

bug 1479298 adds the following options to 'readability-implicit-bool-conversion':

 1. AllowIntegerConditions - The check will allow conditional integer conversions.
 2. AllowPointerConditions - The check will allow conditional pointer conversions.

Having this in place I'm sure we will drastically decrease the noise that was generated buy bug 1253844. 
Jeff, please feel free to open a new bug if you still thing that the analysis is overly cautious and n-i me directly. By no mean we intend to break our coding style and the practices that we have in place with these checkers.

Comment 14

[Jason Orendorff [:jorendorff]]

I'm still seeing these warnings. See bug 1500241.