[clang-tidy] misc-assign-operator-signature checker checks return value of all assign operators

[baloghadamsoftware]

Summary

The return value of every assign operator should be Type&, not only for copy and move assign operators. This check and its test was implemented.

[hokein]

LGTM.

[aaron.ballman]

I don't think this is a safe assumption to make -- for instance, DSLs tend to have operator overload return types that aren't necessarily the same as the class type. Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

How many additional diagnostics does this modification produce over some large code bases (particularly ones with DSLs if you can narrow the test field a bit)?

[xazax.hun]

In D18264#379092, @aaron.ballman wrote:

Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

This checker will not warn on private methods.

About EDSLs, I think they are not the common case. In case it is important, it might make sense to make this configurable, but I think the minority of the C++ projects have EDSLs so it make more sense to warn on those cases by default. What do you think?

[aaron.ballman]

In D18264#379159, @xazax.hun wrote:

In D18264#379092, @aaron.ballman wrote:

Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

This checker will not warn on private methods.

Good deal.

About EDSLs, I think they are not the common case. In case it is important, it might make sense to make this configurable, but I think the minority of the C++ projects have EDSLs so it make more sense to warn on those cases by default. What do you think?

That's why I am wondering about how much this increases the false positive rate. I don't think DSLs are going to be a huge problem, but if it turns out they are, then a configuration option makes sense. If we don't see a large increase, then no need to waste the effort on an option.

Also, this appears to be changing the behavior of the cppcoreguidelines-c-copy-assignment-signature alias. If the core guidelines don't prohibit this behavior under that rule, we should not trigger the diagnostic if the user enabled the check via cppcoreguidelines-c-copy-assignment-signature instead of misc-assign-operator-signature. Check out MoveConstructorInitCheck as an example of how we have handled this before.

[etienneb]

clang-tidy/misc/AssignOperatorSignatureCheck.cpp

48

nit: could you +2 the line to be consistent with the rest of the code

[baloghadamsoftware]

Check for non copy and move assign operators made optional.

[xazax.hun]

According to cpp core guidelines any assignment operator should return *this.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-assignment-op

So in case this check is activated by using an alias it should not be a special case.

[baloghadamsoftware]

Oh, I was searching in the C++ Core Guidlines, but at the wrong place because I did not find it. So I will change this option to be enabled by default. DSL users who do not follow this rule for the non copy and non move assign operators can disable it.

[alexfh]

In D18264#381846, @baloghadamsoftware wrote:

Oh, I was searching in the C++ Core Guidlines, but at the wrong place because I did not find it. So I will change this option to be enabled by default. DSL users who do not follow this rule for the non copy and non move assign operators can disable it.

I'd go without the option until we find a use case for it in real code.

clang-tidy/misc/AssignOperatorSignatureCheck.cpp

48

Just do whatever clang-format -style=llvm does.

[baloghadamsoftware]

Reverted to the original (accepted) version.

[baloghadamsoftware]

Code reformatted using: clang-format -style="LLVM"

[alexfh]

LG. Tell me if you need someone to commit the patch for you.

clang-tidy/misc/AssignOperatorSignatureCheck.h

 /// 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) {}
+  AssignOperatorSignatureCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool CheckAllAssignOperators;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ASSIGNOPERATORSIGNATURECHECK_H

clang-tidy/misc/AssignOperatorSignatureCheck.cpp

 #include "clang/ASTMatchers/ASTMatchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
+AssignOperatorSignatureCheck::AssignOperatorSignatureCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckAllAssignOperators(Options.get("CheckAllAssignOperators", false)) {}
+
+void AssignOperatorSignatureCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckAllAssignOperators", CheckAllAssignOperators);
+}
+
 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 IsSelfAssign =
+  const auto IsAssign =
       cxxMethodDecl(unless(anyOf(isDeleted(), isPrivate(), isImplicit())),
-                    hasName("operator="), ofClass(recordDecl().bind("class")),
-                    hasParameter(0, parmVarDecl(hasType(IsSelf))))
+                    hasName("operator="), ofClass(recordDecl().bind("class")))
+          .bind("method");
+  const auto IsSelfAssign =
+      cxxMethodDecl(IsAssign, hasParameter(0, parmVarDecl(hasType(IsSelf))))
           .bind("method");
 
   Finder->addMatcher(
-      cxxMethodDecl(IsSelfAssign, unless(HasGoodReturnType)).bind("ReturnType"),
+      cxxMethodDecl(CheckAllAssignOperators ? IsAssign : IsSelfAssign,
+                    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");
+  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'"}
-  };
+      {"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

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

-// RUN: %check_clang_tidy %s misc-assign-operator-signature %t
+// RUN: %check_clang_tidy %s misc-assign-operator-signature %t -- -config="{CheckOptions: [{key: misc-assign-operator-signature.CheckAllAssignOperators, value: 1}]}" --
 
 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
 };