[PATCH] New diagnostic for non-idiomatic copy or move operations

[aaron.ballman]

Summary

All copy operations were created equal (as far as the core language is concerned), but some copy operations are more equal than others (as far as the library is concerned). The same is true of move operations. This patch diagnoses copy and move operations (constructors and assignment operators) that are non-idiomatic so that users are not surprised when their copyable type cannot be placed in a standard library container or used with a standard library algorithm.

Idiomatic copy and move operations are (where T is the class type for the object):
Copy Constructor: takes a const T&
Move constructor: takes a T&&
Copy Assignment: takes a const T&, returns a T&, and has no ref-qualifiers
Move Assignment: takes a T&&, returns a T&, and has no ref-qualifiers

One thing this patch does not have, but I would like to add, are fix-it hints to correct the types involved. However, I cannot find a way to get the full source range for the type, including qualifiers. For a parameter of type 'const volatile T&', the TypeLoc appears to only track the location of T&, but not the qualifiers. Assistance in understanding how to do this would be appreciated.

[aaron.ballman]

I should note for clarity, this is a WIP. There are still a lot of test cases that require updating due to the new diagnostic. I also wanted to get a feel for whether people felt this belongs under -Wall or not.

[aaron.ballman]

As I wade through a lot of diagnostics from our test suite, I notice a recurring pattern is that this is diagnosing in the presence of idiomatic constructors or assignment operators. e.g.,

struct S {
  S(const S&) = delete;
  S(S&); // We diagnose, and suggest const T& here
};

struct T {
  T(const T&) = default;
  T(T&); // We diagnose, and suggest const T& here
};

Would it make sense to delay this checking until the class definition is complete, and then diagnose only the operations where the suggestion in the diagnostic would not result in an ambiguous declaration? Or, perhaps suppress the diagnostic entirely in the presence of multiple overloads of the operation. e.g, not diagnose the following:

struct U {
  U(U&);
  U(volatile U&);

  U& operator=(U&);
  void operator=(const U&);
};

[aaron.ballman]

Abandoning this revision, will post a new patch approach shortly.

include/clang/Basic/DiagnosticGroups.td

 def GNUZeroLineDirective : DiagGroup<"gnu-zero-line-directive">;
 def GNUZeroVariadicMacroArguments : DiagGroup<"gnu-zero-variadic-macro-arguments">;
 def Fallback : DiagGroup<"fallback">;
+def NonIdiomaticCopyMove : DiagGroup<"non-idiomatic-copy-move">;
 
 // This covers both the deprecated case (in C++98)
 // and the extension case (in C++11 onwards).

include/clang/Basic/DiagnosticSemaKinds.td

   "%select{copy|move}0 assignment operator of %1 is implicitly deleted "
   "because field %2 is of %select{reference|const-qualified}4 type %3">;
 
+def warn_non_idiomatic_copy_move_ctor : Warning<
+  "non-idiomatic %select{copy|move}0 constructor declaration; consider "
+  "%select{'const T&'|'T&&'}0 instead">,
+  InGroup<NonIdiomaticCopyMove>;
+def warn_non_idiomatic_copy_move_assign : Warning<
+  "non-idiomatic %select{copy|move}0 assignment operator declaration; consider "
+  "%select{returning 'T&'|'%select{const T&|T&&}0'|removing ref-qualifiers}1 "
+  "instead">, InGroup<NonIdiomaticCopyMove>;
+
 // These should be errors.
 def warn_undefined_internal : Warning<
   "%select{function|variable}0 %q1 has internal linkage but is not defined">,

include/clang/Sema/Sema.h

   QualType CheckConstructorDeclarator(Declarator &D, QualType R,
                                       StorageClass& SC);
   void CheckConstructor(CXXConstructorDecl *Constructor);
+  void CheckAssignmentOperatorOverload(CXXMethodDecl *Assignment);
   QualType CheckDestructorDeclarator(Declarator &D, QualType R,
                                      StorageClass& SC);
   bool CheckDestructor(CXXDestructorDecl *Destructor);

lib/Sema/SemaDeclCXX.cpp

 }
 
 /// CheckConstructor - Checks a fully-formed constructor for
