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

[mboehme]

Summary

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.

[sammccall]

Nice! Only substantive suggestion: I think requiring the base type to be exactly a standard map type is too conservative.

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

396

I'd suggest dropping this condition.

Custom containers often emulate the standard library (e.g. llvm::DenseMap implements try_emplace, as do the absl maps and I found several other examples).

It seems very unlikely to encounter a try_emplace function without these semantics.
(The only case I can imagine is if for some reason they didn't bother to implement the bool return value, so there was no way to know whether emplacement happened. False negative there doesn't seem bad).

402

Maybe explain why try_emplace is special?

try_emplace is a common maybe-moving function that returns a bool that tells callers whether it moved.
Ignore std::move inside try_emplace to avoid false positives, as we don't track uses of the bool.

405

i'm trying to think if there's an important case where the call isn't the immediate parent of the move and yet we're still maybe-moving... but I can't think of one. (Just mentioning in case one occurs to you)

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

173

s/ignored/conservatively assumed *not* to move/?

(It wasn't obvious to me when reading how "ignored" in the implementation translates to the domain)

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

510

calling this function exactly try_emplace seems gratuitiously confusing - since the condition we're changing here is "calls inside a function named try_emplace", just with a different definition of "inside"...

818

hahaha :-)

[mboehme]

Responses to review comments.

[mboehme]

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

396

Thanks -- I wasn't sure myself whether I should be so restrictive, and you make some very convincing points. Done!

402

I've stolen your wording -- thanks!

405

Yeah, I thought about that myself but couldn't come up with anything.

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

818

Yeah, I was being a bit sloppy with my definition of what a map is...

[njames93]

While try_emplace is a special case, it's may not be the only special case in someone's codebase, this should be extended with options to let users handle their special containers.

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

406

Going with a custom list of bad names I'd suggest matching on function decls instead of method decls.

[njames93]

This comment was removed by njames93.

[mboehme]

In D98034#2606488, @njames93 wrote:

While try_emplace is a special case, it's may not be the only special case in someone's codebase, this should be extended with options to let users handle their special containers.

Sorry, I didn't see your comment before submitting.

I agree that there may be other cases of "maybe move" functions, but try_emplace seems to be the most common case. Handling "maybe move" functions more general would require some way of annotating them; I felt it was useful to get the try_emplace case handled first.

[njames93]

In D98034#2606535, @mboehme wrote:

In D98034#2606488, @njames93 wrote:

While try_emplace is a special case, it's may not be the only special case in someone's codebase, this should be extended with options to let users handle their special containers.

Sorry, I didn't see your comment before submitting.

I agree that there may be other cases of "maybe move" functions, but try_emplace seems to be the most common case. Handling "maybe move" functions more general would require some way of annotating them; I felt it was useful to get the try_emplace case handled first.

The canonical way of doing this in clang-tidy is to add an option to the check which takes a semi-colon delimited list.
clang::tidy::utils::options::parseStringList can then turn that into a vector of string.
Annoyingly the hasAnyName matcher needs a vector of StringRef's so most checks do something like this

hasAnyName(SmallVector<StringRef, 4>(
      StringLikeClasses.begin(), StringLikeClasses.end())

As you are reading options, you will need to override the storeOptions method in the check to then store the value read from config, serializeStringList is your friend here.
The documentation will also need updating to describe the option and what it does.

[mboehme]

First of all, sorry for submitting this change prematurely. I don't contribute to LLVM/Clang regularly and forgot that reviews should be kept open for a while after a reviewer has approved them to give others a chance to chime in. I jumped the gun here -- apologies.

In D98034#2606642, @njames93 wrote:

In D98034#2606535, @mboehme wrote:

In D98034#2606488, @njames93 wrote:

While try_emplace is a special case, it's may not be the only special case in someone's codebase, this should be extended with options to let users handle their special containers.

Sorry, I didn't see your comment before submitting.

I agree that there may be other cases of "maybe move" functions, but try_emplace seems to be the most common case. Handling "maybe move" functions more general would require some way of annotating them; I felt it was useful to get the try_emplace case handled first.

The canonical way of doing this in clang-tidy is to add an option to the check which takes a semi-colon delimited list.
clang::tidy::utils::options::parseStringList can then turn that into a vector of string.
Annoyingly the hasAnyName matcher needs a vector of StringRef's so most checks do something like this

hasAnyName(SmallVector<StringRef, 4>(
      StringLikeClasses.begin(), StringLikeClasses.end())

As you are reading options, you will need to override the storeOptions method in the check to then store the value read from config, serializeStringList is your friend here.
The documentation will also need updating to describe the option and what it does.

I think I shouldn't just use a functionDecl(hasAnyName()) matcher with a string list; here's why.

In the case of try_emplace, it makes sense to match on any method (or even a free function) with that name. I was originally restricting myself to only the standard map types, but as @sammccall pointed out to me, someone using this pretty unique name in their own code is very likely to be using the same semantics as the standard map types.

This may not be true for all methods that a user might want to specify though. Say a user has

class MyClass {
public:
  bool Insert(OtherClass &&);
};

where, as for try_emplace, the boolean return value says whether the insertion actually happened.

We wouldn't simply want to ignore std::move arguments on any method called Insert. It's likely that there are other methods called Insert in the codebase that take an rvalue reference and move from it unconditionally, and we want the check to look at those.

So we'd want the user to be able to specify the qualified name MyClass::Insert as the function to ignore. This makes the matcher a bit more interesting as we'd need to work out when seeing a string like this whether MyClass is a class name or a namespace name. Or we might simply do something like

anyOf(
    // maybe `MyClass` is a namespace...
    functionDecl(hasName("MyClass::Insert")),
    // ...or maybe it's a class
    cxxMethodDecl(ofClass(hasName("MyClass")), hasName("Insert"))
)

I assume there isn't an existing matcher that does this -- or are you aware of one?

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

static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
   } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the move",
                 DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto CallMoveMatcher =
       callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
                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(
       traverse(
           TK_AsIs,
           // 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().

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

void f(std::string &&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.
 
+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.
 
 Use
 ---
 
 Any occurrence of the moved variable that is not a reinitialization (see below)

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

 };
 
 template <typename T>
 struct weak_ptr {
   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 {                          \
     name();                              \
     void clear();                        \
     bool empty();                        \
   }
 
 #define DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(name) \
   template <typename T>                              \
   struct name {                                      \
     name();                                          \
     void clear();                                    \
     bool empty();                                    \
     void assign(size_t, const T &);                  \
   }
 
 DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(basic_string);
 DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(vector);
 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);
 
 typedef basic_string<char> string;
 
 template <typename>
 struct remove_reference;
 
void f() {
     std::move(a);
     a.foo();
 
     std::move(static_a);
     static_a.foo();
   }
 };
 
+// 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.
 
 void useAndMoveInLoop() {
   // Warn about use-after-moves if they happen in a later loop iteration than
   // the std::move().
   {
     A a;
void standardContainerClearIsReinit() {
   }
   {
     std::set<int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
-    std::map<int> container;
+    std::map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
     std::multiset<int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
     std::multimap<int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
     std::unordered_set<int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
-    std::unordered_map<int> container;
+    std::unordered_map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();
   }
   {
     std::unordered_multiset<int> container;
     std::move(container);
     container.clear();