[clang-tidy] Add proper emplace checks to modernize-use-emplace

modernize-use-emplace only recommends going from a push_back to an
emplace_back, but does not provide a recommendation when emplace_back is
improperly used. This adds the functionality of warning the user when
an unecessary temporary is created while calling emplace_back or other "emplacy"
functions from the STL containers.

Reviewed By: kuhar, ivanmurashko

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

Author: nicovank

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp

@@ -15,6 +15,69 @@ namespace tidy {
 namespace modernize {
 
 namespace {
+// Identical to hasAnyName, except it does not take template specifiers into
+// account. This is used to match the functions names as in
+// DefaultEmplacyFunctions below without caring about the template types of the
+// containers.
+AST_MATCHER_P(NamedDecl, hasAnyNameIgnoringTemplates, std::vector<StringRef>,
+              Names) {
+  const std::string FullName = "::" + Node.getQualifiedNameAsString();
+
+  // This loop removes template specifiers by only keeping characters not within
+  // template brackets. We keep a depth count to handle nested templates. For
+  // example, it'll transform a::b<c<d>>::e<f> to simply a::b::e.
+  std::string FullNameTrimmed;
+  int Depth = 0;
+  for (const auto &Character : FullName) {
+    if (Character == '<') {
+      ++Depth;
+    } else if (Character == '>') {
+      --Depth;
+    } else if (Depth == 0) {
+      FullNameTrimmed.append(1, Character);
+    }
+  }
+
+  // This loop is taken from HasNameMatcher::matchesNodeFullSlow in
+  // clang/lib/ASTMatchers/ASTMatchersInternal.cpp and checks whether
+  // FullNameTrimmed matches any of the given Names.
+  const StringRef FullNameTrimmedRef = FullNameTrimmed;
+  for (const StringRef Pattern : Names) {
+    if (Pattern.startswith("::")) {
+      if (FullNameTrimmed == Pattern)
+        return true;
+    } else if (FullNameTrimmedRef.endswith(Pattern) &&
+               FullNameTrimmedRef.drop_back(Pattern.size()).endswith("::")) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+// Checks if the given matcher is the last argument of the given CallExpr.
+AST_MATCHER_P(CallExpr, hasLastArgument,
+              clang::ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (Node.getNumArgs() == 0)
+    return false;
+
+  return InnerMatcher.matches(*Node.getArg(Node.getNumArgs() - 1), Finder,
+                              Builder);
+}
+
+// Checks if the given member call has the same number of arguments as the
+// function had parameters defined (this is useful to check if there is only one
+// variadic argument).
+AST_MATCHER(CXXMemberCallExpr, hasSameNumArgsAsDeclNumParams) {
+  if (Node.getMethodDecl()->isFunctionTemplateSpecialization())
+    return Node.getNumArgs() == Node.getMethodDecl()
+                                    ->getPrimaryTemplate()
+                                    ->getTemplatedDecl()
+                                    ->getNumParams();
+
+  return Node.getNumArgs() == Node.getMethodDecl()->getNumParams();
+}
+
 AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
   return Node.hasExplicitTemplateArgs();
 }
@@ -25,6 +88,20 @@ const auto DefaultSmartPointers =
     "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
 const auto DefaultTupleTypes = "::std::pair; ::std::tuple";
 const auto DefaultTupleMakeFunctions = "::std::make_pair; ::std::make_tuple";
+const auto DefaultEmplacyFunctions =
+    "vector::emplace_back; vector::emplace;"
+    "deque::emplace; deque::emplace_front; deque::emplace_back;"
+    "forward_list::emplace_after; forward_list::emplace_front;"
+    "list::emplace; list::emplace_back; list::emplace_front;"
+    "set::emplace; set::emplace_hint;"
+    "map::emplace; map::emplace_hint;"
+    "multiset::emplace; multiset::emplace_hint;"
+    "multimap::emplace; multimap::emplace_hint;"
+    "unordered_set::emplace; unordered_set::emplace_hint;"
+    "unordered_map::emplace; unordered_map::emplace_hint;"
+    "unordered_multiset::emplace; unordered_multiset::emplace_hint;"
+    "unordered_multimap::emplace; unordered_multimap::emplace_hint;"
+    "stack::emplace; queue::emplace; priority_queue::emplace";
 } // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
@@ -37,7 +114,9 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
       TupleTypes(utils::options::parseStringList(
           Options.get("TupleTypes", DefaultTupleTypes))),
       TupleMakeFunctions(utils::options::parseStringList(
-          Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))) {}
+          Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))),
+      EmplacyFunctions(utils::options::parseStringList(
+          Options.get("EmplacyFunctions", DefaultEmplacyFunctions))) {}
 
 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   // FIXME: Bunch of functionality that could be easily added:
