[clang-tidy] `readability-redundant-casting` false positive on inheritance

[felix642]

The following code snippet will trigger readability-redundant-casting and suggests removing the static_cast. Doing so would lead to a compilation failure.

class A
{
    int a;
};

class B : public A
{

};

int main()
{
    A a;
    B b = static_cast<B>(a);  // [warning: redundant explicit casting to the same type 'B' as the sub-expression, remove this casting [readability-redundant-casting]]
}

Godbolt : https://godbolt.org/z/qY1v196qG

[dl8sd11]

Hi, I am new to clang-tidy and I would like to try on this issue :)

I think the RedundantCastingCheck checks the casting for pointers but for this example the casting is done on value type.

I try to add the example to the unit test but it gets a compiler error:

/usr/local/google/home/gcchen/Workspace/llvm-project/ninja-build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/redundant-casting.cpp.tmp.cpp:61:9: error: no matching conversion for static_cast from 'A' to 'B' [clang-diagnostic-error]
   61 |   B b = static_cast<B>(value);

I found the code cannot compile on the c++17 standard but it can compile on c++20.

cmake -S llvm -B ninja-build -G Ninja -DCMAKE_BUILD_TYPE=Debug
cd ninja-build
cmake -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -GNinja .
ninja check-clang-tools

Do you know how to compile it with the c++20 standard?

BTW, I am also thinking when this kind of casting could be useful. (why is it allowed by c++20)

[felix642]

Hi @dl8sd11, please have a look at the file llvm-project/clang-tools-extra/test/clang-tidy/make-unique-inaccessible-ctors.cpp for an example that uses both C++14/17 and C++20.

[dl8sd11]

Thanks @felix642, setting std=c++20-or-later compiles this test.

I found the matcher of RedundantCastingCheck a little confusing.

In clang-query:

m explicitCastExpr(
    hasSourceExpression(
        expr().bind("source")
    )
)

void testCastingToBaseClass(A value) {
  B b = static_cast<B>(value);
}

void testDiffrentTypesCast(A& value) {
  B& a7 = static_cast<B&>(value);
}

The hasSourceExpression cannot bind the correct node for the non-reference casting.

[<source>:7:9: note: "source" binds here](javascript:;)
    7 |   B b = static_cast<B>(value);
      |         ^~~~~~~~~~~~~~~~~~~~~

[<source>:11:27: note: "source" binds here](javascript:;)
   11 |   B& a7 = static_cast<B&>(value);
      |                           ^~~~~

https://godbolt.org/z/oEGe7ajY3

Currently, I still don't really understand the matcher. I also found the AST of two castings are quite different.

[felix642]

@dl8sd11 I think you've partially found the issue already.
If you look at the cast on the aggregate, you can see that the AST matcher hasSourceExpression() will bind on the CXXParenListInitExpr rather than the CXXConstructExpr like the other cast.
The CXXParenListInitExpr was added fairly recently with the paper p0960r3 in C++20.

This means that the issue is related to initialization of aggregates with a parenthesized list of values as hinted by the following code :

struct C {
    int a;
    int b;
};

void nonRedondantCast()
{
    C c = static_cast<C>(0); // [warning: redundant explicit casting to the same type 'C' as the sub-expression, remove this casting]
}

https://godbolt.org/z/Wb9PPen13

My guess is that the check should not warn if the sourceExpr is a CXXParenListInitExpr, but I'm not completely sure. There might be some cases where we need to display a warning, I just can't think of any right now.

If that's the case we should do something similar to what the check already does with initListExpr where only a few cases will display a warning.

[dl8sd11]

Hi @felix642,

Thanks for your guide!

I've created a fix for this, and this is my first PR on the project. Feel free to give me some suggestions.