Author: Andrey Karlov Date: 2025-07-23T13:19:23+03:00 New Revision: 5de443a4d37e1b7580f9ccee389572aef7233a85
URL: https://github.com/llvm/llvm-project/commit/5de443a4d37e1b7580f9ccee389572aef7233a85 DIFF: https://github.com/llvm/llvm-project/commit/5de443a4d37e1b7580f9ccee389572aef7233a85.diff LOG: [clang-tidy] Make copy-and-swap idiom more general for `bugprone-unhandled-self-assignment` (#147066) This change enhances the `bugprone-unhandled-self-assignment` checker by adding an additional matcher that generalizes the copy-and-swap idiom pattern detection. # What Changed Added a new matcher that checks for: - An instance of the current class being created in operator= (regardless of constructor arguments) - That instance being passed to a `swap` function call # Problem Solved This fix reduces false positives in PMR-like scenarios where "extended" constructors are used (typically taking an additional allocator argument). The checker now properly recognizes copy-and-swap implementations that use extended copy/move constructors instead of flagging them as unhandled self-assignment cases. Fixes #146324 --------- Co-authored-by: Baranov Victor <bar.victor.2...@gmail.com> Added: Modified: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-self-assignment.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp index 1f432c4ccc5f0..c4c4267545b59 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp @@ -69,6 +69,28 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { cxxMethodDecl(unless(hasDescendant(cxxMemberCallExpr(callee(cxxMethodDecl( hasName("operator="), ofClass(equalsBoundNode("class")))))))); + // Checking that some kind of constructor is called and followed by a `swap`: + // T& operator=(const T& other) { + // T tmp{this->internal_data(), some, other, args}; + // swap(tmp); + // return *this; + // } + const auto HasCopyAndSwap = cxxMethodDecl( + ofClass(cxxRecordDecl()), + hasBody(compoundStmt( + hasDescendant( + varDecl(hasType(cxxRecordDecl(equalsBoundNode("class")))) + .bind("tmp_var")), + hasDescendant(stmt(anyOf( + cxxMemberCallExpr(hasArgument( + 0, declRefExpr(to(varDecl(equalsBoundNode("tmp_var")))))), + callExpr( + callee(functionDecl(hasName("swap"))), argumentCountIs(2), + hasAnyArgument( + declRefExpr(to(varDecl(equalsBoundNode("tmp_var"))))), + hasAnyArgument(unaryOperator(has(cxxThisExpr()), + hasOperatorName("*")))))))))); + DeclarationMatcher AdditionalMatcher = cxxMethodDecl(); if (WarnOnlyIfThisHasSuspiciousField) { // Matcher for standard smart pointers. @@ -89,14 +111,14 @@ void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) { hasType(arrayType()))))))); } - Finder->addMatcher(cxxMethodDecl(ofClass(cxxRecordDecl().bind("class")), - isCopyAssignmentOperator(), IsUserDefined, - HasReferenceParam, HasNoSelfCheck, - unless(HasNonTemplateSelfCopy), - unless(HasTemplateSelfCopy), - HasNoNestedSelfAssign, AdditionalMatcher) - .bind("copyAssignmentOperator"), - this); + Finder->addMatcher( + cxxMethodDecl( + ofClass(cxxRecordDecl().bind("class")), isCopyAssignmentOperator(), + IsUserDefined, HasReferenceParam, HasNoSelfCheck, + unless(HasNonTemplateSelfCopy), unless(HasTemplateSelfCopy), + unless(HasCopyAndSwap), HasNoNestedSelfAssign, AdditionalMatcher) + .bind("copyAssignmentOperator"), + this); } void UnhandledSelfAssignmentCheck::check( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index bccb0ca83c79c..bd2d6eb55b29a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -114,6 +114,11 @@ Changes in existing checks <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for variables introduced by structured bindings. +- Improved :doc:`bugprone-unhandled-self-assignment + <clang-tidy/checks/bugprone/unhandled-self-assignment>` check by adding + an additional matcher that generalizes the copy-and-swap idiom pattern + detection. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-self-assignment.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-self-assignment.cpp index 8610393449f97..c2a8ddc08d330 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-self-assignment.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unhandled-self-assignment.cpp @@ -28,6 +28,13 @@ template <class T> class auto_ptr { }; +namespace pmr { + template <typename TYPE = void> + class allocator {}; +} + +struct allocator_arg_t {} allocator_arg; + } // namespace std void assert(int expression){}; @@ -229,6 +236,122 @@ bool operator!=(Foo2 &, Foo2 &) { }; } +// Missing call to `swap` function +class AllocatorAwareClassNoSwap { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassNoSwap(const AllocatorAwareClassNoSwap& other) { + } + + AllocatorAwareClassNoSwap(const AllocatorAwareClassNoSwap& other, const allocator_type& alloc) { + } + + AllocatorAwareClassNoSwap& operator=(const AllocatorAwareClassNoSwap& other) { + // CHECK-MESSAGES: [[@LINE-1]]:32: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + AllocatorAwareClassNoSwap tmp(other, get_allocator()); + return *this; + } + + allocator_type get_allocator() const { + return allocator_type(); + } +}; + +// "Wrong" type is passed to member `swap` function +class AllocatorAwareClassSwapWrongArgType { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassSwapWrongArgType(const AllocatorAwareClassSwapWrongArgType& other) { + } + + AllocatorAwareClassSwapWrongArgType(const AllocatorAwareClassSwapWrongArgType& other, const allocator_type& alloc) { + } + + AllocatorAwareClassSwapWrongArgType& operator=(const AllocatorAwareClassSwapWrongArgType& other) { + // CHECK-MESSAGES: [[@LINE-1]]:42: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + int tmp = 0; + swap(tmp); + + return *this; + } + + void swap(int&) noexcept { + } + + allocator_type get_allocator() const { + return allocator_type(); + } +}; + + +// Making ADL `swap` call but with wrong argument type +class AllocatorAwareClassAdlSwapWrongArgType { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassAdlSwapWrongArgType(const AllocatorAwareClassAdlSwapWrongArgType& other) { + } + + AllocatorAwareClassAdlSwapWrongArgType(const AllocatorAwareClassAdlSwapWrongArgType& other, const allocator_type& alloc) { + } + + AllocatorAwareClassAdlSwapWrongArgType& operator=(const AllocatorAwareClassAdlSwapWrongArgType& other) { + // CHECK-MESSAGES: [[@LINE-1]]:45: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + int tmp = 0; + swap(*this, tmp); + + return *this; + } + + allocator_type get_allocator() const { + return allocator_type(); + } + + friend void swap(AllocatorAwareClassAdlSwapWrongArgType&, int&) { + } +}; + +// `this` isn't passed to `swap` +class AllocatorAwareClassAdlSwapWrongArgs { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassAdlSwapWrongArgs(const AllocatorAwareClassAdlSwapWrongArgs& other) { + } + + AllocatorAwareClassAdlSwapWrongArgs(const AllocatorAwareClassAdlSwapWrongArgs& other, const allocator_type& alloc) { + } + + AllocatorAwareClassAdlSwapWrongArgs& operator=(const AllocatorAwareClassAdlSwapWrongArgs& other) { + // CHECK-MESSAGES: [[@LINE-1]]:42: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment] + AllocatorAwareClassAdlSwapWrongArgs tmp1(other, get_allocator()); + AllocatorAwareClassAdlSwapWrongArgs tmp2(*this, get_allocator()); + swap(tmp1, tmp2); + return *this; + } + + allocator_type get_allocator() const { + return allocator_type(); + } + + friend void swap(AllocatorAwareClassAdlSwapWrongArgs&, AllocatorAwareClassAdlSwapWrongArgs&) { + } +}; + /////////////////////////////////////////////////////////////////// /// Test cases correctly ignored by the check. @@ -540,6 +663,89 @@ class NotACopyAssignmentOperator { Uy *getUy() const { return Ptr2; } }; +// Support "extended" copy/move constructors +class AllocatorAwareClass { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClass(const AllocatorAwareClass& other) { + } + + AllocatorAwareClass(const AllocatorAwareClass& other, const allocator_type& alloc) { + } + + AllocatorAwareClass& operator=(const AllocatorAwareClass& other) { + AllocatorAwareClass tmp(other, get_allocator()); + swap(tmp); + return *this; + } + + void swap(AllocatorAwareClass& other) noexcept { + } + + allocator_type get_allocator() const { + return allocator_type(); + } +}; + +// Support "extended" copy/move constructors + std::swap +class AllocatorAwareClassStdSwap { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassStdSwap(const AllocatorAwareClassStdSwap& other) { + } + + AllocatorAwareClassStdSwap(const AllocatorAwareClassStdSwap& other, const allocator_type& alloc) { + } + + AllocatorAwareClassStdSwap& operator=(const AllocatorAwareClassStdSwap& other) { + using std::swap; + + AllocatorAwareClassStdSwap tmp(other, get_allocator()); + swap(*this, tmp); + return *this; + } + + allocator_type get_allocator() const { + return allocator_type(); + } +}; + +// Support "extended" copy/move constructors + ADL swap +class AllocatorAwareClassAdlSwap { + // pointer member to trigger bugprone-unhandled-self-assignment + void *foo = nullptr; + + public: + using allocator_type = std::pmr::allocator<>; + + AllocatorAwareClassAdlSwap(const AllocatorAwareClassAdlSwap& other) { + } + + AllocatorAwareClassAdlSwap(const AllocatorAwareClassAdlSwap& other, const allocator_type& alloc) { + } + + AllocatorAwareClassAdlSwap& operator=(const AllocatorAwareClassAdlSwap& other) { + AllocatorAwareClassAdlSwap tmp(other, get_allocator()); + swap(*this, tmp); + return *this; + } + + allocator_type get_allocator() const { + return allocator_type(); + } + + friend void swap(AllocatorAwareClassAdlSwap&, AllocatorAwareClassAdlSwap&) { + } +}; + /////////////////////////////////////////////////////////////////// /// Test cases which should be caught by the check. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
