[clang-tidy] Sequence statements with multiple parents correctly (PR39149)

Summary:
Before this fix, the bugprone-use-after-move check could incorrectly
conclude that a use and move in a function template were not sequenced.
For details, see

https://bugs.llvm.org/show_bug.cgi?id=39149

Reviewers: alexfh, hokein, aaron.ballman, JonasToth

Reviewed By: aaron.ballman

Subscribers: xazax.hun, cfe-commits

Tags: #clang-tools-extra

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

4 files changed, 26 insertions, 6 deletions

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index ea6454920d4..7245986663d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -102,8 +102,9 @@ bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
   if (!TheCFG)
     return false;
 
-  Sequence.reset(new ExprSequence(TheCFG.get(), Context));
-  BlockMap.reset(new StmtToBlockMap(TheCFG.get(), Context));
+  Sequence =
+      llvm::make_unique<ExprSequence>(TheCFG.get(), FunctionBody, Context);
+  BlockMap = llvm::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
   Visited.clear();
 
   const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
index 48c3de5450f..c3602ff8ad7 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -63,8 +63,9 @@ bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
 }
 }
 
-ExprSequence::ExprSequence(const CFG *TheCFG, ASTContext *TheContext)
-    : Context(TheContext) {
+ExprSequence::ExprSequence(const CFG *TheCFG, const Stmt *Root,
+                           ASTContext *TheContext)
+    : Context(TheContext), Root(Root) {
   for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
     SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
   }
@@ -99,6 +100,11 @@ bool ExprSequence::potentiallyAfter(const Stmt *After,
 
 const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const {
   for (const Stmt *Parent : getParentStmts(S, Context)) {
+    // If a statement has multiple parents, make sure we're using the parent
+    // that lies within the sub-tree under Root.
+    if (!isDescendantOrEqual(Parent, Root, Context))
+      continue;
+
     if (const auto *BO = dyn_cast<BinaryOperator>(Parent)) {
       // Comma operator: Right-hand side is sequenced after the left-hand side.
       if (BO->getLHS() == S && BO->getOpcode() == BO_Comma)
diff --git a/clang-tools-extra/clang-tidy/utils/ExprSequence.h b/clang-tools-extra/clang-tidy/utils/ExprSequence.h
index 2b355d9a94f..0868a899779 100644
--- a/clang-tools-extra/clang-tidy/utils/ExprSequence.h
+++ b/clang-tools-extra/clang-tidy/utils/ExprSequence.h
@@ -69,8 +69,8 @@ namespace utils {
 class ExprSequence {
 public:
   /// Initializes this `ExprSequence` with sequence information for the given
-  /// `CFG`.
-  ExprSequence(const CFG *TheCFG, ASTContext *TheContext);
+  /// `CFG`. `Root` is the root statement the CFG was built from.
+  ExprSequence(const CFG *TheCFG, const Stmt *Root, ASTContext *TheContext);
 
   /// Returns whether \p Before is sequenced before \p After.
   bool inSequence(const Stmt *Before, const Stmt *After) const;
@@ -94,6 +94,7 @@ private:
   const Stmt *resolveSyntheticStmt(const Stmt *S) const;
 
   ASTContext *Context;
+  const Stmt *Root;
 
   llvm::DenseMap<const Stmt *, const Stmt *> SyntheticStmtSourceMap;
 };
diff --git a/clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp b/clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
index ae1f021e011..59dcb90cb49 100644
--- a/clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/bugprone-use-after-move.cpp
@@ -1195,6 +1195,18 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
   }
 }
 
+// Some statements in templates (e.g. null, break and continue statements) may
+// be shared between the uninstantiated and instantiated versions of the
+// template and therefore have multiple parents. Make sure the sequencing code
+// handles this correctly.
+template <class> void nullStatementSequencesInTemplate() {
+  int c = 0;
+  (void)c;
+  ;
+  std::move(c);
+}
+template void nullStatementSequencesInTemplate<int>();
+
 namespace PR33020 {
 class D {
   ~D();