This fixes a bug in clang-tidy (PR#37902), even though the code for the cppcoreguidelines-pro-type-member-init check itself is perfectly fine. It uses the isDelegatingConstructor() matcher, which does not match delegating constructors in templates because the AST contains incorrect information (constructors in templates are never marked as delegating). This patch fixes this behavior by constructing a correct CXXCtorInitializer for delegating constructors in templates.
Amazing, thanks a lot for fixing this long-standing issue! I have a couple comments. I'm not familiar at all with the Sema part so someone that knows should look at it :)
Not really sure what this test is meant to do. Why would it call the destructor of the own class in it's own constructor? Looks very strange to me.
If anything, the constructor should call the constructor of the base:
PositiveSelfInitialization() : NegativeAggregateType()
What do you think?
Not sure if it makes a difference
Could you add another test where X is initialized via NSDMI instead of in the constructor initializer list?
Thanks for your review @carlosgalvezp! I have addressed your inline comments. I'm not sure about the failing test, but it looks like a regression test internal to libFuzzer? Maybe it goes away with the rebuild.
The comment above talks about a "pathological template", so my guess is that this checks that clang-tidy doesn't crash for this input. The only reason why I had to touch this test at all is that the constructor is now treated as a delegating constructor, which suppresses the warning.
Hmm, I see. I would like to make sure we still catch the failure mode "this constructor does not initialize these base classes" for class templates.
I don't see such test existing (only for non-template classes), maybe you can add that too?
Thanks for the finding and sorry for delaying the review, I didn't know that :( Do you know if there's an option for signaling "LGTM but I want someone else to review?". Otherwise I can just leave a comment and don't accept the revision.
There is an action "resign from review", but as I have taken you off explicitly and by force, no need for that. 🙂 I just wrote the comment FYI so that you know I'm not just interfering destructively! (And due to no accepting reviewers remaining acting as such, the patch automatically went back to "now require review to proceed".)
Looking at the source for BuildDelegatingInitializer(), it looks like we should still be able to call this in a dependent context. In fact, it has a fixme comment specifically about that:
// If we are in a dependent context, template instantiation will // perform this type-checking again. Just save the arguments that we // received in a ParenListExpr. // FIXME: This isn't quite ideal, since our ASTs don't capture all // of the information that we have about the base // initializer. However, deconstructing the ASTs is a dicey process, // and this approach is far more likely to get the corner cases right.
I'm wondering if the better fix here is to hoist the delegation check out of the if (!Dependent). Did you try that and run into issues?
Can you explain why this change was needed?
Yes, I have tried this, and it leads to a crash here (because the cast fails). Apparently, this code path in BuildDelegatingInitializer() is never exercised for dependent contexts, despite the comment, and therefore doesn't work.
Without this, clang crashes because LookupDestructor() calls LookupSpecialMember(), which then runs into this assertion:
clang-14: /[...]/llvm-project/clang/lib/Sema/SemaLookup.cpp:3064: clang::Sema::SpecialMemberOverloadResult clang::Sema::LookupSpecialMember(clang::CXXRecordDecl*, clang::Sema::CXXSpecialMember, bool, bool, bool, bool, bool): Assertion `CanDeclareSpecialMemberFunction(RD) && "doing special member lookup into record that isn't fully complete"' failed.
Previously, this wasn't necessary because no delegating constructors would be generated for templates, and so this function wasn't ever called in a dependent context.