[clang-tidy] Fix cppcoreguidelines-pro-type-member-init false negatives

[malcolm.parsons]

Summary

Handle classes with default constructors that are defaulted or are not
present in the AST.
Classes with virtual methods or virtual bases are not trivially
default constructible, so their members and bases need to be initialized.

[malcolm.parsons]

Rebase

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

307

Please add an explanatory string literal to the assert (that way, if it triggers, some more information is immediately available).

372–374

Should also update the comment.

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

401

Can you also add a test like:

template <typename T>
struct S {
  T Val;
  virtual ~S() = default;
};

template struct S<int>;

[malcolm.parsons]

Don't match template instantiations.
Add assert message.
Update macro comment.
Add tests for templates and macros.

[aaron.ballman]

Generally looks good to me, sorry I missed these tiny nits last time around.

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h

49

Since this is not allowed to be a null pointer, would it make more sense to use a reference instead (same below)?

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

409

Sorry, I missed one bit to my test (my fault), can you also add int F; to ensure that it still triggers the diagnostic?

[malcolm.parsons]

Pass records by reference.
Change test to trigger diagnostic for non-template type

[aaron.ballman]

LGTM, thank you!

[malcolm.parsons]

In D24965#560785, @aaron.ballman wrote:

LGTM, thank you!

I don't have commit access, so please commit this for me.
Thanks.

[aaron.ballman]

