[clang-tidy] add check cppcoreguidelines-special-member-functions

[jbcoe]

Summary

Check for classes that violate the rule of five and zero as specified in CppCoreGuidelines:

"If a class defines or deletes a default operation then it should define or delete them all."

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst. See pre-4.0 branch versions as example.

docs/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.rst

16

Please add space between = and delete.

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp

13

Please separate different cases with empty lines.

40

Looks like = delete is missed fall all class methods.

[Eugene.Zelenko]

Will be good idea to introduce similar check for C++98/03.

[Prazek]

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp

74–78

I think it would be much better to aggregate the diagnosics.
E.g. if I declare 4 special functions then I will get 4 lines of warnings, and I won't even know what function did I forgot to declare.

So it should be better to fire diag on the first, or just one of the special function and say "class %0 defines {{list_here}} but doesn't define {{other_list}}"

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h

20

no comma after all. But I might be wrong because I am not certified grammar nazi

docs/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.rst

12

typo s/expliciti/explicit

[aaron.ballman]

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp

64

No newline.

73

No newline.

74–78

I tend to agree -- this will get very chatty if I define 4 out of the 5 special member functions, so it might be best if the diagnostic instead reads:

"class %0 defines %1, but does not define %2"

Where %1 is the list of things the class defines and %2 is the list of things the class fails to define.

Note, "define or delete" is a bit weird because a deleted function is a definition, just like an explicitly-defaulted function is a definition. It seems strange to mention delete but not default, so I just went with "define" by itself for brevity.

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h

20

The comma is correct there. :-)

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp

45

I'd also like to see a test case with mixed =default and =delete (to ensure we do not trigger when explicitly defaulting or deleting the some special member functions).

[jbcoe]

Addressed review comments, thanks for those, check is nicer now.

[jbcoe]

Address comments from review, thanks for those.

[Eugene.Zelenko]

Since rule name is different in C++98/03 and C++11 or newer it will make sense to create two checks which will work only for their respective versions (of course, implementation should be shared). Or create alias name.

Check is still not mentioned in docs/ReleaseNotes.rst.

[Prazek]

I will look again soon, but it looks much better right now!

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp

44–58

maybe make it a DenseMap? (or maybe there is SmallMap or SmallDense map or something similar)

86–106

