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

[ztamas]

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.

[aaron.ballman]

LGTM aside from some nits, thank you for this!

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

89–91

Nothing to be changed here, but it sure would be nice to have a clang-tidy-specific matcher to be used for option handling so we could do this modification using something like unless(!WarnOnlyIfThisHasSuspiciousField).

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

6

cert alias -> CERT alias, the

7

Add backticks around the option name.

124

field -> fields

[Eugene.Zelenko]

Please mention alias (in aliases list) in Release Notes.

[ztamas]

Fixed docs.

[ztamas]

Link burprone module to cert module

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

 /// 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.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unhandled-self-assignment.html
 class UnhandledSelfAssignmentCheck : public ClangTidyCheck {
 public:
-  UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UnhandledSelfAssignmentCheck(StringRef Name, ClangTidyContext *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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDSELFASSIGNMENTCHECK_H

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

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 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;
 
   // We don't care about deleted, default or implicit operator implementations.
   const auto IsUserDefined = cxxMethodDecl(
       isDefinition(), unless(anyOf(isDeleted(), isImplicit(), isDefaulted())));
 
void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
 
   // If inside the copy assignment operator another assignment operator is
   // called on 'this' we assume that self-check might be handled inside
   // this nested operator.
   const auto HasNoNestedSelfAssign =
       cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl(
           hasName("operator="), ofClass(equalsBoundNode("class"))))))));
 
+  DeclarationMatcher AdditionalMatcher = cxxMethodDecl();
+  if (WarnOnlyIfThisHasSuspiciousField) {
     // Matcher for standard smart pointers.
     const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
         recordType(hasDeclaration(classTemplateSpecializationDecl(
             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
-  // probably causes a problem during self-assignment (e.g. first resetting the
-  // pointer member, then trying to access the object pointed by the pointer, or
-  // memcpy overlapping arrays).
-  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+    // probably causes a problem during self-assignment (e.g. first resetting
+    // the pointer member, then trying to access the object pointed by the
+    // pointer, or memcpy overlapping arrays).
+    AdditionalMatcher = cxxMethodDecl(ofClass(cxxRecordDecl(
         has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
                             hasType(arrayType())))))));
+  }
 
-  Finder->addMatcher(
-      cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
+  Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")),
                                    isCopyAssignmentOperator(), IsUserDefined,
                                    HasReferenceParam, HasNoSelfCheck,
-                    unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy),
-                    HasNoNestedSelfAssign, ThisHasSuspiciousField)
+                                   unless(HasNonTemplateSelfCopy),
+                                   unless(HasTemplateSelfCopy),
+                                   HasNoNestedSelfAssign, AdditionalMatcher)
                          .bind("copyAssignmentOperator"),
                      this);
 }
 
 void UnhandledSelfAssignmentCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl =
       Result.Nodes.getNodeAs<CXXMethodDecl>("copyAssignmentOperator");
   diag(MatchedDecl->getLocation(),
        "operator=() does not handle self-assignment properly");
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

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

 //===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "../performance/MoveConstructorInitCheck.h"
 #include "../readability/UppercaseLiteralSuffixCheck.h"
 #include "CommandProcessorCheck.h"
CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<DontModifyStdNamespaceCheck>(
         "cert-dcl58-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP
     CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
+    CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
+        "cert-oop54-cpp");
     // ERR
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err09-cpp");
     CheckFactories.registerCheck<SetLongJmpCheck>("cert-err52-cpp");
     CheckFactories.registerCheck<StaticObjectExceptionCheck>("cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>("cert-err60-cpp");
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err61-cpp");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
     Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU";
+    Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "0";
     return Options;
   }
 };
 
 } // namespace cert
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<cert::CERTModule>

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

 .. title:: clang-tidy - bugprone-unhandled-self-assignment
 
 bugprone-unhandled-self-assignment
 ==================================
 
+`cert-oop54-cpp` redirects here as an alias for this check. For the cert alias
+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.
 
 See also:
 `OOP54-CPP. Gracefully handle self-copy assignment
 <https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP54-CPP.+Gracefully+handle+self-copy+assignment>`_
 
 A copy assignment operator must prevent that self-copy assignment ruins the
public:
     }
 
     T& operator=(T &&rhs) {
       p = rhs.p;
       rhs.p = nullptr;
       return *this;
     }
   };
+
+.. option:: WarnOnlyIfThisHasSuspiciousField
+
+  When non-zero, the check will warn only if the container class of the copy assignment operator
+  has any suspicious field (pointer or C array). This option is set to `1` by default.

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

+.. 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

.. toctree::
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc32-c (redirects to cert-msc51-cpp) <cert-msc32-c>
    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>
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
    cppcoreguidelines-explicit-virtual-functions (redirects to modernize-use-override) <cppcoreguidelines-explicit-virtual-functions>
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-macro-usage
    cppcoreguidelines-narrowing-conversions

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

+// 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

+// 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;
+};