[clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[Sockke]

Summary

At most one variant member of a union may have a default member initializer. The case of anonymous records with multiple levels of nesting like the following also needs to meet this rule. The original logic is to horizontally obtain all the member variables in a record that need to be initialized and then filter to the variables that need to be fixed. Obviously, it is impossible to correctly initialize the desired variables according to the nesting relationship.

See Example 3 in class.union

union U {
  U() {}
  int x;  // int x{};
  union {
    int k;  // int k{};  <==  wrong fix
  };
  union {
    int z;  // int z{};  <== wrong fix
    int y;
  };
};

[MTC]

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

49

Can it be modified to the following form? Or further abstract filter into a parameter to make this function more general.

template <typename T, typename Func>
void forEachFieldWithFilter(const RecordDecl &Record, const T &Fields,
                            bool &AnyMemberHasInitPerUnion, Func &&Fn) {
  forEachField(Record, Fields, Fn);
  if (Record.isUnion() && AnyMemberHasInitPerUnion)
    break;
}

[Sockke]

This comment was removed by Sockke.

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

49

Can it be modified to the following form? Or further abstract filter into a parameter to make this function more general.

template <typename T, typename Func>
void forEachFieldWithFilter(const RecordDecl &Record, const T &Fields,
                            bool &AnyMemberHasInitPerUnion, Func &&Fn) {
  forEachField(Record, Fields, Fn);
  if (Record.isUnion() && AnyMemberHasInitPerUnion)
    break;
}

This does not seem to work, because the "AnyMemberHasInitPerUnion" needs to be passed along the call stack.

[Sockke]

Any thoughts? : )

[Sockke]

Any thoughts? : )

[Sockke]

What do you think? @aaron.ballman @bkramer

[Sockke]

Hi, Could anyone please review this diff?

[aaron.ballman]

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

526

Can you add a CHECK-FIXES-NOT that we're not adding the fix for this case?

529

Same here.

534

A related interesting test would be:

union U3 {
  U3() {}

  struct {
    int B;
  } b;
  int A;
};

I would expect A or b to need initialization for the union, and B to need initialization for the struct.

[Sockke]

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

534

A related interesting test would be:

union U3 {
  U3() {}

  struct {
    int B;
  } b;
  int A;
};

I would expect A or b to need initialization for the union, and B to need initialization for the struct.

I apologize for the delay in responding. It seems to me that struct {}b has no user-defined constructor, so all members are initialized to their defaults, it is unnecessary to initialize B explicitly.

[Sockke]

Hi, Could you please take some time to review this diff again? @aaron.ballman

[aaron.ballman]

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

484

Given that we're touching this code, we might as well make clang-format happy with it (though I can't quite spot what it wants changed, so if it turns out to be a bad idea for some reason, I don't insist).

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

526

I think you missed this request? Same for the one immediately below.

534

Okay, fair point!

[Sockke]

Update!

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

484

Given that we're touching this code, we might as well make clang-format happy with it (though I can't quite spot what it wants changed, so if it turns out to be a bad idea for some reason, I don't insist).

Yes, I have tried to format this code with clang-format but it does not work perfectly.

[aaron.ballman]

LGTM!

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

484

Then as-is is fine by me, thank you!

[Sockke]

This comment was removed by Sockke.

[Sockke]

In D108370#3017800, @aaron.ballman wrote:

LGTM!

Thanks for your review! I don't have commit access, here is my information:
Name: liuke
Email: [e-mail]

[aaron.ballman]

In D108370#3020793, @Sockke wrote:

In D108370#3017800, @aaron.ballman wrote:

LGTM!

Thanks for your review! I don't have commit access, here is my information:
Name: liuke
Email: [e-mail]

Thanks! I've commit on your behalf in e4902480f1e2f12f73c2b504e3d717536653dd7b.

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

if (F->isAnonymousStructOrUnion()) {
       if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl())
         forEachField(*R, R->fields(), Fn);
     } else {
       Fn(F);
     }
   }
 }
 
+template <typename T, typename Func>
+void forEachFieldWithFilter(const RecordDecl &Record, const T &Fields,
+                            bool &AnyMemberHasInitPerUnion, Func &&Fn) {
+  for (const FieldDecl *F : Fields) {
+    if (F->isAnonymousStructOrUnion()) {
+      if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl()) {
+        AnyMemberHasInitPerUnion = false;
+        forEachFieldWithFilter(*R, R->fields(), AnyMemberHasInitPerUnion, Fn);
+      }
+    } else {
+      Fn(F);
+    }
+    if (Record.isUnion() && AnyMemberHasInitPerUnion)
+      break;
+  }
+}
+
 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);
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;
-  SmallPtrSet<const RecordDecl *, 4> UnionsSeen;
-  forEachField(ClassDecl, OrderedFields, [&](const FieldDecl *F) {
+  bool 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() ||
         (!getLangOpts().CPlusPlus20 && F->isBitField()))
       return;
-    if (!F->getParent()->isUnion() || UnionsSeen.insert(F->getParent()).second)
-      FieldsToFix.insert(F);
+    AnyMemberHasInitPerUnion = true;
   });
   if (FieldsToFix.empty())
     return;
 
   // Use in-class initialization if possible.
   if (Context.getLangOpts().CPlusPlus11) {
     for (const FieldDecl *Field : FieldsToFix) {
       Diag << FixItHint::CreateInsertion(

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

 struct PositiveDefaultConstructorOutOfDecl {
   PositiveDefaultConstructorOutOfDecl();
   int F;
   // CHECK-FIXES: int F{};
 };
 
 PositiveDefaultConstructorOutOfDecl::PositiveDefaultConstructorOutOfDecl() = default;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F
+
+union U1 {
+  U1() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: X, K, Z, Y
+  int X;
+  // CHECK-FIXES: int X{};
+  union {
+    int K;
+    // CHECK-FIXES-NOT: int K{};
+  };
+  union {
+    int Z;
+    // CHECK-FIXES-NOT: int Z{};
+    int Y;
+    // CHECK-FIXES-NOT: int Y{};
+  };
+};
+
+union U2 {
+  U2() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: B, C, A
+  struct {
+    int B;
+    // CHECK-FIXES: int B{};
+    union {
+      struct {
+        PositiveMultipleConstructors Value;
+        // CHECK-FIXES-NOT: PositiveMultipleConstructors Value{};
+      };
+      int C;
+      // CHECK-FIXES: int C{};
+    };
+  };
+  int A;
+  // CHECK-FIXES-NOT: int A{};
+};