[clang-tools-extra] [clang-tidy] properly handle private move constructors in `modernize-pass-by-value` check (PR #141304)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

<details>
<summary>Changes</summary>

Fixed false positives when class passed by const-reference had a private move constructor, which could not be used for a fix-it.

---
Full diff: https://github.com/llvm/llvm-project/pull/141304.diff


3 Files Affected:

- (modified) clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp (+44-11) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp (+59) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index 35f90fb8da15b..343cc966b7e09 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -20,8 +20,21 @@ using namespace llvm;
 
 namespace clang::tidy::modernize {
 
+static bool isFirstFriendOfSecond(const CXXRecordDecl *Friend,
+                                  const CXXRecordDecl *Class) {
+  return llvm::any_of(
+      Class->friends(), [Friend](FriendDecl *FriendDecl) -> bool {
+        if (TypeSourceInfo *FriendTypeSource = FriendDecl->getFriendType()) {
+          const QualType FriendType = FriendTypeSource->getType();
+          return FriendType->getAsCXXRecordDecl() == Friend;
+        }
+        return false;
+      });
+}
+
 namespace {
-/// Matches move-constructible classes.
+/// Matches move-constructible classes whose constructor can be called inside
+/// a CXXRecordDecl with a bound ID. 
 ///
 /// Given
 /// \code
@@ -32,15 +45,33 @@ namespace {
 ///     Bar(Bar &&) = deleted;
 ///     int a;
 ///   };
+///
+///   class Buz {
+///     Buz(Buz &&);
+///     int a;
+///     friend class Outer; 
+///   };
+///
+///   class Outer {
+///   };
 /// \endcode
-/// recordDecl(isMoveConstructible())
-///   matches "Foo".
-AST_MATCHER(CXXRecordDecl, isMoveConstructible) {
-  for (const CXXConstructorDecl *Ctor : Node.ctors()) {
-    if (Ctor->isMoveConstructor() && !Ctor->isDeleted())
-      return true;
-  }
-  return false;
+/// recordDecl(isMoveConstructibleInBoundCXXRecordDecl("Outer"))
+///   matches "Foo", "Buz".
+AST_MATCHER_P(CXXRecordDecl, isMoveConstructibleInBoundCXXRecordDecl, StringRef,
+              RecordDeclID) {
+  return Builder->removeBindings(
+      [this,
+       &Node](const ast_matchers::internal::BoundNodesMap &Nodes) -> bool {
+        const auto *BoundClass =
+            Nodes.getNode(this->RecordDeclID).get<CXXRecordDecl>();
+        for (const CXXConstructorDecl *Ctor : Node.ctors()) {
+          if (Ctor->isMoveConstructor() && !Ctor->isDeleted() &&
+              (Ctor->getAccess() == AS_public ||
+               (BoundClass && isFirstFriendOfSecond(BoundClass, &Node))))
+            return false;
+        }
+        return true;
+      });
 }
 } // namespace
 
@@ -202,6 +233,7 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) {
       traverse(
           TK_AsIs,
           cxxConstructorDecl(
+              ofClass(cxxRecordDecl().bind("outer")),
               forEachConstructorInitializer(
                   cxxCtorInitializer(
                       unless(isBaseInitializer()),
@@ -225,8 +257,9 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) {
                                   .bind("Param"))))),
                           hasDeclaration(cxxConstructorDecl(
                               isCopyConstructor(), unless(isDeleted()),
-                              hasDeclContext(
-                                  cxxRecordDecl(isMoveConstructible())))))))
+                              hasDeclContext(cxxRecordDecl(
+                                  isMoveConstructibleInBoundCXXRecordDecl(
+                                      "outer"))))))))
                       .bind("Initializer")))
               .bind("Ctor")),
       this);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8032f0d23f494..75f3471070646 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -203,6 +203,10 @@ Changes in existing checks
   excluding variables with ``thread_local`` storage class specifier from being
   matched.
 
+- Improved :doc:`modernize-pass-by-value
+  <clang-tidy/checks/modernize/pass-by-value>` check by fixing false positives
+  when class passed by const-reference had a private move constructor.
+
 - Improved :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` check by matching
   ``constexpr`` and ``static``` values on member initialization and by detecting
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
index be33988607b27..7538862dd83f8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp
@@ -252,3 +252,62 @@ struct W3 {
   W3(W1 &&, Movable &&M);
   Movable M;
 };
+
+struct ProtectedMovable {
+  ProtectedMovable() = default;
+  ProtectedMovable(const ProtectedMovable &) {}
+protected:
+  ProtectedMovable(ProtectedMovable &&) {}
+};
+
+struct PrivateMovable {
+  PrivateMovable() = default;
+  PrivateMovable(const PrivateMovable &) {}
+private:
+  PrivateMovable(PrivateMovable &&) {}
+
+  friend struct X5;
+};
+
+struct InheritedProtectedMovable : ProtectedMovable {
+  InheritedProtectedMovable() = default;
+  InheritedProtectedMovable(const InheritedProtectedMovable &) {}
+  InheritedProtectedMovable(InheritedProtectedMovable &&) {}
+};
+
+struct InheritedPrivateMovable : PrivateMovable {
+  InheritedPrivateMovable() = default;
+  InheritedPrivateMovable(const InheritedPrivateMovable &) {}
+  InheritedPrivateMovable(InheritedPrivateMovable &&) {}
+};
+
+struct X1 {
+  X1(const ProtectedMovable &M) : M(M) {}
+  ProtectedMovable M;
+};
+
+struct X2 {
+  X2(const PrivateMovable &M) : M(M) {}
+  PrivateMovable M;
+};
+
+struct X3 {
+  X3(const InheritedProtectedMovable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+  // CHECK-FIXES: X3(InheritedProtectedMovable M) : M(std::move(M)) {}
+  InheritedProtectedMovable M;
+};
+
+struct X4 {
+  X4(const InheritedPrivateMovable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+  // CHECK-FIXES: X4(InheritedPrivateMovable M) : M(std::move(M)) {}
+  InheritedPrivateMovable M;
+};
+
+struct X5 {
+  X5(const PrivateMovable &M) : M(M) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+  // CHECK-FIXES: X5(PrivateMovable M) : M(std::move(M)) {}
+  PrivateMovable M;
+};

``````````

</details>


https://github.com/llvm/llvm-project/pull/141304