I've commit in r283224.

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h

 private:
   // Checks Type.6 part 1:
   // Issue a diagnostic for any constructor of a non-trivially-constructible
   // type that does not initialize all member variables.
   //
   // To fix: Write a data member initializer, or mention it in the member
   // initializer list.
   void checkMissingMemberInitializer(ASTContext &Context,
+                                     const CXXRecordDecl &ClassDecl,
                                      const CXXConstructorDecl *Ctor);
 
   // A subtle side effect of Type.6 part 2:
   // Make sure to initialize trivially constructible base classes.
   void checkMissingBaseClassInitializer(const ASTContext &Context,
+                                        const CXXRecordDecl &ClassDecl,
                                         const CXXConstructorDecl *Ctor);
 
   // Checks Type.6 part 2:
   // Issue a diagnostic when constructing an object of a trivially constructible
   // type without () or {} to initialize its members.
   //
   // To fix: Add () or {}.
   void checkUninitializedTrivialType(const ASTContext &Context,

clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

 
 namespace {
 
 // Iterate over all the fields in a record type, both direct and indirect (e.g.
 // if the record contains an anonmyous struct). If OneFieldPerUnion is true and
 // the record type (or indirect field) is a union, forEachField will stop after
 // the first field.
 template <typename T, typename Func>
-void forEachField(const RecordDecl *Record, const T &Fields,
+void forEachField(const RecordDecl &Record, const T &Fields,
                   bool OneFieldPerUnion, Func &&Fn) {
   for (const FieldDecl *F : Fields) {
     if (F->isAnonymousStructOrUnion()) {
       if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
-        forEachField(R, R->fields(), OneFieldPerUnion, Fn);
+        forEachField(*R, R->fields(), OneFieldPerUnion, Fn);
     } else {
       Fn(F);
     }
 
-    if (OneFieldPerUnion && Record->isUnion())
+    if (OneFieldPerUnion && Record.isUnion())
       break;
   }
 }
 
 void removeFieldsInitializedInBody(
     const Stmt &Stmt, ASTContext &Context,
     SmallPtrSetImpl<const FieldDecl *> &FieldDecls) {
   auto Matches =
if (const T *D = dyn_cast<T>(*Decl)) {
         Insertions.back().Initializers.emplace_back(getName(D));
     }
   }
   return Insertions;
 }
 
 // Gets the list of bases and members that could possibly be initialized, in
 // order as they appear in the class declaration.
-void getInitializationsInOrder(const CXXRecordDecl *ClassDecl,
+void getInitializationsInOrder(const CXXRecordDecl &ClassDecl,
                                SmallVectorImpl<const NamedDecl *> &Decls) {
   Decls.clear();
-  for (const auto &Base : ClassDecl->bases()) {
+  for (const auto &Base : ClassDecl.bases()) {
     // Decl may be null if the base class is a template parameter.
     if (const NamedDecl *Decl = getCanonicalRecordDecl(Base.getType())) {
       Decls.emplace_back(Decl);
     }
   }
-  forEachField(ClassDecl, ClassDecl->fields(), false,
+  forEachField(ClassDecl, ClassDecl.fields(), false,
                [&](const FieldDecl *F) { Decls.push_back(F); });
 }
 
 template <typename T>
 void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag,
                         const CXXConstructorDecl *Ctor,
                         const SmallPtrSetImpl<const T *> &DeclsToInit) {
   // Do not propose fixes in macros since we cannot place them correctly.
   if (Ctor->getLocStart().isMacroID())
     return;
 
   SmallVector<const NamedDecl *, 16> OrderedDecls;
-  getInitializationsInOrder(Ctor->getParent(), OrderedDecls);
+  getInitializationsInOrder(*Ctor->getParent(), OrderedDecls);
 
   for (const auto &Insertion :
        computeInsertions(Ctor->inits(), OrderedDecls, DeclsToInit)) {
     if (!Insertion.Initializers.empty())
       Diag << FixItHint::CreateInsertion(Insertion.getLocation(Context, *Ctor),
                                          Insertion.codeToInsert());
   }
 }
auto IsNonTrivialDefaultConstructor = allOf(
       isDefaultConstructor(), unless(isUserProvided()),
       hasParent(cxxRecordDecl(unless(isTriviallyDefaultConstructible()))));
   Finder->addMatcher(
       cxxConstructorDecl(isDefinition(),
                          anyOf(IsUserProvidedNonDelegatingConstructor,
                                IsNonTrivialDefaultConstructor))
           .bind("ctor"),
       this);
+
+  // Match classes with a default constructor that is defaulted or is not in the
+  // AST.
+  Finder->addMatcher(
+      cxxRecordDecl(
+          isDefinition(), unless(isInstantiated()),
+          anyOf(has(cxxConstructorDecl(isDefaultConstructor(), isDefaulted(),
+                                       unless(isImplicit()))),
+                unless(has(cxxConstructorDecl()))),
+          unless(isTriviallyDefaultConstructible()))
+          .bind("record"),
+      this);
+
   auto HasDefaultConstructor = hasInitializer(
       cxxConstructExpr(unless(requiresZeroInitialization()),
                        hasDeclaration(cxxConstructorDecl(
                            isDefaultConstructor(), unless(isUserProvided())))));
   Finder->addMatcher(
       varDecl(isDefinition(), HasDefaultConstructor,
               hasAutomaticStorageDuration(),
               hasType(recordDecl(has(fieldDecl()),
                                  isTriviallyDefaultConstructible())))
           .bind("var"),
       this);
 }
 
 void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor")) {
     // Skip declarations delayed by late template parsing without a body.
     if (!Ctor->getBody())
       return;
-    checkMissingMemberInitializer(*Result.Context, Ctor);
-    checkMissingBaseClassInitializer(*Result.Context, Ctor);
+    checkMissingMemberInitializer(*Result.Context, *Ctor->getParent(), Ctor);
+    checkMissingBaseClassInitializer(*Result.Context, *Ctor->getParent(), Ctor);
+  } else if (const auto *Record =
+                 Result.Nodes.getNodeAs<CXXRecordDecl>("record")) {
+    assert(Record->hasDefaultConstructor() &&
+           "Matched record should have a default constructor");
+    checkMissingMemberInitializer(*Result.Context, *Record, nullptr);
+    checkMissingBaseClassInitializer(*Result.Context, *Record, nullptr);
   } else if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var")) {
     checkUninitializedTrivialType(*Result.Context, Var);
   }
 }
 
 void ProTypeMemberInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreArrays", IgnoreArrays);
 }
 
 void ProTypeMemberInitCheck::checkMissingMemberInitializer(
-    ASTContext &Context, const CXXConstructorDecl *Ctor) {
-  const CXXRecordDecl *ClassDecl = Ctor->getParent();
-  bool IsUnion = ClassDecl->isUnion();
+    ASTContext &Context, const CXXRecordDecl &ClassDecl,
+    const CXXConstructorDecl *Ctor) {
+  bool IsUnion = ClassDecl.isUnion();
 
-  if (IsUnion && ClassDecl->hasInClassInitializer())
+  if (IsUnion && ClassDecl.hasInClassInitializer())
     return;
 
   // Gather all fields (direct and indirect) that need to be initialized.
   SmallPtrSet<const FieldDecl *, 16> FieldsToInit;
-  forEachField(ClassDecl, ClassDecl->fields(), false, [&](const FieldDecl *F) {
+  forEachField(ClassDecl, ClassDecl.fields(), false, [&](const FieldDecl *F) {
     if (!F->hasInClassInitializer() &&
         utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
                                                             Context))
       FieldsToInit.insert(F);
   });
   if (FieldsToInit.empty())
     return;
 
+  if (Ctor) {
     for (const CXXCtorInitializer *Init : Ctor->inits()) {
       // Remove any fields that were explicitly written in the initializer list
       // or in-class.
       if (Init->isAnyMemberInitializer() && Init->isWritten()) {
         if (IsUnion)
           return; // We can only initialize one member of a union.
         FieldsToInit.erase(Init->getAnyMember());
       }
     }
     removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
+  }
 
   // Collect all fields in order, both direct fields and indirect fields from
   // anonmyous record types.
   SmallVector<const FieldDecl *, 16> OrderedFields;
-  forEachField(ClassDecl, ClassDecl->fields(), false,
+  forEachField(ClassDecl, ClassDecl.fields(), false,
                [&](const FieldDecl *F) { OrderedFields.push_back(F); });
 
   // Collect all the fields we need to initialize, including indirect fields.
   SmallPtrSet<const FieldDecl *, 16> AllFieldsToInit;
   forEachField(ClassDecl, FieldsToInit, false,
                [&](const FieldDecl *F) { AllFieldsToInit.insert(F); });
   if (AllFieldsToInit.empty())
     return;
 
   DiagnosticBuilder Diag =
-      diag(Ctor->getLocStart(),
+      diag(Ctor ? Ctor->getLocStart() : ClassDecl.getLocation(),
            IsUnion
                ? "union constructor should initialize one of these fields: %0"
                : "constructor does not initialize these fields: %0")
       << toCommaSeparatedString(OrderedFields, AllFieldsToInit);
 
-  // Do not propose fixes in macros since we cannot place them correctly.
-  if (Ctor->getLocStart().isMacroID())
+  // Do not propose fixes for constructors in macros since we cannot place them
+  // correctly.
+  if (Ctor && Ctor->getLocStart().isMacroID())
     return;
 
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet<const FieldDecl *, 16> FieldsToFix;
   forEachField(ClassDecl, FieldsToInit, true, [&](const FieldDecl *F) {
     // Don't suggest fixes for enums because we don't know a good default.
     if (!F->getType()->isEnumeralType())
       FieldsToFix.insert(F);
   });
   if (FieldsToFix.empty())
     return;
 
   // Use in-class initialization if possible.
   if (Context.getLangOpts().CPlusPlus11) {
     for (const FieldDecl *Field : FieldsToFix) {
       Diag << FixItHint::CreateInsertion(
           getLocationForEndOfToken(Context, Field->getSourceRange().getEnd()),
           "{}");
     }
-  } else {
+  } else if (Ctor) {
     // Otherwise, rewrite the constructor's initializer list.
     fixInitializerList(Context, Diag, Ctor, FieldsToFix);
   }
 }
 
 void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(
-    const ASTContext &Context, const CXXConstructorDecl *Ctor) {
-  const CXXRecordDecl *ClassDecl = Ctor->getParent();
+    const ASTContext &Context, const CXXRecordDecl &ClassDecl,
+    const CXXConstructorDecl *Ctor) {
 
   // Gather any base classes that need to be initialized.
   SmallVector<const RecordDecl *, 4> AllBases;
   SmallPtrSet<const RecordDecl *, 4> BasesToInit;
-  for (const CXXBaseSpecifier &Base : ClassDecl->bases()) {
+  for (const CXXBaseSpecifier &Base : ClassDecl.bases()) {
     if (const auto *BaseClassDecl = getCanonicalRecordDecl(Base.getType())) {
       AllBases.emplace_back(BaseClassDecl);
       if (!BaseClassDecl->field_empty() &&
           utils::type_traits::isTriviallyDefaultConstructible(Base.getType(),
                                                               Context))
         BasesToInit.insert(BaseClassDecl);
     }
   }
 
   if (BasesToInit.empty())
     return;
 
   // Remove any bases that were explicitly written in the initializer list.
+  if (Ctor) {
     for (const CXXCtorInitializer *Init : Ctor->inits()) {
       if (Init->isBaseInitializer() && Init->isWritten())
         BasesToInit.erase(Init->getBaseClass()->getAsCXXRecordDecl());
     }
+  }
 
   if (BasesToInit.empty())
     return;
 
   DiagnosticBuilder Diag =
-      diag(Ctor->getLocStart(),
+      diag(Ctor ? Ctor->getLocStart() : ClassDecl.getLocation(),
            "constructor does not initialize these bases: %0")
       << toCommaSeparatedString(AllBases, BasesToInit);
 
+  if (Ctor)
       fixInitializerList(Context, Diag, Ctor, BasesToInit);
 }
 
 void ProTypeMemberInitCheck::checkUninitializedTrivialType(
     const ASTContext &Context, const VarDecl *Var) {
   DiagnosticBuilder Diag =
       diag(Var->getLocStart(), "uninitialized record type: %0") << Var;
 
   Diag << FixItHint::CreateInsertion(
       getLocationForEndOfToken(Context, Var->getSourceRange().getEnd()),
       Context.getLangOpts().CPlusPlus11 ? "{}" : " = {}");
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tidy/utils/TypeTraits.cpp

bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
   const auto *ClassDecl = dyn_cast<CXXRecordDecl>(&RecordDecl);
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
     return true;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())
     return false;
+  // A polymorphic class is not trivially constructible
+  if (ClassDecl->isPolymorphic())
+    return false;
   // A class is trivially constructible if it has a trivial default constructor.
   if (ClassDecl->hasTrivialDefaultConstructor())
     return true;
 
   // If all its fields are trivially constructible and have no default initializers.
   for (const FieldDecl *Field : ClassDecl->fields()) {
     if (Field->hasInClassInitializer())
       return false;
     if (!isTriviallyDefaultConstructible(Field->getType(), Context))
       return false;
   }
   // If all its direct bases are trivially constructible.
   for (const CXXBaseSpecifier &Base : ClassDecl->bases()) {
     if (!isTriviallyDefaultConstructible(Base.getType(), Context))
       return false;
+    if (Base.isVirtual())
+      return false;
   }
 
   return true;
 }
 
 // Based on QualType::isTrivial.
 bool isTriviallyDefaultConstructible(QualType Type,
                                      const ASTContext &Context) {

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

class PositiveIndirectMember {
   PositiveIndirectMember() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A
 };
 
 void Bug30487()
 {
   NegativeInClassInitializedDefaulted s;
 }
+
+struct PositiveVirtualMethod {
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+  int F;
+  // CHECK-FIXES: int F{};
+  virtual int f() = 0;
+};
+
+struct PositiveVirtualDestructor {
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+  PositiveVirtualDestructor() = default;
+  int F;
+  // CHECK-FIXES: int F{};
+  virtual ~PositiveVirtualDestructor() {}
+};
+
+struct PositiveVirtualBase : public virtual NegativeAggregateType {
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these bases: NegativeAggregateType
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: constructor does not initialize these fields: F
+  int F;
+  // CHECK-FIXES: int F{};
+};
+
+template <typename T>
+struct PositiveTemplateVirtualDestructor {
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: constructor does not initialize these fields: F
+  T Val;
+  int F;
+  // CHECK-FIXES: int F{};
+  virtual ~PositiveTemplateVirtualDestructor() = default;
+};
+
+template struct PositiveTemplateVirtualDestructor<int>;
+
+#define UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(FIELD) \
+  struct UninitializedFieldVirtual##FIELD {              \
+    int FIELD;                                           \
+    virtual ~UninitializedFieldVirtual##FIELD() {}       \
+  };                                                     \
+// Ensure FIELD is not initialized since fixes inside of macros are disabled.
+// CHECK-FIXES: int FIELD;
+
+UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(F);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F
+UNINITIALIZED_FIELD_IN_MACRO_BODY_VIRTUAL(G);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: G