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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     has(ignoringParenCasts(cxxThisExpr()))))));
----------------
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;
}
```



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+      recordType(hasDeclaration(classTemplateSpecializationDecl(
+          hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+                     "std::auto_ptr"),
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:61
+  // pointer member, then trying to access the object pointed by the pointer, or
+  // memcpy overlapping arrays)
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
----------------
Missing a full stop at the end of the comment.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+      has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+                          hasType(arrayType())))))));
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+       "operator=() might not handle self-assignment properly");
+}
----------------
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?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+`cert-oop54-cpp` redirects here as an alias for this check.
+
----------------
ztamas wrote:
> aaron.ballman wrote:
> > You should add a link to CERT's documentation somewhere around this text.
> 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.
Thank you for adding the link!


================
Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};
----------------
ztamas wrote:
> aaron.ballman wrote:
> > ztamas wrote:
> > > JonasToth wrote:
> > > > Please add tests with templated classes and self-assignment.
> > > 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.
> > 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.
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507