[clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

Fixes https://github.com/llvm/llvm-project/issues/53515.

Reviewed By: LegalizeAdulthood

Differential Revision: https://reviews.llvm.org/D118927

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

@@ -201,30 +201,42 @@ void PreferMemberInitializerCheck::check(
             diag(S->getBeginLoc(), "%0 should be initialized in an in-class"
                                    " default member initializer")
             << Field;
-        if (!InvalidFix) {
-          CharSourceRange StmtRange =
-              CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
+        if (InvalidFix)
+          continue;
+        CharSourceRange StmtRange =
+            CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
 
-          SmallString<128> Insertion(
-              {UseAssignment ? " = " : "{",
-               Lexer::getSourceText(
-                   CharSourceRange(InitValue->getSourceRange(), true),
-                   *Result.SourceManager, getLangOpts()),
-               UseAssignment ? "" : "}"});
+        SmallString<128> Insertion(
+            {UseAssignment ? " = " : "{",
+             Lexer::getSourceText(
+                 CharSourceRange(InitValue->getSourceRange(), true),
+                 *Result.SourceManager, getLangOpts()),
+             UseAssignment ? "" : "}"});
+
+        Diag << FixItHint::CreateInsertion(FieldEnd, Insertion)
+             << FixItHint::CreateRemoval(StmtRange);
 
-          Diag << FixItHint::CreateInsertion(FieldEnd, Insertion)
-               << FixItHint::CreateRemoval(StmtRange);
-        }
       } else {
         StringRef InsertPrefix = "";
+        bool HasInitAlready = false;
         SourceLocation InsertPos;
+        SourceRange ReplaceRange;
         bool AddComma = false;
         bool InvalidFix = false;
         unsigned Index = Field->getFieldIndex();
         const CXXCtorInitializer *LastInListInit = nullptr;
         for (const CXXCtorInitializer *Init : Ctor->inits()) {
-          if (!Init->isWritten())
+          if (!Init->isWritten() || Init->isInClassMemberInitializer())
             continue;
+          if (Init->getMember() == Field) {
+            HasInitAlready = true;
+            if (isa<ImplicitValueInitExpr>(Init->getInit()))
+              InsertPos = Init->getRParenLoc();
+            else {
+              ReplaceRange = Init->getInit()->getSourceRange();
+            }
+            break;
+          }
           if (Init->isMemberInitializer() &&
               Index < Init->getMember()->getFieldIndex()) {
             InsertPos = Init->getSourceLocation();
@@ -235,30 +247,38 @@ void PreferMemberInitializerCheck::check(
           }
           LastInListInit = Init;
         }
-        if (InsertPos.isInvalid()) {
-          if (LastInListInit) {
-            InsertPos = Lexer::getLocForEndOfToken(
-                LastInListInit->getRParenLoc(), 0, *Result.SourceManager,
-                getLangOpts());
-            // Inserting after the last constructor initializer, so we need a
-            // comma.
-            InsertPrefix = ", ";
-          } else {
-            InsertPos = Lexer::getLocForEndOfToken(
-                Ctor->getTypeSourceInfo()
-                    ->getTypeLoc()
-                    .getAs<clang::FunctionTypeLoc>()
-                    .getLocalRangeEnd(),
-                0, *Result.SourceManager, getLangOpts());
+        if (HasInitAlready) {
+          if (InsertPos.isValid())
+            InvalidFix |= InsertPos.isMacroID();
+          else
+            InvalidFix |= ReplaceRange.getBegin().isMacroID() ||
+                          ReplaceRange.getEnd().isMacroID();
+        } else {
+          if (InsertPos.isInvalid()) {
+            if (LastInListInit) {
+              InsertPos = Lexer::getLocForEndOfToken(
+                  LastInListInit->getRParenLoc(), 0, *Result.SourceManager,
+                  getLangOpts());
+              // Inserting after the last constructor initializer, so we need a
+              // comma.
+              InsertPrefix = ", ";
+            } else {
+              InsertPos = Lexer::getLocForEndOfToken(
+                  Ctor->getTypeSourceInfo()
+                      ->getTypeLoc()
+                      .getAs<clang::FunctionTypeLoc>()
+                      .getLocalRangeEnd(),
+                  0, *Result.SourceManager, getLangOpts());
 
-            // If this is first time in the loop, there are no initializers so
-            // `:` declares member initialization list. If this is a subsequent
-            // pass then we have already inserted a `:` so continue with a
-            // comma.
-            InsertPrefix = FirstToCtorInits ? " : " : ", ";
+              // If this is first time in the loop, there are no initializers so
+              // `:` declares member initialization list. If this is a
+              // subsequent pass then we have already inserted a `:` so continue
+              // with a comma.
+              InsertPrefix = FirstToCtorInits ? " : " : ", ";
+            }
           }
+          InvalidFix |= InsertPos.isMacroID();
         }
-        InvalidFix |= InsertPos.isMacroID();
 
         SourceLocation SemiColonEnd;
         if (auto NextToken = Lexer::findNextToken(
@@ -271,21 +291,25 @@ void PreferMemberInitializerCheck::check(
             diag(S->getBeginLoc(), "%0 should be initialized in a member"
                                    " initializer of the constructor")
             << Field;
-        if (!InvalidFix) {
-
-          CharSourceRange StmtRange =
-              CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd);
-          SmallString<128> Insertion(
-              {InsertPrefix, Field->getName(), "(",
-               Lexer::getSourceText(
-                   CharSourceRange(InitValue->getSourceRange(), true),
-                   *Result.SourceManager, getLangOpts()),
-               AddComma ? "), " : ")"});
+        if (InvalidFix)
+          continue;
+        StringRef NewInit = Lexer::getSourceText(
+            CharSourceRange(InitValue->getSourceRange(), true),
+            *Result.SourceManager, getLangOpts());
+        if (HasInitAlready) {
+          if (InsertPos.isValid())
+            Diag << FixItHint::CreateInsertion(InsertPos, NewInit);
+          else
+            Diag << FixItHint::CreateReplacement(ReplaceRange, NewInit);
+        } else {
+          SmallString<128> Insertion({InsertPrefix, Field->getName(), "(",
+                                      NewInit, AddComma ? "), " : ")"});
           Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
-                                             FirstToCtorInits)
-               << FixItHint::CreateRemoval(StmtRange);
-          FirstToCtorInits = false;
+                                             FirstToCtorInits);
         }
+        Diag << FixItHint::CreateRemoval(
+            CharSourceRange::getCharRange(S->getBeginLoc(), SemiColonEnd));
+        FirstToCtorInits = false;
       }
     }
   }

clang-tools-extra/docs/ReleaseNotes.rst

@@ -139,6 +139,12 @@ Changes in existing checks
 
 - Fixed a crash in :doc:`bugprone-sizeof-expression <clang-tidy/checks/bugprone-sizeof-expression>` when
   `sizeof(...)` is compared agains a `__int128_t`.
+  
+- Improved :doc:`cppcoreguidelines-prefer-member-initializer
+  <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
+
+  Fixed an issue when there was already an initializer in the constructor and
+  the check would try to create another initializer for the same member.
 
 Removed checks
 ^^^^^^^^^^^^^^

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp

@@ -526,14 +526,32 @@ struct InitFromVarDecl {
   }
 };
 
-struct AlreadyHasInit {
+struct HasInClassInit {
   int m = 4;
-  AlreadyHasInit() {
+  HasInClassInit() {
     m = 3;
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'm' should be initialized in a member initializer of the constructor
   }
 };
 
+struct HasInitListInit {
+  int M;
+  // CHECK-MESSAGES: :[[@LINE+5]]:5: warning: 'M' should be initialized in a member initializer of the constructor
+  // CHECK-FIXES: HasInitListInit(const HasInitListInit &Other) : M(Other.M) {
+  // CHECK-FIXES-NEXT: {{^    $}}
+  // CHECK-FIXES-NEXT: }
+  HasInitListInit(const HasInitListInit &Other) : M(4) {
+    M = Other.M;
+  }
+  // CHECK-MESSAGES: :[[@LINE+5]]:5: warning: 'M' should be initialized in a member initializer of the constructor
+  // CHECK-FIXES: HasInitListInit(HasInitListInit &&Other) : M(Other.M) {
+  // CHECK-FIXES-NEXT: {{^    $}}
+  // CHECK-FIXES-NEXT: }
+  HasInitListInit(HasInitListInit &&Other) : M() {
+    M = Other.M;
+  }
+};
+
 #define ASSIGN_IN_MACRO(FIELD, VALUE) FIELD = (VALUE);
 
 struct MacroCantFix {