[clang-tools-extra] [clang-tidy] fix false positive when detecting templated classes with inheritance (PR #115180)

[via cfe-commits]

llvmbot wrote:


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

@llvm/pr-subscribers-clang-tools-extra

Author: Congcong Cai (HerrCai0907)

<details>
<summary>Changes</summary>

`hasSimpleCopyConstructor` series of functions are not reliable when these functions are not resolved. We need to manually resolve the status of these functions from its base classes.
Fixes: #<!-- -->111985.


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


3 Files Affected:

- (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp (+67-55) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp (+22) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
index 6a6e620a4387b0..e1914eabd93f08 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
@@ -19,75 +19,87 @@ AST_MATCHER(FieldDecl, isMemberOfLambda) {
   return Node.getParent()->isLambda();
 }
 
-struct MemberFunctionInfo {
-  bool Declared{};
-  bool Deleted{};
-};
-
-struct MemberFunctionPairInfo {
-  MemberFunctionInfo Copy{};
-  MemberFunctionInfo Move{};
-};
-
-MemberFunctionPairInfo getConstructorsInfo(CXXRecordDecl const &Node) {
-  MemberFunctionPairInfo Constructors{};
-
-  for (CXXConstructorDecl const *Ctor : Node.ctors()) {
-    if (Ctor->isCopyConstructor()) {
-      Constructors.Copy.Declared = true;
-      if (Ctor->isDeleted())
-        Constructors.Copy.Deleted = true;
-    }
-    if (Ctor->isMoveConstructor()) {
-      Constructors.Move.Declared = true;
-      if (Ctor->isDeleted())
-        Constructors.Move.Deleted = true;
+bool hasCopyConstructor(CXXRecordDecl const &Node) {
+  if (Node.needsOverloadResolutionForCopyConstructor() &&
+      Node.needsImplicitCopyConstructor()) {
+    // unresolved
+    for (CXXBaseSpecifier const &BS : Node.bases()) {
+      CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+      if (BRD != nullptr)
+        if (!hasCopyConstructor(*BRD))
+          return false;
     }
   }
-
-  return Constructors;
+  if (Node.hasSimpleCopyConstructor())
+    return true;
+  for (CXXConstructorDecl const *Ctor : Node.ctors())
+    if (Ctor->isCopyConstructor())
+      return !Ctor->isDeleted();
+  return false;
 }
 
-MemberFunctionPairInfo getAssignmentsInfo(CXXRecordDecl const &Node) {
-  MemberFunctionPairInfo Assignments{};
-
-  for (CXXMethodDecl const *Method : Node.methods()) {
-    if (Method->isCopyAssignmentOperator()) {
-      Assignments.Copy.Declared = true;
-      if (Method->isDeleted())
-        Assignments.Copy.Deleted = true;
+bool hasMoveConstructor(CXXRecordDecl const &Node) {
+  if (Node.needsOverloadResolutionForMoveConstructor() &&
+      Node.needsImplicitMoveConstructor()) {
+    // unresolved
+    for (CXXBaseSpecifier const &BS : Node.bases()) {
+      CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+      if (BRD != nullptr)
+        if (!hasMoveConstructor(*BRD))
+          return false;
     }
+  }
+  if (Node.hasSimpleMoveConstructor())
+    return true;
+  for (CXXConstructorDecl const *Ctor : Node.ctors())
+    if (Ctor->isMoveConstructor())
+      return !Ctor->isDeleted();
+  return false;
+}
 
-    if (Method->isMoveAssignmentOperator()) {
-      Assignments.Move.Declared = true;
-      if (Method->isDeleted())
-        Assignments.Move.Deleted = true;
+bool hasCopyAssignment(CXXRecordDecl const &Node) {
+  if (Node.needsOverloadResolutionForCopyAssignment() &&
+      Node.needsImplicitCopyAssignment()) {
+    // unresolved
+    for (CXXBaseSpecifier const &BS : Node.bases()) {
+      CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+      if (BRD != nullptr)
+        if (!hasCopyAssignment(*BRD))
+          return false;
     }
   }
-
-  return Assignments;
+  if (Node.hasSimpleCopyAssignment())
+    return true;
+  for (CXXMethodDecl const *Method : Node.methods())
+    if (Method->isCopyAssignmentOperator())
+      return !Method->isDeleted();
+  return false;
 }
 
-AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
-  MemberFunctionPairInfo Constructors = getConstructorsInfo(Node);
-  MemberFunctionPairInfo Assignments = getAssignmentsInfo(Node);
-
-  if (Node.hasSimpleCopyConstructor() ||
-      (Constructors.Copy.Declared && !Constructors.Copy.Deleted))
-    return true;
-  if (Node.hasSimpleMoveConstructor() ||
-      (Constructors.Move.Declared && !Constructors.Move.Deleted))
-    return true;
-  if (Node.hasSimpleCopyAssignment() ||
-      (Assignments.Copy.Declared && !Assignments.Copy.Deleted))
-    return true;
-  if (Node.hasSimpleMoveAssignment() ||
-      (Assignments.Move.Declared && !Assignments.Move.Deleted))
+bool hasMoveAssignment(CXXRecordDecl const &Node) {
+  if (Node.needsOverloadResolutionForMoveAssignment() &&
+      Node.needsImplicitMoveAssignment()) {
+    // unresolved
+    for (CXXBaseSpecifier const &BS : Node.bases()) {
+      CXXRecordDecl const *BRD = BS.getType()->getAsCXXRecordDecl();
+      if (BRD != nullptr)
+        if (!hasMoveAssignment(*BRD))
+          return false;
+    }
+  }
+  if (Node.hasSimpleMoveAssignment())
     return true;
-
+  for (CXXMethodDecl const *Method : Node.methods())
+    if (Method->isMoveAssignmentOperator())
+      return !Method->isDeleted();
   return false;
 }
 
+AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) {
+  return hasCopyConstructor(Node) || hasMoveConstructor(Node) ||
+         hasCopyAssignment(Node) || hasMoveAssignment(Node);
+}
+
 } // namespace
 
 void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 51ba157ab05deb..f4cc32fbe50711 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -190,6 +190,10 @@ Changes in existing checks
   fix false positive that floating point variable is only used in increment
   expression.
 
+- Improved :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check to
+  avoid false positive when detecting templated class with inheritance.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   avoid false positive when member initialization depends on a structured
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
index e3864be134da3c..19da88300aec46 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -285,6 +285,28 @@ struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
   int& x;  // OK, non copyable nor movable
 };
 
+template<class T> struct TemplateInheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+template<class T> struct TemplateInheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+template<class T> struct TemplateInheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+template<class T> struct TemplateInheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
 // Test composition
 struct ContainsNonCopyable
 {

``````````

</details>


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