[clang-tidy]: fix false positive of cert-oop54-cpp check.

[ztamas]

Summary

It seems we need a different matcher for binary operator
in a template context.

Fixes this issue:
https://bugs.llvm.org/show_bug.cgi?id=44499

[ztamas]

Run clang-format on test code.

[njames93]

I'm not entirely sure this is where the fix needs to be for this. The test case code is whacky as hell, but from what I can see clang should always emit a BinaryOperator for dependent type operators. No idea why it is spewing out a CXXOperatorCallExpr instead. Would need someone with more knowledge on Sema to confirm(or deny) this though.

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

516

clang format artefact? This change should be undone as its unrelated to the patch.

[ztamas]

Rebase, TODO comment, remove unrelated change.

[ztamas]

In D76990#1948286, @njames93 wrote:

I'm not entirely sure this is where the fix needs to be for this. The test case code is whacky as hell, but from what I can see clang should always emit a BinaryOperator for dependent type operators. No idea why it is spewing out a CXXOperatorCallExpr instead. Would need someone with more knowledge on Sema to confirm(or deny) this though.

I agree, it seems suspicious that a BinaryOperator matcher does not work in this case. However, I'm working on this level of the code, I'm looking at the matchers like an API, what I'm just using in the clang-tidy code, without changing them. So that's why I added this workaround. I don't know how much time it would take to fix this issue in the matcher code or whether it will be fixed in the near future or not, but until then I think it useful to have this workaround in the caller code, so this clang-tidy check works better. I added a TODO comment, so it's more visible that we have an issue here, so it's more probable somebody will fix that working with the matchers.

[njames93]

In D76990#1948516, @ztamas wrote:

I agree, it seems suspicious that a BinaryOperator matcher does not work in this case. However, I'm working on this level of the code, I'm looking at the matchers like an API, what I'm just using in the clang-tidy code, without changing them. So that's why I added this workaround. I don't know how much time it would take to fix this issue in the matcher code or whether it will be fixed in the near future or not, but until then I think it useful to have this workaround in the caller code, so this clang-tidy check works better. I added a TODO comment, so it's more visible that we have an issue here, so it's more probable somebody will fix that working with the matchers.

It's not the matchers fault, that's working as intended. its the AST that clang has generated for that source code that looks suspicious.

[ztamas]

Remove false TODO comment.

[aaron.ballman]

LGTM with a testing request. I agree that the Clang AST is a bit surprising, but not so surprising that I could definitely call it a bug.

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

286–299

Thank you for this! Can you also add a test case where the diagnostic should trigger? e.g.,

class Foo;
template <int a>
bool operator!=(Foo &, Foo &) {
  class Bar {
    Bar &operator=(const Bar &other) {
      p = other.p;
      return *this;
    }

    int *p;
  };
}

[ztamas]

Add suggested test case and rebase.

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

void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
       hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-                                   has(ignoringParenCasts(cxxThisExpr()))))));
+                                   has(ignoringParenCasts(cxxThisExpr())))),
+      hasDescendant(cxxOperatorCallExpr(
+          hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+          has(ignoringParenCasts(cxxThisExpr())))))));
 
   // Both copy-and-swap and copy-and-move method creates a copy first and
   // assign it to 'this' with swap or move.
   // In the non-template case, we can search for the copy constructor call.
   const auto HasNonTemplateSelfCopy = cxxMethodDecl(
       ofClass(cxxRecordDecl(unless(hasAncestor(classTemplateDecl())))),
       hasDescendant(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
           isCopyConstructor(), ofClass(equalsBoundNode("class")))))));

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

TemplateSelfCheck<T> &operator=(const TemplateSelfCheck<T> &object) {
     }
     return *this;
   }
 
 private:
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template <int a>
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+    Bar &operator=(const Bar &other) {
+      if (this != &other) {
+      }
+      return *this;
+    }
+
+    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;
   }
 

 // Make sure we don't catch this operator=() as a copy assignment operator.
 // Note that RHS has swapped template arguments.
 template <typename Ty, typename Uy>
 class NotACopyAssignmentOperator {
   Ty *Ptr1;
   Uy *Ptr2;
 
 public:
   NotACopyAssignmentOperator& operator=(const NotACopyAssignmentOperator<Uy, Ty> &RHS) {
     Ptr1 = RHS.getUy();
     Ptr2 = RHS.getTy();
     return *this;
   }
 
   Ty *getTy() const { return Ptr1; }
   Uy *getUy() const { return Ptr2; }
 };