[clang-tools-extra] e67d91f - [clang-tidy] Use-after-move: Ignore moves inside a try_emplace.

[Martin Boehme via cfe-commits]

Author: Martin Boehme
Date: 2021-03-05T15:05:09+01:00
New Revision: e67d91faec2183789b220c15444929fb2efa6ecd

URL: https://github.com/llvm/llvm-project/commit/e67d91faec2183789b220c15444929fb2efa6ecd
DIFF: https://github.com/llvm/llvm-project/commit/e67d91faec2183789b220c15444929fb2efa6ecd.diff

LOG: [clang-tidy] Use-after-move: Ignore moves inside a try_emplace.

We have no way to reason about the bool returned by try_emplace, so we
simply ignore any std::move()s that happen in a try_emplace argument.
A lot of the time in this situation, the code will be checking the
bool and doing something else if it turns out the value wasn't moved
into the map, and this has been causing false positives so far.

I don't currently have any intentions of handling "maybe move" functions
more generally.

Reviewed By: sammccall

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

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
    clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
    clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index 028fefa9b99e..136d8f862b95 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -398,7 +398,13 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
                hasArgument(0, declRefExpr().bind("arg")),
                anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
                      hasAncestor(functionDecl().bind("containing-func"))),
-               unless(inDecltypeOrTemplateArg()))
+               unless(inDecltypeOrTemplateArg()),
+               // try_emplace is a common maybe-moving function that returns a
+               // bool to tell callers whether it moved. Ignore std::move inside
+               // try_emplace to avoid false positives as we don't track uses of
+               // the bool.
+               unless(hasParent(cxxMemberCallExpr(
+                   callee(cxxMethodDecl(hasName("try_emplace")))))))
           .bind("call-move");
 
   Finder->addMatcher(

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
index aab7cfd0ccd4..8509292eff99 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
@@ -169,6 +169,10 @@ that a move always takes place:
 The check will assume that the last line causes a move, even though, in this
 particular case, it does not. Again, this is intentional.
 
+There is one special case: A call to ``std::move`` inside a ``try_emplace`` call
+is conservatively assumed not to move. This is to avoid spurious warnings, as
+the check has no way to reason about the ``bool`` returned by ``try_emplace``.
+
 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.

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 527c79840696..73ca59ccc91b 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
@@ -32,6 +32,31 @@ struct weak_ptr {
   bool expired() const;
 };
 
+template <typename T1, typename T2>
+struct pair {};
+
+template <typename Key, typename T>
+struct map {
+  struct iterator {};
+
+  map();
+  void clear();
+  bool empty();
+  template <class... Args>
+  pair<iterator, bool> try_emplace(const Key &key, Args &&...args);
+};
+
+template <typename Key, typename T>
+struct unordered_map {
+  struct iterator {};
+
+  unordered_map();
+  void clear();
+  bool empty();
+  template <class... Args>
+  pair<iterator, bool> try_emplace(const Key &key, Args &&...args);
+};
+
 #define DECLARE_STANDARD_CONTAINER(name) \
   template <typename T>                  \
   struct name {                          \
@@ -55,11 +80,9 @@ DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(deque);
 DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
 DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
 DECLARE_STANDARD_CONTAINER(set);
-DECLARE_STANDARD_CONTAINER(map);
 DECLARE_STANDARD_CONTAINER(multiset);
 DECLARE_STANDARD_CONTAINER(multimap);
 DECLARE_STANDARD_CONTAINER(unordered_set);
-DECLARE_STANDARD_CONTAINER(unordered_map);
 DECLARE_STANDARD_CONTAINER(unordered_multiset);
 DECLARE_STANDARD_CONTAINER(unordered_multimap);
 
@@ -483,6 +506,22 @@ class IgnoreMemberVariables {
   }
 };
 
+// Ignore moves that happen in a try_emplace.
+void ignoreMoveInTryEmplace() {
+  {
+    std::map<int, A> amap;
+    A a;
+    amap.try_emplace(1, std::move(a));
+    a.foo();
+  }
+  {
+    std::unordered_map<int, A> amap;
+    A a;
+    amap.try_emplace(1, std::move(a));
+    a.foo();
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Tests involving control flow.
 
@@ -776,7 +815,7 @@ void standardContainerClearIsReinit() {
     container.empty();
   }
   {
-    std::map<int> container;
+    std::map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();
@@ -800,7 +839,7 @@ void standardContainerClearIsReinit() {
     container.empty();
   }
   {
-    std::unordered_map<int> container;
+    std::unordered_map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();