[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment
Tamás Zolnai via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 23 11:15:37 PDT 2019
ztamas marked 3 inline comments as done.
ztamas added inline comments.
================
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())))))));
----------------
aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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".
> > 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.
> 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.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+ diag(MatchedDecl->getLocation(),
+ "operator=() might not handle self-assignment properly");
+}
----------------
aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > 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?
> > 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.
> 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.
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).
================
Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+ int *p;
+};
----------------
aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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> &'
> >
> > ```
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60507/new/
https://reviews.llvm.org/D60507
More information about the cfe-commits
mailing list