[Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

[xbolva00]

Summary

Clang version of https://www.viva64.com/en/w/v690/
Also requested by Chromium developers:
https://bugs.chromium.org/p/chromium/issues/detail?id=979077&q=component%3ATools%3ELLVM&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified

[• Quuxplusone]

Please add test cases showing the intended behavior for

• when the copy constructor is explicitly defaulted but the copy assignment operator is {implicitly defaulted, implicitly deleted, user-provided}
• when the copy assignment operator is explicitly defaulted but the copy constructor is {implicitly defaulted, implicitly deleted, user-provided}
• when the {copy constructor, copy assignment operator} is user-provided but the other is implicitly deleted

An example of an implicitly deleted copy assignment operator would be

struct A {
    int& x;
    A(const A& rhs) : x(rhs.x) {}
};

Also, how does the presence of a user-provided destructor interact with this diagnostic? If I provide a destructor but implicitly default my copy operations, isn't that just as bad, Rule-of-Three-wise?

[xbolva00]

if I provide a destructor but implicitly default my copy operations, isn't that just as bad, Rule-of-Three-wise?

Yeah, I am wondering about this too, whether we should check the whole Rule-of-Three. Suggestions from other reviewers?

[thakis]

Since https://en.cppreference.com/w/cpp/language/rule_of_three says "almost always" (note "almost") I'd say this probably belongs more in clang-tidy territory – I'm guessing false positive rate is very high for this. Can you collect some true / false positive statistics on some large open-source project (chromium, firefox, open office, …)?

Since C++11 it's rule of 0/3/5, so this should probably handle the move ctor/assign op too.

[xbolva00]

Yeah, if we want to check rule of 3, clang-tidy is better choice.

[dblaikie]

+1 for clang-tidy. GCC's closest to this is -Weffc++ which limits the rule of 3 warning to only "classes that have dynamic memory allocation" (though I'm not sure exactly what conditions it uses to decide that - it doesn't warn on anything in the test cases provided in this patch, it does warn if you add a dtor to Clz that deletes buf:
rule3.cpp:1:7: warning: ‘class Clz’ has pointer data members [-Weffc++]
class Clz // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}

^~~

rule3.cpp:1:7: warning: but does not override ‘Clz(const Clz&)’ [-Weffc++]

[xbolva00]

Should we reconsider this as a clang warning? I found that the newest GCC can diagnose it.

GCC 9+:

-Wdeprecated-copy, implied by -Wextra, warns about the C++11 deprecation of implicitly declared copy constructor and assignment operator if one of them is user-provided. -Wdeprecated-copy-dtor also warns if the destructor is user-provided, as specified in C++11.

It woule be nice to keep compatibility. -Wextra seems reasonable for me too.

[dblaikie]

Yeah, I'm OK with warning on the deprecated cases (especially for symmetry with GCC's warning) - not sure why that didn't occur to me/why I didn't mention it in my previous comment...

Deprecation wording is, in C++17, 15.8.1 [class.copy.ctor] p6, and 15.8.2 [class.copy.assign] p2.

[xbolva00]

Ah, Clang has it under -Wdeprecated. https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated.

But combo -Wall -Wextra does not enable this warning, sadly.

This should be extracted to -Wdeprecated-copy and put into -Wextra. Seems fine for you?

[dblaikie]

In D68185#1742809, @xbolva00 wrote:

Ah, Clang has it under -Wdeprecated. https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated.

But combo -Wall -Wextra does not enable this warning, sadly.

This should be extracted to -Wdeprecated-copy and put into -Wextra. Seems fine for you?

Sounds plausible to me - though I'd expect @rsmith should probably have a final say before that's submitted.

[xbolva00]

Yes, i will prepare a new patch and add him as a reviewer.

[xbolva00]

https://reviews.llvm.org/D70342

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, InGroup<AbstractFinalClass>;
 def warn_final_dtor_non_final_class : Warning<
   "class with destructor marked '%select{final|sealed}0' cannot be inherited from">,
   InGroup<FinalDtorNonFinalClass>;
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
+def warn_missing_custom_copy : Warning<
+  "class implements custom %select{copy assignment operator|copy constructor}0 "
+  "but missing custom %select{copy assignment operator|copy constructor}1">,
+  InGroup<DiagGroup<"custom-copy">>;
+
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
 
 // C++11 final
 def err_final_function_overridden : Error<
   "declaration of %0 overrides a '%select{final|sealed}1' function">;
 
 // C++11 scoped enumerations

lib/Sema/SemaDeclCXX.cpp

for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E;
           isa<IndirectFieldDecl>(D)) {
         Diag((*I)->getLocation(), diag::err_member_name_of_class)
           << D->getDeclName();
         break;
       }
     }
   }
 
+  // Warn if the class implements custom copy constructor but missing
+  // custom copy assignment operator and vice versa.
+  if (Record->hasUserDeclaredCopyAssignment() !=
+      Record->hasUserDeclaredCopyConstructor())
+    Diag(Record->getLocation(), diag::warn_missing_custom_copy)
+        << Record->hasUserDeclaredCopyConstructor()
+        << !Record->hasUserDeclaredCopyConstructor();
+
   // Warn if the class has virtual methods but non-virtual public destructor.
   if (Record->isPolymorphic() && !Record->isDependentType()) {
     CXXDestructorDecl *dtor = Record->getDestructor();
     if ((!dtor || (!dtor->isVirtual() && dtor->getAccess() == AS_public)) &&
         !Record->hasAttr<FinalAttr>())
       Diag(dtor ? dtor->getLocation() : Record->getLocation(),
            diag::warn_non_virtual_dtor) << Context.getRecordType(Record);
   }

test/SemaCXX/warn-custom-copy.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wcustom-copy %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class Clz // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz() : buf(nullptr) {}
+  Clz &operator=(const Clz &r) {
+    buf = r.buf;
+    return *this;
+  }
+};
+
+class Clz2 // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz2() : buf(nullptr) {}
+  Clz2(const Clz2 &r) : buf(nullptr) {
+    (void)buf;
+    (void)r;
+  }
+};
+
+class Clz3 {
+  char *buf;
+
+public:
+  Clz3() : buf(nullptr) {}
+  Clz3(const Clz3 &r) : buf(nullptr) {
+    (void)buf;
+    (void)r;
+  }
+  Clz3 &operator=(const Clz3 &r) {
+    buf = r.buf;
+    return *this;
+  }
+};