-/// well-formedness, issuing any diagnostics required. Returns true if
-/// the constructor declarator is invalid.
+/// well-formedness, issuing any diagnostics required.
 void Sema::CheckConstructor(CXXConstructorDecl *Constructor) {
   CXXRecordDecl *ClassDecl
     = dyn_cast<CXXRecordDecl>(Constructor->getDeclContext());

         Constructor->getParamDecl(1)->hasDefaultArg())) &&
       Constructor->getTemplateSpecializationKind()
                                               != TSK_ImplicitInstantiation) {
-    QualType ParamType = Constructor->getParamDecl(0)->getType();
     QualType ClassTy = Context.getTagDeclType(ClassDecl);
-    if (Context.getCanonicalType(ParamType).getUnqualifiedType() == ClassTy) {
+    unsigned Quals;
+    if (Context.getCanonicalType(Constructor->getParamDecl(0)->getType())
+            .getUnqualifiedType() == ClassTy) {
       SourceLocation ParamLoc = Constructor->getParamDecl(0)->getLocation();
       const char *ConstRef 
         = Constructor->getParamDecl(0)->getIdentifier() ? "const &" 

       // FIXME: Rather that making the constructor invalid, we should endeavor
       // to fix the type.
       Constructor->setInvalidDecl();
+    } else if (!Constructor->isDeletedAsWritten() &&
+               Constructor->isCopyConstructor(Quals) &&
+               ((Quals & Qualifiers::Volatile) ||
+                !(Quals & Qualifiers::Const))) {
+      // Warn about copy constructors of the form T&, volatile T&, or
+      // const volatile T&, because they are not idiomatic copy constructors.
+      // Do not warn about copy constructors that are deleted; presumably, the
+      // user knows what they are doing with those.
+      Diag(Constructor->getParamDecl(0)->getLocation(),
+           diag::warn_non_idiomatic_copy_move_ctor)
+          << /*copy*/ 0;
+
+      // TODO: FixIt hint to correct the type
+    } else if (!Constructor->isDeletedAsWritten() &&
+               Constructor->isMoveConstructor(Quals) && Quals) {
+      // Warn about move constructors of the form const T&&, volatile T&&, or
+      // const volatile T&&, because they are not idiomatic move constructors.
+      // Do not warn about move constructors that are deleted; presumably, the
+      // user knows what they are doing with those.
+      Diag(Constructor->getParamDecl(0)->getLocation(),
+           diag::warn_non_idiomatic_copy_move_ctor) << /*move*/1;
+
+      // TODO: FixIt hint to correct the type
     }
   }
 }
 
+/// \brief Checks a fully-formed assignment operator definition for
+/// well-formedness, including any diagnostics required.
+void Sema::CheckAssignmentOperatorOverload(CXXMethodDecl *Assignment) {
+  // Diagnose any non-idiomatic copy assignment operators. We determine that it
+  // is meant to be a copy assignment operator by looking at the parameter.
+  // Because we already know this is an assignment operator, we can look at the
+  // only parameter; if it is an rvalue reference, it is a move assignment.
+  // Anything else will be considered an attempt at a copy assignment operator,
+  // since those are more commonly used.
+  if (Assignment->isInvalidDecl())
+    return;
+
+  assert(Assignment->getNumParams() == 1 && "Invalid assignment operator");
+
+  CXXRecordDecl *ClassDecl =
+      dyn_cast<CXXRecordDecl>(Assignment->getDeclContext());
+  if (!ClassDecl)
+    return Assignment->setInvalidDecl();
+
+  if (!Assignment->isDeletedAsWritten()) {
+    QualType ParamType = Assignment->getParamDecl(0)->getType();
+    QualType ReturnType = Assignment->getReturnType();
+    QualType ClassTy = Context.getTagDeclType(ClassDecl);
+    QualType ParamNonRefTy = ParamType.getNonReferenceType();
+
+    if (Assignment->getRefQualifier() != RQ_None) {
+      // Diagnose a copy or move constructor if it has any ref qualifiers.
+
+      // FIXME: this location isn't ideal, but the ref qualifier location
+      // information is not available.
+      Diag(Assignment->getLocation(), diag::warn_non_idiomatic_copy_move_assign)
+          << ParamType->isRValueReferenceType() << /*ref-qualifier*/ 2;
+
+      // FIXME: FixIt hint to remove the ref qualifier. This requires knowing
+      // where the ref qualifier is written, however.
+    } else if (!ReturnType->isLValueReferenceType() ||
+               ReturnType.getNonReferenceType().hasQualifiers() ||
+               Context.getCanonicalType(ReturnType.getNonReferenceType())
+                       .getUnqualifiedType() != ClassTy) {
+      // Diagnose a copy or move constructor if its return type is anything but
+      // T&.
+      Diag(Assignment->getReturnTypeSourceRange().getBegin(),
+           diag::warn_non_idiomatic_copy_move_assign)
+          << ParamType->isRValueReferenceType() << /*return*/ 0;
+
+      // TODO: FixIt hint to correct the return type.
+    } else if (Assignment->isMoveAssignmentOperator()) {
+      // Diagnose a move assignment operator that has qualifiers on its
+      // parameter. An rvalue reference type without qualifiers is idiomatic.
+      if (ParamNonRefTy.hasQualifiers())
+        Diag(Assignment->getParamDecl(0)->getLocation(),
+             diag::warn_non_idiomatic_copy_move_assign)
+            << /*move*/ 1 << /*param*/ 1;
+
+      // TODO: FixIt hint to correct the parameter type to T&&.
+    } else if (Assignment->isCopyAssignmentOperator()) {
+      // Diagnose a copy assignment operator that has a parameter type that is
+      // not an lvalue reference to an only-const object.
+      if (!ParamType->isLValueReferenceType() ||
+          (ParamType->isLValueReferenceType() &&
+           (ParamNonRefTy.isVolatileQualified() ||
+            !ParamNonRefTy.isConstQualified()))) {
+        Diag(Assignment->getParamDecl(0)->getLocation(),
+             diag::warn_non_idiomatic_copy_move_assign)
+            << /*copy*/ 0 << /*param*/ 1;
+
+        // TODO: FixIt hint to correct the parameter type to const T&.
+      }
+    }
+  }
+}
+
 /// CheckDestructor - Checks a fully-formed destructor definition for
 /// well-formedness, issuing any diagnostics required.  Returns true
 /// on error.

     if (MethodDecl->isStatic())
       return Diag(FnDecl->getLocation(),
                   diag::err_operator_overload_static) << FnDecl->getDeclName();
