diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -46,6 +46,7 @@ #include "TooSmallLoopVariableCheck.h" #include "UndefinedMemoryManipulationCheck.h" #include "UndelegatedConstructorCheck.h" +#include "UnhandledSelfAssignmentCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" @@ -96,8 +97,6 @@ "bugprone-move-forwarding-reference"); CheckFactories.registerCheck( "bugprone-multiple-statement-macro"); - CheckFactories.registerCheck( - "bugprone-too-small-loop-variable"); CheckFactories.registerCheck( "bugprone-narrowing-conversions"); CheckFactories.registerCheck( @@ -128,10 +127,14 @@ "bugprone-terminating-continue"); CheckFactories.registerCheck( "bugprone-throw-keyword-missing"); + CheckFactories.registerCheck( + "bugprone-too-small-loop-variable"); CheckFactories.registerCheck( "bugprone-undefined-memory-manipulation"); CheckFactories.registerCheck( "bugprone-undelegated-constructor"); + CheckFactories.registerCheck( + "bugprone-unhandled-self-assignment"); CheckFactories.registerCheck( "bugprone-unused-raii"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -38,6 +38,7 @@ TooSmallLoopVariableCheck.cpp UndefinedMemoryManipulationCheck.cpp UndelegatedConstructorCheck.cpp + UnhandledSelfAssignmentCheck.cpp UnusedRaiiCheck.cpp UnusedReturnValueCheck.cpp UseAfterMoveCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h @@ -0,0 +1,36 @@ +//===--- UnhandledSelfAssignmentCheck.h - clang-tidy ------------*- C++ -*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDSELFASSIGNMENTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDSELFASSIGNMENTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// 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) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDSELFASSIGNMENTCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp @@ -0,0 +1,84 @@ +//===--- UnhandledSelfAssignmentCheck.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 "UnhandledSelfAssignmentCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { + // We don't care about deleted, default or implicit operator implementations. + const auto IsUserDefined = cxxMethodDecl( + isDefinition(), unless(anyOf(isDeleted(), isImplicit(), isDefaulted()))); + + // We don't need to worry when a copy assignment operator gets the other + // object by value. + const auto HasReferenceParam = + cxxMethodDecl(hasParameter(0, parmVarDecl(hasType(referenceType())))); + + // Self-check: Code compares something with 'this' pointer. We don't check + // whether it is actually the parameter what we compare. + const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant( + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + has(ignoringParenCasts(cxxThisExpr())))))); + + // Both copy-and-swap and copy-and-move method creates a copy first and + // assign it to 'this' with swap or move. + const auto HasNoSelfCopy = cxxMethodDecl( + unless(hasDescendant(cxxConstructExpr(hasDeclaration(cxxConstructorDecl( + isCopyConstructor(), ofClass(equalsBoundNode("class")))))))); + + // 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")))))))); + + // 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( + has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType), + hasType(arrayType()))))))); + + Finder->addMatcher( + cxxMethodDecl( + ofClass(cxxRecordDecl(unless(hasAncestor(classTemplateDecl()))) + .bind("class")), + isCopyAssignmentOperator(), IsUserDefined, HasReferenceParam, + HasNoSelfCheck, HasNoSelfCopy, HasNoNestedSelfAssign, + ThisHasSuspiciousField) + .bind("copyAssignmentOperator"), + this); +} + +void UnhandledSelfAssignmentCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = + Result.Nodes.getNodeAs("copyAssignmentOperator"); + diag(MatchedDecl->getLocation(), + "operator=() might not handle self-assignment properly"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,13 @@ Finds and fixes ``absl::Time`` subtraction expressions to do subtraction in the Time domain instead of the numeric domain. +- New :doc:`bugprone-unhandled-self-assignment + ` check. + + 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. + - New :doc:`google-readability-avoid-underscore-in-googletest-name ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst @@ -0,0 +1,114 @@ +.. title:: clang-tidy - bugprone-unhandled-self-assignment + +bugprone-unhandled-self-assignment +================================== + +`cert-oop54-cpp` redirects here as an alias for this check. + +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 +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. + +A copy assignment operator must prevent that self-copy assignment ruins the +object state. The typical use case is when the class has a pointer field +and the copy assignment operator first releases the pointed object and +then tries to assign it: + +.. code-block:: c++ + + class T { + int* p; + + public: + T(const T &rhs) : p(rhs.p ? new int(*rhs.p) : nullptr) {} + ~T() { delete p; } + + // ... + + T& operator=(const T &rhs) { + delete p; + p = new int(*rhs.p); + return *this; + } + }; + +There are two common C++ patterns to avoid this problem. The first is +the self-assignment check: + +.. code-block:: c++ + + class T { + int* p; + + public: + T(const T &rhs) : p(rhs.p ? new int(*rhs.p) : nullptr) {} + ~T() { delete p; } + + // ... + + T& operator=(const T &rhs) { + if(this == &rhs) + return *this; + + delete p; + p = new int(*rhs.p); + return *this; + } + }; + +The second one is the copy-and-swap method when we create a temporary copy +(using the copy constructor) and then swap this temporary object with ``this``: + +.. code-block:: c++ + + class T { + int* p; + + public: + T(const T &rhs) : p(rhs.p ? new int(*rhs.p) : nullptr) {} + ~T() { delete p; } + + // ... + + void swap(T &rhs) { + using std::swap; + swap(p, rhs.p); + } + + T& operator=(const T &rhs) { + T(rhs).swap(*this); + return *this; + } + }; + +There is a third pattern which is less common. Let's call it the copy-and-move method +when we create a temporary copy (using the copy constructor) and then move this +temporary object into ``this`` (needs a move assignment operator): + +.. code-block:: c++ + + class T { + int* p; + + public: + T(const T &rhs) : p(rhs.p ? new int(*rhs.p) : nullptr) {} + ~T() { delete p; } + + // ... + + T& operator=(const T &rhs) { + T t = rhs; + *this = std::move(t); + return *this; + } + + T& operator=(T &&rhs) { + p = rhs.p; + rhs.p = nullptr; + return *this; + } + }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst b/clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst new file mode 100644 --- /dev/null +++ b/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 `_ +for more information. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -71,6 +71,7 @@ bugprone-too-small-loop-variable bugprone-undefined-memory-manipulation bugprone-undelegated-constructor + bugprone-unhandled-self-assignment bugprone-unused-raii bugprone-unused-return-value bugprone-use-after-move @@ -96,6 +97,7 @@ cert-msc50-cpp cert-msc51-cpp cert-oop11-cpp (redirects to performance-move-constructor-init) + cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) cppcoreguidelines-avoid-goto cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) diff --git a/clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp b/clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp @@ -0,0 +1,434 @@ +// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t + +namespace std { + +template +void swap(T x, T y) { +} + +template +T &&move(T x) { +} + +template +class unique_ptr { +}; + +template +class shared_ptr { +}; + +template +class weak_ptr { +}; + +template +class auto_ptr { +}; + +} // namespace std + +void assert(int expression){}; + +/////////////////////////////////////////////////////////////////// +/// Test cases correctly caught by the check. + +class PtrField { +public: + PtrField &operator=(const PtrField &object); + +private: + int *p; +}; + +PtrField &PtrField::operator=(const PtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; +} + +// Class with an inline operator definition. +class InlineDefinition { +public: + InlineDefinition &operator=(const InlineDefinition &object) { + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + int *p; +}; + +class UniquePtrField { +public: + UniquePtrField &operator=(const UniquePtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + std::unique_ptr p; +}; + +class SharedPtrField { +public: + SharedPtrField &operator=(const SharedPtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + std::shared_ptr p; +}; + +class WeakPtrField { +public: + WeakPtrField &operator=(const WeakPtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + std::weak_ptr p; +}; + +class AutoPtrField { +public: + AutoPtrField &operator=(const AutoPtrField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + std::auto_ptr p; +}; + +// Class with C array field. +class CArrayField { +public: + CArrayField &operator=(const CArrayField &object) { + // CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + // ... + return *this; + } + +private: + int array[256]; +}; + +// Make sure to not ignore cases when the operator definition calls +// a copy constructor of another class. +class CopyConstruct { +public: + CopyConstruct &operator=(const CopyConstruct &object) { + // CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + WeakPtrField a; + WeakPtrField b(a); + // ... + return *this; + } + +private: + int *p; +}; + +// Make sure to not ignore cases when the operator definition calls +// a copy assignment operator of another class. +class AssignOperator { +public: + AssignOperator &operator=(const AssignOperator &object) { + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment] + a.operator=(object.a); + // ... + return *this; + } + +private: + int *p; + WeakPtrField a; +}; + +/////////////////////////////////////////////////////////////////// +/// Test cases correctly ignored by the check. + +// Self-assignment is checked using the equality operator. +class SelfCheck1 { +public: + SelfCheck1 &operator=(const SelfCheck1 &object) { + if (this == &object) + return *this; + // ... + return *this; + } + +private: + int *p; +}; + +class SelfCheck2 { +public: + SelfCheck2 &operator=(const SelfCheck2 &object) { + if (&object == this) + return *this; + // ... + return *this; + } + +private: + int *p; +}; + +// Self-assignment is checked using the inequality operator. +class SelfCheck3 { +public: + SelfCheck3 &operator=(const SelfCheck3 &object) { + if (this != &object) { + // ... + } + return *this; + } + +private: + int *p; +}; + +class SelfCheck4 { +public: + SelfCheck4 &operator=(const SelfCheck4 &object) { + if (&object != this) { + // ... + } + return *this; + } + +private: + int *p; +}; + +template +class TemplateSelfCheck { +public: + TemplateSelfCheck &operator=(const TemplateSelfCheck &object) { + if (&object != this) { + // ... + } + return *this; + } + +private: + T *p; +}; + +// There is no warning if the copy assignment operator gets the object by value. +class PassedByValue { +public: + PassedByValue &operator=(PassedByValue object) { + // ... + return *this; + } + +private: + int *p; +}; + +// User-defined swap method calling std::swap inside. +class CopyAndSwap1 { +public: + CopyAndSwap1 &operator=(const CopyAndSwap1 &object) { + CopyAndSwap1 temp(object); + doSwap(temp); + return *this; + } + +private: + int *p; + + void doSwap(CopyAndSwap1 &object) { + using std::swap; + swap(p, object.p); + } +}; + +// User-defined swap method used with passed-by-value parameter. +class CopyAndSwap2 { +public: + CopyAndSwap2 &operator=(CopyAndSwap2 object) { + doSwap(object); + return *this; + } + +private: + int *p; + + void doSwap(CopyAndSwap2 &object) { + using std::swap; + swap(p, object.p); + } +}; + +// Copy-and-swap method is used but without creating a separate method for it. +class CopyAndSwap3 { +public: + CopyAndSwap3 &operator=(const CopyAndSwap3 &object) { + CopyAndSwap3 temp(object); + std::swap(p, temp.p); + return *this; + } + +private: + int *p; +}; + +template +class TemplateCopyAndSwap { +public: + TemplateCopyAndSwap &operator=(const TemplateCopyAndSwap &object) { + TemplateCopyAndSwap temp(object); + std::swap(p, temp.p); + return *this; + } + +private: + T *p; +}; + +// Move semantics is used on a temporary copy of the object. +class CopyAndMove1 { +public: + CopyAndMove1 &operator=(const CopyAndMove1 &object) { + CopyAndMove1 temp(object); + *this = std::move(temp); + return *this; + } + +private: + int *p; +}; + +template +class TemplateCopyAndMove { +public: + TemplateCopyAndMove &operator=(const TemplateCopyAndMove &object) { + TemplateCopyAndMove temp(object); + *this = std::move(temp); + return *this; + } + +private: + T *p; +}; + +// We should not catch move assignment operators. +class MoveAssignOperator { +public: + MoveAssignOperator &operator=(MoveAssignOperator &&object) { + // ... + return *this; + } + +private: + int *p; +}; + +// We ignore copy assignment operators without user-defined implementation. +class DefaultOperator { +public: + DefaultOperator &operator=(const DefaultOperator &object) = default; + +private: + int *p; +}; + +class DeletedOperator { +public: + DeletedOperator &operator=(const DefaultOperator &object) = delete; + +private: + int *p; +}; + +class ImplicitOperator { +private: + int *p; +}; + +// Check ignores those classes which has no any pointer or array field. +class TrivialFields { +public: + TrivialFields &operator=(const TrivialFields &object) { + // ... + return *this; + } + +private: + int m; + float f; + double d; + bool b; +}; + +// There is no warning when the class calls another assignment operator on 'this' +// inside the copy assignment operator's definition. +class AssignIsForwarded { +public: + AssignIsForwarded &operator=(const AssignIsForwarded &object) { + operator=(object.p); + return *this; + } + + AssignIsForwarded &operator=(int *pp) { + if (p != pp) { + delete p; + p = new int(*pp); + } + return *this; + } + +private: + int *p; +}; + +// Assertion is a valid way to say that self-assignment is not expected to happen. +class AssertGuard { +public: + AssertGuard &operator=(const AssertGuard &object) { + assert(this != &object); + // ... + return *this; + } + +private: + int *p; +}; + +/////////////////////////////////////////////////////////////////// +/// Test cases which should be caught by the check. + +template +class TemplatePtrField { +public: + TemplatePtrField &operator=(const TemplatePtrField &object) { + // ... + return *this; + } + +private: + T *p; +}; + +template +class TemplateCArrayField { +public: + TemplateCArrayField &operator=(const TemplateCArrayField &object) { + // ... + return *this; + } + +private: + T p[256]; +};