clang-tidy: readability-implicit-bool-conversion not working with std::optional

[Amaroker]

I am using llvm-13.0.1

I would expect this to trigger a warning:

std::optional<int> x = false;

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[huangperry]

I'd like to give this a try. Could someone point me in the right direction as to which file under clang-tools-extra/clang-tidy I should look at?

[EugeneZelenko]

@huangperry: readability/ImplicitBoolConversionCheck.cpp.

[JonasToth]

I'd like to give this a try. Could someone point me in the right direction as to which file under clang-tools-extra/clang-tidy I should look at?

Additionally, tests for the clang-tidy checks live here:

Code:
/home/jonas/software/llvm-project/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
/home/jonas/software/llvm-project/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h

Tests:
clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-allow-in-conditions.cpp
clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.cpp
clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-cxx98.cpp

Running the readability tests:
cmake --build . --target check-clang-extra-clang-tidy-checkers-readability
or
ninja check-clang-extra-clang-tidy-checkers-readability

• matcher reference: https://clang.llvm.org/docs/LibASTMatchersReference.html
• you can use clang-query to experiment with matchers
• godbolt.org can be used for matching (select clang -> "Add Tool" -> clang-query)

[huangperry]

Thanks for the tips! I'm following this tutorial for contributing: https://llvm.org/docs/MyFirstTypoFix.html

[JonasToth]

Additionally: https://clang.llvm.org/extra/clang-tidy/Contributing.html
This page contains many tips and pointers to more resources specifically to develop clang-tidy.
Most of it is for a new check from scratch, but that might help understand how everything fits together as well.

I'd recommend something along those lines:

1. make whitespace change in check-code + break a test -> compile & test cycle works
2. figure out the matching stuff. For me it was a bit magic at first :) Godbolt helps out a lot!
3. add the reproducer to the test cases + the expected CHECK-MESSAGES
4. find a way to change the matcher (i think the fix only involves adjusting the matcher, could be more though!)

If you have specific questions, just ask :)

[huangperry]

There's a lot to learn about the AST and matchers for sure, but I think this is a great issue to start learning about checks.

[huangperry]

std::optional<int> value = false;
The following is the AST for the implicit bool conversion above:

-VarDecl <line:3:1, col:28> col:20 value 'std::optional<int>':'std::optional<int>' cinit
  -ExprWithCleanups <col:28> 'std::optional<int>':'std::optional<int>'
    -ImplicitCastExpr <col:28> 'std::optional<int>':'std::optional<int>' <ConstructorConversion>
      -CXXConstructExpr <col:28> 'std::optional<int>':'std::optional<int>' 'void (bool &&) noexcept(is_nothrow_constructible_v<int, bool>)'
        -MaterializeTemporaryExpr <col:28> 'bool' xvalue
          -CXXBoolLiteralExpr <col:28> 'bool' false

Unlike most of the "FromBool" matchers in ImplicitBoolConversionCheck.cpp, the ImplicitCastExpr node here doesn't register as a Bool -> Integer cast kind. Seems like it's converting std::optional to std::optional. Should I try to match for CXXConstructExpr where RHS is cxxBoolLiteralExpr?

[JonasToth]

I think the key here is the MaterializeTemporaryExpr that makes the conversion from bool to int possible. Then the CXXConstructExpr (that has a reference to bool) binds to the temporary int.
https://clang.llvm.org/doxygen/classclang_1_1MaterializeTemporaryExpr.html

Example Matching

The boolean conversion cases should be all the same as already matched. I think the current matcher of the check simply does not consider the MaterializeTemporaryExpr as well.
My guess is, that the ImplicitCastFromBool needs to be extended to detect casts in MaterializeTemporaryExpr.

For tests, the case std::optional<bool> v = 0.0; should be very similar in nature. And assignments that induce these weirder conversions need to be tested as well.

[njames93]

The key here is that optional has a templated constructor which will accept a bool. This will do the conversion in the constructor rather than at the call site. In order to detect this you'd need to actually detect a construct of optional with a bool.