[Diagnostics] Put "deprecated copy" warnings into -Wdeprecated-copy

[xbolva00]

Summary

GCC 9 added -Wdeprecated-copy (as part of -Wextra). This diagnostic is already implemented in Clang too, just hidden under -Wdeprecated (not on by default).
This patch adds -Wdeprecated-copy and makes it compatible with GCC 9+.
This diagnostic is heavily tested in deprecated.cpp, so I added simple tests just to check we warn when new flag/-Wextra is enabled.

[rsmith]

Generally OK, but I'm concerned about including this in -Wextra. In particular, the deprecation of copy/move operations when a destructor is explicitly declared has experimentally been found to have a high false-positive rate. Does GCC include this in -Wextra? (Its documentation doesn't seem to say so.)

[xbolva00]

Added -Wdeprecated-copy-dtor (not part of -Wextra). Now we should be fully compatible with GCC.

[xbolva00]

Newline fixed.

[xbolva00]

In D70342#1748555, @rsmith wrote:

Generally OK, but I'm concerned about including this in -Wextra. In particular, the deprecation of copy/move operations when a destructor is explicitly declared has experimentally been found to have a high false-positive rate. Does GCC include this in -Wextra? (Its documentation doesn't seem to say so.)

Yes, right. Fixed. New revision should be OK.

[xbolva00]

ping :)

[dblaikie]

Looks good to me - can handle anything else from @rsmith in post-commit review.

[xbolva00]

Thank you!

[hans]

We're now getting a ton of warnings about this in Chromium. (https://crbug.com/1028110)

And I'm not really sure what the warning wants us to do?

[xbolva00]

Add copy ctor/op= manually.

If you have a lot of warnings, you can use -Wno-deprecated-copy.

[hans]

In D70342#1758832, @xbolva00 wrote:

Add copy ctor/op= manually.

Does the warning mean that the implicitly defined copy ctor is not going to work in some later version of the language?

I'm just trying to understand what it's warning about. If it's pointing out bugs in our code, we should fix them obviously.

[xbolva00]

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0619r4.html#3.2

So C++20 or C++23.

[thakis]

Why do we emit this before C++20? It's only deprecated in C++20 onwards, no? Shouldn't this be default-enabled based off language version instead of being in -Wextra? No other language deprecation warning so far (register etc) has worked like this warning does.

(Also, this being in -Wextra has confused users for us: We added -Wno-deprecated-copy somewhere in our central build flags, and then some target added -Wextra later on and was confused that the -Wno-deprecated-copy flag didn't have an effect – but that's a minor point.)

[thakis]

Ah, Hans points out that the paper says that it's been deprecated in C++11, so this should either be on by default (and mention "C++11" somewhere), or it should be in -pedantic given that no other compiler diagnoses this and given that it fires heavily on existing code (including LLVM).

[xbolva00]

No other compiler ? GCC 9 introduced it even in non pedantic mode. Warnings in LLVM are fixed.

Not sure if breaking compatibility with gcc is good idea. We wanted to unify behaviours here, indeed. Users should fix ther codebases - or they can use -Wno flag. Your suggestion to mention C++11 somewhere is reasonable, +1.

[thakis]

Re-re-reading http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0619r4.html#3.2, the recommendation is "We will consider again for C++23". So I think this should be enabled with -std=c++20 at the earliest, since that's where the idea is from. But even there it's uncertain if it's going to happen. So I'd say put it in -pedantic, and possibly enable it by default with -std=c++20.

clang/include/clang/Basic/DiagnosticGroups.td

 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
 def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;
 
 def DeprecatedAttributes : DiagGroup<"deprecated-attributes">;
 def DeprecatedCommaSubscript : DiagGroup<"deprecated-comma-subscript">;
