[clang-tidy] Fix false positive in cppcoreguidelines-avoid-const-or-ref-data-members when detecting templated classes with inheritance

[HerrCai0907]

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.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

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

Author: Congcong Cai (HerrCai0907)

Changes

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 --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
 {

[HerrCai0907]

ping @5chmidti @PiotrZSL

[carlosgalvezp]

Nit, a class can "have" a copy constructor, but it can be deleted, so it doesn't help here.

I think it would be more accurate to say return isCopyConstructible(Node) || ...

[carlosgalvezp]

LGTM!