[clang-tidy] Don't warn about call to unresolved operator*

[malcolm.parsons]

Summary

The misc-unconventional-assign-operator check had a false positive
warning when the 'operator*' in 'return *this' was unresolved.

Change matcher to allow calls to unresolved operator.

Fixes PR31531.

[idlecode]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

63 ↗

(On Diff #86623)

Seems that it will catch all unary operators with this as first argument.
e.g. in case operator- is defined somewhere, check will not warn about returning -this.
Do you think adding hasOverloadedOperatorName("*") for such abstract case is worth it?

[malcolm.parsons]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

63 ↗

(On Diff #86623)

I'm just trying to suppress them warning from the template. The operator can be checked properly in an instantiation.

[idlecode]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

63 ↗

(On Diff #86623)

LGTM then :)

[aaron.ballman]

LGTM with a minor nit that can be fixed without further review.

test/clang-tidy/misc-unconventional-assign-operator.cpp

92 ↗

(On Diff #86623)

You should put a comment near here to explain why this is a necessary part of the test.

Also, when fixing PRs, it's sometimes helpful to put the entire test into a namespace named after the PR (so people can do historical tracking a bit more easily). e.g., namespace pr31531. We've not done this often for clang-tidy, but it's quite common in clang's tests and might be a nice habit to get into -- it helps reduce accidental name collisions as a benefit.

clang-tools-extra/trunk/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

Finder->addMatcher(
           .bind("ArgumentType"),
       this);
 
   Finder->addMatcher(
       cxxMethodDecl(IsSelfAssign, anyOf(isConst(), isVirtual())).bind("cv"),
       this);
 
   const auto IsBadReturnStatement = returnStmt(unless(has(ignoringParenImpCasts(
-      unaryOperator(hasOperatorName("*"), hasUnaryOperand(cxxThisExpr()))))));
+      anyOf(unaryOperator(hasOperatorName("*"), hasUnaryOperand(cxxThisExpr())),
+            cxxOperatorCallExpr(argumentCountIs(1),
+                                callee(unresolvedLookupExpr()),
+                                hasArgument(0, cxxThisExpr())))))));
   const auto IsGoodAssign = cxxMethodDecl(IsAssign, HasGoodReturnType);
 
   Finder->addMatcher(returnStmt(IsBadReturnStatement, forFunction(IsGoodAssign))
                          .bind("returnStmt"),
                      this);
 }
 
 void UnconventionalAssignOperatorCheck::check(

clang-tools-extra/trunk/test/clang-tidy/misc-unconventional-assign-operator.cpp

// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this'
 
   // Do not check if return type is different from '&BadReturnStatement'
   int operator=(int i) {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
     n = i;
     return n;
   }
 };
+
+namespace pr31531 {
+enum E { e };
+// This declaration makes the 'return *this' below have an unresolved operator
+// in the class template, but not in an instantiation.
+E operator*(E, E);
+
+template <typename>
+struct UnresolvedOperator {
+  UnresolvedOperator &operator=(const UnresolvedOperator &) { return *this; }
+};
+
+UnresolvedOperator<int> UnresolvedOperatorInt;
+
+template <typename>
+struct Template {
+  Template &operator=(const Template &) { return this; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: operator=() should always return '*this'
+};
+
+Template<int> TemplateInt;
+}