[clang-tidy] Add check 'misc-use-after-move'

Summary:
The check warns if an object is used after it has been moved, without an
intervening reinitialization.

See user-facing documentation for details.

Reviewers: sbenza, Prazek, alexfh

Subscribers: beanz, mgorny, shadeware, omtcyfz, Eugene.Zelenko, Prazek, fowles, ioeric, cfe-commits

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

llvm-svn: 281453

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

@@ -43,6 +43,7 @@ add_clang_library(clangTidyMiscModule
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UnusedUsingDeclsCheck.cpp
+  UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -51,6 +51,7 @@
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UnusedUsingDeclsCheck.h"
+#include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
@@ -139,6 +140,7 @@ public:
     CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");
     CheckFactories.registerCheck<UnusedUsingDeclsCheck>(
         "misc-unused-using-decls");
+    CheckFactories.registerCheck<UseAfterMoveCheck>("misc-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "misc-virtual-near-miss");
   }

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

@@ -0,0 +1,643 @@
+//===--- UseAfterMoveCheck.cpp - clang-tidy -------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseAfterMoveCheck.h"
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a `CFGBlock`.
+///
+/// While a `CFGBlock` does contain individual `CFGElement`s for some
+/// sub-expressions, the order in which those `CFGElement`s appear reflects
+/// only one possible order in which the sub-expressions may be evaluated.
+/// However, we want to warn if any of the potential evaluation orders can lead
+/// to a use-after-move, not just the one contained in the `CFGBlock`.
+///
+/// This class implements only a simplified version of the C++ sequencing rules
+/// that is, however, sufficient for the purposes of this check. The main
+/// limitation is that we do not distinguish between value computation and side
+/// effect -- see the "Implementation" section for more details.
+///
+/// Note: `SequenceChecker` from SemaChecking.cpp does a similar job (and much
+/// more thoroughly), but using it would require
+/// - Pulling `SequenceChecker` out into a header file (i.e. making it part of
+///   the API),
+/// - Removing the dependency of `SequenceChecker` on `Sema`, and
+/// - (Probably) modifying `SequenceChecker` to make it suitable to be used in
+///   this context.
+/// For the moment, it seems preferable to re-implement our own version of
+/// sequence checking that is special-cased to what we need here.
+///
+/// Implementation
+/// --------------
+///
+/// `ExprSequence` uses two types of sequencing edges between nodes in the AST:
+///
+/// - Every `Stmt` is assumed to be sequenced after its children. This is
+///   overly optimistic because the standard only states that value computations
+///   of operands are sequenced before the value computation of the operator,
+///   making no guarantees about side effects (in general).
+///
+///   For our purposes, this rule is sufficient, however, because this check is
+///   interested in operations on objects, which are generally performed through
+///   function calls (whether explicit and implicit). Function calls guarantee
+///   that the value computations and side effects for all function arguments
+///   are sequenced before the execution fo the function.
+///
+/// - In addition, some `Stmt`s are known to be sequenced before or after
+///   their siblings. For example, the `Stmt`s that make up a `CompoundStmt`are
+///   all sequenced relative to each other. The function
+///   `getSequenceSuccessor()` implements these sequencing rules.
+class ExprSequence {
+public:
+  /// Initializes this `ExprSequence` with sequence information for the given
+  /// `CFG`.
+  ExprSequence(const CFG *TheCFG, ASTContext *TheContext);
+
+  /// Returns whether \p Before is sequenced before \p After.
+  bool inSequence(const Stmt *Before, const Stmt *After) const;
+
+  /// Returns whether \p After can potentially be evaluated after \p Before.
+  /// This is exactly equivalent to `!inSequence(After, Before)` but makes some
+  /// conditions read more naturally.
+  bool potentiallyAfter(const Stmt *After, const Stmt *Before) const;
+
+private:
+  // Returns the sibling of \p S (if any) that is directly sequenced after \p S,
+  // or nullptr if no such sibling exists. For example, if \p S is the child of
+  // a `CompoundStmt`, this would return the Stmt that directly follows \p S in
+  // the `CompoundStmt`.
+  //
+  // As the sequencing of many constructs that change control flow is already
+  // encoded in the `CFG`, this function only implements the sequencing rules
+  // for those constructs where sequencing cannot be inferred from the `CFG`.
+  const Stmt *getSequenceSuccessor(const Stmt *S) const;
+
+  const Stmt *resolveSyntheticStmt(const Stmt *S) const;
+
+  ASTContext *Context;
+
+  llvm::DenseMap<const Stmt *, const Stmt *> SyntheticStmtSourceMap;
+};
+
+/// Maps `Stmt`s to the `CFGBlock` that contains them. Some `Stmt`s may be
+/// contained in more than one `CFGBlock`; in this case, they are mapped to the
+/// innermost block (i.e. the one that is furthest from the root of the tree).
+class StmtToBlockMap {
+public:
+  /// Initializes the map for the given `CFG`.
+  StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext);
+
+  /// Returns the block that \p S is contained in. Some `Stmt`s may be contained
+  /// in more than one `CFGBlock`; in this case, this function returns the
+  /// innermost block (i.e. the one that is furthest from the root of the tree).
+  const CFGBlock *blockContainingStmt(const Stmt *S) const;
+
+private:
+  ASTContext *Context;
+
+  llvm::DenseMap<const Stmt *, const CFGBlock *> Map;
+};
+
+/// 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;
+};
+
+/// 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 function body, 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 *FunctionBody, const Expr *MovingCall,
+            const ValueDecl *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;
+};
+
+} // namespace
+
+// Returns the Stmt nodes that are parents of 'S', skipping any potential
+// intermediate non-Stmt nodes.
+//
+// In almost all cases, this function returns a single parent or no parents at
+// all.
+//
+// The case that a Stmt has multiple parents is rare but does actually occur in
+// the parts of the AST that we're interested in. Specifically, InitListExpr
+// nodes cause ASTContext::getParent() to return multiple parents for certain
+// nodes in their subtree because RecursiveASTVisitor visits both the syntactic
+// and semantic forms of InitListExpr, and the parent-child relationships are
+// different between the two forms.
+static SmallVector<const Stmt *, 1> getParentStmts(const Stmt *S,
+                                                   ASTContext *Context) {
+  SmallVector<const Stmt *, 1> Result;
+
+  ASTContext::DynTypedNodeList Parents = Context->getParents(*S);
+
+  SmallVector<ast_type_traits::DynTypedNode, 1> NodesToProcess(Parents.begin(),
+                                                               Parents.end());
+
+  while (!NodesToProcess.empty()) {
+    ast_type_traits::DynTypedNode Node = NodesToProcess.back();
+    NodesToProcess.pop_back();
+
+    if (const auto *S = Node.get<Stmt>()) {
+      Result.push_back(S);
+    } else {
+      Parents = Context->getParents(Node);
+      NodesToProcess.append(Parents.begin(), Parents.end());
+    }
+  }
+
+  return Result;
+}
+
+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 false;
+}
+
+ExprSequence::ExprSequence(const CFG *TheCFG, ASTContext *TheContext)
+    : Context(TheContext) {
+  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;
+  }
+
+  // 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)) {
+    if (Parent == After || inSequence(Parent, After))
+      return true;
+  }
+
+  return false;
+}
+
+bool ExprSequence::potentiallyAfter(const Stmt *After,
+                                    const Stmt *Before) const {
+  return !inSequence(After, Before);
+}
+
+const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const {
+  for (const Stmt *Parent : getParentStmts(S, Context)) {
+    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)
+        return BO->getRHS();
+    } 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);
+      }
+    } else if (const auto *Compound = dyn_cast<CompoundStmt>(Parent)) {
+      // Compound statement: Each sub-statement is sequenced after the
+      // statements that precede it.
+      const Stmt *Previous = nullptr;
+      for (const auto *Child : Compound->body()) {
+        if (Previous == S)
+          return Child;
+        Previous = Child;
+      }
+    } else if (const auto *TheDeclStmt = dyn_cast<DeclStmt>(Parent)) {
+      // Declaration: Every initializer expression is sequenced after the
+      // initializer expressions that precede it.
+      const Expr *PreviousInit = nullptr;
+      for (const Decl *TheDecl : TheDeclStmt->decls()) {
+        if (const auto *TheVarDecl = dyn_cast<VarDecl>(TheDecl)) {
+          if (const Expr *Init = TheVarDecl->getInit()) {
+            if (PreviousInit == S)
+              return Init;
+            PreviousInit = Init;
+          }
+        }
+      }
+    } else if (const auto *ForRange = dyn_cast<CXXForRangeStmt>(Parent)) {
+      // Range-based for: Loop variable declaration is sequenced before the
+      // body. (We need this rule because these get placed in the same
+      // CFGBlock.)
+      if (S == ForRange->getLoopVarStmt())
+        return ForRange->getBody();
+    } else if (const auto *TheIfStmt = dyn_cast<IfStmt>(Parent)) {
+      // If statement: If a variable is declared inside the condition, the
+      // expression used to initialize the variable is sequenced before the
+      // evaluation of the condition.
+      if (S == TheIfStmt->getConditionVariableDeclStmt())
+        return TheIfStmt->getCond();
+    }
+  }
+
+  return nullptr;
+}
+
+const Stmt *ExprSequence::resolveSyntheticStmt(const Stmt *S) const {
+  if (SyntheticStmtSourceMap.count(S))
+    return SyntheticStmtSourceMap.lookup(S);
+  else
+    return S;
+}
+
+StmtToBlockMap::StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext)
+    : Context(TheContext) {
+  for (const auto *B : *TheCFG) {
+    for (const auto &Elem : *B) {
+      if (Optional<CFGStmt> S = Elem.getAs<CFGStmt>())
+        Map[S->getStmt()] = B;
+    }
+  }
+}
+
+const CFGBlock *StmtToBlockMap::blockContainingStmt(const Stmt *S) const {
+  while (!Map.count(S)) {
+    SmallVector<const Stmt *, 1> Parents = getParentStmts(S, Context);
+    if (Parents.empty())
+      return nullptr;
+    S = Parents[0];
+  }
+
+  return Map.lookup(S);
+}
+
+// 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())))));
+}
+
+UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
+    : Context(TheContext) {}
+
+bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
+                              const ValueDecl *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
+  // labmda.
+  //
+  // 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, FunctionBody, Context, Options);
+  if (!TheCFG)
+    return false;
+
+  Sequence.reset(new ExprSequence(TheCFG.get(), Context));
+  BlockMap.reset(new StmtToBlockMap(TheCFG.get(), Context));
+  Visited.clear();
+
+  const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
+  if (!Block)
+    return false;
+
+  return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+}
+
+bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
+                                      const Expr *MovingCall,
+                                      const ValueDecl *MovedVariable,
+                                      UseAfterMove *TheUseAfterMove) {
+  if (Visited.count(Block))
+    return false;
+
+  // Mark the block as visited (except if this is the block containing the
+  // std::move() and it's being visited the first time).
+  if (!MovingCall)
+    Visited.insert(Block);
+
+  // Get all uses and reinits in the block.
+  llvm::SmallVector<const DeclRefExpr *, 1> Uses;
+  llvm::SmallPtrSet<const Stmt *, 1> Reinits;
+  getUsesAndReinits(Block, MovedVariable, &Uses, &Reinits);
+
+  // Ignore all reinitializations where the move potentially comes after the
+  // reinit.
+  llvm::SmallVector<const Stmt *, 1> ReinitsToDelete;
+  for (const Stmt *Reinit : Reinits) {
+    if (MovingCall && Sequence->potentiallyAfter(MovingCall, Reinit))
+      ReinitsToDelete.push_back(Reinit);
+  }
+  for (const Stmt *Reinit : ReinitsToDelete) {
+    Reinits.erase(Reinit);
+  }
+
+  // Find all uses that potentially come after the move.
+  for (const DeclRefExpr *Use : Uses) {
+    if (!MovingCall || Sequence->potentiallyAfter(Use, MovingCall)) {
+      // Does the use have a saving reinit? A reinit is saving if it definitely
+      // comes before the use, i.e. if there's no potential that the reinit is
+      // after the use.
+      bool HaveSavingReinit = false;
+      for (const Stmt *Reinit : Reinits) {
+        if (!Sequence->potentiallyAfter(Reinit, Use))
+          HaveSavingReinit = true;
+      }
+
+      if (!HaveSavingReinit) {
+        TheUseAfterMove->DeclRef = Use;
+
+        // 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);
+
+        return true;
+      }
+    }
+  }
+
+  // If the object wasn't reinitialized, call ourselves recursively on all
+  // successors.
+  if (Reinits.empty()) {
+    for (const auto &Succ : Block->succs()) {
+      if (Succ && findInternal(Succ, nullptr, MovedVariable, TheUseAfterMove))
+        return true;
+    }
+  }
+
+  return false;
+}
+
+void UseAfterMoveFinder::getUsesAndReinits(
+    const CFGBlock *Block, const ValueDecl *MovedVariable,
+    llvm::SmallVectorImpl<const DeclRefExpr *> *Uses,
+    llvm::SmallPtrSetImpl<const Stmt *> *Reinits) {
+  llvm::SmallPtrSet<const DeclRefExpr *, 1> DeclRefs;
+  llvm::SmallPtrSet<const DeclRefExpr *, 1> ReinitDeclRefs;
+
+  getDeclRefs(Block, MovedVariable, &DeclRefs);
+  getReinits(Block, MovedVariable, Reinits, &ReinitDeclRefs);
+
+  // All references to the variable that aren't reinitializations are uses.
+  Uses->clear();
+  for (const DeclRefExpr *DeclRef : DeclRefs) {
+    if (!ReinitDeclRefs.count(DeclRef))
+      Uses->push_back(DeclRef);
+  }
+
+  // Sort the uses by their occurrence in the source code.
+  std::sort(Uses->begin(), Uses->end(),
+            [](const DeclRefExpr *D1, const DeclRefExpr *D2) {
+              return D1->getExprLoc() < D2->getExprLoc();
+            });
+}
+
+void UseAfterMoveFinder::getDeclRefs(
+    const CFGBlock *Block, const Decl *MovedVariable,
+    llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
+  DeclRefs->clear();
+  for (const auto &Elem : *Block) {
+    Optional<CFGStmt> S = Elem.getAs<CFGStmt>();
+    if (!S)
+      continue;
+
+    SmallVector<BoundNodes, 1> Matches =
+        match(findAll(declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
+                                  unless(inDecltypeOrTemplateArg()))
+                          .bind("declref")),
+              *S->getStmt(), *Context);
+
+    for (const auto &Match : Matches) {
+      const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref");
+      if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block)
+        DeclRefs->insert(DeclRef);
+    }
+  }
+}
+
+void UseAfterMoveFinder::getReinits(
+    const CFGBlock *Block, const ValueDecl *MovedVariable,
+    llvm::SmallPtrSetImpl<const Stmt *> *Stmts,
+    llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
+  auto DeclRefMatcher =
+      declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
+
+  auto StandardContainerTypeMatcher = hasType(cxxRecordDecl(
+      hasAnyName("::std::basic_string", "::std::vector", "::std::deque",
+                 "::std::forward_list", "::std::list", "::std::set",
+                 "::std::map", "::std::multiset", "::std::multimap",
+                 "::std::unordered_set", "::std::unordered_map",
+                 "::std::unordered_multiset", "::std::unordered_multimap")));
+
+  // Matches different types of reinitialization.
+  auto ReinitMatcher =
+      stmt(anyOf(
+               // Assignment. In addition to the overloaded assignment operator,
+               // test for built-in assignment as well, since template functions
+               // may be instantiated to use std::move() on built-in types.
+               binaryOperator(hasOperatorName("="), hasLHS(DeclRefMatcher)),
+               cxxOperatorCallExpr(hasOverloadedOperatorName("="),
+                                   hasArgument(0, DeclRefMatcher)),
+               // Declaration. We treat this as a type of reinitialization too,
+               // so we don't need to treat it separately.
+               declStmt(hasDescendant(equalsNode(MovedVariable))),
+               // clear() and assign() on standard containers.
+               cxxMemberCallExpr(
+                   on(allOf(DeclRefMatcher, StandardContainerTypeMatcher)),
+                   // To keep the matcher simple, we check for assign() calls
+                   // on all standard containers, even though only vector,
+                   // deque, forward_list and list have assign(). If assign()
+                   // is called on any of the other containers, this will be
+                   // flagged by a compile error anyway.
+                   callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
+               // Passing variable to a function as a non-const pointer.
+               callExpr(forEachArgumentWithParam(
+                   unaryOperator(hasOperatorName("&"),
+                                 hasUnaryOperand(DeclRefMatcher)),
+                   unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))),
+               // Passing variable to a function as a non-const lvalue reference
+               // (unless that function is std::move()).
+               callExpr(forEachArgumentWithParam(
+                            DeclRefMatcher,
+                            unless(parmVarDecl(hasType(
+                                references(qualType(isConstQualified())))))),
+                        unless(callee(functionDecl(hasName("::std::move")))))))
+          .bind("reinit");
+
+  Stmts->clear();
+  DeclRefs->clear();
+  for (const auto &Elem : *Block) {
+    Optional<CFGStmt> S = Elem.getAs<CFGStmt>();
+    if (!S)
+      continue;
+
+    SmallVector<BoundNodes, 1> Matches =
+        match(findAll(ReinitMatcher), *S->getStmt(), *Context);
+
+    for (const auto &Match : Matches) {
+      const auto *TheStmt = Match.getNodeAs<Stmt>("reinit");
+      const auto *TheDeclRef = Match.getNodeAs<DeclRefExpr>("declref");
+      if (TheStmt && BlockMap->blockContainingStmt(TheStmt) == Block) {
+        Stmts->insert(TheStmt);
+
+        // We count DeclStmts as reinitializations, but they don't have a
+        // DeclRefExpr associated with them -- so we need to check 'TheDeclRef'
+        // before adding it to the set.
+        if (TheDeclRef)
+          DeclRefs->insert(TheDeclRef);
+      }
+    }
+  }
+}
+
+static void emitDiagnostic(const Expr *MovingCall,
+                           const ValueDecl *MovedVariable,
+                           const UseAfterMove &Use, ClangTidyCheck *Check,
+                           ASTContext *Context) {
+  Check->diag(Use.DeclRef->getExprLoc(), "'%0' used after it was moved")
+      << MovedVariable->getName();
+  Check->diag(MovingCall->getExprLoc(), "move occurred here",
+              DiagnosticIDs::Note);
+  if (Use.EvaluationOrderUndefined) {
+    Check->diag(Use.DeclRef->getExprLoc(),
+                "the use and move are unsequenced, i.e. there is no guarantee "
+                "about the order in which they are evaluated",
+                DiagnosticIDs::Note);
+  }
+}
+
+void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  auto StandardSmartPointerTypeMatcher = hasType(
+      cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr")));
+
+  auto CallMoveMatcher =
+      callExpr(
+          callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+          hasArgument(
+              0,
+              declRefExpr(unless(StandardSmartPointerTypeMatcher)).bind("arg")),
+          anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+                hasAncestor(functionDecl().bind("containing-func"))),
+          unless(inDecltypeOrTemplateArg()))
+          .bind("call-move");
+
+  Finder->addMatcher(
+      // To find the Stmt that we assume performs the actual move, we look for
+      // the direct ancestor of the std::move() that isn't one of the node
+      // types ignored by ignoringParenImpCasts().
+      stmt(forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
+           unless(expr(ignoringParenImpCasts(equalsBoundNode("call-move")))))
+          .bind("moving-call"),
+      this);
+}
+
+void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ContainingLambda =
+      Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
+  const auto *ContainingFunc =
+      Result.Nodes.getNodeAs<FunctionDecl>("containing-func");
+  const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
+  const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call");
+  const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg");
+
+  if (!MovingCall)
+    MovingCall = CallMove;
+
+  Stmt *FunctionBody = nullptr;
+  if (ContainingLambda)
+    FunctionBody = ContainingLambda->getBody();
+  else if (ContainingFunc)
+    FunctionBody = ContainingFunc->getBody();
+  else
+    return;
+
+  const ValueDecl *MovedVariable = Arg->getDecl();
+
+  // Ignore the std::move if the variable that was passed to it isn't a local
+  // variable.
+  if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
+    return;
+
+  UseAfterMoveFinder finder(Result.Context);
+  UseAfterMove Use;
+  if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use))
+    emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context);
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.h