+
+    if (Op == OO_Equal)
+      CheckAssignmentOperatorOverload(MethodDecl);
   } else {
     bool ClassOrEnumParam = false;
     for (auto Param : FnDecl->params()) {

test/SemaCXX/warn-non-idiomatic-copy-move.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++14 -verify %s
+
+struct A {
+  // Copy constructor
+  A(A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}}
+  A(volatile A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}}
+  A(const volatile A&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}}
+  A(const A&); // ok
+
+  // Move constructor
+  A(const A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}}
+  A(volatile A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}}
+  A(const volatile A&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}}
+  A(A&&); // ok
+
+  // Copy assignment
+  A& operator=(A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+  A& operator=(volatile A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+  A& operator=(const volatile A&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+  A& operator=(A); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+  A& operator=(const A&); // ok
+
+  // Move assignment
+  A& operator=(const A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}}
+  A& operator=(volatile A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}}
+  A& operator=(const volatile A&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider 'T&&' instead}}
+  A& operator=(A&&); // ok
+};
+
+struct B {
+  B&& operator=(const B&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}}
+  B&& operator=(B&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}}
+};
+
+struct C {
+  C operator=(const C&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}}
+  C operator=(C&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}}
+};
+
+struct D {
+  C& operator=(const D&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}}
+  C& operator=(D&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}}
+};
+
+struct E {
+  E& operator=(const E&) &; // expected-warning {{non-idiomatic copy assignment operator declaration; consider removing ref-qualifiers instead}}
+  E& operator=(E&&) &; // expected-warning {{non-idiomatic move assignment operator declaration; consider removing ref-qualifiers instead}}
+};
+
+struct F {
+  F(F&) = delete; // okay
+  F& operator=(F&) = default; // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+  F& operator=(const F&&) = delete; // okay
+};
+
+struct G {
+  G(G&) = default; // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}}
+  G(const G&, int = 0); // okay, but should be different diagnostic
+
+private:
+  G& operator=(G&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider 'const T&' instead}}
+};
+
+struct H {
+private:
+  H(H&); // expected-warning {{non-idiomatic copy constructor declaration; consider 'const T&' instead}}
+  H(const H&); // ok
+  H(const H&&); // expected-warning {{non-idiomatic move constructor declaration; consider 'T&&' instead}}
+
+  void operator=(const H&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}}
+  void operator=(H&); // expected-warning {{non-idiomatic copy assignment operator declaration; consider returning 'T&' instead}}
+  void operator=(H&&); // expected-warning {{non-idiomatic move assignment operator declaration; consider returning 'T&' instead}}
+};
+
+struct I {
+  I(int); // ok
+  I(int&); // ok
+  I(int&&); // ok
+  I(H&); // ok
+
+  I& operator=(int); // ok
+  I& operator=(int&); // ok
+  I& operator=(int&&); // ok
+  I& operator=(H&); // ok
+  I& operator=(H&&); // ok
+};