+def DeprecatedCopy : DiagGroup<"deprecated-copy">;
+def DeprecatedCopyDtor : DiagGroup<"deprecated-copy-dtor">;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
 def UnavailableDeclarations : DiagGroup<"unavailable-declarations">;
 def UnguardedAvailabilityNew : DiagGroup<"unguarded-availability-new">;
 def UnguardedAvailability : DiagGroup<"unguarded-availability",
                                       [UnguardedAvailabilityNew]>;
 // partial-availability is an alias of unguarded-availability.
 def : DiagGroup<"partial-availability", [UnguardedAvailability]>;
 def DeprecatedDynamicExceptionSpec
     : DiagGroup<"deprecated-dynamic-exception-spec">;
 def DeprecatedImplementations :DiagGroup<"deprecated-implementations">;
 def DeprecatedIncrementBool : DiagGroup<"deprecated-increment-bool">;
 def DeprecatedRegister : DiagGroup<"deprecated-register">;
 def DeprecatedThisCapture : DiagGroup<"deprecated-this-capture">;
 def DeprecatedVolatile : DiagGroup<"deprecated-volatile">;
 def DeprecatedWritableStr : DiagGroup<"deprecated-writable-strings",
                                       [CXX11CompatDeprecatedWritableStr]>;
 // FIXME: Why is DeprecatedImplementations not in this group?
 def Deprecated : DiagGroup<"deprecated", [DeprecatedAttributes,
                                           DeprecatedCommaSubscript,
+                                          DeprecatedCopy,
+                                          DeprecatedCopyDtor,
                                           DeprecatedDeclarations,
                                           DeprecatedDynamicExceptionSpec,
                                           DeprecatedIncrementBool,
                                           DeprecatedRegister,
                                           DeprecatedThisCapture,
                                           DeprecatedVolatile,
                                           DeprecatedWritableStr]>,
                  DiagCategory<"Deprecations">;

 def Move : DiagGroup<"move", [
     PessimizingMove,
     RedundantMove,
     ReturnStdMove,
     SelfMove
   ]>;
 
 def Extra : DiagGroup<"extra", [
+    DeprecatedCopy,
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
     NullPointerArithmetic,

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_access_decl_deprecated : Warning<
   "access declarations are deprecated; use using declarations instead">,
   InGroup<Deprecated>;
 def err_access_decl : Error<
   "ISO C++11 does not allow access declarations; "
   "use using declarations instead">;
 def warn_deprecated_copy_operation : Warning<
   "definition of implicit copy %select{constructor|assignment operator}1 "
-  "for %0 is deprecated because it has a user-declared "
-  "%select{copy %select{assignment operator|constructor}1|destructor}2">,
-  InGroup<Deprecated>, DefaultIgnore;
+  "for %0 is deprecated because it has a user-declared copy "
+  "%select{assignment operator|constructor}1">,
+  InGroup<DeprecatedCopy>, DefaultIgnore;
+def warn_deprecated_copy_dtor_operation : Warning<
+  "definition of implicit copy %select{constructor|assignment operator}1 "
+  "for %0 is deprecated because it has a user-declared destructor">,
+  InGroup<DeprecatedCopyDtor>, DefaultIgnore;
 def warn_cxx17_compat_exception_spec_in_signature : Warning<
   "mangled name of %0 will change in C++17 due to non-throwing exception "
   "specification in function signature">, InGroup<CXX17CompatMangling>;
 
 def warn_global_constructor : Warning<
   "declaration requires a global constructor">,
   InGroup<GlobalConstructors>, DefaultIgnore;
 def warn_global_destructor : Warning<

clang/lib/Sema/SemaDeclCXX.cpp

for (auto *I : RD->methods()) {
         break;
       }
     }
     assert(UserDeclaredOperation);
   }
 
   if (UserDeclaredOperation) {
     S.Diag(UserDeclaredOperation->getLocation(),
-         diag::warn_deprecated_copy_operation)
-      << RD << /*copy assignment*/!isa<CXXConstructorDecl>(CopyOp)
-      << /*destructor*/isa<CXXDestructorDecl>(UserDeclaredOperation);
+           isa<CXXDestructorDecl>(UserDeclaredOperation)
+               ? diag::warn_deprecated_copy_dtor_operation
+               : diag::warn_deprecated_copy_operation)
+        << RD << /*copy assignment*/ !isa<CXXConstructorDecl>(CopyOp);
   }
 }
 
 void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
                                         CXXMethodDecl *CopyAssignOperator) {
   assert((CopyAssignOperator->isDefaulted() &&
           CopyAssignOperator->isOverloadedOperator() &&
           CopyAssignOperator->getOverloadedOperator() == OO_Equal &&

clang/test/SemaCXX/deprecated-copy.cpp

+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -DDEPRECATED_COPY_DTOR -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wextra -verify
+
+#ifdef DEPRECATED_COPY_DTOR
+struct A {
+  int *ptr;
+  ~A() { delete ptr; } // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared destructor}}
+};
+
+void foo() {
+  A a{};
+  A b = a; // expected-note {{implicit copy constructor for 'A' first required here}}
+}
+#else
+struct B {
+  B &operator=(const B &); // expected-warning {{definition of implicit copy constructor for 'B' is deprecated because it has a user-declared copy assignment operator}}
+};
+
+void bar() {
+  B b1, b2(b1); // expected-note {{implicit copy constructor for 'B' first required here}}
+}
+#endif