[clang-tidy] Fix false positives involving type aliases in `misc-unconventional-assign-operator` check

[fwolff]

Summary

clang-tidy currently reports false positives even for simple cases such as:

struct S {
    using X = S;
    X &operator=(const X&) { return *this; }
};

This is due to the fact that the misc-unconventional-assign-operator check fails to look at the canonical types. This patch fixes this behavior.

[mizvekov]

LGTM

[fwolff]

Thanks for the quick review! Can you also commit it for me? You can use name and email as in commit 738e7f1231949ec248c1d8d154783338215613d1. Thank you!

[mizvekov]

In D114197#3141400, @fwolff wrote:

Thanks for the quick review! Can you also commit it for me? You can use name and email as in commit 738e7f1231949ec248c1d8d154783338215613d1. Thank you!

Sure np, I will give some time for other reviewers to chime in though :)

But FYI in case you are interested in contributing more, you can get your own license to kill here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[whisperity]

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

151

This is a no-warn due to the parameter being a completely unrelated type, right? Might worth a comment. I don't see at first glance why a warning should not happen here.

152

What about Alias3<TypeAlias::Alias>& operator =(const Alias1&) { return *this; }? That should trigger a warning as it is an unrelated type, right?

[fwolff]

Thanks for your comments @whisperity. I think I've addressed them, could you have another look?

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

151

Exactly. I've added a comment in the TypeAlias struct above, because that one comes first. I've also updated the documentation for this check to make this clearer.

152

Yes, I've added a test for this (but with double as the argument type, because const Alias1& gives a warning:

warning: 'const' qualifier on reference type 'TemplateTypeAlias::Alias1' (aka 'TemplateTypeAlias<T> &') has no effect [-Wignored-qualifiers]

[• Quuxplusone]

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

151

This is a no-warn due to the parameter being a completely unrelated type, right?

No. If that were the case, this would be a terrible test case, full of irrelevant cruft. :)

Once you look past all the levels of type aliases, this is equivalent to

template<class T>
struct TTA {
    TTA<T>& operator=(int) { return *this; }
};

and so no warning is given because it's correct code.

[aaron.ballman]

LGTM aside from some small nits.

clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst

12

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

131

Can you also add tests which use typedef instead of using? (They should just work as-is)

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

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 void UnconventionalAssignOperatorCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
-  const auto HasGoodReturnType = cxxMethodDecl(returns(lValueReferenceType(
-      pointee(unless(isConstQualified()),
-              anyOf(autoType(), hasDeclaration(equalsBoundNode("class")))))));
+  const auto HasGoodReturnType =
+      cxxMethodDecl(returns(hasCanonicalType(lValueReferenceType(pointee(
+          unless(isConstQualified()),
+          anyOf(autoType(), hasDeclaration(equalsBoundNode("class"))))))));
 
-  const auto IsSelf = qualType(
+  const auto IsSelf = qualType(hasCanonicalType(
       anyOf(hasDeclaration(equalsBoundNode("class")),
-            referenceType(pointee(hasDeclaration(equalsBoundNode("class"))))));
+            referenceType(pointee(hasDeclaration(equalsBoundNode("class")))))));
   const auto IsAssign =
       cxxMethodDecl(unless(anyOf(isDeleted(), isPrivate(), isImplicit())),
                     hasName("operator="), ofClass(recordDecl().bind("class")))
           .bind("method");
   const auto IsSelfAssign =
       cxxMethodDecl(IsAssign, hasParameter(0, parmVarDecl(hasType(IsSelf))))
           .bind("method");
 
   Finder->addMatcher(
       cxxMethodDecl(IsAssign, unless(HasGoodReturnType)).bind("ReturnType"),
       this);
 
-  const auto BadSelf = referenceType(
+  const auto BadSelf = qualType(hasCanonicalType(referenceType(
       anyOf(lValueReferenceType(pointee(unless(isConstQualified()))),
-            rValueReferenceType(pointee(isConstQualified()))));
+            rValueReferenceType(pointee(isConstQualified()))))));
 
   Finder->addMatcher(
       cxxMethodDecl(IsSelfAssign,
                     hasParameter(0, parmVarDecl(hasType(BadSelf))))
           .bind("ArgumentType"),
       this);
 
   Finder->addMatcher(

clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst

 .. title:: clang-tidy - misc-unconventional-assign-operator
 
 misc-unconventional-assign-operator
 ===================================
 
 
 Finds declarations of assign operators with the wrong return and/or argument
 types and definitions with good return type but wrong ``return`` statements.
 
   * The return type must be ``Class&``.
-  * Works with move-assign and assign by value.
+  * The assignment may be from the class type by value, const lvalue
+    reference, non-const rvalue reference, or from a completely different
+    type (e.g. ``int``).
   * Private and deleted operators are ignored.
   * The operator must always return ``*this``.

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

AssignmentCallAtReturn &operator=(char rhs) {
     return (*this = static_cast<int>(rhs));
   }
   AssignmentCallAtReturn &operator=(float rhs) {
     // Do not allow calls to other functions.
     return returnThis();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this'
   }
 };
+
+// Check that no false positives are issued when using type aliases.
+struct TypeAlias {
+  using Alias = TypeAlias;
+  // This is correct and should not produce any warnings:
+  Alias &operator=(const Alias &) { return *this; }
+
+  using AliasRef = Alias &;
+  // So is this (assignments from other types are fine):
+  AliasRef operator=(int) { return *this; }
+};
+
+// Same check as above with typedef instead of using
+struct TypeAliasTypedef {
+  typedef TypeAliasTypedef Alias;
+  Alias &operator=(const Alias &) { return *this; }
+
+  typedef Alias &AliasRef;
+  AliasRef operator=(int) { return *this; }
+};
+
+// Same check as above for a template class
+template <typename T>
+struct TemplateTypeAlias {
+  using Alias1 = TemplateTypeAlias &;
+  using Alias2 = TemplateTypeAlias const &;
+  Alias1 operator=(Alias2) { return *this; }
+
+  template <typename U>
+  using Alias3 = TemplateTypeAlias<U>;
+  Alias3<T> &operator=(int) { return *this; }
+
+  // Using a different type parameter in the return type should give a warning
+  Alias3<TypeAlias::Alias> &operator=(double) { return *this; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'TemplateTypeAlias&' [misc-unconventional-assign-operator]
+};