[clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[carlosgalvezp]

Summary

Since that's what the guidelines require.

Fixes #63733

[carlosgalvezp]

Remove confusing comment.

[carlosgalvezp]

Fix typo in release notes.

[carlosgalvezp]

Remove extra newline

[carlosgalvezp]

Split getInfo function into 2 functions, remove
structured bindings.

[PiotrZSL]

LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for a user defined special members it looks ok, but for some implicit ones I worry it may not always work. Probably things like "(hasSimpleCopyAssigment()) || (hasUserDeclaredCopyAssigment() && check here if its not deleted)" would be needed.
Thing is that CXXRecordDecl got most info (in confused way), there are things like DefaultedMoveConstructorIsDeleted that could be used to verify somehow base class.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

94–106

Check first type, should be cheaper and consider mering those two.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

6–9

[carlosgalvezp]

In D155625#4512123, @PiotrZSL wrote:

LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for a user defined special members it looks ok, but for some implicit ones I worry it may not always work. Probably things like "(hasSimpleCopyAssigment()) || (hasUserDeclaredCopyAssigment() && check here if its not deleted)" would be needed.
Thing is that CXXRecordDecl got most info (in confused way), there are things like DefaultedMoveConstructorIsDeleted that could be used to verify somehow base class.

Yeah I spent a lot of time trying to figure this out, somehow I expected this logic to already exist somewhere in Sema! Thanks for the pointers, I will experiment with those functions as well.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

94–106

Thanks for the tip! I'm not familiar with having multiple binds in the same addMatcher call. Do I still need to keep the bind("ref") at the end?

[PiotrZSL]

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

94–106

No, bind at the end need to be removed (I forgot about that).

[carlosgalvezp]

Address review comments

[carlosgalvezp]

Using hasSimpleCopyConstructor and so on greatly simplifies the logic, great! Let me know if you are happy with it or I should go ahead and merge.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

94–106

Somehow I didn't manage to get it to work with your proposed change and removing bind, I feel I'm walking on thin ice here :)

[carlosgalvezp]

Merge matchers.

[carlosgalvezp]

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

94–106

Doing it from scratch helped, probably some parenthesis that was off!

[PiotrZSL]

Looks fine.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp

 
 namespace clang::tidy::cppcoreguidelines {
 namespace {
 
 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;
+    }
+  }
+
+  return Constructors;
+}
+
+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;
+    }
+
+    if (Method->isMoveAssignmentOperator()) {
+      Assignments.Move.Declared = true;
+      if (Method->isDeleted())
+        Assignments.Move.Deleted = true;
+    }
+  }
+
+  return Assignments;
+}
+
+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))
+    return true;
+
+  return false;
+}
+
 } // namespace
 
 void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()),
-                               hasType(hasCanonicalType(referenceType())))
-                         .bind("ref"),
-                     this);
-  Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()),
-                               hasType(qualType(isConstQualified())))
-                         .bind("const"),
-                     this);
+  Finder->addMatcher(
+      fieldDecl(unless(isMemberOfLambda()),
+                hasType(hasCanonicalType(referenceType())),
+                hasDeclContext(cxxRecordDecl(isCopyableOrMovable())))
+  Finder->addMatcher(
+      fieldDecl(unless(isMemberOfLambda()),
+                hasType(qualType(isConstQualified())),
+                hasDeclContext(cxxRecordDecl(isCopyableOrMovable())))
 }
 
 void AvoidConstOrRefDataMembersCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("ref"))
     diag(MatchedDecl->getLocation(), "member %0 of type %1 is a reference")
         << MatchedDecl << MatchedDecl->getType();
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("const"))
     diag(MatchedDecl->getLocation(), "member %0 of type %1 is const qualified")
         << MatchedDecl << MatchedDecl->getType();
 }
 
 } // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>` check. Detect uses and moves in
   constructor initializers. Correctly handle constructor arguments as being
   sequenced when constructor call is written as list-initialization. Understand
   that there is a sequence point between designated initializers.
 
 - Deprecated :doc:`cert-dcl21-cpp
   <clang-tidy/checks/cert/dcl21-cpp>` check.
 
+- Fixed :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check
+  to emit warnings only on classes that are copyable/movable, as required by the
+  corresponding rule.
+
 - Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`. Please use
   :doc:`cppcoreguidelines-use-default-member-init
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>` instead.
 
 - Improved :doc:`cppcoreguidelines-pro-bounds-constant-array-index
   <clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index>` check
   to cover type aliases of ``std::array``.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

 .. title:: clang-tidy - cppcoreguidelines-avoid-const-or-ref-data-members
 
 cppcoreguidelines-avoid-const-or-ref-data-members
 =================================================
 
-This check warns when structs or classes have const-qualified or reference
-(lvalue or rvalue) data members. Having such members is rarely useful, and
-makes the class only copy-constructible but not copy-assignable.
+This check warns when structs or classes that are copyable or movable, and have
+const-qualified or reference (lvalue or rvalue) data members. Having such
+members is rarely useful, and makes the class only copy-constructible but not
+copy-assignable.
 
 Examples:
 
 .. code-block:: c++
 
   // Bad, const-qualified member
   struct Const {
     const int x;

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

void lambdas()
   auto ir = [&]{
     auto c1 = x1;
     auto c2 = x2;
     auto c3 = x3;
     auto c4 = x4;
     auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};