[clang-tidy]: Add cert-oop54-cpp alias for bugprone-unhandled-self-assignment

Summary:
Added WarnOnlyIfThisHasSuspiciousField option to allow
to catch any copy assignment operator independently from
the container class's fields.
Added the cert alias using this option.

Reviewers: aaron.ballman

Reviewed By: aaron.ballman

Subscribers: mgorny, Eugene.Zelenko, xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D62192

llvm-svn: 361550

clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp

@@ -16,6 +16,18 @@ namespace clang {
 namespace tidy {
 namespace bugprone {
 
+UnhandledSelfAssignmentCheck::UnhandledSelfAssignmentCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOnlyIfThisHasSuspiciousField(
+          Options.get("WarnOnlyIfThisHasSuspiciousField", true)) {}
+
+void UnhandledSelfAssignmentCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnlyIfThisHasSuspiciousField",
+                WarnOnlyIfThisHasSuspiciousField);
+}
+
 void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
@@ -61,29 +73,32 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
       cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl(
           hasName("operator="), ofClass(equalsBoundNode("class"))))))));
 
-  // Matcher for standard smart pointers.
+  DeclarationMatcher AdditionalMatcher = cxxMethodDecl();
-  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  if (WarnOnlyIfThisHasSuspiciousField) {
-      recordType(hasDeclaration(classTemplateSpecializationDecl(
+    // Matcher for standard smart pointers.
-          hasAnyName("::std::shared_ptr", "::std::unique_ptr",
+    const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
-                     "::std::weak_ptr", "::std::auto_ptr"),
+        recordType(hasDeclaration(classTemplateSpecializationDecl(
-          templateArgumentCountIs(1))))));
+            hasAnyName("::std::shared_ptr", "::std::unique_ptr",
+                       "::std::weak_ptr", "::std::auto_ptr"),
+            templateArgumentCountIs(1))))));
 
-  // We will warn only if the class has a pointer or a C array field which
+    // We will warn only if the class has a pointer or a C array field which
-  // probably causes a problem during self-assignment (e.g. first resetting the
+    // probably causes a problem during self-assignment (e.g. first resetting
-  // pointer member, then trying to access the object pointed by the pointer, or
+    // the pointer member, then trying to access the object pointed by the
-  // memcpy overlapping arrays).
+    // pointer, or memcpy overlapping arrays).
-  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+    AdditionalMatcher = cxxMethodDecl(ofClass(cxxRecordDecl(
-      has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+        has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
-                          hasType(arrayType())))))));
+                            hasType(arrayType())))))));
+  }
 
-  Finder->addMatcher(
+  Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
-      cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
+                                   isCopyAssignmentOperator(), IsUserDefined,
-                    isCopyAssignmentOperator(), IsUserDefined,
+                                   HasReferenceParam, HasNoSelfCheck,
-                    HasReferenceParam, HasNoSelfCheck,
+                                   unless(HasNonTemplateSelfCopy),
-                    unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy),
+                                   unless(HasTemplateSelfCopy),
-                    HasNoNestedSelfAssign, ThisHasSuspiciousField)
+                                   HasNoNestedSelfAssign, AdditionalMatcher)
-          .bind("copyAssignmentOperator"),
+                         .bind("copyAssignmentOperator"),
-      this);
+                     this);
 }
 
 void UnhandledSelfAssignmentCheck::check(

clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h

@@ -23,10 +23,14 @@ namespace bugprone {
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unhandled-self-assignment.html
 class UnhandledSelfAssignmentCheck : public ClangTidyCheck {
 public:
-  UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context)
+  UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context);
-      : ClangTidyCheck(Name, Context) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool WarnOnlyIfThisHasSuspiciousField;
 };
 
 } // namespace bugprone

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

@@ -9,6 +9,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../bugprone/UnhandledSelfAssignmentCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
@@ -49,6 +50,8 @@ public:
     // OOP
     CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
