[clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.

[mboehme]

Summary

This eliminates false positives in bugprone-use-after-move where a variable
is used in the callee and moved from in the arguments.

We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr as its base, we consider it to be sequenced after the arguments. This is because the variable referenced in the base will only actually be accessed when the call happens, i.e. once all of the arguments have been evaluated. This has no basis in the C++ standard, but it reflects actual behavior that is relevant to a use-after-move scenario:

a.bar(consumeA(std::move(a));

In this example, we end up accessing a after it has been moved from, even though nominally the callee a.bar is evaluated before the argument consumeA(std::move(a)).

Treating this scenario correctly has required rewriting the logic in bugprone-use-after-move that governs whether the use happens in a later loop iteration than the move. This was previously based on an unsound heuristic (does the use come lexically before the move?); we now use a more rigourous criterion based on reachability in the CFG.

[Eugene.Zelenko]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1555

Please add.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1337

This entire file tests only C++17 and later, when std::move is C++11 feature.
Add separate test file for C++11 (or even better split this one between C++11 and C++17)

Scenario to test in C++11, should warn ?

a->foo(std::move(a));

1345

What about scenario like this:

b.foo(a->saveBIntoAAndReturnBool(std::move(b)));

Is first "b" still guaranteed to be alive after std::move ?

1346

I didn't found any test for correct warning in this case:

std::unique_ptr<A> getArg(std::unique_ptr<A>);
getArg(std::move(a))->foo(std::move(a));

[PiotrZSL]

Add more tests (those missing one), to make sure that this wont break proper cases.
Refer to inline comments.

[mboehme]

Changes in response to review comments.

[mboehme]

Added entry to release notes.

[mboehme]

Sorry for the delay in replying -- I was caught up in other projects.

clang-tools-extra/docs/ReleaseNotes.rst

337

Reworded according to comments in https://reviews.llvm.org/D145906.

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1337

This entire file tests only C++17 and later, when std::move is C++11 feature.
Add separate test file for C++11 (or even better split this one between C++11 and C++17)

Good point. I've added a new RUN line that also runs this in C++11 mode.

Scenario to test in C++11, should warn ?

See below -- this does produce a warning in C++11 mode (but not C++17 mode).

I've also redone the tests to reuse the existing class A without having to define a new one.

1345

I'm not exactly sure what you're asking here... or how this scenario is materially different from the other scenarios we already have?

Is first "b" still guaranteed to be alive after std::move ?

The b in b.foo is guaranteed to be evaluated before the call a->saveBIntoAAndReturnBool(std::move(b)) -- but I'm not sure if this is what you're asking?

Or are you asking whether the a->saveBIntoAAndReturnBool(std::move(b)) can cause the underlying object to be destroyed before the call to b.foo happenss? In other words, do we potentially have a use-after-free here?

I think the answer to this depends on what exactly saveBIntoAAndReturnBool() does (what was your intent here?). I also think it's probably beyond the scope of this check in any case, as this check is about diagnosing use-after-move, not use-after-free.

1346

Good point -- added below (in slightly different form, but testing the same thing).

Note that this is an error in both C++11 and C++17, but in C++11 the use and move are unsequenced, whereas in C++17 the use definitely comes after the move.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1345

I see this `b.foo(a->saveBIntoAAndReturnBool(std::move(b)));` like this:
we call saveBIntoAAndReturnBool, that takes b by std::move, then we call foo on already moved object.
For me this is use after move, that's why I was asking.

And in "b.foo" there is almost nothing to evaluate, maybe address of foo, but at the end foo will be called on already moved object.
If we would have something like "getSomeObj(b).boo(std::move(b))" then we can think about "evaluate", but when we directly call method on moved object, then we got use after move

[mboehme]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1345

Ah, I think I understand what you're getting at now. I was assuming for some reason that b was also a unique_ptr in this example, but of course that doesn't make sense because in that case we wouldn't be able to use the dot operator on b (i.e. b.foo).

Distinguishing between these two cases will require making the check more sophisticated -- the logic that the callee is sequenced before the arguments is not sufficient on its own. I'll have to take a closer look at how to do this, but it will likely involve looking at the MemberExpr inside the CXXMemberCallExpr. If MemberExpr::getBase() is simply a DeclRefExpr, we'll want to do one thing, and if MemberExpr::getBase() is some sort of CallExpr, we'll want to do something else. There will likely need to be other considerations involved as well, but I wanted to sketch out in broad lines where I think this should go.

I'll likely take a few days to turn this around, but in the meantime I wanted to get this comment out to let you know that I now understand the issue.

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1345

Yes but that's not so easy, as there can be thing like:
x.y.foo(std::move(x));

To be honest probably easiest way would be to extract isIdenticalStmt from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp, and then we could just check if callExpr callee contain argument of std::move, and that argument does not contain any other callExpr before current one.
For such cases we could warn, but for all other cases when there any other sub callExpr involved we woudn't need to wanr.

to be honest I need isIdenticalStmt for my other checks, so if you decide to go this route do this under separate patch.

Reasn why I mention isIdenticalStmt is because this would handle also things like this:

x.z.foo(std::move(y)), where x and y are same types.

However if you decide to do some tricks with MemberExpr, good luck (i wouldn't bother) there are other usecases to watch out:
like getX().z.y.foo(std::move(getX().z)), and partial moving examples...

[PiotrZSL]

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

[PiotrZSL]

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

[mboehme]

In D145581#4215602, @PiotrZSL wrote:

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

Thanks, and sorry for the delayed response on this -- I've been juggling this patch with other work.

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks for making me aware of this!

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

335–336

Please keep alphabetical order (by check name) in this section.

[mboehme]

Changes in reponse to review comments.

[mboehme]

Changes in response to review comments

[mboehme]

In D145581#4215602, @PiotrZSL wrote:

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

Did I do this correctly? It says "Needs Review" now, though I think I didn't do anything specific to trigger this.

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks, I'll update this once this change is submitted.

clang-tools-extra/docs/ReleaseNotes.rst

335–336

Please keep alphabetical order (by check name) in this section.

Done.

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1345

Yes but that's not so easy, as there can be thing like:
x.y.foo(std::move(x));

To be honest probably easiest way would be to extract isIdenticalStmt from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp, and then we could just check if callExpr callee contain argument of std::move, and that argument does not contain any other callExpr before current one.
For such cases we could warn, but for all other cases when there any other sub callExpr involved we woudn't need to wanr.

to be honest I need isIdenticalStmt for my other checks, so if you decide to go this route do this under separate patch.

Reasn why I mention isIdenticalStmt is because this would handle also things like this:

x.z.foo(std::move(y)), where x and y are same types.

However if you decide to do some tricks with MemberExpr, good luck (i wouldn't bother) there are other usecases to watch out:
like getX().z.y.foo(std::move(getX().z)), and partial moving examples...

Sorry for the really late response to this.

The delay is partially because I was busy with other projects and partially because I found it hard to decide how far to take this.

In the end, I implemented something that will only happen the very basic case where the base of the MemberExpr is the same as the argument of the std::move. This seems a common case that's worth handling correctly. (To make this work correctly, by the way, I had to change the logic that decides whether the use happens on a later loop iteration than the move. This was previously based on a purely syntactic criterion, but that no longer works in this case.)

There's a lot more that could be done, but I wonder a) how many of these cases are synthetic cases that don't turn up in reality, and b) you quickly get into territory where you need interprocedural analysis. For example:

A& id(A& a) { return a; }
A a;
a.bar(consumeA(std::move(a)));
id(a).bar(consumeA(std::move(a)));

We would like to treat the last two lines identically, as a use-after-move, but the second one is impossible to see without interprocedural analysis.

[PiotrZSL]

First, re-base code, looks like there were some changes in this check, and now there are conflicts with this path.
Second I don't any more comments, for me this code looks fine, BUT I'm not familiar too much with this check.
Check history for this check, and maybe consider adding to review some other people who modify it recently.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

79–84

NOTE: This looks like llvm::any_of

88–93

NOTE: this looks like llvm::any_of, or even something like contains/find

153–157

NOTE: probably you could move call outside if, and do rest with single if...

clang-tools-extra/docs/ReleaseNotes.rst

214

NOTE: Probably better would be to keep similar template to rest of checks, just list fixes in sentences (or separated with ,), not as an list.

[mboehme]

Changes in response to review comments, and rebased to head.

[mboehme]

In D145581#4290839, @PiotrZSL wrote:

First, re-base code, looks like there were some changes in this check, and now there are conflicts with this path.

Done. (Not sure what the etiquette is with respect to rebasing -- that's why I kept it based on an old revision.)

Second I don't any more comments, for me this code looks fine, BUT I'm not familiar too much with this check.
Check history for this check, and maybe consider adding to review some other people who modify it recently.

Thanks, will do.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

79–84

Done.

88–93

Done (with find).

153–157

Or as a single if statement like this?

clang-tools-extra/docs/ReleaseNotes.rst

214

Done.

[mboehme]

@MarcoFalke Would you be willing to review? (Thanks for your recent fix in D146288!)

[mboehme]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1439

Rewriting the logic for the "use happens in a later loop iteration" has also eliminated this erroneous note.

[ymandel]

In D145581#4223656, @mboehme wrote:

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks for making me aware of this!

You can auto-link it by mentioning the issue in the commit message of the patch. If it fixes it, for example, add "Fixes #57758" and github will pick up on it automatically.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

 //===--- UseAfterMoveCheck.cpp - clang-tidy -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UseAfterMoveCheck.h"
 
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 #include "../utils/ExprSequence.h"
 #include "../utils/Matchers.h"
 #include <optional>
 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::utils;
 
 namespace clang::tidy::bugprone {
 
 using matchers::hasUnevaluatedContext;
 
 namespace {
 
 /// Contains information about a use-after-move.
 struct UseAfterMove {
   // The DeclRefExpr that constituted the use of the object.
   const DeclRefExpr *DeclRef;
 
   // Is the order in which the move and the use are evaluated undefined?
   bool EvaluationOrderUndefined;
+
+  // Does the use happen in a later loop iteration than the move?
+  bool UseHappensInLaterLoopIteration;
 };
 
 /// Finds uses of a variable after a move (and maintains state required by the
 /// various internal helper functions).
 class UseAfterMoveFinder {
 public:
   UseAfterMoveFinder(ASTContext *TheContext);
 
   // Within the given code block, finds the first use of 'MovedVariable' that
   // occurs after 'MovingCall' (the expression that performs the move). If a
   // use-after-move is found, writes information about it to 'TheUseAfterMove'.
   // Returns whether a use-after-move was found.
   bool find(Stmt *CodeBlock, const Expr *MovingCall,
-            const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+            const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
 
 private:
   bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
                     const ValueDecl *MovedVariable,
                     UseAfterMove *TheUseAfterMove);
   void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
                          llvm::SmallVectorImpl<const DeclRefExpr *> *Uses,
                          llvm::SmallPtrSetImpl<const Stmt *> *Reinits);
   void getDeclRefs(const CFGBlock *Block, const Decl *MovedVariable,
                    llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs);
   void getReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
                   llvm::SmallPtrSetImpl<const Stmt *> *Stmts,
                   llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs);
 
   ASTContext *Context;
   std::unique_ptr<ExprSequence> Sequence;
   std::unique_ptr<StmtToBlockMap> BlockMap;
   llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
 };
 
+/// Returns whether the `Before` block can reach the `After` block.
+bool reaches(const CFGBlock *Before, const CFGBlock *After) {
+  llvm::SmallVector<const CFGBlock *> Stack;
+  llvm::SmallPtrSet<const CFGBlock *, 1> Visited;
+
+  Stack.push_back(Before);
+  while (!Stack.empty()) {
+    const CFGBlock *Current = Stack.back();
+    Stack.pop_back();
+
+    if (Current == After)
+      return true;
+
+    Visited.insert(Current);
+
+    for (const auto &Succ : Current->succs()) {
+      if (!Visited.count(Succ))
+        Stack.push_back(Succ);
+    }
+  }
+
+  return false;
+}
+
 } // namespace
 
 // Matches nodes that are
 // - Part of a decltype argument or class template argument (we check this by
 //   seeing if they are children of a TypeLoc), or
 // - Part of a function template argument (we check this by seeing if they are
 //   children of a DeclRefExpr that references a function template).
 // DeclRefExprs that fulfill these conditions should not be counted as a use or
 // move.
 static StatementMatcher inDecltypeOrTemplateArg() {
   return anyOf(hasAncestor(typeLoc()),
                hasAncestor(declRefExpr(
                    to(functionDecl(ast_matchers::isTemplateInstantiation())))),
                hasAncestor(expr(hasUnevaluatedContext())));
 }
 
 UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
     : Context(TheContext) {}
 
 bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
-                              const ValueDecl *MovedVariable,
+                              const DeclRefExpr *MovedVariable,
                               UseAfterMove *TheUseAfterMove) {
   // Generate the CFG manually instead of through an AnalysisDeclContext because
   // it seems the latter can't be used to generate a CFG for the body of a
   // lambda.
   //
   // We include implicit and temporary destructors in the CFG so that
   // destructors marked [[noreturn]] are handled correctly in the control flow
   // analysis. (These are used in some styles of assertion macros.)
   CFG::BuildOptions Options;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   std::unique_ptr<CFG> TheCFG =
       CFG::buildCFG(nullptr, CodeBlock, Context, Options);
   if (!TheCFG)
     return false;
 
   Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context);
   BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
   Visited.clear();
 
-  const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
-  if (!Block) {
+  const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall);
+  if (!MoveBlock) {
     // This can happen if MovingCall is in a constructor initializer, which is
     // not included in the CFG because the CFG is built only from the function
     // body.
-    Block = &TheCFG->getEntry();
+    MoveBlock = &TheCFG->getEntry();
   }
 
-  return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+  bool found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+                            TheUseAfterMove);
+
+  if (found) {
+    if (const CFGBlock *UseBlock =
+            BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef))
+      // Does the use happen in a later loop iteration than the move?
+      // - If they are in the same CFG block, we know the use happened in a
+      //   later iteration if we visited that block a second time.
+      // - Otherwise, we know the use happened in a later iteration if the
+      //   move is reachable from the use.
+      TheUseAfterMove->UseHappensInLaterLoopIteration =
+          UseBlock == MoveBlock ? Visited.count(UseBlock) != 0
+                                : reaches(UseBlock, MoveBlock);
+  }
+
+  return found;
 }
 
 bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
                                       const Expr *MovingCall,
                                       const ValueDecl *MovedVariable,
                                       UseAfterMove *TheUseAfterMove) {
   if (Visited.count(Block))
     return false;
if (!MovingCall || Sequence->potentiallyAfter(Use, MovingCall)) {
         // Is this a use-after-move that depends on order of evaluation?
         // This is the case if the move potentially comes after the use (and we
         // already know that use potentially comes after the move, which taken
         // together tells us that the ordering is unclear).
         TheUseAfterMove->EvaluationOrderUndefined =
             MovingCall != nullptr &&
             Sequence->potentiallyAfter(MovingCall, Use);
 
+        // We default to false here and change this to true if required in
+        // find().
+        TheUseAfterMove->UseHappensInLaterLoopIteration = false;
+
         return true;
       }
     }
   }
 
   // If the object wasn't reinitialized, call ourselves recursively on all
   // successors.
   if (Reinits.empty()) {
static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
   Check->diag(UseLoc, "'%0' used after it was moved")
       << MoveArg->getDecl()->getName();
   Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
   if (Use.EvaluationOrderUndefined) {
     Check->diag(UseLoc,
                 "the use and move are unsequenced, i.e. there is no guarantee "
                 "about the order in which they are evaluated",
                 DiagnosticIDs::Note);
-  } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+  } else if (Use.UseHappensInLaterLoopIteration) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the move",
                 DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   // try_emplace is a common maybe-moving function that returns a
