[PATCH] Support C++ Core Guidelines copy assignment restrictions

[aaron.ballman]

Summary

Eugene pointed out to me that our existing misc-assign-operator-signature check was almost perfectly implementing the C++ Core Guideline guidance for Copy and move, found here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-ccopy-copy-and-move. The only part that was missing was ensuring that the operator wasn't marked as virtual.

In this patch, I've implemented the check for the virtual qualifier, and registered the checker as a cppcoreguideline checker in addition to its usual place in misc.

~Aaron

[Eugene.Zelenko]

I think it'll be fine to rename check without leaving traces of misc. Same thing happened with modernize-shrink-to-fit.

[aaron.ballman]

In D13510#261825, @Eugene.Zelenko wrote:

I think it'll be fine to rename check without leaving traces of misc. Same thing happened with modernize-shrink-to-fit.

I think the difference here is that many C++ Core Guideline checks are... chatty, and so these checks are likely to not be enabled (especially on existing code bases). By leaving the check in misc-*, it is more likely to provide value to users that aren't able to use the cppcoreguidelines-* checks yet.

[mgehre]

The user will see [cppcoreguidelines-c-copy-assignment-signature] in warnings, but there is no
docs/clang-tidy/checks/cppcoreguidelines-c-copy-assignment-signature.rst.

The user won't know that he needs to look for misc-assign-operator-signature.

Maybe create a link to docs/clang-tidy/checks/misc-assign-operator-signature.rst?
(and also add it to docs/clang-tidy/checks/list.rst)

Also, users reading the documentation will not see that cppcoreguidelines-c-copy-assignment-signature is available.

[sbenza]

In D13510#261925, @aaron.ballman wrote:

In D13510#261825, @Eugene.Zelenko wrote:

I think it'll be fine to rename check without leaving traces of misc. Same thing happened with modernize-shrink-to-fit.

I think the difference here is that many C++ Core Guideline checks are... chatty, and so these checks are likely to not be enabled (especially on existing code bases). By leaving the check in misc-*, it is more likely to provide value to users that aren't able to use the cppcoreguidelines-* checks yet.

Now that we are registering checks with more than one name, it might be a good idea to add a dedup step to avoid redundant warnings and/or wasted resources.
Not on this change but something to consider.

[aaron.ballman]

In D13510#262084, @mgehre wrote:

The user will see [cppcoreguidelines-c-copy-assignment-signature] in warnings, but there is no
docs/clang-tidy/checks/cppcoreguidelines-c-copy-assignment-signature.rst.

The user won't know that he needs to look for misc-assign-operator-signature.

I think this is a good point, but requires more engineering and discussion than this single patch should handle. Checkers registered under multiple names will have to figure something out for documentation.

Maybe create a link to docs/clang-tidy/checks/misc-assign-operator-signature.rst?
(and also add it to docs/clang-tidy/checks/list.rst)

That's not a bad fallback solution if we don't come up with something better as part of the larger documentation discussion.

Also, users reading the documentation will not see that cppcoreguidelines-c-copy-assignment-signature is available.

-checks=* -list-checks does show it as being available.

I have commit in r250165. As for the documentation discussion, I would like to have one, but not when I'm about to embark on a two-week vacation. If the discussion doesn't start by early Nov, I hope to circle back around to it then.

Thanks!

~Aaron

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../misc/AssignOperatorSignatureCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 

         "cppcoreguidelines-pro-type-const-cast");
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
+    CheckFactories.registerCheck<misc::AssignOperatorSignatureCheck>(
+        "cppcoreguidelines-c-copy-assignment-signature");
   }
 };
 

clang-tidy/misc/AssignOperatorSignatureCheck.cpp

           .bind("ArgumentType"),
       this);
 
-  Finder->addMatcher(cxxMethodDecl(IsSelfAssign, isConst()).bind("Const"),
-                     this);
+  Finder->addMatcher(
+      cxxMethodDecl(IsSelfAssign, anyOf(isConst(), isVirtual())).bind("cv"),
+      this);
 }
 
-
 void AssignOperatorSignatureCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto* Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");

   static const char *Messages[][2] = {
       {"ReturnType", "operator=() should return '%0&'"},
       {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
-      {"Const", "operator=() should not be marked 'const'"},
+      {"cv", "operator=() should not be marked '%1'"}
   };
 
-  for (const auto& Message : Messages) {
+  for (const auto &Message : Messages) {
     if (Result.Nodes.getNodeAs<Decl>(Message[0]))
-      diag(Method->getLocStart(), Message[1]) << Name;
+      diag(Method->getLocStart(), Message[1])
+          << Name << (Method->isConst() ? "const" : "virtual");
   }
 }
 

test/clang-tidy/misc-assign-operator-signature.cpp

   // Pre-C++11 way of disabling assignment.
   void operator=(const Private &);
 };
+
+struct Virtual {
+  virtual Virtual& operator=(const Virtual &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'virtual'
+};