Fix some false-positives with cppcoreguidelines-pro-type-member-init. 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.

Patch by Malcolm Parsons.

llvm-svn: 283224

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

@@ -32,17 +32,17 @@ namespace {
 // 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;
   }
 }
@@ -214,16 +214,16 @@ computeInsertions(const CXXConstructorDecl::init_const_range &Inits,
 
 // 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); });
 }
 
@@ -236,7 +236,7 @@ void fixInitializerList(const ASTContext &Context, DiagnosticBuilder &Diag,
     return;
 
   SmallVector<const NamedDecl *, 16> OrderedDecls;
-  getInitializationsInOrder(Ctor->getParent(), OrderedDecls);
+  getInitializationsInOrder(*Ctor->getParent(), OrderedDecls);
 
   for (const auto &Insertion :
        computeInsertions(Ctor->inits(), OrderedDecls, DeclsToInit)) {
@@ -269,6 +269,19 @@ void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) {
                                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(
@@ -287,8 +300,14 @@ void ProTypeMemberInitCheck::check(const MatchFinder::MatchResult &Result) {
     // Skip declarations delayed by late template parsing without a body.
     if (!Ctor->getBody())
       return;
-    checkMissingMemberInitializer(*Result.Context, Ctor);
+    checkMissingMemberInitializer(*Result.Context, *Ctor->getParent(), Ctor);
-    checkMissingBaseClassInitializer(*Result.Context, 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);
   }
@@ -299,16 +318,16 @@ void ProTypeMemberInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 }
 
 void ProTypeMemberInitCheck::checkMissingMemberInitializer(
-    ASTContext &Context, const CXXConstructorDecl *Ctor) {
+    ASTContext &Context, const CXXRecordDecl &ClassDecl,
-  const CXXRecordDecl *ClassDecl = Ctor->getParent();
+    const CXXConstructorDecl *Ctor) {
-  bool IsUnion = ClassDecl->isUnion();
+  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))
@@ -317,21 +336,23 @@ void ProTypeMemberInitCheck::checkMissingMemberInitializer(
   if (FieldsToInit.empty())
     return;
 
-  for (const CXXCtorInitializer *Init : Ctor->inits()) {
+  if (Ctor) {
-    // Remove any fields that were explicitly written in the initializer list
+    for (const CXXCtorInitializer *Init : Ctor->inits()) {
-    // or in-class.
+      // Remove any fields that were explicitly written in the initializer list
-    if (Init->isAnyMemberInitializer() && Init->isWritten()) {
+      // or in-class.
-      if (IsUnion)
+      if (Init->isAnyMemberInitializer() && Init->isWritten()) {
-        return; // We can only initialize one member of a union.
+        if (IsUnion)
-      FieldsToInit.erase(Init->getAnyMember());
+          return; // We can only initialize one member of a union.
+        FieldsToInit.erase(Init->getAnyMember());
+      }
     }
+    removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
   }
-  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.
@@ -342,14 +363,15 @@ void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     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.
+  // Do not propose fixes for constructors in macros since we cannot place them
-  if (Ctor->getLocStart().isMacroID())
+  // correctly.
+  if (Ctor && Ctor->getLocStart().isMacroID())
     return;
 
   // Collect all fields but only suggest a fix for the first member of unions,
@@ -370,20 +392,20 @@ void ProTypeMemberInitCheck::checkMissingMemberInitializer(
           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 ASTContext &Context, const CXXRecordDecl &ClassDecl,
-  const CXXRecordDecl *ClassDecl = Ctor->getParent();
+    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() &&
@@ -397,20 +419,23 @@ void ProTypeMemberInitCheck::checkMissingBaseClassInitializer(
     return;
 
   // Remove any bases that were explicitly written in the initializer list.
-  for (const CXXCtorInitializer *Init : Ctor->inits()) {
+  if (Ctor) {
-    if (Init->isBaseInitializer() && Init->isWritten())
+    for (const CXXCtorInitializer *Init : Ctor->inits()) {
-      BasesToInit.erase(Init->getBaseClass()->getAsCXXRecordDecl());
+      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);
 
-  fixInitializerList(Context, Diag, Ctor, BasesToInit);
+  if (Ctor)
+      fixInitializerList(Context, Diag, Ctor, BasesToInit);
 }
 
 void ProTypeMemberInitCheck::checkUninitializedTrivialType(

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.h

@@ -46,11 +46,13 @@ private:
   // 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:

clang-tools-extra/clang-tidy/utils/TypeTraits.cpp

@@ -58,6 +58,9 @@ bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
   // 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;
@@ -73,6 +76,8 @@ bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
   for (const CXXBaseSpecifier &Base : ClassDecl->bases()) {
     if (!isTriviallyDefaultConstructible(Base.getType(), Context))
       return false;
+    if (Base.isVirtual())
+      return false;
   }
 
   return true;

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

@@ -377,3 +377,49 @@ 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