if (ContainingCtor) {
     CodeBlocks.push_back(ContainingLambda->getBody());
   } else if (ContainingFunc) {
     CodeBlocks.push_back(ContainingFunc->getBody());
   }
 
   for (Stmt *CodeBlock : CodeBlocks) {
     UseAfterMoveFinder Finder(Result.Context);
     UseAfterMove Use;
-    if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+    if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
       emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
   }
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

 }
 
 namespace {
 
 bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
                          ASTContext *Context) {
   if (Descendant == Ancestor)
     return true;
-  for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
-    if (isDescendantOrEqual(Parent, Ancestor, Context))
-      return true;
-  }
+  return llvm::any_of(getParentStmts(Descendant, Context),
+                      [Ancestor, Context](const Stmt *Parent) {
+                        return isDescendantOrEqual(Parent, Ancestor, Context);
+                      });
 
-  return false;
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+                        ASTContext *Context) {
+  return llvm::any_of(Call->arguments(),
+                      [Descendant, Context](const Expr *Arg) {
+                        return isDescendantOrEqual(Descendant, Arg, Context);
+                      });
+}
+
+bool argsContain(const CallExpr *Call, const Stmt *TheStmt) {
+  return std::find(Call->arguments().begin(), Call->arguments().end(),
+                   TheStmt) != Call->arguments().end();
 }
 
 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,
                            ASTContext *TheContext)
     : Context(TheContext), Root(Root) {
   for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
     SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
   }
 }
 
 bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const {
   Before = resolveSyntheticStmt(Before);
   After = resolveSyntheticStmt(After);
 
   // If 'After' is in the subtree of the siblings that follow 'Before' in the
   // chain of successors, we know that 'After' is sequenced after 'Before'.
   for (const Stmt *Successor = getSequenceSuccessor(Before); Successor;
        Successor = getSequenceSuccessor(Successor)) {
     if (isDescendantOrEqual(After, Successor, Context))
       return true;
   }
 
+  SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
+
+  // Since C++17, the callee of a call expression is guaranteed to be sequenced
+  // before all of the arguments.
+  // We handle this as a special case rather than using the general
+  // `getSequenceSuccessor` logic above because the callee expression doesn't
+  // have an unambiguous successor; the order in which arguments are evaluated
+  // is indeterminate.
+  for (const Stmt *Parent : BeforeParents) {
+    // Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as its
+    // base, we consider it to be sequenced _after_ the arguments. This is
+    // because the variable referenced in the base will only actually be
+    // accessed when the call happens, i.e. once all of the arguments have been
+    // evaluated. This has no basis in the C++ standard, but it reflects actual
+    // behavior that is relevant to a use-after-move scenario:
+    //
+    // ```
+    // a.bar(consumeA(std::move(a));
+    // ```
+    //
+    // In this example, we end up accessing `a` after it has been moved from,
+    // even though nominally the callee `a.bar` is evaluated before the argument
+    // `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+    // we implement this logic unconditionally.
+    if (const auto *call = dyn_cast<CXXMemberCallExpr>(Parent)) {
+      if (argsContain(call, Before) &&
+          isa<DeclRefExpr>(
+              call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+          isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+                              Context))
+        return true;
+
+      // We need this additional early exit so that we don't fall through to the
+      // more general logic below.
+      if (auto *Member = dyn_cast<MemberExpr>(Before);
+          Member && call->getCallee() == Member &&
+          isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) &&
+          isDescendantOfArgs(After, call, Context))
+        return false;
+    }
+
+    if (!Context->getLangOpts().CPlusPlus17)
+      continue;
+
+    if (const auto *call = dyn_cast<CallExpr>(Parent);
+        call && call->getCallee() == Before &&
+        isDescendantOfArgs(After, call, Context))
+      return true;
+  }
+
   // If 'After' is a parent of 'Before' or is sequenced after one of these
   // parents, we know that it is sequenced after 'Before'.
