From ddbcd985602dcb5fe78fcf2246cf53922db1f3c3 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme@google.com>
Date: Thu, 16 Mar 2023 07:33:49 +0000
Subject: [PATCH] [clang-tidy] Correctly handle evaluation order of designated
 initializers.

As designated initializers show up only in the syntactic form of the
InitListExpr, we need to make sure we're searching both forms of the
InitListExpr when determining successors in the evaluation order.

This fixes a bug in bugprone-use-after-move where previously we erroneously
concluded that two designated initializers were unsequenced. The newly added
tests fail without the fix.

Differential Revision: https://reviews.llvm.org/D145906
---
 .../clang-tidy/utils/ExprSequence.cpp              | 22 +++++++++++++++---
 clang-tools-extra/docs/ReleaseNotes.rst            |  4 ++++
 .../checkers/bugprone/use-after-move.cpp           | 26 +++++++++++++++++-----
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index beb4c44..f9555be 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -8,6 +8,7 @@
 
 #include "ExprSequence.h"
 #include "clang/AST/ParentMapContext.h"
+#include "llvm/ADT/SmallVector.h"
 #include <optional>
 
 namespace clang::tidy::utils {
@@ -49,6 +50,7 @@ static SmallVector<const Stmt *, 1> getParentStmts(const Stmt *S,
 }
 
 namespace {
+
 bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
                          ASTContext *Context) {
   if (Descendant == Ancestor)
@@ -60,6 +62,17 @@ bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
 
   return false;
 }
+
+llvm::SmallVector<const InitListExpr *>
+getAllInitListForms(const InitListExpr *InitList) {
+  llvm::SmallVector<const InitListExpr *> result = {InitList};
+  if (const InitListExpr *AltForm = InitList->getSyntacticForm())
+    result.push_back(AltForm);
+  if (const InitListExpr *AltForm = InitList->getSemanticForm())
+    result.push_back(AltForm);
+  return result;
+}
+
 } // namespace
 
 ExprSequence::ExprSequence(const CFG *TheCFG, const Stmt *Root,
@@ -111,9 +124,12 @@ const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const {
     } else if (const auto *InitList = dyn_cast<InitListExpr>(Parent)) {
       // Initializer list: Each initializer clause is sequenced after the
       // clauses that precede it.
-      for (unsigned I = 1; I < InitList->getNumInits(); ++I) {
-        if (InitList->getInit(I - 1) == S)
-          return InitList->getInit(I);
+      for (const InitListExpr *Form : getAllInitListForms(InitList)) {
+        for (unsigned I = 1; I < Form->getNumInits(); ++I) {
+          if (Form->getInit(I - 1) == S) {
+            return Form->getInit(I);
+          }
+        }
       }
     } else if (const auto *Compound = dyn_cast<CompoundStmt>(Parent)) {
       // Compound statement: Each sub-statement is sequenced after the
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2be8bfc..b53516a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,6 +219,10 @@ Changes in existing checks
   <clang-tidy/checks/cppcoreguidelines/slicing>` check when warning would be
   emitted in constructor for virtual base class initialization.
 
+- Improved :doc:`bugprone-use-after-move
+  <clang-tidy/checks/bugprone/use-after-move>` to understand that there is a
+  sequence point between designated initializers.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 281f208..45cef8a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1155,18 +1155,32 @@ void initializerListSequences() {
       int i;
       A a;
     };
-    A a;
-    S1 s1{a.getInt(), std::move(a)};
+    {
+      A a;
+      S1 s1{a.getInt(), std::move(a)};
+    }
+    {
+      A a;
+      S1 s1{.i = a.getInt(), .a = std::move(a)};
+    }
   }
   {
     struct S2 {
       A a;
       int i;
     };
-    A a;
-    S2 s2{std::move(a), a.getInt()};
-    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+    {
+      A a;
+      S2 s2{std::move(a), a.getInt()};
+      // CHECK-NOTES: [[@LINE-1]]:27: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
+    }
+    {
+      A a;
+      S2 s2{.a = std::move(a), .i = a.getInt()};
+      // CHECK-NOTES: [[@LINE-1]]:37: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
+    }
   }
 }
 
-- 
2.7.4