[clang-tidy] Ignore other members in a union if any member of it is initialized in cppcoreguidelines-pro-type-member-init

[Sockke]

Summary

If a union member is initialized, the other members of the union are still suggested to be initialized in this check. This patch fixes this behavior.
Reference issue: https://github.com/llvm/llvm-project/issues/54748

[njames93]

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

558 ↗

(On Diff #435106)

This line is redundant and likely to cause unintended test failures, same goes below.
If there was a warning on this line and no directive for it, the check script would fail due to that.

[Sockke]

Fix test file.

[Sockke]

rebased.

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[Sockke]

rebased.

[LegalizeAdulthood]

Please add a summary of the fix to the release notes for this check.

[Sockke]

Added release notes.

[Sockke]

Friendly ping.

[Sockke]

Friendly ping.

[njames93]

Can you add test cases where there is a struct with an anonymous union and another field that does need an initializer, just to verify warning and fixit behaviour.

struct S3 {
  S3() {}
  int A;
  union {
    int B;
    int C = 0; // Possibly a case where this is initialized in the member init list.
  };
};

clang-tools-extra/docs/ReleaseNotes.rst

137–139

Can't think of the nicest way to say this, but I think this is a little more understandable.

[Sockke]

Added test cases where there is a struct with an anonymous union and another field that does need an initializer.

[Sockke]

Friendly ping.

[njames93]

LGTM, You'd need to rebase before landing as the release notes have been reset for the 15 release branching.

[Sockke]

Rebased.

[thieta]

Hi @Sockke - this seems to be ready to land. Is there something holding it back? We ran into the same issue recently.

[Sockke]

Hi, @LegalizeAdulthood, are you okay with the way this has progressed?

[Sockke]

Friendly ping @LegalizeAdulthood.

[Sockke]

Rebased.

[Sockke]

This patch has been around for a long time, I have added the release notes. There are users waiting for this fix and I committed it myself first.
@LegalizeAdulthood I'm looking forward to your follow-up reply.

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

for (const FieldDecl *F : Fields) {
     } else {
       Fn(F);
     }
     if (Record.isUnion() && AnyMemberHasInitPerUnion)
       break;
   }
 }
 
+void removeFieldInitialized(const FieldDecl *M,
+                            SmallPtrSetImpl<const FieldDecl *> &FieldDecls) {
+  const RecordDecl *R = M->getParent();
+  if (R && R->isUnion()) {
+    // Erase all members in a union if any member of it is initialized.
+    for (const auto *F : R->fields())
+      FieldDecls.erase(F);
+  } else
+    FieldDecls.erase(M);
+}
+
 void removeFieldsInitializedInBody(
     const Stmt &Stmt, ASTContext &Context,
     SmallPtrSetImpl<const FieldDecl *> &FieldDecls) {
   auto Matches =
       match(findAll(binaryOperator(
                 hasOperatorName("="),
                 hasLHS(memberExpr(member(fieldDecl().bind("fieldDecl")))))),
             Stmt, Context);
   for (const auto &Match : Matches)
-    FieldDecls.erase(Match.getNodeAs<FieldDecl>("fieldDecl"));
+    removeFieldInitialized(Match.getNodeAs<FieldDecl>("fieldDecl"), FieldDecls);
 }
 
 StringRef getName(const FieldDecl *Field) { return Field->getName(); }
 
 StringRef getName(const RecordDecl *Record) {
   // Get the typedef name if this is a C-style anonymous struct and typedef.
   if (const TypedefNameDecl *Typedef = Record->getTypedefNameForAnonDecl())
     return Typedef->getName();
void ProTypeMemberInitCheck::checkMissingMemberInitializer(
     const CXXConstructorDecl *Ctor) {
   bool IsUnion = ClassDecl.isUnion();
 
   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(), [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+                         AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
     if (IgnoreArrays && F->getType()->isArrayType())
       return;
+    if (F->hasInClassInitializer() && F->getParent()->isUnion()) {
+      AnyMemberHasInitPerUnion = true;
+      removeFieldInitialized(F, FieldsToInit);
+    }
     if (!F->hasInClassInitializer() &&
         utils::type_traits::isTriviallyDefaultConstructible(F->getType(),
                                                             Context) &&
-        !isEmpty(Context, F->getType()) && !F->isUnnamedBitfield())
+        !isEmpty(Context, F->getType()) && !F->isUnnamedBitfield() &&
+        !AnyMemberHasInitPerUnion)
       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());
+        removeFieldInitialized(Init->getAnyMember(), FieldsToInit);
       }
     }
     removeFieldsInitializedInBody(*Ctor->getBody(), Context, FieldsToInit);
   }
 
   // Collect all fields in order, both direct fields and indirect fields from
   // anonymous record types.
   SmallVector<const FieldDecl *, 16> OrderedFields;
void ProTypeMemberInitCheck::checkMissingMemberInitializer(
   // Do not propose fixes for constructors in macros since we cannot place them
   // correctly.
   if (Ctor && Ctor->getBeginLoc().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;
-  bool AnyMemberHasInitPerUnion = false;
+  AnyMemberHasInitPerUnion = false;
   forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
                          AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
     if (!FieldsToInit.count(F))
       return;
     // Don't suggest fixes for enums because we don't know a good default.
     // Don't suggest fixes for bitfields because in-class initialization is not
     // possible until C++20.
     if (F->getType()->isEnumeralType() ||

clang-tools-extra/docs/ReleaseNotes.rst

 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
-  
+- Fixed a false positive in :doc:`cppcoreguidelines-pro-type-member-init
+  <clang-tidy/checks/cppcoreguidelines/pro-type-member-init>` when warnings
+  would be emitted for uninitialized members of an anonymous union despite
+  there being an initializer for one of the other members.
+  
 - Improved `modernize-use-emplace <clang-tidy/checks/modernize/use-emplace.html>`_ check.
 
   The check now supports detecting inefficient invocations of ``push`` and
   ``push_front`` on STL-style containers and replacing them with ``emplace``
   or ``emplace_front``.
 
 - Improved `modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default.html>`_ check.
 
   The check now skips unions since in this case a default constructor with empty body
   is not equivalent to the explicitly defaulted one.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

union {
       };
       int C;
       // CHECK-FIXES: int C{};
     };
   };
   int A;
   // CHECK-FIXES-NOT: int A{};
 };
+
+struct S1 {
+  S1() {}
+  union {
+    int a = 0;
+    int b;
+  };
+};
+
+struct S2 {
+  S2() : a{} {}
+  union {
+    int a;
+    int b;
+  };
+};
+
+struct S3 {
+  S3() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these fields: A [cppcoreguidelines-pro-type-member-init]
+  int A;
+  // CHECK-FIXES: int A{};
+  union {
+    int B;
+    int C = 0;
+  };
+};