+    CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
+        "cert-oop54-cpp");
     // ERR
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err09-cpp");
@@ -85,6 +88,7 @@ public:
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
     Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
+    Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0";
     return Options;
   }
 };

clang-tools-extra/clang-tidy/cert/CMakeLists.txt

@@ -20,6 +20,7 @@ add_clang_library(clangTidyCERTModule
   clangBasic
   clangLex
   clangTidy
+  clangTidyBugproneModule
   clangTidyGoogleModule
   clangTidyMiscModule
   clangTidyPerformanceModule

clang-tools-extra/docs/ReleaseNotes.rst

@@ -134,6 +134,11 @@ Improvements to clang-tidy
   subclasses of ``NSObject`` and recommends calling a superclass initializer
   instead.
 
+- New alias :doc:`cert-oop54-cpp
+  <clang-tidy/checks/cert-oop54-cpp>` to
+  :doc:`bugprone-unhandled-self-assignment
+  <clang-tidy/checks/bugprone-unhandled-self-assignment>` was added.
+
 - New alias :doc:`cppcoreguidelines-explicit-virtual-functions
   <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to
   :doc:`modernize-use-override

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst

@@ -3,11 +3,14 @@
 bugprone-unhandled-self-assignment
 ==================================
 
+`cert-oop54-cpp` redirects here as an alias for this check. For the CERT alias,
+the `WarnOnlyIfThisHasSuspiciousField` option is set to `0`.
+
 Finds user-defined copy assignment operators which do not protect the code
 against self-assignment either by checking self-assignment explicitly or
 using the copy-and-swap or the copy-and-move method.
 
-This check now searches only those classes which have any pointer or C array field
+By default, this check searches only those classes which have any pointer or C array field
 to avoid false positives. In case of a pointer or a C array, it's likely that self-copy
 assignment breaks the object if the copy assignment operator was not written with care.
 
@@ -114,3 +117,8 @@ temporary object into ``this`` (needs a move assignment operator):
       return *this;
     }
   };
+
+.. option:: WarnOnlyIfThisHasSuspiciousField
+
+  When non-zero, the check will warn only if the container class of the copy assignment operator
+  has any suspicious fields (pointer or C array). This option is set to `1` by default.

clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst

@@ -0,0 +1,10 @@
+.. title:: clang-tidy - cert-oop54-cpp
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-unhandled-self-assignment.html
+
+cert-oop54-cpp
+==============
+
+The cert-oop54-cpp check is an alias, please see
+`bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_
+for more information.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -98,6 +98,7 @@ Clang-Tidy Checks
    cert-msc50-cpp
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
+   cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
    cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) <cppcoreguidelines-avoid-c-arrays>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment-warn-only-if-this-has-suspicious-field.cpp

@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField, \
+// RUN:               value: 0}]}"
+
+// Classes with pointer field are still caught.
+class PtrField {
+public:
+  PtrField &operator=(const PtrField &object) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+    return *this;
+  }
+
+private:
+  int *p;
+};
+
+// With the option, check catches classes with trivial fields.
+class TrivialFields {
+public:
+  TrivialFields &operator=(const TrivialFields &object) {
+    // CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+    return *this;
+  }
+
+private:
+  int m;
+  float f;
+  double d;
+  bool b;
+};
+
+// The check warns also when there is no field at all.
+// In this case, user-defined copy assignment operator is useless anyway.
+class ClassWithoutFields {
+public:
+  ClassWithoutFields &operator=(const ClassWithoutFields &object) {
+    // CHECK-MESSAGES: [[@LINE-1]]:23: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+    return *this;
+  }
+};

clang-tools-extra/test/clang-tidy/cert-oop54-cpp.cpp

@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s cert-oop54-cpp %t
+
+// Test whether bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField option is set correctly.
+class TrivialFields {
+public:
+  TrivialFields &operator=(const TrivialFields &object) {
+    // CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [cert-oop54-cpp]
+    return *this;
+  }
+
+private:
+  int m;
+  float f;
+  double d;
+  bool b;
+};