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

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.

Reviewers: aaron.ballman, alexfh, sbenza

Subscribers: o.gyorgy, baloghadamsoftware, LegalizeAdulthood, aaron.ballman, Eugene.Zelenko, xazax.hun, cfe-commits

Differential Revision: http://reviews.llvm.org/D18265

llvm-svn: 268492

10 files changed, 91 insertions, 49 deletions

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
index 27fda49a4e5..97c352ba952 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -10,7 +10,7 @@
 #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"
@@ -53,7 +53,7 @@ public:
         "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");
   }
 };
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 3aee73ecb80..c7f3c14f624 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -3,7 +3,7 @@ set(LLVM_LINK_COMPONENTS support)
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
-  AssignOperatorSignatureCheck.cpp
+  UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 9aecdc1c447..cae3f0dc138 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -12,7 +12,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
-#include "AssignOperatorSignatureCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
@@ -61,8 +61,8 @@ public:
     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>(
diff --git a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
index 1112431192b..53cc0825b7a 100644
--- a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -1,4 +1,4 @@
-//===--- AssignOperatorSignatureCheck.cpp - clang-tidy ----------*- C++ -*-===//
+//===--- UnconventionalUnconventionalAssignOperatorCheck.cpp - clang-tidy -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,7 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "AssignOperatorSignatureCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
@@ -17,8 +17,7 @@ namespace clang {
 namespace tidy {
 namespace misc {
 
-void AssignOperatorSignatureCheck::registerMatchers(
-    ast_matchers::MatchFinder *Finder) {
+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)
@@ -56,22 +55,32 @@ void AssignOperatorSignatureCheck::registerMatchers(
   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();
+  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);
+}
 
-  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'"}};
+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'"}};
 
-  for (const auto &Message : Messages) {
-    if (Result.Nodes.getNodeAs<Decl>(Message[0]))
-      diag(Method->getLocStart(), Message[1])
-          << Name << (Method->isConst() ? "const" : "virtual");
+    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");
+    }
   }
 }
 
diff --git a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.h b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h
index a2530f9314c..5545de07729 100644
--- a/clang-tools-extra/clang-tidy/misc/AssignOperatorSignatureCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.h
@@ -1,4 +1,4 @@
-//===--- AssignOperatorSignatureCheck.h - clang-tidy ------------*- C++ -*-===//
+//===--- UnconventionalUnconventionalAssignOperatorCheck.h - clang-tidy -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -16,15 +16,20 @@ namespace clang {
 namespace tidy {
 namespace misc {
 
-/// Finds declarations of assign operators with the wrong return and/or argument
-/// types.
+/// 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.
-class AssignOperatorSignatureCheck : public ClangTidyCheck {
+///   * 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:
-  AssignOperatorSignatureCheck(StringRef Name, ClangTidyContext *Context)
+  UnconventionalAssignOperatorCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index e4a88b21de9..5a246115b8c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -236,6 +236,12 @@ identified.  The improvements since the 3.8 release include:
 
   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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index d5c680c653c..7459734afda 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -52,7 +52,7 @@ Clang-Tidy Checks
    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
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst
deleted file mode 100644
index dc34e1181a1..00000000000
--- a/clang-tools-extra/docs/clang-tidy/checks/misc-assign-operator-signature.rst
+++ /dev/null
@@ -1,12 +0,0 @@
-.. 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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst
new file mode 100644
index 00000000000..e12241b071b
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst
@@ -0,0 +1,13 @@
+.. 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``.
diff --git a/clang-tools-extra/test/clang-tidy/misc-assign-operator-signature.cpp b/clang-tools-extra/test/clang-tidy/misc-unconventional-assign-operator.cpp
index 8609fb91684..3041f593ec1 100644
--- a/clang-tools-extra/test/clang-tidy/misc-assign-operator-signature.cpp
+++ b/clang-tools-extra/test/clang-tidy/misc-unconventional-assign-operator.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s misc-assign-operator-signature %t
+// 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&);
@@ -13,18 +15,19 @@ struct AlsoGood {
   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&&);
+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 BadReturn2 {
-  BadReturn2&& operator=(const BadReturn2&);
+
+struct BadReturnType2 {
+  BadReturnType2&& operator=(const BadReturnType2&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
-  int operator=(BadReturn2&&);
+  int operator=(BadReturnType2&&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should return 'Bad
 };
 
@@ -56,3 +59,21 @@ 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;
+  }
+};