[clang-tidy] new check: bugprone-unhandled-self-assignment

[ztamas]

Summary

This check searches for copy assignment operators which might not handle self-assignment properly. There are three patterns of
handling a self assignment situation: self check, copy-and-swap or the less common copy-and-move. The new check warns if none of
these patterns is found in a user defined implementation.

See also:
OOP54-CPP. Gracefully handle self-copy assignment
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP54-CPP.+Gracefully+handle+self-copy+assignment

[ztamas]

On llvm source code the check finds three suspicious methods:

/home/zolnai/libreoffice/clang/llvm-project/clang/lib/AST/NestedNameSpecifier.cpp:534:1: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
operator=(const NestedNameSpecifierLocBuilder &Other) {

/home/zolnai/libreoffice/clang/llvm-project/llvm/unittests/ADT/ArrayRefTest.cpp:75:10: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
    void operator=(const NonAssignable &RHS) { assert(RHS.Ptr != nullptr); }

/home/zolnai/libreoffice/clang/llvm-project/llvm/lib/Analysis/TargetLibraryInfo.cpp:594:47: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(const TargetLibraryInfoImpl &TLI) {

Two of them seems a good catch to me. NestedNameSpecifierLocBuilder and TargetLibraryInfoImpl are using memcpy without self-check, and using memcpy on overlapping regions leads to undefined behavior.

The third one is not an actual working copy assignment operator, as I see. It is used only for some testing purposes.

[ztamas]

On LibreOffice source code I found 36 catches with this check.
16 of them was worth to fix because in those cases the object state was changed in some way after a self-assignment:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=3a5d78365dd172881c16c03e67f2d170ffc6d7d4

The remaining 20 are false positives. They are working copy assignment operators without using any of the three checked patterns.
However, some of these warnings could be suppressed with modernizing the code or with removing code duplication:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=adfba503c792fdbd4748d6680c2dd8d8d5bb0d69

[JonasToth]

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

102

please order this list alphabetically.

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

51

what about auto_ptr? I am actually not sure if we should care, as its deprecated and removed already, on the other hand legacy code probably still has it.

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

10

It is worth an alias into the cert module. Please add this with this revision already.

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

Please add tests with templated classes and self-assignment.

[alexfh]

Thanks for the useful check! I have a few comments, see inline.

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

34–35

I guess, has(ignoringParenCasts(cxxThisExpr()) will have the same effect as anyOf(hasLHS(...), hasRHS(...)).

53

Please use hasAnyName. It's more efficient and readable.

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

10–12

Please add a corresponding alias into the cert module.

[Eugene.Zelenko]

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

20

Unnecessary empty line.

76

Unnecessary empty line.

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

6

Please synchronize this statement with Release Notes.

[Eugene.Zelenko]

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

10–12

See also other aliased checks for documentation examples.

41

Unnecessary empty line.

66

Unnecessary empty line.

[aaron.ballman]

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

10

I kind of wonder whether we want it as an alias in the CERT module or just move it into the CERT module directly (and maybe provide an alias to bugprone, if we think the fp rate is reasonable enough).

[ztamas]

I'll update the patch based on the comments.
Just a note about the false positive ratio. In LibreOffice we run other static analyzers too, that's why there were less positive catches there than false positives. For example, PVS studio also catches unprotected copy assignment operators. Some of these issues were already fixed before I run this new check on the code:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=e5e0cc68f70d35e1849aeaf21c0ce68afd6a1f59

[ztamas]

Updated the code based on reviewer comments:

• Alphabetical order in BugproneTidyModule.cpp
• Handling of auto_ptr
• Test cases for template classes (I made the check ignore these case otherwise this can lead to false positives)
• Added cert alias
• Simplified the matcher as requested (hasAnyName(), has(ignoringParenCasts(cxxThisExpr())
• Remove empty lines and synchronized the corresponding comment with the Release Notes.

[Eugene.Zelenko]

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

7

Please use back-tick to highlight cert-oop54-cpp.

13

Should be removed, since it's in cert-oop54-cpp.rst now.

[ztamas]

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

10

Now, I just added a cert alias. I can move this check to cert module entirely if needed. In general, I prefer a meaningful name over a cert number, that's why it might be more useful to have also a bugprone prefixed check for this. And it seems to me cert checks used to be the aliases.

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

I tested with template classes, but TemplateCopyAndSwap test case, for example, was wrongly caught by the check. So I just changed the code to ignore template classes. I did not find any template class related catch either in LibreOffice or LLVM when I run the first version of this check, so we don't lose too much with ignoring template classes, I think.

[ztamas]

Documentation fixes.

[ztamas]

Missed to syncronize ReleasNotes with the corresponding doc.

[xazax.hun]

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

51

I am perfectly fine with addressing this in a follow-up patch or even not addressing at all, but I think for some users it might be useful to be able to specify a set of suspicious types as a configuration option such as custom smart pointers, handles and so on.

[ztamas]

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

51

Seems a good idea for a follow-up change.
Actually, with an earlier version of this patch I found another vulnerable copy assignment operator in llvm code which uses a custom pointer type (PointerIntPair):
FunctionInfo &operator=(const FunctionInfo &RHS)

[xazax.hun]

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

327

While I do agree move assignment operators should not be checked by default some would argue that move assignments should still be tolerant to self assignments as one could write something like:

x = std::move(aliasToX);

Having an option to also check move assignments in a follow-up patch would be also nice to have.

[aaron.ballman]

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

134–135

This should be done in a separate patch, as it's unrelated to this patch.

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

20

You should only register these matchers when in C++ mode.

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

7

You should add a link to CERT's documentation somewhere around this text.

10

If the purpose to the check is to conform to a specific coding standard, we usually put the check with the coding standard it supports unless the check is generally useful (then we put it into a module and alias into the coding standard module). I'm having a hard time convincing myself which way to go in this case, so I guess that means I don't have a strong opinion. :-)

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

I am not keen on ignoring template classes for the check; that seems like a bug in the check that should be addressed. At a minimum, the test cases should be present with FIXME comments about the unwanted diagnostics.

[ztamas]

Update patch based on reviewer comments.

[ztamas]

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

7

In an earlier version of this patch, there was a link. I removed it because of a reviewer comment. Now I add it back, I hope now are OK.

[ztamas]

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

I don't think it's a good idea to change the check now to catch template classes and produce false positives.

First of all the check does not work with template classes because the AST is different. Check TemplateCopyAndSwap test case for example. It's expected that the definition of operator= contains a CXXConstructExpr, but something is different for templates. It might be a lower level problem, how to detect a constructor call in a template class or templates just need different matcher. So implementing this check with correct template handling might be tricky and it might make the checker more complex. I'm not sure it worth the time, because as I mentioned I did not see any template related catch in the tested code bases. However, it might be a good idea to mention this limitation in the documentation.

About the false positives. This template related false positives are different from other false positives. In general, check doesn't warn, if the code uses one of the three patterns (self-check, copy-and-move, copy-and-swap). However, TemplateCopyAndSwap test case was wrongly caught by the check even though it uses copy-and-swap method. I would not introduce this kind of false positives. So the user of the check can expect that if he / she uses one of the three patterns, then there will be no warning in his / her code.

I already added five template related test cases. I think the comments are clear about which test case should be ignored by the check and which test cases are incorrectly ignored by now.

[ztamas]

Add false positive test cases.
Added a note about template related limitation to the docs.

[aaron.ballman]

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

35–37

Will this also match code like:

Frobble &Frobble::operator=(const Frobble &F) {
  if (&F == this->whatever())
    return *this;
}

or, more insidiously:

Frobble &Frobble::operator=(const Frobble &F) {
  if (&F == whatever()) // whatever is a member function of Frobble
    return *this;
}

55

These should be using ::std::whatever to ensure we only care about names in the global namespace named std.

Should this list be configurable? For instance, boost has a fair number of smart pointers.

62

Missing a full stop at the end of the comment.

63–65

Hmm, while I understand why you're doing this, I'm worried that it will miss some pretty important cases. For instance, improper thread locking could result in deadlocks, improper releasing of non-memory resources could be problematic (such as network connections, file streams, etc), even simple integer assignments could be problematic in theory:

Yobble& Yobble::operator=(const Yobble &Y) {
  superSecretHashVal = 0; // Being secure!
  ... some code that may early return ...
  superSecretHashVal = Y.superSecretHashVal;
}

I wonder whether we want an option here to allow users to diagnose regardless of whether we think it's suspicious or not.

At the very least, this code should not be enabled for the CERT version of the check as it doesn't conform to the CERT requirements.

83

Hmm, the "might not" seems a bit flat to me. How about: 'operator=()' does not properly test for self-assignment?

Also, do we want to have a fix-it to insert a check for self assignment at the start of the function?

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

7

Thank you for adding the link!

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

So implementing this check with correct template handling might be tricky and it might make the checker more complex.

I would be surprised if it added that much complexity. You wouldn't be checking the template declarations, but the template instantiations themselves, which should have the same AST representation as similar non-templated code.

I'm not sure it worth the time, because as I mentioned I did not see any template related catch in the tested code bases.

It's needed to conform to the CERT coding standard, which has no exceptions for templates here.

However, it might be a good idea to mention this limitation in the documentation.

My preference is to support it from the start, but if we don't support it, it should definitely be mentioned in the documentation.

[ztamas]

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

35–37

I'll check that case. The original hasLHS(...), hasRHS(...) might work with this use case too.

55

Should this list be configurable? For instance, boost has a fair number of smart pointers.

xazax.hun has the same suggestion. I think it's a good idea for a follow-up patch. I actually added a test case with the name CustomPtrField to document this future possibility.

63–65

It's starting to be too much for me.
First, the problem was false positives. If there are too many false positives then better to have it in the cert module.
Now your problem is that this is not fit with the CERT requirements, because of this matcher which reduces the false positives. Adding this check to the CERT module was not my idea in the first place. So I suggest to have it a simple bugprone check (and remove the cert alias) and also we can mention that it is related to the corresponding cert rule (but it does not conform with it entirely).
This check allows the usage of copy-and-move pattern, which does not conform with the cert specification either where only the self-check and copy-and-swap is mentioned. So your next suggestion will be to not allow copy-and-move because it does not fit with the cert rule? So I think it's better to have this not a cert check then, but a bugprone check. I prefer to have a working check then implementing a theoretical documentation.

Apart from that cert thing, it actually seems a good idea to add a config option to allow the user to get all catches, not just the "suspicious ones".

83

I don't think "test for self-assignment" will be good, because it's only one way to make the operator self assignment safe. In case of copy-and-swap and copy-and-move there is no testing of self assignment, but swaping/moving works in all cases without explicit self check.

A fix-it can be a good idea for a follow-up patch.

[ztamas]

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

I added instatiation of template classes to the test code (locally):

template <class T>
class TemplateCopyAndMove {
public:
  TemplateCopyAndMove<T> &operator=(const TemplateCopyAndMove<T> &object) {
    TemplateCopyAndMove<T> temp(object);
    *this = std::move(temp);
    return *this;
  }

private:
  T *p;
};

int instaniateTemplateCopyAndMove() {
    TemplateCopyAndMove<int> x;
    (void) x;
}

However I don't see the expected AST neither in the ClassTemplateDecl or in the ClassTemplateSpecializationDecl. So how can I get that AST which is similar to non-template case?

|-ClassTemplateDecl 0x117ed20 <line:341:1, line:352:1> line:342:7 TemplateCopyAndMove
| |-TemplateTypeParmDecl 0x117ec08 <line:341:11, col:17> col:17 referenced class depth 0 index 0 T
| |-CXXRecordDecl 0x117ec90 <line:342:1, line:352:1> line:342:7 class TemplateCopyAndMove definition
| | |-DefinitionData standard_layout
| | | |-DefaultConstructor exists trivial needs_implicit
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor
| | | |-CopyAssignment non_trivial has_const_param user_declared implicit_has_const_param
| | | |-MoveAssignment
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x117ef70 <col:1, col:7> col:7 implicit class TemplateCopyAndMove
| | |-AccessSpecDecl 0x117f000 <line:343:1, col:7> col:1 public
| | |-CXXMethodDecl 0x117f9d0 <line:344:3, line:348:3> line:344:27 operator= 'TemplateCopyAndMove<T> &(const TemplateCopyAndMove<T> &)'
| | | |-ParmVarDecl 0x117f820 <col:37, col:67> col:67 referenced object 'const TemplateCopyAndMove<T> &'
| | | `-CompoundStmt 0x117fe30 <col:75, line:348:3>
| | |   |-DeclStmt 0x117fcb8 <line:345:5, col:40>
| | |   | `-VarDecl 0x117fc10 <col:5, col:39> col:28 referenced temp 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' callinit
| | |   |   `-ParenListExpr 0x117fc98 <col:32, col:39> 'NULL TYPE'
| | |   |     `-DeclRefExpr 0x117fbc0 <col:33> 'const TemplateCopyAndMove<T>':'const TemplateCopyAndMove<T>' lvalue ParmVar 0x117f820 'object' 'const TemplateCopyAndMove<T> &'
| | |   |-BinaryOperator 0x117fda8 <line:346:5, col:27> '<dependent type>' '='
| | |   | |-UnaryOperator 0x117fce0 <col:5, col:6> '<dependent type>' prefix '*' cannot overflow
| | |   | | `-CXXThisExpr 0x117fcd0 <col:6> 'TemplateCopyAndMove<T> *' this
| | |   | `-CallExpr 0x117fd80 <col:13, col:27> '<dependent type>'
| | |   |   |-UnresolvedLookupExpr 0x117fd18 <col:13, col:18> '<overloaded function type>' lvalue (no ADL) = 'move' 0x1137968
| | |   |   `-DeclRefExpr 0x117fd60 <col:23> 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' lvalue Var 0x117fc10 'temp' 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>'
| | |   `-ReturnStmt 0x117fe20 <line:347:5, col:13>
| | |     `-UnaryOperator 0x117fe08 <col:12, col:13> '<dependent type>' prefix '*' cannot overflow
| | |       `-CXXThisExpr 0x117fdf8 <col:13> 'TemplateCopyAndMove<T> *' this
| | |-AccessSpecDecl 0x117fa78 <line:350:1, col:8> col:1 private
| | `-FieldDecl 0x117fad8 <line:351:3, col:6> col:6 p 'T *'
| `-ClassTemplateSpecializationDecl 0x117ff38 <line:341:1, line:352:1> line:342:7 class TemplateCopyAndMove definition
|   |-DefinitionData pass_in_registers standard_layout literal
|   | |-DefaultConstructor exists trivial
|   | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
|   | |-MoveConstructor
|   | |-CopyAssignment non_trivial has_const_param user_declared implicit_has_const_param
|   | |-MoveAssignment
|   | `-Destructor simple irrelevant trivial needs_implicit
|   |-TemplateArgument type 'int'
|   |-CXXRecordDecl 0x11801b0 prev 0x117ff38 <col:1, col:7> col:7 implicit class TemplateCopyAndMove
|   |-AccessSpecDecl 0x1180240 <line:343:1, col:7> col:1 public
|   |-CXXMethodDecl 0x1180550 <line:344:3, line:348:3> line:344:27 operator= 'TemplateCopyAndMove<int> &(const TemplateCopyAndMove<int> &)'
|   | `-ParmVarDecl 0x1180430 <col:37, col:67> col:67 object 'const TemplateCopyAndMove<int> &'
|   |-AccessSpecDecl 0x1180608 <line:350:1, col:8> col:1 private
|   |-FieldDecl 0x1180668 <line:351:3, col:6> col:6 p 'int *'
|   |-CXXConstructorDecl 0x11806e8 <line:342:7> col:7 implicit used TemplateCopyAndMove 'void () noexcept' inline default trivial
|   | `-CompoundStmt 0x1181528 <col:7>
|   `-CXXConstructorDecl 0x11813a0 <col:7> col:7 implicit constexpr TemplateCopyAndMove 'void (const TemplateCopyAndMove<int> &)' inline default trivial noexcept-unevaluated 0x11813a0
|     `-ParmVarDecl 0x11814b8 <col:7> col:7 'const TemplateCopyAndMove<int> &'

[ztamas]

Added missing fullstop
Added a new test cases making sure that HasNoSelfCheck works correctly
Added template instantiation to tests
Remove the alias from cert module and add the CERT link as a see also

[ztamas]

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

35–37

I added NotSelfCheck test case for this, which works correctly.

[ztamas]

Ok, so I removed the alias from cert module and added CERT rule link as a "see also".
So I think we solved the problem that things do not conform with the CERT requirements and can focus on the actual problem.

Summary, templates are still ignored. If there is any idea how to solve this for templates easily or there is any sample code for this, then I happy to implement template support.
I see a related comment in clang-tidy/modernize/PassByValueCheck.cpp:

// Clang builds a CXXConstructExpr only when it knows which
// constructor will be called. In dependent contexts a
// ParenListExpr is generated instead of a CXXConstructExpr,
// filtering out templates automatically for us.

So It's not only me who sees that template AST is different.

Also, two good ideas were mentioned about extra options during the review.

1. Allow finding catches not just pointers and arrays
2. Allow configuring list of suspicious field types / pointer types

I think these two options are good ideas for future improvements. For now, the check seems useful and produces not many false positives in the current form, I think.

[aaron.ballman]

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

55

Yeah, I think it's fine to leave the list until a follow-up patch (though the names should still be corrected for this patch).

63–65

It's starting to be too much for me.

It can be tricky to get this stuff right; I'm sorry the difficulties are aggravating.

First, the problem was false positives. If there are too many false positives then better to have it in the cert module.
Now your problem is that this is not fit with the CERT requirements, because of this matcher which reduces the false positives.
Adding this check to the CERT module was not my idea in the first place. So I suggest to have it a simple bugprone check (and remove the cert alias) and also we can mention that it is related to the corresponding cert rule (but it does not conform with it entirely).

We typically ask authors to support the various coding standard modules when plausible because of the considerable amount of overlap between the functionalities. I don't particularly like the idea of ignoring the CERT coding standard here given that the solution is almost trivial to implement. However, if you want to remove it from the CERT module and not support it, that's your choice.

This check allows the usage of copy-and-move pattern, which does not conform with the cert specification either where only the self-check and copy-and-swap is mentioned.

I don't get that from my reading of the CERT rule, where it says, "A user-provided copy assignment operator must prevent self-copy assignment from leaving the object in an indeterminate state. This can be accomplished by self-assignment tests, copy-and-swap, or other idiomatic design patterns.". Copy-and-move is another idiomatic design pattern for dealing with this, and I'm glad your check incorporates it. (tbh, it would be nice for the CERT rule to have a compliant solution demonstrating it -- I'll recommend it on their rule.)

So your next suggestion will be to not allow copy-and-move because it does not fit with the cert rule? So I think it's better to have this not a cert check then, but a bugprone check. I prefer to have a working check then implementing a theoretical documentation.

Theoretical documentation? The CERT standard is a published standard used in industry that's supported by other analyzers as well as clang-tidy, so it's not really theoretical.

Apart from that cert thing, it actually seems a good idea to add a config option to allow the user to get all catches, not just the "suspicious ones".

Agreed. FWIW, having a config option is what would make it trivial to support in both modules. See getModuleOptions() in CERTTidyModule.cpp for an example of how we control the behavior of readability-uppercase-literal-suffix differently when its enabled through the CERT check name.

83

Good point on the use of "test" in the diagnostic. My concern is more with it sounding like we don't actually know -- the "might" is what's bothering me. I think if we switch it to "does not", it'd be a bit better.

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

Hmm, I believe it's this bit (which is different than the non-template case, but still shows the initialization of a local variable of the expected type):

| | |   |-DeclStmt 0x117fcb8 <line:345:5, col:40>
| | |   | `-VarDecl 0x117fc10 <col:5, col:39> col:28 referenced temp 'TemplateCopyAndMove<T>':'TemplateCopyAndMove<T>' callinit
| | |   |   `-ParenListExpr 0x117fc98 <col:32, col:39> 'NULL TYPE'
| | |   |     `-DeclRefExpr 0x117fbc0 <col:33> 'const TemplateCopyAndMove<T>':'const TemplateCopyAndMove<T>' lvalue ParmVar 0x117f820 'object' 'const TemplateCopyAndMove<T> &'

but the fact that the ParenListExpr has a null type is surprising to me. Curiously enough, you get better type information with an initializer list or an assignment expression as the initializer.

[ztamas]

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

63–65

I don't get that from my reading of the CERT rule, where it says [...]

Ah, Ok. I missed that part, so copy-and-move won't be a problem.

Now, I removed the cert alias, but later it can be added back when the check can be configured so it conforms with the cert, I think.

83

We don't actually know. We check only some patterns, but there might be other operator=() implementations which are self assignment safe (see false positive test cases).

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

351

OK, I'll give it another try. Maybe it's better to not searching for a copy constructor, but a variable declaration with the corresponding type.

[ztamas]

Make the check to work with templates too

[ztamas]

I also reformatted the code with clang-format.

So now the templates are handled, however, it's still not fit with the cert rule because we not catch all classes, but only those who have suspicious fields. I think this one can be added in a follow-up patch and then this check can be added to the cert module as an alias.

[ztamas]

Mark some comments Done, which were handled some way.

[ztamas]

Remove outdated comment from docs.

[ztamas]

Ok, is there anything else should be included in this patch?
I saw more ideas about how to extend the functionality (custom pointers, fix-it, etc). I interpreted these ideas as nice-to-have things for the future.

[ztamas]

Better handling of template and non-template self-copy

[ztamas]

Ping.
Is it good to go or is there anything else I need to include in this patch?
Among the lots of idea, I'm not sure which is just brainstorming and which is a change request.
The check seems to work (has useful catches and does not produce too many false positives), however, it does not conform with the corresponding cert rule. I expect that a cert alias with an option can be added in a follow-up patch (option to ignore ThisHasSuspiciousField pattern). The bugprone-* check would have the same default behavior as it has now in this patch. So adding the cert alias would not change this behavior, right? In this case, this code change can be added in a separate patch I guess.

[aaron.ballman]

In D60507#1491095, @ztamas wrote:

Ping.
Is it good to go or is there anything else I need to include in this patch?

Sorry for the delay, I was gone for WG14 meetings last week, which made reviewing more involved patches somewhat difficult. I'm back now and starting to dig out from under the mountain of emails.

Among the lots of idea, I'm not sure which is just brainstorming and which is a change request.

Generally speaking, all feedback is a bit of a mixture of the two. They're change requests, but we can definitely brainstorm whether the request makes sense, how to do it, whether there are better changes to make instead, etc. We usually say something explicit like "but I don't insist" when we're just spitballing ideas. That said, I'm sorry if I was unclear on what changes I was insisting on.

I definitely want to see the diagnostic text changed away from "might be" -- that's too noncommittal for my tastes. I'd also like to see the additional test case (I suspect it will just work out of the box, but if it doesn't, it would be good to know about it). The CERT alias is a bit more of a grey area -- I'd like to insist on it being added because it's trivial, it improves the behavior of this check by introducing an option some users will want, and checks conforming to coding standards are always more compelling than one-off checks. However, you seem very resistant to that and I don't want to add to your work load if you are opposed to doing it, so I don't insist on the change.

The check seems to work (has useful catches and does not produce too many false positives), however, it does not conform with the corresponding cert rule. I expect that a cert alias with an option can be added in a follow-up patch (option to ignore ThisHasSuspiciousField pattern). The bugprone-* check would have the same default behavior as it has now in this patch. So adding the cert alias would not change this behavior, right? In this case, this code change can be added in a separate patch I guess.

Agreed.

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

83

I understand that we're using a heuristic, but that doesn't mean we use language like "might not" in the resulting diagnostic. Either we think the code is correct (we don't diagnose) or the code doesn't do what it needs to do to pass our check (we diagnose). That's why I recommended "does not" -- the operator=() does not handle self-assignment properly according to your check. I couldn't find any existing diagnostics saying the code "might not" do something properly.

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

215

Does the check diagnose code like this?

template <typename Ty, typename Uy>
class C {
  Ty *Ptr1;
  Uy *Ptr2;

public:
  C& operator=(const C<Uy, Ty> &RHS) {
    Ptr1 = RHS.getUy();
    Ptr2 = RHS.getTy();
    return *this;
  }

  Ty *getTy() const { return Ptr1; }
  Uy *getUy() const { return Ptr2; }
};

I'm hoping we don't consider it a copy assignment operator and thus don't diagnose (note that the template argument order is reversed in the assignment operator parameter type).

[ztamas]

Changed "might not" to "does not" in the warning message.
Added the requested test case with the swapped template arguments.

[ztamas]

copy operator -> copy assignment operator

[ztamas]

I definitely want to see the diagnostic text changed away from "might be" -- that's too noncommittal for my tastes. I'd also like to see the additional test case (I suspect it will just work out of the box, but if it doesn't, it would be good to know about it). The CERT alias is a bit more of a grey area -- I'd like to insist on it being added because it's trivial, it improves the behavior of this check by introducing an option some users will want, and checks conforming to coding standards are always more compelling than one-off checks. However, you seem very resistant to that and I don't want to add to your work load if you are opposed to doing it, so I don't insist on the change.

During LibreOffice development, I'm socialized to keep a patch as small as possible. Versioning history can be more "clean" with smaller patches. For example, it's easier to find and fix regressions with bisecting. Also smaller patches make the review process faster, etc. That's why I'm trying to separate what is absolutely necessary to include and what can be cut off from this patch.

I changed the warning message and I added the requested test case. This test case is ignored by the check as expected.

[aaron.ballman]

LGTM!

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

 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
+#include "UnhandledSelfAssignmentCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
+    CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
+        "bugprone-unhandled-self-assignment");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "bugprone-suspicious-string-compare");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
     CheckFactories.registerCheck<TerminatingContinueCheck>(
         "bugprone-terminating-continue");
     CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
         "bugprone-throw-keyword-missing");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnusedRaiiCheck>(
         "bugprone-unused-raii");
     CheckFactories.registerCheck<UnusedReturnValueCheck>(
         "bugprone-unused-return-value");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");

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

add_clang_library(clangTidyBugproneModule
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
+  UnhandledSelfAssignmentCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyCppCoreGuidelinesModule
   clangTidyUtils
   clangTooling
   )

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

+//===--- 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 {
+
+/// Find suspicious copy assignment operators which do not handle
+/// self-assignment. There are three patterns of handling self-assignment:
+/// self-assignment check, copy-and-swap or copy-and-move method. This check
+/// warns if it does not find any of these methods in a copy assignment
+/// operator's implementation.
+///
+/// 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

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

+//===--- 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("!=")),
+                     anyOf(hasLHS(ignoringParenCasts(cxxThisExpr())),
+                           hasRHS(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(
+          anyOf(hasName("std::shared_ptr"), hasName("std::unique_ptr"),
+                hasName("std::weak_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(isCopyAssignmentOperator(), IsUserDefined,
+                                   HasReferenceParam, HasNoSelfCheck,
+                                   ofClass(cxxRecordDecl().bind("class")),
+                                   HasNoSelfCopy, HasNoNestedSelfAssign,
+                                   ThisHasSuspiciousField)
+                         .bind("copyAssignmentOperator"),
+                     this);
+}
+
+void UnhandledSelfAssignmentCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<CXXMethodDecl>("copyAssignmentOperator");
+  diag(MatchedDecl->getLocation(),
+       "operator=() might not handle self-assignment properly");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`abseil-time-comparison
   domain.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 
   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
+  <clang-tidy/checks/bugprone-unhandled-self-assignment>` check.
+
+  Checks copy assignment operators whether they handle self-assignment
+  either by checking self-assignment explicitly or using the copy-and-swap
+  or copy-and-move method.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - New alias :doc:`cppcoreguidelines-explicit-virtual-functions

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

+.. title:: clang-tidy - bugprone-unhandled-self-assignment
+
+bugprone-unhandled-self-assignment
+==================================
+
+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 corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment
+<https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP54-CPP.+Gracefully+handle+self-copy+assignment>`_.
+
+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;
+    }
+  };

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

.. toctree::
    bugprone-suspicious-semicolon
    bugprone-suspicious-string-compare
    bugprone-swapped-arguments
    bugprone-terminating-continue
    bugprone-throw-keyword-missing
    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
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl16-c (redirects to readability-uppercase-literal-suffix) <cert-dcl16-c>
    cert-dcl21-cpp
    cert-dcl50-cpp

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template <class T>
+void swap(T x, T y) {
+}
+
+template <class T>
+T &&move(T x) {
+}
+
+template <class T>
+class unique_ptr {
+};
+
+template <class T>
+class shared_ptr {
+};
+
+template <class T>
+class weak_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<int> 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<int> 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<int> 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;
+};
+
+// 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;
+};
+
+// 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;
+};
+
+// 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;
+};