@@ -52,6 +131,13 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
       hasDeclaration(functionDecl(hasName("push_back"))),
       on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPushBack)))));
 
+  auto CallEmplacy = cxxMemberCallExpr(
+      hasDeclaration(
+          functionDecl(hasAnyNameIgnoringTemplates(EmplacyFunctions))),
+      on(hasType(cxxRecordDecl(has(typedefNameDecl(
+          hasName("value_type"), hasType(type(hasUnqualifiedDesugaredType(
+                                     recordType().bind("value_type"))))))))));
+
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
@@ -73,8 +159,9 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   auto ConstructingDerived =
       hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
-  // emplace_back can't access private constructor.
-  auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
+  // emplace_back can't access private or protected constructors.
+  auto IsPrivateOrProtectedCtor =
+      hasDeclaration(cxxConstructorDecl(anyOf(isPrivate(), isProtected())));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
                            has(cxxStdInitializerListExpr()));
@@ -85,7 +172,7 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
       cxxConstructExpr(
           unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
                        InitializerListAsArgument, NewExprAsArgument,
-                       ConstructingDerived, IsPrivateCtor)))
+                       ConstructingDerived, IsPrivateOrProtectedCtor)))
           .bind("ctor");
   auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
@@ -102,36 +189,86 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
       hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(TupleTypes))))));
 
   auto SoughtParam = materializeTemporaryExpr(
-      anyOf(has(MakeTuple), has(MakeTupleCtor),
-            HasConstructExpr, has(cxxFunctionalCastExpr(HasConstructExpr))));
+      anyOf(has(MakeTuple), has(MakeTupleCtor), HasConstructExpr,
+            has(cxxFunctionalCastExpr(HasConstructExpr))));
+
+  auto HasConstructExprWithValueTypeType =
+      has(ignoringImplicit(cxxConstructExpr(
+          SoughtConstructExpr, hasType(type(hasUnqualifiedDesugaredType(
+                                   type(equalsBoundNode("value_type"))))))));
+
+  auto HasConstructExprWithValueTypeTypeAsLastArgument =
+      hasLastArgument(materializeTemporaryExpr(anyOf(
+          HasConstructExprWithValueTypeType,
+          has(cxxFunctionalCastExpr(HasConstructExprWithValueTypeType)))));
 
   Finder->addMatcher(
       traverse(TK_AsIs, cxxMemberCallExpr(CallPushBack, has(SoughtParam),
                                           unless(isInTemplateInstantiation()))
-                            .bind("call")),
+                            .bind("push_back_call")),
+      this);
+
+  Finder->addMatcher(
+      traverse(TK_AsIs,
+               cxxMemberCallExpr(
+                   CallEmplacy, HasConstructExprWithValueTypeTypeAsLastArgument,
+                   hasSameNumArgsAsDeclNumParams(),
+                   unless(isInTemplateInstantiation()))
+                   .bind("emplacy_call")),
+      this);
+
+  Finder->addMatcher(
+      traverse(
+          TK_AsIs,
+          cxxMemberCallExpr(
+              CallEmplacy,
+              on(hasType(cxxRecordDecl(has(typedefNameDecl(
+                  hasName("value_type"),
+                  hasType(type(
+                      hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+                          cxxRecordDecl(hasAnyName(SmallVector<StringRef, 2>(
+                              TupleTypes.begin(), TupleTypes.end()))))))))))))),
+              has(MakeTuple), hasSameNumArgsAsDeclNumParams(),
+              unless(isInTemplateInstantiation()))
+              .bind("emplacy_call")),
       this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+  const auto *PushBackCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
+  const auto *EmplacyCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
   const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make");
+
+  assert((PushBackCall || EmplacyCall) && "No call matched");
   assert((CtorCall || MakeCall) && "No push_back parameter matched");
 
+  const CXXMemberCallExpr *Call = PushBackCall ? PushBackCall : EmplacyCall;
+
   if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 &&
       CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange())
     return;
 
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       Call->getExprLoc(), Call->getArg(0)->getExprLoc());
 
-  auto Diag = diag(Call->getExprLoc(), "use emplace_back instead of push_back");
+  auto Diag =
+      PushBackCall
+          ? diag(Call->getExprLoc(), "use emplace_back instead of push_back")
+          : diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(),
+                 "unnecessary temporary object created while calling " +
+                     Call->getMethodDecl()->getName().str());
 
   if (FunctionNameSourceRange.getBegin().isMacroID())
     return;
 
-  const auto *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back(";
-  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix);
+  if (PushBackCall) {
+    const char *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back(";
+    Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
+                                         EmplacePrefix);
+  }
 
   const SourceRange CallParensRange =
       MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(),