This looks like it could be changed into for
for (auto [str, kind] : kinds) {

if( ... Result.Nodes.getNodeAs<FunctionDecl>(str))

I havent tried putting Base type to getNodeAs, but I think it should work.

[Prazek]

In D22513#493303, @Eugene.Zelenko wrote:

Since rule name is different in C++98/03 and C++11 or newer it will make sense to create two checks which will work only for their respective versions (of course, implementation should be shared). Or create alias name.

Check is still not mentioned in docs/ReleaseNotes.rst.

This seems like too much work, maybe just cppcoreguidelines-rule-of-3-or-5?

[jbcoe]

Rename to cppcoreguidelines-special-member-functions to avoid the duplication required with naming it rule-of-3 and rule-of-5.

Reduce code duplication by iterating over a list of matchers.

Use dense map. [FIXME: I can't get DenseMap to work with the DenseMapInfo defined in the header. IdentifierNamingCheck.cpp does this without any issues but I hit compiler errors around DenseMap and SourceLocation. Moving the specialisation from the header to the source of SpecialMemberFunctionsCheck suffices to repeat the problem. Any input here would be appreciated.]

[Eugene.Zelenko]

docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst

4 ↗

(On Diff #65414)

Size should be same as size of name above.

[jbcoe]

Fix underline length in docs.

[aaron.ballman]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

62 ↗

(On Diff #65423)

I think this join function can maybe be replaced by a call to llvm::join() from StringExtras.h?

64 ↗

(On Diff #65423)

It's good to put an && along with some explanatory text for debugging if this assertion ever triggers.

73 ↗

(On Diff #65423)

Elide braces per usual style conventions.

94 ↗

(On Diff #65423)

The & should bind to KV rather than auto (clang-format should handle this for you).

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

42–44 ↗

(On Diff #65423)

Do these need to be public?

62–63 ↗

(On Diff #65423)

Any reason why not to do this as part of this patch?

[jbcoe]

Minor changes from review.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

62 ↗

(On Diff #65423)

I want to join the last member function with "and" or "for", llvm::join won't let me do that.
Thanks for the suggestion though. I need to explore more of llvm/ADT.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

42–44 ↗

(On Diff #65423)

I think so, I can't get the specialisation of DenseMapInfo to work otherwise.

62–63 ↗

(On Diff #65423)

I've spent days trying. I can't get code to compile if it's moved and can't work out why. As the FIXME says, it works in IdentifierNaming.

[aaron.ballman]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

60 ↗

(On Diff #66011)

Btw, with MSVC, this will give you a "not all control paths return a value" warning. You should put an llvm_unreachable() after the switch to silence that diagnostic.

63 ↗

(On Diff #66011)

Ah, crud, good point.

[jbcoe]

Fix MSVC warning.

[aaron.ballman]

LGTM, thank you for the check! The oddness with the DenseMap can wait until a follow-up patch, btw.

[alexfh]

Thanks for the new check! Looks awesome! A couple of late comments inline.

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

91 ↗

(On Diff #66219)

Is the explicit use of the initializer_list needed to pacify MSVC? I wonder whether std::string can be replaced with a StringRef?

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp

6 ↗

(On Diff #66219)

Native speakers can correct me, but "does not define X or Y" reads as if defining either X or Y is enough to fix the issue, while in fact we're expecting all special members to be defined. I'm not sure what the best wording would be, maybe "class 'C' defines X and Y, but does not define all other special members: T, U, V"?

[ericLemanissier]

I have an segfault on all the source files of my project when I enable this check (it works ok when I disable this check). Sorry I don't have time to post a minimal source file producing the segfault. I will maybe tomorrow, or in two weeks.

clang-tidy/cppcoreguidelines/CMakeLists.txt

   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
+  RuleOfFiveAndZeroCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
+#include "RuleOfFiveAndZeroCheck.h"
 
 namespace clang {
 namespace tidy {

         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
+    CheckFactories.registerCheck<RuleOfFiveAndZeroCheck>(
+        "cppcoreguidelines-rule-of-five-and-zero");
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h

+//===--- RuleOfFiveAndZeroCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RULE_OF_FIVE_AND_ZERO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RULE_OF_FIVE_AND_ZERO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Checks for classes where some, but not all, of the special member functions
+/// are defined.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.html
+class RuleOfFiveAndZeroCheck : public ClangTidyCheck {
+public:
+  RuleOfFiveAndZeroCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void onEndOfTranslationUnit() override;
+  
+  enum class SpecialMemberFunctionKind {
+    Destructor,
+    CopyConstructor,
+    CopyAssignment,
+    MoveConstructor,
+    MoveAssignment
+  };
+
+  using ClassDefId = std::pair<SourceLocation, std::string>;
+
+  using ClassDefiningSpecialMembersMap = std::map<ClassDefId, llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
+
+private:
+  
+  static llvm::StringRef toString(SpecialMemberFunctionKind K);
+
+  static std::string join(llvm::ArrayRef<SpecialMemberFunctionKind> SMFS,
+                          llvm::StringRef AndOr);
+  
+  ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RULE_OF_FIVE_AND_ZERO_H

clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp

+//===--- RuleOfFiveAndZeroCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RuleOfFiveAndZeroCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#define DEBUG_TYPE "clang-tidy"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void RuleOfFiveAndZeroCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(
+      cxxRecordDecl(
+          eachOf(
+              has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
+              has(cxxConstructorDecl(isCopyConstructor(), unless(isImplicit()))
+                      .bind("copy-ctor")),
+              has(cxxMethodDecl(isCopyAssignmentOperator(),
+                                unless(isImplicit()))
+                      .bind("copy-assign")),
+              has(cxxConstructorDecl(isMoveConstructor(), unless(isImplicit()))
+                      .bind("move-ctor")),
+              has(cxxMethodDecl(isMoveAssignmentOperator(),
+                                unless(isImplicit()))
+                      .bind("move-assign"))))
+          .bind("class-def"),
+      this);
+}
+
+llvm::StringRef RuleOfFiveAndZeroCheck::toString(
+    RuleOfFiveAndZeroCheck::SpecialMemberFunctionKind K) {
+  switch (K) {
+  case SpecialMemberFunctionKind::Destructor:
+    return "a destructor";
+  case SpecialMemberFunctionKind::CopyConstructor:
+    return "a copy constructor";
+  case SpecialMemberFunctionKind::CopyAssignment:
+    return "a copy assignment operator";
+  case SpecialMemberFunctionKind::MoveConstructor:
+    return "a move constructor";
+  case SpecialMemberFunctionKind::MoveAssignment:
+    return "a move assignment operator";
+  }
+}
+
+std::string
+RuleOfFiveAndZeroCheck::join(llvm::ArrayRef<SpecialMemberFunctionKind> SMFS,
+                             llvm::StringRef AndOr) {
+  assert(!SMFS.empty());
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+
+  Stream << toString(SMFS[0]);
+  size_t LastIndex = SMFS.size() - 1;
+  for (size_t i = 1; i < LastIndex; ++i) {
+    Stream << ", " << toString(SMFS[i]);
+  }
+  if (LastIndex != 0) {
+    Stream << AndOr << toString(SMFS[LastIndex]);
+  }
+  return Stream.str();
+}
+
+void RuleOfFiveAndZeroCheck::check(const MatchFinder::MatchResult &Result) {
+  const CXXRecordDecl *MatchedDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("class-def");
+  if (!MatchedDecl)
+    return;
+
+  ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName());
+
+  if (Result.Nodes.getNodeAs<CXXDestructorDecl>("dtor")) {
+    ClassWithSpecialMembers[ID].push_back(
+        SpecialMemberFunctionKind::Destructor);
+  }
+  if (Result.Nodes.getNodeAs<CXXConstructorDecl>("copy-ctor")) {
+    ClassWithSpecialMembers[ID].push_back(
+        SpecialMemberFunctionKind::CopyConstructor);
+  }
+  if (Result.Nodes.getNodeAs<CXXMethodDecl>("copy-assign")) {
+    ClassWithSpecialMembers[ID].push_back(
+        SpecialMemberFunctionKind::CopyAssignment);
+  }
+  if (Result.Nodes.getNodeAs<CXXConstructorDecl>("move-ctor")) {
+    ClassWithSpecialMembers[ID].push_back(
+        SpecialMemberFunctionKind::MoveConstructor);
+  }
+  if (Result.Nodes.getNodeAs<CXXMethodDecl>("move-assign")) {
+    ClassWithSpecialMembers[ID].push_back(
+        SpecialMemberFunctionKind::MoveAssignment);
+  }
+}
+
+void RuleOfFiveAndZeroCheck::onEndOfTranslationUnit() {
+  llvm::SmallVector<SpecialMemberFunctionKind, 5> AllSpecialMembers = {
+      SpecialMemberFunctionKind::Destructor,
+      SpecialMemberFunctionKind::CopyConstructor,
+      SpecialMemberFunctionKind::CopyAssignment};
+
+  if (getLangOpts().CPlusPlus11) {
+    AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveConstructor);
+    AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveAssignment);
+  }
+
+  for (const auto &C : ClassWithSpecialMembers) {
+    ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers = C.second;
+
+    if (DefinedSpecialMembers.size() == AllSpecialMembers.size())
+      continue;
+
+    llvm::SmallVector<SpecialMemberFunctionKind, 5> UndefinedSpecialMembers;
+    std::set_difference(AllSpecialMembers.begin(), AllSpecialMembers.end(),
+                        DefinedSpecialMembers.begin(),
+                        DefinedSpecialMembers.end(),
+                        std::back_inserter(UndefinedSpecialMembers));
+
+    diag(C.first.first, "class '%0' defines %1 but does not define %2")
+        << C.first.second << join(DefinedSpecialMembers, " and ")
+        << join(UndefinedSpecialMembers, " or ");
+  }
+}
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/cppcoreguidelines-rule-of-five-and-zero.rst

+.. title:: clang-tidy - cppcoreguidelines-rule-of-five-and-zero
+
+cppcoreguidelines-rule-of-five-and-zero
+=======================================
+
+The check finds classes where some but not all of the special member functions
+are defined.
+
+By default the compiler defines a copy constructor, copy assignment operator,
+move constructor, move assignment operator and destructor. The default can be
+supressed by explicit user-definitions. The relationship between which
+functions will be supressed by definitions of other functions is complicated
+and it is advised that all five are defaulted or explicitly defined.
+
+Note that defining a function with ``= delete`` is considered to be a
+definition. 
+
+This rule is part of the "Constructors, assignments, and destructors" profile of the C++ Core
+Guidelines, corresponding to rule C.21. See
+
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.

docs/clang-tidy/checks/list.rst

    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
+   cppcoreguidelines-rule-of-five-and-zero
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero-cxx-03.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-rule-of-five-and-zero %t -- -- -std=c++03
+
+class DefinesDestructor {
+  ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-rule-of-five-and-zero] 
+
+class DefinesCopyConstructor {
+  DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-rule-of-five-and-zero]
+
+class DefinesCopyAssignment {
+  DefinesCopyAssignment &operator=(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-rule-of-five-and-zero]
+
+class DefinesNothing {
+};
+
+class DefinesEverything {
+  DefinesEverything(const DefinesEverything &);
+  DefinesEverything &operator=(const DefinesEverything &);
+  ~DefinesEverything();
+};
+
+class DeletesEverything {
+  DeletesEverything(const DeletesEverything &);
+  DeletesEverything &operator=(const DeletesEverything &);
+  ~DeletesEverything();
+};
+

test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-rule-of-five-and-zero %t
+
+class DefinesDestructor {
+  ~DefinesDestructor();
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-rule-of-five-and-zero] 
+
+class DefinesCopyConstructor {
+  DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-rule-of-five-and-zero]
+
+class DefinesCopyAssignment {
+  DefinesCopyAssignment &operator=(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor, a copy constructor, a move constructor or a move assignment operator [cppcoreguidelines-rule-of-five-and-zero]
+
+class DefinesMoveConstructor {
+  DefinesMoveConstructor(DefinesMoveConstructor &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-rule-of-five-and-zero]
+
+class DefinesMoveAssignment {
+  DefinesMoveAssignment &operator=(DefinesMoveAssignment &&);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-rule-of-five-and-zero]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+  DefinesEverything(const DefinesEverything &);
+  DefinesEverything &operator=(const DefinesEverything &);
+  DefinesEverything(DefinesEverything &&);
+  DefinesEverything &operator=(DefinesEverything &&);
+  ~DefinesEverything();
+};
+
+class DeletesEverything {
+  DeletesEverything(const DeletesEverything &);
+  DeletesEverything &operator=(const DeletesEverything &);
+  DeletesEverything(DeletesEverything &&);
+  DeletesEverything &operator=(DeletesEverything &&);
+  ~DeletesEverything();
+};
+
+class DeletesCopyDefaultsMove {
+  DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete;
+  DeletesCopyDefaultsMove &operator=(const DeletesCopyDefaultsMove &) = delete;
+  DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default;
+  DeletesCopyDefaultsMove &operator=(DeletesCopyDefaultsMove &&) = default;
+  ~DeletesCopyDefaultsMove() = default;
+};