[clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature

[baloghadamsoftware]

Summary

Finds return statements in assign operator bodies where the return value is different from '*this'. Only assignment operators with correct return value Class& are checked.

[Eugene.Zelenko]

I think will be reasonable to merge misc-assign-operator-return and misc-assign-operator-signature into one check.

[baloghadamsoftware]

My first thought was also to extend existing checker misc-assign-operator-signature and rename it to just misc-assign-operator. However, there is little benefit doing this: the two checkers check different locations, one checks the signature while the other one of the return statements. Signature is always one, return statements can be multiple per function. So the checker function itself would consist of one branching statement and report different errors for different locations in the two branches. Only some matcher expressions can be common. Of course, if such a merge is beneficial from the user's perspective, then we should do it. Thoughts, opinions?

[Eugene.Zelenko]

As user I could tell that it's much easier to have one check which will check all nuances of particular language construction then multiple checks.

[baloghadamsoftware]

Merged into misc-assign-operator-signature and thus renamed to misc-assign-operator

[aaron.ballman]

clang-tidy/misc/AssignOperatorCheck.cpp

74 ↗

(On Diff #51554)

This can be lowered into the if statement in place of Name.

clang-tidy/misc/AssignOperatorCheck.h

19 ↗

(On Diff #51554)

s/assign/assignment.

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

15 ↗

(On Diff #51554)

This seems at odds with the core guidelines for cppcoreguidelines-c-copy-assignment-signature.

[baloghadamsoftware]

Thank you for your comments, but they are not related to my changes. These lines were present in the original file and I did not change them.

[aaron.ballman]

In D18265#385300, @baloghadamsoftware wrote:

Thank you for your comments, but they are not related to my changes. These lines were present in the original file and I did not change them.

Ah, good to know (hard to tell context in Phab sometimes). Can you fix up the trivial changes, and leave the C++ core guideline behavior for now?

[baloghadamsoftware]

Requested fixes done (not related to the changes).

[aaron.ballman]

Aside from the copy-pasta, looks good.

clang-tidy/misc/AssignOperatorCheck.cpp

74 ↗

(On Diff #51906)

This doesn't look like it will compile. ;-) You can fix as part of the commit (does not require additional review).

[baloghadamsoftware]

Faulty line deleted.

[baloghadamsoftware]

clang-tidy/misc/AssignOperatorCheck.cpp

75 ↗

(On Diff #52023)

Oh, sorry, I mixed up things yesterday: started compilation, went to a meeting and then forgot that I did not check compilation results.

[alexfh]

Adding the original author in case he has something to say here.

clang-tidy/misc/AssignOperatorCheck.h

24 ↗

(On Diff #52023)

Please add:

/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/misc-assign-operator.html

clang-tidy/misc/MiscTidyModule.cpp

61

Check names usually contain hints at the class of problems they detect (e.g. "misc-inaccurate-erase" or "misc-macro-repeated-side-effects"), while misc-assign-operator doesn't serve this purpose and is overall a bit too vague (there's nothing wrong with assign operators per se).

I wonder whether a more specific name would be more helpful to the users. I'm thinking of something like misc-assign-operator-conventions. WDYT?

docs/clang-tidy/checks/misc-assign-operator.rst

13 ↗

(On Diff #52023)

nit: Please add a trailing period.

[baloghadamsoftware]

clang-tidy/misc/MiscTidyModule.cpp

61

I have no better idea either. Should I rename it or let us wait for other potential reviewers proposing another name?

[xazax.hun]

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

[baloghadamsoftware]

Unchainable is not enough: the original checker (which was extended) also checks for parameters and other qualifiers such as const or virtual.

[alexfh]

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

[aaron.ballman]

In D18265#386676, @alexfh wrote:

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

[xazax.hun]

In D18265#386717, @aaron.ballman wrote:

In D18265#386676, @alexfh wrote:

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

IMHO T &operator=(T) can be idiomatic e.g. when one uses copy and swap idiom or T &operator=(S) where S is a type that can be copied efficiently.

[alexfh]

In D18265#386717, @aaron.ballman wrote:

In D18265#386676, @alexfh wrote:

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

Maybe you like misc-assign-operator-conventions more? ;)

[baloghadamsoftware]

Actually, there was nothing wrong with assign operator signatures per se either although the original name of the checker was AssignOperatorSignature. The only change here is that it does not check the signature only anymore, but also the body (if present).

[alexfh]

In D18265#386720, @baloghadamsoftware wrote:

Actually, there was nothing wrong with assign operator signatures per se either although the original name of the checker was AssignOperatorSignature. The only change here is that it does not check the signature only anymore, but also the body (if present).

Maybe the old check name should have been misc-unconventional-assign-operator-signature or something like that, but even the old name limited the scope enough to make it easy to guess about the possible issues it flags. However, further expanding the scope makes the name even less informative. There are just too many things that could be wrong with assignment operators. If/when we add a check for another bug-prone pattern related to assignment operators, the name misc-assign-operator could be broad enough to cover this hypothetical new check as well. This will inevitably lead to confusion.

[baloghadamsoftware]

misc-unconventional-assign-operator?

[LegalizeAdulthood]

Please summarize this check in docs/ReleaseNotes.rst.

There is a pending review that will stub this out automatically, but it hasn't been approved yet....

[baloghadamsoftware]

In D18265#387078, @LegalizeAdulthood wrote:

Please summarize this check in docs/ReleaseNotes.rst.

OK, I will do it after the name is decided.

[baloghadamsoftware]

This comment was removed by baloghadamsoftware.

[sbenza]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

I dislike these uses of hasAnscestor. They are kind of slow.
But more importantly, they break with nested functions/types.
This particular example is not checking that the return statement is from the assignment operator, only that it is within it. For example, it would match a lambda.
I think this would trip the check:

F& operator=(const F& o) {
  std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; });
  return *this;
}

69 ↗

(On Diff #52023)

Move this closer to where it is used.

70 ↗

(On Diff #52023)

Move this into the if () statement.

clang-tidy/misc/AssignOperatorCheck.h

19 ↗

(On Diff #52023)

This does not talk about the return statement, only the return type.

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

16 ↗

(On Diff #52023)

This is a very common C++98 way of implementing copy-and-swap with copy elision support.
You do: T& operator=(T t) { swap(t); return *this; }
And it will avoid the copy if the argument is already a temporary due to copy elision on the caller.

69 ↗

(On Diff #52023)

This case is not a bad return statement, so it should not be in this class.

[aaron.ballman]

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

16 ↗

(On Diff #52023)

I wasn't arguing that it wasn't useful, but this check is also registered as cppcoreguidelines-c-copy-assignment-signature, and so we need to make sure that we aren't breaking that check. Basically, this can be resolved by looking at the spelling of the check and deciding whether to diagnose this particular case or not and add appropriate tests. (We do this for a few checks shared with CERT rules as well.)

[baloghadamsoftware]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

I can change it to hasDescendant if it is faster, but it does not solve the lambda problem. No solution for that comes to my mind with the existing matchers. Maybe a new matcher hasDescendantStatement could help which only traverses statements down the AST. Is this the right way to go?

[baloghadamsoftware]

And what about the final name?

[sbenza]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

hasDescendant has the same problem has hasAnscestor.
I think the best is to write a matcher like:

AST_MATCHER_P(ReturnStmt, forFunction, internal::Matcher<FunctionDecl>, InnerMatcher) {
  ...
}

In there we can find the right FunctionDecl that encloses the return statement and apply the matcher.
This matcher seems like a good candidate to add to ASTMatchers.h

[baloghadamsoftware]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

Maybe I am wrong, but your proposal also seems a bottom-up matcher which is slow. That is the reason I proposed a top-down matcher, e.g. hasDescendantStatement or something like this which is top-down and only traverses statements so it does not search in a lambda which is a declaration.

[sbenza]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

hasAnscestor is slow because it is way too general. There are tons of virtual function calls, cache lookups, memory allocations, etc. And the search will not stop until it find a match or runs out of anscestors. This means it will go all the way to the translationUnitDecl before it figures out that there are no matches.
Every parent will be run through the matcher.

What I propose is a simple matcher that:

1. Finds the enclosing FunctionDecl for a statement.
2. Runs the matcher on it, if there is an enclosing function.

(1) can be done without any virtual function call and with no/minimal memory allocations.
(2) will be done only once on the found node.

[baloghadamsoftware]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

I did something like your proposal and it is working, but before uploading a patch I have a question. There are some nodes, even statement having multiple parents, probably due to template instantiations. Is it enough if we chose the first parent here (thus going up only to the template itself) or should we do a depth-first (better to say height-search here, since we are not searching downwards in a tree but upwards in a DAG) search here and go up to every parent? hasAncestor uses breadth-first search which is out of the question here.

[baloghadamsoftware]

misc-unconventional-assign-operator, misc-assign-operator-conventions, misc-non-idiomatic-assign-operator or something else then?

[sbenza]

clang-tidy/misc/AssignOperatorCheck.cpp

63 ↗

(On Diff #52023)

Yes, with templates you can have multiple parents.
I think you can use any of the parents, but it would be unspecified which FunctionDecl you get.
Maybe step (1) can return a list of FunctionDecls instead.
In this particular case you might need the particular template instantiation because the return type might be a dependent type.

[baloghadamsoftware]

Issues fixed and checker renamed.

[alexfh]

There still seem to be a few comments from sbenza and myself that need to be resolved.

[alexfh]

Wait, I probably was looking at an older diff somehow. After returning to the page I don't see any unresolved comments. Reviewing...

[alexfh]

No objections from me, but please wait for Samuel's review.

[sbenza]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

69

couldn't this be folded into the Messages array below?

[baloghadamsoftware]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

69

There would be no benefit of it since we use the location of RetStmt in this message instead of Method.

[sbenza]

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

69

Right. So it can't really be merged. That's fine.

[baloghadamsoftware]

I see this one is accepted, but the prerequisite is not reviewed yet (after the update). Without that this one should not be merged into the code because it will not compile.

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "../misc/AssignOperatorSignatureCheck.h"
+#include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
-    CheckFactories.registerCheck<misc::AssignOperatorSignatureCheck>(
+    CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/misc/AssignOperatorSignatureCheck.h

-//===--- AssignOperatorSignatureCheck.h - clang-tidy ------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H
-
-#include "../ClangTidy.h"
-
-namespace clang {
-namespace tidy {
-namespace misc {
-
-/// Finds declarations of assign operators with the wrong return and/or argument
-/// types.
-///
-///   * The return type must be `Class&`.
-///   * Works with move-assign and assign by value.
-///   * Private and deleted operators are ignored.
-class AssignOperatorSignatureCheck : public ClangTidyCheck {
-public:
-  AssignOperatorSignatureCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
-  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
-  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-};
-
-} // namespace misc
-} // namespace tidy
-} // namespace clang
-
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H

clang-tidy/misc/AssignOperatorSignatureCheck.cpp

-//===--- AssignOperatorSignatureCheck.cpp - clang-tidy ----------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "AssignOperatorSignatureCheck.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
-
-using namespace clang::ast_matchers;
-
-namespace clang {
-namespace tidy {
-namespace misc {
-
-void AssignOperatorSignatureCheck::registerMatchers(
-    ast_matchers::MatchFinder *Finder) {
-  // Only register the matchers for C++; the functionality currently does not
-  // provide any benefit to other languages, despite being benign.
-  if (!getLangOpts().CPlusPlus)
-    return;
-
-  const auto HasGoodReturnType = cxxMethodDecl(returns(
-      lValueReferenceType(pointee(unless(isConstQualified()),
-                                  hasDeclaration(equalsBoundNode("class"))))));
-
-  const auto IsSelf = qualType(
-      anyOf(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(
-      anyOf(lValueReferenceType(pointee(unless(isConstQualified()))),
-            rValueReferenceType(pointee(isConstQualified()))));
-
-  Finder->addMatcher(
-      cxxMethodDecl(IsSelfAssign,
-                    hasParameter(0, parmVarDecl(hasType(BadSelf))))
-          .bind("ArgumentType"),
-      this);
-
-  Finder->addMatcher(
-      cxxMethodDecl(IsSelfAssign, anyOf(isConst(), isVirtual())).bind("cv"),
-      this);
-}
-
-void AssignOperatorSignatureCheck::check(
-    const MatchFinder::MatchResult &Result) {
-  const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
-  std::string Name = Method->getParent()->getName();
-
-  static const char *const Messages[][2] = {
-      {"ReturnType", "operator=() should return '%0&'"},
-      {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
-      {"cv", "operator=() should not be marked '%1'"}};
-
-  for (const auto &Message : Messages) {
-    if (Result.Nodes.getNodeAs<Decl>(Message[0]))
-      diag(Method->getLocStart(), Message[1])
-          << Name << (Method->isConst() ? "const" : "virtual");
-  }
-}
-
-} // namespace misc
-} // namespace tidy
-} // namespace clang

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
-  AssignOperatorSignatureCheck.cpp
+  UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
-#include "AssignOperatorSignatureCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"

 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
-    CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
-        "misc-assign-operator-signature");
+    CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
+        "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "misc-forward-declaration-namespace");

clang-tidy/misc/UnconventionalAssignOperatorCheck.h

+//===--- UnconventionalUnconventionalAssignOperatorCheck.h - clang-tidy -------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds declarations of assignment 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.
+///   * Private and deleted operators are ignored.
+///   * The operator must always return ``*this``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-unconventional-assign-operator.html
+class UnconventionalAssignOperatorCheck : public ClangTidyCheck {
+public:
+  UnconventionalAssignOperatorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

+//===--- UnconventionalUnconventionalAssignOperatorCheck.cpp - clang-tidy -----*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnconventionalAssignOperatorCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void UnconventionalAssignOperatorCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  // Only register the matchers for C++; the functionality currently does not
+  // provide any benefit to other languages, despite being benign.
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  const auto HasGoodReturnType = cxxMethodDecl(returns(
+      lValueReferenceType(pointee(unless(isConstQualified()),
+                                  hasDeclaration(equalsBoundNode("class"))))));
+
+  const auto IsSelf = qualType(
+      anyOf(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(
+      anyOf(lValueReferenceType(pointee(unless(isConstQualified()))),
+            rValueReferenceType(pointee(isConstQualified()))));
+
+  Finder->addMatcher(
+      cxxMethodDecl(IsSelfAssign,
+                    hasParameter(0, parmVarDecl(hasType(BadSelf))))
+          .bind("ArgumentType"),
+      this);
+
+  Finder->addMatcher(
+      cxxMethodDecl(IsSelfAssign, anyOf(isConst(), isVirtual())).bind("cv"),
+      this);
+
+  const auto IsBadReturnStatement = returnStmt(unless(has(
+      unaryOperator(hasOperatorName("*"), hasUnaryOperand(cxxThisExpr())))));
+  const auto IsGoodAssign = cxxMethodDecl(IsAssign, HasGoodReturnType);
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, forFunction(IsGoodAssign))
+                         .bind("returnStmt"),
+                     this);
+}
+
+void UnconventionalAssignOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *RetStmt = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt")) {
+    diag(RetStmt->getLocStart(), "operator=() should always return '*this'");
+  } else {
+    static const char *const Messages[][2] = {
+        {"ReturnType", "operator=() should return '%0&'"},
+        {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+        {"cv", "operator=() should not be marked '%1'"}};
+
+    const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
+    for (const auto &Message : Messages) {
+      if (Result.Nodes.getNodeAs<Decl>(Message[0]))
+        diag(Method->getLocStart(), Message[1])
+            << Method->getParent()->getName()
+            << (Method->isConst() ? "const" : "virtual");
+    }
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `readability-redundant-string-init
 
   Finds unnecessary string initializations.
 
 - New `readability-static-definition-in-anonymous-namespace
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html>`_ check
 
   Finds static function and variable definitions in anonymous namespace.
 
+- New `misc-unconventional-assign-operator
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-unconventional-assign-operator.html>`_ check replacing old `misc-assign-operator-signature` check
+
+  Does not only checks for correct signature but also for correct ``return``
+  statements (returning ``*this``)
+
 Fixed bugs:
 
 - Crash when running on compile database with relative source files paths.
 
 - Crash when running with the `-fdelayed-template-parsing` flag.
 
 - The `modernize-use-override` check: incorrect fix-its placement around
   ``__declspec`` and other attributes.

docs/clang-tidy/checks/list.rst

.. toctree::
    google-runtime-operator
    google-runtime-references
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
-   misc-assign-operator-signature
+   misc-unconventional-assign-operator
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
    misc-definitions-in-headers
    misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses

docs/clang-tidy/checks/misc-assign-operator-signature.rst

-.. title:: clang-tidy - misc-assign-operator-signature
-
-misc-assign-operator-signature
-==============================
-
-
-Finds declarations of assign operators with the wrong return and/or argument
-types.
-
-  * The return type must be ``Class&``.
-  * Works with move-assign and assign by value.
-  * Private and deleted operators are ignored.

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.
+  * Private and deleted operators are ignored.
+  * The operator must always return ``*this``.

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

-// RUN: %check_clang_tidy %s misc-assign-operator-signature %t
-
-struct Good {
-  Good& operator=(const Good&);
-  Good& operator=(Good&&);
-
-  // Assign from other types is fine too.
-  Good& operator=(int);
-};
-
-struct AlsoGood {
-  // By value is also fine.
-  AlsoGood& operator=(AlsoGood);
-};
-
-struct BadReturn {
-  void operator=(const BadReturn&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'BadReturn&' [misc-assign-operator-signature]
-  const BadReturn& operator=(BadReturn&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
-  void operator=(int);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
-};
-struct BadReturn2 {
-  BadReturn2&& operator=(const BadReturn2&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
-  int operator=(BadReturn2&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
-};
-
-struct BadArgument {
-  BadArgument& operator=(BadArgument&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
-  BadArgument& operator=(const BadArgument&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
-};
-
-struct BadModifier {
-  BadModifier& operator=(const BadModifier&) const;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'const'
-};
-
-struct Deleted {
-  // We don't check the return value of deleted operators.
-  void operator=(const Deleted&) = delete;
-  void operator=(Deleted&&) = delete;
-};
-
-class Private {
-  // We don't check the return value of private operators.
-  // Pre-C++11 way of disabling assignment.
-  void operator=(const Private &);
-};
-
-struct Virtual {
-  virtual Virtual& operator=(const Virtual &);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'virtual'
-};

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

+// RUN: %check_clang_tidy %s misc-unconventional-assign-operator %t -- -- -std=c++11 -isystem %S/Inputs/Headers
+
+#include <utility>
+
+struct Good {
+  Good& operator=(const Good&);
+  Good& operator=(Good&&);
+
+  // Assign from other types is fine too.
+  Good& operator=(int);
+};
+
+struct AlsoGood {
+  // By value is also fine.
+  AlsoGood& operator=(AlsoGood);
+};
+
+struct BadReturnType {
+  void operator=(const BadReturnType&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'BadReturnType&' [misc-unconventional-assign-operator]
+  const BadReturnType& operator=(BadReturnType&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+  void operator=(int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+};
+
+struct BadReturnType2 {
+  BadReturnType2&& operator=(const BadReturnType2&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+  int operator=(BadReturnType2&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
+};
+
+struct BadArgument {
+  BadArgument& operator=(BadArgument&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
+  BadArgument& operator=(const BadArgument&&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
+};
+
+struct BadModifier {
+  BadModifier& operator=(const BadModifier&) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'const'
+};
+
+struct Deleted {
+  // We don't check the return value of deleted operators.
+  void operator=(const Deleted&) = delete;
+  void operator=(Deleted&&) = delete;
+};
+
+class Private {
+  // We don't check the return value of private operators.
+  // Pre-C++11 way of disabling assignment.
+  void operator=(const Private &);
+};
+
+struct Virtual {
+  virtual Virtual& operator=(const Virtual &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should not be marked 'virtual'
+};
+
+class BadReturnStatement {
+  int n;
+
+public:
+  BadReturnStatement& operator=(BadReturnStatement&& rhs) {
+    n = std::move(rhs.n);
+    return rhs;
+// 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;
+  }
+};