This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ztamas on Apr 10 2019, 5:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ztamas created this revision.Apr 10 2019, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2019, 5:22 AM
ztamas added a comment.EditedApr 10 2019, 5:26 AM

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.

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 added a project: Restricted Project.
JonasToth added inline comments.Apr 10 2019, 6:09 AM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
102 ↗(On Diff #194496)

please order this list alphabetically.

clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
51 ↗(On Diff #194496)

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 ↗(On Diff #194496)

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 ↗(On Diff #194496)

Please add tests with templated classes and self-assignment.

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

clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
34–35 ↗(On Diff #194496)

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

53 ↗(On Diff #194496)

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

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
10–12 ↗(On Diff #194496)

Please add a corresponding alias into the cert module.

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
20 ↗(On Diff #194496)

Unnecessary empty line.

76 ↗(On Diff #194496)

Unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
6 ↗(On Diff #194496)

Please synchronize this statement with Release Notes.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
10–12 ↗(On Diff #194496)

See also other aliased checks for documentation examples.

41 ↗(On Diff #194496)

Unnecessary empty line.

66 ↗(On Diff #194496)

Unnecessary empty line.

aaron.ballman added inline comments.Apr 11 2019, 10:12 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
10 ↗(On Diff #194496)

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

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 this revision to Diff 194921.EditedApr 12 2019, 11:17 AM

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.
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
6 ↗(On Diff #194921)

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

12 ↗(On Diff #194921)

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

ztamas marked 13 inline comments as done.Apr 12 2019, 11:29 AM
ztamas added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
10 ↗(On Diff #194496)

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 ↗(On Diff #194496)

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 updated this revision to Diff 194925.Apr 12 2019, 11:33 AM
ztamas marked an inline comment as done.

Documentation fixes.

ztamas marked 2 inline comments as done.Apr 12 2019, 11:34 AM
ztamas updated this revision to Diff 195061.Apr 14 2019, 6:04 AM

Missed to syncronize ReleasNotes with the corresponding doc.

xazax.hun added inline comments.Apr 14 2019, 6:33 AM
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
51 ↗(On Diff #194496)

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 marked an inline comment as done.Apr 14 2019, 7:19 AM
ztamas added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
51 ↗(On Diff #194496)

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 added inline comments.Apr 14 2019, 7:43 AM
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
326 ↗(On Diff #195061)

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 added inline comments.Apr 16 2019, 5:20 AM
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
130–131 ↗(On Diff #195061)

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

clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
19 ↗(On Diff #195061)

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

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
6 ↗(On Diff #195061)

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

10 ↗(On Diff #194496)

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 ↗(On Diff #194496)

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 updated this revision to Diff 195382.Apr 16 2019, 7:41 AM

Update patch based on reviewer comments.

ztamas marked 4 inline comments as done.Apr 16 2019, 7:45 AM
ztamas added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
6 ↗(On Diff #195061)

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 marked 2 inline comments as done.Apr 16 2019, 8:15 AM
ztamas added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
351 ↗(On Diff #194496)

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 updated this revision to Diff 196066.Apr 22 2019, 6:55 AM

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

aaron.ballman marked an inline comment as done.Apr 23 2019, 7:47 AM
aaron.ballman added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
34–36 ↗(On Diff #196066)

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;
}
54 ↗(On Diff #196066)

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.

61 ↗(On Diff #196066)

Missing a full stop at the end of the comment.

62–64 ↗(On Diff #196066)

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.

82 ↗(On Diff #196066)

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
6 ↗(On Diff #195061)

Thank you for adding the link!

clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
351 ↗(On Diff #194496)

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 marked 4 inline comments as done.Apr 23 2019, 8:31 AM
ztamas added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
34–36 ↗(On Diff #196066)

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

54 ↗(On Diff #196066)

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.

62–64 ↗(On Diff #196066)

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

82 ↗(On Diff #196066)

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 marked an inline comment as done.Apr 23 2019, 10:05 AM
ztamas added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
351 ↗(On Diff #194496)

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 updated this revision to Diff 196279.Apr 23 2019, 10:37 AM

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 marked 5 inline comments as done.Apr 23 2019, 10:40 AM
ztamas added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
34–36 ↗(On Diff #196066)

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

ztamas added a comment.EditedApr 23 2019, 10:57 AM

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 added inline comments.Apr 23 2019, 10:59 AM
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
54 ↗(On Diff #196066)

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

62–64 ↗(On Diff #196066)

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.

82 ↗(On Diff #196066)

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 ↗(On Diff #194496)

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 marked 3 inline comments as done.Apr 23 2019, 11:15 AM
ztamas added inline comments.
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
62–64 ↗(On Diff #196066)

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.

82 ↗(On Diff #196066)

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 ↗(On Diff #194496)

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 updated this revision to Diff 196295.Apr 23 2019, 11:49 AM

Make the check to work with templates too

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 marked 8 inline comments as done.Apr 23 2019, 11:56 AM

Mark some comments Done, which were handled some way.

ztamas updated this revision to Diff 196410.Apr 24 2019, 3:26 AM

Remove outdated comment from docs.

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 updated this revision to Diff 197014.Apr 28 2019, 3:15 AM

Better handling of template and non-template self-copy

ztamas added a comment.May 5 2019, 5:05 AM

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.

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
82 ↗(On Diff #196066)

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
214 ↗(On Diff #197014)

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 updated this revision to Diff 198787.May 9 2019, 4:22 AM

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

ztamas updated this revision to Diff 198789.May 9 2019, 4:27 AM

copy operator -> copy assignment operator

ztamas marked 4 inline comments as done.May 9 2019, 4:29 AM
ztamas added a comment.EditedMay 9 2019, 4:35 AM

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.

This revision is now accepted and ready to land.May 9 2019, 5:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 5:23 AM