[clang][Sema] Create delegating constructors even in templates

[fwolff]

Summary

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.

[carlosgalvezp]

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 :)

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp

374–375

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?

555

typename T?

Not sure if it makes a difference

559

Could you add another test where X is initialized via NSDMI instead of in the constructor initializer list?

int X{};
TemplateWithDelegatingCtor() = default;
TemplateWithDelegatingCtor(int) : TemplateWithDelegatingCtor() {}

[carlosgalvezp]

Also, what's with the pre-merge test, is it related?

[fwolff]

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.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp

374–375

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.

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp

374–375

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?

[fwolff]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp

374–375

Good point, done now.

[carlosgalvezp]

Looks great, thanks! I'm not comfortable reviewing the Sema part, maybe someone else can have a look?

[whisperity]

(@carlosgalvezp Removing you from reviewers so the patch shows up as not yet reviewed for others! The significant part is the Sema which should be reviewed.)

[carlosgalvezp]

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.

[whisperity]

In D113518#3155040, @carlosgalvezp wrote:

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".)

[aaron.ballman]

clang/lib/Sema/SemaDeclCXX.cpp

4524–4525

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?

5071–5075

Can you explain why this change was needed?

[fwolff]

clang/lib/Sema/SemaDeclCXX.cpp

4524–4525

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.

5071–5075

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.

[fwolff]

@aaron.ballman Ping? I think I've responded to all comments so far; let me know if you still have concerns.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp

 };
 
 // This pathological template fails to compile if actually instantiated. It
 // results in the check seeing a null RecordDecl when examining the base class
 // initializer list.
 template <typename T>
 class PositiveSelfInitialization : NegativeAggregateType
 {
   PositiveSelfInitialization() : PositiveSelfInitialization() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these bases: NegativeAggregateType
-  // CHECK-FIXES: PositiveSelfInitialization() : NegativeAggregateType(), PositiveSelfInitialization() {}
 };
 
 class PositiveIndirectMember {
   struct {
     int *A;
     // CHECK-FIXES: int *A{};
   };
 
   PositiveIndirectMember() {}
union {
       };
       int C;
       // CHECK-FIXES: int C{};
     };
   };
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+// Check that a delegating constructor in a template does not trigger false positives (PR#37902).
+template <typename T>
+struct TemplateWithDelegatingCtor {
+  int X;
+  TemplateWithDelegatingCtor() : X{} {};
+  TemplateWithDelegatingCtor(int) : TemplateWithDelegatingCtor(){};
+};
+
+template <typename T>
+struct TemplateWithDelegatingCtorNSDMI {
+  int X{};
+  TemplateWithDelegatingCtorNSDMI() = default;
+  TemplateWithDelegatingCtorNSDMI(int) : TemplateWithDelegatingCtorNSDMI() {}
+};

clang/lib/Sema/SemaDeclCXX.cpp

Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
   //   mem-initializer-list can initialize a base class using any
   //   name that denotes that base class type.
 
   // We can store the initializers in "as-written" form and delay analysis until
   // instantiation if the constructor is dependent. But not for dependent
   // (broken) code in a non-template! SetCtorInitializers does not expect this.
   bool Dependent = CurContext->isDependentContext() &&
                    (BaseType->isDependentType() || Init->isTypeDependent());
+  bool Delegating = Context.hasSameUnqualifiedType(
+      QualType(ClassDecl->getTypeForDecl(), 0), BaseType);
 
   SourceRange InitRange = Init->getSourceRange();
   if (EllipsisLoc.isValid()) {
     // This is a pack expansion.
     if (!BaseType->containsUnexpandedParameterPack())  {
       Diag(EllipsisLoc, diag::err_pack_expansion_without_parameter_packs)
         << SourceRange(BaseLoc, InitRange.getEnd());
 
       EllipsisLoc = SourceLocation();
     }
   } else {
     // Check for any unexpanded parameter packs.
     if (DiagnoseUnexpandedParameterPack(BaseLoc, BaseTInfo, UPPC_Initializer))
       return true;
 
     if (DiagnoseUnexpandedParameterPack(Init, UPPC_Initializer))
       return true;
   }
 
   // Check for direct and virtual base classes.
   const CXXBaseSpecifier *DirectBaseSpec = nullptr;
   const CXXBaseSpecifier *VirtualBaseSpec = nullptr;
   if (!Dependent) {
-    if (Context.hasSameUnqualifiedType(QualType(ClassDecl->getTypeForDecl(),0),
-                                       BaseType))
+    if (Delegating)
       return BuildDelegatingInitializer(BaseTInfo, Init, ClassDecl);
 
     FindBaseInitializer(*this, ClassDecl, BaseType, DirectBaseSpec,
                         VirtualBaseSpec);
 
     // C++ [base.class.init]p2:
     // Unless the mem-initializer-id names a nonstatic data member of the
     // constructor's class or a direct or virtual base of that class, the
     // mem-initializer is ill-formed.
if (!DirectBaseSpec && !VirtualBaseSpec) {
           << BaseType << Context.getTypeDeclType(ClassDecl)
           << BaseTInfo->getTypeLoc().getLocalSourceRange();
     }
   }
 
   if (Dependent) {
     DiscardCleanupsInEvaluationContext();
 
-    return new (Context) CXXCtorInitializer(Context, BaseTInfo,
-                                            /*IsVirtual=*/false,
-                                            InitRange.getBegin(), Init,
-                                            InitRange.getEnd(), EllipsisLoc);
+    if (!Delegating)
+      return new (Context)
+          CXXCtorInitializer(Context, BaseTInfo,
+                             /*IsVirtual=*/false, InitRange.getBegin(), Init,
+    else
+      return new (Context) CXXCtorInitializer(
+          Context, BaseTInfo, InitRange.getBegin(), Init, InitRange.getEnd());
   }
 
   // C++ [base.class.init]p2:
   //   If a mem-initializer-id is ambiguous because it designates both
   //   a direct non-virtual base class and an inherited virtual base
   //   class, the mem-initializer is ill-formed.
   if (DirectBaseSpec && VirtualBaseSpec)
     return Diag(BaseLoc, diag::err_base_init_direct_and_virtual)
Sema::SetDelegatingInitializer(CXXConstructorDecl *Constructor,
                                CXXCtorInitializer *Initializer) {
   assert(Initializer->isDelegatingInitializer());
   Constructor->setNumCtorInitializers(1);
   CXXCtorInitializer **initializer =
     new (Context) CXXCtorInitializer*[1];
   memcpy(initializer, &Initializer, sizeof (CXXCtorInitializer*));
   Constructor->setCtorInitializers(initializer);
 
+  if (!Constructor->isDependentContext()) {
     if (CXXDestructorDecl *Dtor = LookupDestructor(Constructor->getParent())) {
       MarkFunctionReferenced(Initializer->getSourceLocation(), Dtor);
       DiagnoseUseOfDecl(Dtor, Initializer->getSourceLocation());
     }
 
     DelegatingCtorDecls.push_back(Constructor);
 
     DiagnoseUninitializedFields(*this, Constructor);
+  }
 
   return false;
 }
 
 bool Sema::SetCtorInitializers(CXXConstructorDecl *Constructor, bool AnyErrors,
                                ArrayRef<CXXCtorInitializer *> Initializers) {
   if (Constructor->isDependentContext()) {
     // Just store the initializers as written, they will be checked during