[clang] Add FixItHint for designated init order

[MythreyaK]

Generate fix-it for C++20 designated initializers when the initializers do not match the declaration order in the structure.

---

vid.webm

From issue clangd/clangd#965 (comment)

What do you think about adding/improving fix actions for misordered initializers instead. Would it help much?

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mythreya Kuricheti (MythreyaK)

Changes

Generates FixItHint for reordering incorrectly initialized designated initializers.

vid.webm

From issue clangd/clangd#965 (comment)
> What do you think about adding/improving fix actions for misordered initializers instead. Would it help much?

---

Full diff: https://github.com/llvm/llvm-project/pull/173136.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/Sema/SemaInit.cpp (+65-3)
• (added) clang/test/SemaCXX/cxx20-designated-initializer-fixits.cpp (+105)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 994ac444d4aa1..ba14a50c1f177 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -493,6 +493,9 @@ Improvements to Clang's diagnostics
   carries messages like 'In file included from ...' or 'In module ...'.
   Now the include/import locations are written into `sarif.run.result.relatedLocations`.
 
+- Clang now generates a fix-it for C++20 designated initializers when the 
+  initializers do not match the declaration order in the structure. 
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index cc6ddf568d346..6decf0d809153 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -31,8 +31,10 @@
 #include "clang/Sema/SemaHLSL.h"
 #include "clang/Sema/SemaObjC.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -3092,6 +3094,65 @@ InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
         PrevField = *FI;
       }
 
+      const auto GenerateDesignatedInitReorderingFixit =
+          [&](SemaBase::SemaDiagnosticBuilder &Diags) {
+            struct ReorderInfo {
+              int Pos{};
+              const Expr *InitExpr{};
+            };
+
+            llvm::SmallDenseMap<IdentifierInfo *, int> MemberNameInx{};
+            llvm::SmallVector<ReorderInfo, 16> ReorderedInitExprs{};
+
+            const auto *CxxRecord =
+                IList->getSemanticForm()->getType()->getAsCXXRecordDecl();
+
+            for (const auto &Field : CxxRecord->fields()) {
+              MemberNameInx[Field->getIdentifier()] = Field->getFieldIndex();
+            }
+
+            for (const auto *Init : IList->inits()) {
+              if (const auto *DI =
+                      dyn_cast_if_present<DesignatedInitExpr>(Init)) {
+                // We expect only one Designator
+                if (DI->size() != 1)
+                  return;
+
+                const auto *const FieldName =
+                    DI->getDesignator(0)->getFieldName();
+                // In case we have an unknown initializer in the source, not in
+                // the record
+                if (MemberNameInx.contains(FieldName))
+                  ReorderedInitExprs.emplace_back(
+                      ReorderInfo{MemberNameInx.at(FieldName), Init});
+              }
+            }
+
+            llvm::sort(ReorderedInitExprs, [](const auto &A, const auto &B) {
+              return A.Pos < B.Pos;
+            });
+
+            llvm::SmallString<128> FixedInitList{};
+            SourceManager &SM = SemaRef.getSourceManager();
+            const LangOptions &LangOpts = SemaRef.getLangOpts();
+
+            // In a derived Record, first n base-classes are initialized first.
+            // They do not use designated init, so skip them
+            const auto IListInits =
+                IList->inits().drop_front(CxxRecord->getNumBases());
+            // loop over each existing expressions and apply replacement
+            for (const auto &[OrigExpr, Repl] :
+                 llvm::zip(IListInits, ReorderedInitExprs)) {
+              CharSourceRange CharRange = CharSourceRange::getTokenRange(
+                  Repl.InitExpr->getSourceRange());
+              const auto InitText =
+                  Lexer::getSourceText(CharRange, SM, LangOpts);
+
+              Diags << FixItHint::CreateReplacement(OrigExpr->getSourceRange(),
+                                                    InitText.str());
+            }
+          };
+
       if (PrevField &&
           PrevField->getFieldIndex() > KnownField->getFieldIndex()) {
         SemaRef.Diag(DIE->getInit()->getBeginLoc(),
@@ -3101,9 +3162,10 @@ InitListChecker::CheckDesignatedInitializer(const InitializedEntity &Entity,
         unsigned OldIndex = StructuredIndex - 1;
         if (StructuredList && OldIndex <= StructuredList->getNumInits()) {
           if (Expr *PrevInit = StructuredList->getInit(OldIndex)) {
-            SemaRef.Diag(PrevInit->getBeginLoc(),
-                         diag::note_previous_field_init)
-                << PrevField << PrevInit->getSourceRange();
+            auto Diags = SemaRef.Diag(PrevInit->getBeginLoc(),
+                                      diag::note_previous_field_init)
+                         << PrevField << PrevInit->getSourceRange();
+            GenerateDesignatedInitReorderingFixit(Diags);
           }
         }
       }
diff --git a/clang/test/SemaCXX/cxx20-designated-initializer-fixits.cpp b/clang/test/SemaCXX/cxx20-designated-initializer-fixits.cpp
new file mode 100644
index 0000000000000..7dfdd62df292f
--- /dev/null
+++ b/clang/test/SemaCXX/cxx20-designated-initializer-fixits.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -std=c++20 %s -Wreorder-init-list -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
+
+// These tests are from clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+struct C { int :0, x, :0, y, :0; };
+C c = {
+  .x = 1,
+  .x = 1,
+  .y = 1,
+  .y = 1,
+  .x = 1,
+  .x = 1,
+};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".x = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".x = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".x = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".x = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".y = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:3-[[@LINE-7]]:9}:".y = 1"
+
+namespace GH63605 {
+struct A  {
+  unsigned x;
+  unsigned y;
+  unsigned z;
+};
+
+struct B {
+  unsigned a;
+  unsigned b;
+};
+
+struct : public A, public B {
+  unsigned : 2;
+  unsigned a : 6;
+  unsigned : 1;
+  unsigned b : 6;
+  unsigned : 2;
+  unsigned c : 6;
+  unsigned d : 1;
+  unsigned e : 2;
+} data = {
+  {.z=0,
+         
+         
+   .y=1, 
+         
+   .x=2}, 
+  {.b=3,  
+   .a=4}, 
+    .e = 1, 
+            
+    .d = 1, 
+            
+    .c = 1, 
+    .b = 1, 
+    .a = 1, 
+};
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-17]]:4-[[@LINE-17]]:8}:".x=2"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-15]]:4-[[@LINE-15]]:8}:".y=1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-14]]:4-[[@LINE-14]]:8}:".z=0"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-14]]:4-[[@LINE-14]]:8}:".a=4"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-14]]:4-[[@LINE-14]]:8}:".b=3"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-14]]:5-[[@LINE-14]]:11}:".a = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-13]]:5-[[@LINE-13]]:11}:".b = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-12]]:5-[[@LINE-12]]:11}:".c = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-12]]:5-[[@LINE-12]]:11}:".d = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-12]]:5-[[@LINE-12]]:11}:".e = 1"
+// END tests from clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+
+namespace reorder_derived {
+struct col {
+  int r;
+  int g;
+  int b;
+};
+
+struct point {
+  float x;
+  float y;
+  float z;
+};
+
+struct derived : public col, public point {
+  int z2;
+  int z1;
+};
+
+void test() {
+  derived a {
+    {.b = 1, .g = 2, .r = 3}, 
+    { .z = 1, .y=2, .x =  3 },
+    .z1 = 1, 
+    .z2 = 2, 
+  };
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:6-[[@LINE-6]]:12}:".r = 3"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-7]]:14-[[@LINE-7]]:20}:".g = 2"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-8]]:22-[[@LINE-8]]:28}:".b = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-8]]:15-[[@LINE-8]]:19}:".y=2"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-9]]:21-[[@LINE-9]]:28}:".z = 1"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-10]]:7-[[@LINE-10]]:13}:".x =  3"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-10]]:5-[[@LINE-10]]:12}:".z2 = 2"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-10]]:5-[[@LINE-10]]:12}:".z1 = 1"
+} // namespace reorder_derived