@@ -143,15 +280,22 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
     return;
 
   const SourceLocation ExprBegin =
-      MakeCall ? MakeCall->getExprLoc() : CtorCall->getExprLoc();
+      CtorCall ? CtorCall->getExprLoc() : MakeCall->getExprLoc();
 
   // Range for constructor name and opening brace.
   const auto ParamCallSourceRange =
       CharSourceRange::getTokenRange(ExprBegin, CallParensRange.getBegin());
 
   Diag << FixItHint::CreateRemoval(ParamCallSourceRange)
        << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-           CallParensRange.getEnd(), CallParensRange.getEnd()));
+              CallParensRange.getEnd(), CallParensRange.getEnd()));
+
+  if (MakeCall && EmplacyCall) {
+    // Remove extra left parenthesis
+    Diag << FixItHint::CreateRemoval(
+        CharSourceRange::getCharRange(MakeCall->getCallee()->getEndLoc(),
+                                      MakeCall->getArg(0)->getBeginLoc()));
+  }
 }
 
 void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
@@ -164,6 +308,8 @@ void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
                 utils::options::serializeStringList(TupleTypes));
   Options.store(Opts, "TupleMakeFunctions",
                 utils::options::serializeStringList(TupleMakeFunctions));
+  Options.store(Opts, "EmplacyFunctions",
+                utils::options::serializeStringList(EmplacyFunctions));
 }
 
 } // namespace modernize

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h

@@ -40,6 +40,7 @@ class UseEmplaceCheck : public ClangTidyCheck {
   const std::vector<StringRef> SmartPointers;
   const std::vector<StringRef> TupleTypes;
   const std::vector<StringRef> TupleMakeFunctions;
+  const std::vector<StringRef> EmplacyFunctions;
 };
 
 } // namespace modernize

clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst

@@ -15,28 +15,42 @@ because replacing ``insert`` with ``emplace`` may result in
 By default only ``std::vector``, ``std::deque``, ``std::list`` are considered.
 This list can be modified using the :option:`ContainersWithPushBack` option.
 
+This check also reports when an ``emplace``-like method is improperly used,
+for example using ``emplace_back`` while also calling a constructor. This
+creates a temporary that requires at best a move and at worst a copy. Almost all
+``emplace``-like functions in the STL are covered by this, with ``try_emplace``
+on ``std::map`` and ``std::unordered_map`` being the exception as it behaves
+slightly differently than all the others. More containers can be added with the
+:option:`EmplacyFunctions` option, so long as the container defines a
+``value_type`` type, and the ``emplace``-like functions construct a
+``value_type`` object.
+
 Before:
 
 .. code-block:: c++
 
     std::vector<MyClass> v;
     v.push_back(MyClass(21, 37));
+    v.emplace_back(MyClass(21, 37));
 
     std::vector<std::pair<int, int>> w;
 
     w.push_back(std::pair<int, int>(21, 37));
     w.push_back(std::make_pair(21L, 37L));
+    w.emplace_back(std::make_pair(21L, 37L));
 
 After:
 
 .. code-block:: c++
 
     std::vector<MyClass> v;
     v.emplace_back(21, 37);
+    v.emplace_back(21, 37);
 
     std::vector<std::pair<int, int>> w;
     w.emplace_back(21, 37);
     w.emplace_back(21L, 37L);
+    w.emplace_back(21L, 37L);
 
 By default, the check is able to remove unnecessary ``std::make_pair`` and
 ``std::make_tuple`` calls from ``push_back`` calls on containers of
@@ -128,20 +142,30 @@ Options
     function calls will be removed from ``push_back`` calls and turned into
     ``emplace_back``.
 
+.. option:: EmplacyFunctions
+
+    Semicolon-separated list of containers without their template parameters
+    and some ``emplace``-like method of the container. Example:
+    ``vector::emplace_back``. Those methods will be checked for improper use and
+    the check will report when a temporary is unnecessarily created.
+
 Example
 ^^^^^^^
 
 .. code-block:: c++
 
   std::vector<MyTuple<int, bool, char>> x;
   x.push_back(MakeMyTuple(1, false, 'x'));
+  x.emplace_back(MakeMyTuple(1, false, 'x'));
 
 transforms to:
 
 .. code-block:: c++
 
   std::vector<MyTuple<int, bool, char>> x;
   x.emplace_back(1, false, 'x');
+  x.emplace_back(1, false, 'x');
 
-when :option:`TupleTypes` is set to ``MyTuple`` and :option:`TupleMakeFunctions`
-is set to ``MakeMyTuple``.
+when :option:`TupleTypes` is set to ``MyTuple``, :option:`TupleMakeFunctions`
+is set to ``MakeMyTuple``, and :option:`EmplacyFunctions` is set to
+``vector::emplace_back``.