@@ -0,0 +1,36 @@
+//===--- UseAfterMoveCheck.h - clang-tidy ---------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// The check warns if an object is used after it has been moved, without an
+/// intervening reinitialization.
+///
+/// For details, see the user-facing documentation:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html
+class UseAfterMoveCheck : public ClangTidyCheck {
+public:
+  UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEAFTERMOVECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -73,6 +73,12 @@ Improvements to clang-tidy
   Warns when ``std::move`` is applied to a forwarding reference instead of
   ``std::forward``.
 
+- New `misc-use-after-move
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html>`_ check
+
+  Warns if an object is used after it has been moved, without an intervening
+  reinitialization.
+
 - New `mpi-buffer-deref
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html>`_ check
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -93,6 +93,7 @@ Clang-Tidy Checks
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
+   misc-use-after-move
    misc-virtual-near-miss
    modernize-avoid-bind
    modernize-deprecated-headers

clang-tools-extra/docs/clang-tidy/checks/misc-use-after-move.rst

@@ -0,0 +1,197 @@
+.. title:: clang-tidy - misc-use-after-move
+
+misc-use-after-move
+===================
+
+Warns if an object is used after it has been moved, for example:
+
+  .. code-block:: c++
+
+    std::string str = "Hello, world!\n";
+    std::vector<std::string> messages;
+    messages.emplace_back(std::move(str));
+    std::cout << str;
+
+The last line will trigger a warning that ``str`` is used after it has been
+moved.
+
+The check does not trigger a warning if the object is reinitialized after the
+move and before the use. For example, no warning will be output for this code:
+
+  .. code-block:: c++
+
+    messages.emplace_back(std::move(str));
+    str = "Greetings, stranger!\n";
+    std::cout << str;
+
+The check takes control flow into account. A warning is only emitted if the use
+can be reached from the move. This means that the following code does not
+produce a warning:
+
+  .. code-block:: c++
+
+    if (condition) {
+      messages.emplace_back(std::move(str));
+    } else {
+      std::cout << str;
+    }
+
+On the other hand, the following code does produce a warning:
+
+  .. code-block:: c++
+
+    for (int i = 0; i < 10; ++i) {
+      std::cout << str;
+      messages.emplace_back(std::move(str));
+    }
+
+(The use-after-move happens on the second iteration of the loop.)
+
+In some cases, the check may not be able to detect that two branches are
+mutually exclusive. For example (assuming that ``i`` is an int):
+
+  .. code-block:: c++
+
+    if (i == 1) {
+      messages.emplace_back(std::move(str));
+    }
+    if (i == 2) {
+      std::cout << str;
+    }
+
+In this case, the check will erroneously produce a warning, even though it is
+not possible for both the move and the use to be executed.
+
+An erroneous warning can be silenced by reinitializing the object after the
+move:
+
+  .. code-block:: c++
+
+    if (i == 1) {
+      messages.emplace_back(std::move(str));
+      str = "";
+    }
+    if (i == 2) {
+      std::cout << str;
+    }
+
+No warnings are emitted for objects of type ``std::unique_ptr`` and
+``std::shared_ptr``, as they have defined move behavior. (Objects of these
+classes are guaranteed to be empty after they have been moved from.)
+
+Subsections below explain more precisely what exactly the check considers to be
+a move, use, and reinitialization.
+
+Unsequenced moves, uses, and reinitializations
+----------------------------------------------
+
+In many cases, C++ does not make any guarantees about the order in which
+sub-expressions of a statement are evaluated. This means that in code like the
+following, it is not guaranteed whether the use will happen before or after the
+move:
+
+  .. code-block:: c++
+
+    void f(int i, std::vector<int> v);
+    std::vector<int> v = { 1, 2, 3 };
+    f(v[1], std::move(v));
+
+In this kind of situation, the check will note that the use and move are
+unsequenced.
+
+The check will also take sequencing rules into account when reinitializations
+occur in the same statement as moves or uses. A reinitialization is only
+considered to reinitialize a variable if it is guaranteed to be evaluated after
+the move and before the use.
+
+Move
+----
+
+The check currently only considers calls of ``std::move`` on local variables or
+function parameters. It does not check moves of member variables or global
+variables.
+
+Any call of ``std::move`` on a variable is considered to cause a move of that
+variable, even if the result of ``std::move`` is not passed to an rvalue
+reference parameter.
+
+This means that the check will flag a use-after-move even on a type that does
+not define a move constructor or move assignment operator. This is intentional.
+Developers may use ``std::move`` on such a type in the expectation that the type
+will add move semantics in the future. If such a ``std::move`` has the potential
+to cause a use-after-move, we want to warn about it even if the type does not
+implement move semantics yet.
+
+Furthermore, if the result of ``std::move`` *is* passed to an rvalue reference
+parameter, this will always be considered to cause a move, even if the function
+that consumes this parameter does not move from it, or if it does so only
+conditionally. For example, in the following situation, the check will assume
+that a move always takes place:
+
+  .. code-block:: c++
+
+    std::vector<std::string> messages;
+    void f(std::string &&str) {
+      // Only remember the message if it isn't empty.
+      if (!str.empty()) {
+        messages.emplace_back(std::move(str));
+      }
+    }
+    std::string str = "";
+    f(std::move(str));
+
+The check will assume that the last line causes a move, even though, in this
+particular case, it does not. Again, this is intentional.
+
+When analyzing the order in which moves, uses and reinitializations happen (see
+section `Unsequenced moves, uses, and reinitializations`_), the move is assumed
+to occur in whichever function the result of the ``std::move`` is passed to.
+
+Use
+---
+
+Any occurrence of the moved variable that is not a reinitialization (see below)
+is considered to be a use.
+
+If multiple uses occur after a move, only the first of these is flagged.
+
+Reinitialization
+----------------
+
+The check considers a variable to be reinitialized in the following cases:
+
+  - The variable occurs on the left-hand side of an assignment.
+
+  - The variable is passed to a function as a non-const pointer or non-const
+    lvalue reference. (It is assumed that the variable may be an out-parameter
+    for the function.)
+
+  - ``clear()`` or ``assign()`` is called on the variable and the variable is of
+    one of the standard container types ``basic_string``, ``vector``, ``deque``,
+    ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, ``multimap``,
+    ``unordered_set``, ``unordered_map``, ``unordered_multiset``,
+    ``unordered_multimap``.
+
+If the variable in question is a struct and an individual member variable of
+that struct is written to, the check does not consider this to be a
+reinitialization -- even if, eventually, all member variables of the struct are
+written to. For example:
+
+  .. code-block:: c++
+
+    struct S {
+      std::string str;
+      int i;
+    };
+    S s = { "Hello, world!\n", 42 };
+    S s_other = std::move(s);
+    s.str = "Lorem ipsum";
+    s.i = 99;
+
+The check will not consider ``s`` to be reinitialized after the last line;
+instead, the line that assigns to ``s.str`` will be flagged as a use-after-move.
+This is intentional as this pattern of reinitializing a struct is error-prone.
+For example, if an additional member variable is added to ``S``, it is easy to
+forget to add the reinitialization for this additional member. Instead, it is
+safer to assign to the entire struct in one go, and this will also avoid the
+use-after-move warning.