[ojhunt]

This looks good to me, it just needs some style fixups

[ojhunt]

Just that one minor change as I hadn't considered our style choice around Diags, sorry about that :D

[ojhunt]

Looks good! Thanks, if you update it to main I'll push the merge button :D, oh there are no conflicts so no need :D

[ojhunt]

Oh actually, you'll need to edit your description because that's what GitHub will use as the commit message :D

[llvm-ci]

LLVM Buildbot has detected a new failure on builder sanitizer-windows running on sanitizer-windows while building clang at step 7 "test-build-unified-tree-check-compiler-rt".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/107/builds/16228

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-compiler-rt) failure: test (failure)
******************** TEST 'SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test.exe/18/22' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\b\slave\sanitizer-windows\build\runtimes\runtimes-bins\compiler-rt\lib\sanitizer_common\tests\.\Sanitizer-x86_64-Test.exe-SanitizerCommon-Unit-7896-18-22.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=22 GTEST_SHARD_INDEX=18 C:\b\slave\sanitizer-windows\build\runtimes\runtimes-bins\compiler-rt\lib\sanitizer_common\tests\.\Sanitizer-x86_64-Test.exe
--

Script:
--
C:\b\slave\sanitizer-windows\build\runtimes\runtimes-bins\compiler-rt\lib\sanitizer_common\tests\.\Sanitizer-x86_64-Test.exe --gtest_filter=StopTheWorld.SuspendThreadsAdvanced
--
C:/b/slave/sanitizer-windows/llvm-project/compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp(144): error: Value of: argument.threads_stopped
  Actual: false
Expected: true


C:/b/slave/sanitizer-windows/llvm-project/compiler-rt/lib/sanitizer_common/tests/sanitizer_stoptheworld_test.cpp:144
Value of: argument.threads_stopped
  Actual: false
Expected: true



********************

[MythreyaK]

As far as I can tell, the failure does not seem related 🤔. @ojhunt, please let me know if I'm wrong!

[ojhunt]

LLVM Buildbot has detected a new failure on builder sanitizer-windows running on sanitizer-windows while building clang at step 7 "test-build-unified-tree-check-compiler-rt".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/107/builds/16228

Here is the relevant piece of the build log for the reference

@MythreyaK this looks like a flakey test, I see no way your PR could possibly cause this