-  for (const Stmt *Parent : getParentStmts(Before, Context)) {
+  for (const Stmt *Parent : BeforeParents) {
     if (Parent == After || inSequence(Parent, After))
       return true;
   }
 
   return false;
 }
 
 bool ExprSequence::potentiallyAfter(const Stmt *After,

clang-tools-extra/docs/ReleaseNotes.rst

 
 - Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check.
   Global options of the same name should be used instead.
 
 - Improved :doc:`bugprone-unchecked-optional-access
   <clang-tidy/checks/bugprone/unchecked-optional-access>` check to properly handle calls
   to ``std::forward``.
 
-- Improved :doc:`bugprone-use-after-move
-  <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
-  initializers.
+- :doc:`bugprone-use-after-move
+  <clang-tidy/checks/bugprone/use-after-move>`: Improved check to also cover
+  constructor initializers. Fixed sequencing of designated initializers.
+  Fixed sequencing of callees: In C++17 and later, the callee of a function is
+  guaranteed to be sequenced before the arguments, so don't warn if the use
+  happens in the callee and the move happens in one of the arguments.
 
 - Deprecated :doc:`cert-dcl21-cpp
   <clang-tidy/checks/cert/dcl21-cpp>` check.
 
 - Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`. Please use
   :doc:`cppcoreguidelines-use-default-member-init
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>` instead.

 - Improved :doc:`readability-static-accessed-through-instance
   <clang-tidy/checks/readability/static-accessed-through-instance>` check to
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
 - Fixed a false positive in :doc:`cppcoreguidelines-slicing
   <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.
-
 - Fixed an issue in the :doc:`performance-noexcept-move-constructor
   <clang-tidy/checks/performance/noexcept-move-constructor>` checker that was causing
   false-positives when the move constructor or move assign operator were defaulted.
 
 - Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
 // RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
 
 typedef decltype(nullptr) nullptr_t;
 
 namespace std {
 typedef unsigned size_t;
 
 template <typename T>
public:
   A();
   A(const A &);
   A(A &&);
 
   A &operator=(const A &);
   A &operator=(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
 
   int i;
 };
 
 template <class T>
// the std::move().
     for (int i = 0; i < 10; ++i) {
       a.foo();
       // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
       // CHECK-NOTES: [[@LINE+2]]:7: note: move occurred here
       // CHECK-NOTES: [[@LINE-3]]:7: note: the use happens in a later loop
       std::move(a);
     }
   }
+  // Same as above, but the use and the move are in different CFG blocks.
+  {
+    A a;
+    for (int i = 0; i < 10; ++i) {
+      if (i < 10)
+        a.foo();
+      // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+      // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop
+      if (i < 10)
+        std::move(a);
+    }
+  }
   // However, this case shouldn't be flagged -- the scope of the declaration of
   // 'a' is important.
   {
     for (int i = 0; i < 10; ++i) {
       A a;
       a.foo();
       std::move(a);
     }
for (int i = 0; i < 10; ++i) {
     A a1;
     switch (A a2 = a1; A a3 = std::move(a2)) {
       case true:
         std::move(a3);
     }
   }
 }
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments -- but only in C++17 and later.
+namespace CalleeSequencedBeforeArguments {
+int consumeA(std::unique_ptr<A> a);
+int consumeA(A &&a);
+
+void calleeSequencedBeforeArguments() {
+  {
+    std::unique_ptr<A> a;
+    a->bar(consumeA(std::move(a)));
+    // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved
+    // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here
+    // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced
+  }
+  {
+    std::unique_ptr<A> a;
+    std::unique_ptr<A> getArg(std::unique_ptr<A> a);
+    getArg(std::move(a))->bar(a->getInt());
+    // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here
+    // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced
+  }
+  {
+    A a;
+    // Nominally, the callee `a.bar` is evaluated before the argument
+    // `consumeA(std::move(a))`, but in effect `a` is only accessed after the
+    // call to `A::bar()` happens, i.e. after the argument has been evaluted.
+    a.bar(consumeA(std::move(a)));
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+  }
+}
+} // namespace CalleeSequencedBeforeArguments
+
 // 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;
   ;

 class CtorInitOrder {
 public:
   CtorInitOrder(std::string val)
       : a{val.empty()}, // fine
         b{val.empty()},
         // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
         s{std::move(val)} {} // wrong order
   // CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
-  // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
 
 private:
   bool a;
   std::string s;
   bool b;
 };
 
 struct Obj {};
public:
   PR38187(std::string val) : val_(std::move(val)) {
     val.empty();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val' used after it was moved
     // CHECK-NOTES: [[@LINE-3]]:30: note: move occurred here
   }
 
 private:
   std::string val_;
 };