[Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)

[xbolva00]

Summary

Solves https://bugs.llvm.org/show_bug.cgi?id=45634
Be more agressive than GCC with -Wdeprecated-copy. Also provide -W(no-)deprecated-copy-user-provided-copy/dtor options to on/off this behaviour.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticGroups.td

168

If we're going to provide these options at all, I think it would be more grammatical to call them -Wdeprecated-copy-with-deleted-copy and -Wdeprecated-copy-with-deleted-dtor.

The existing code is already confusing by talking about a "copy dtor" as if that's a thing; I think it makes it even worse to talk about "deprecated copy user provided," since the actually deprecated thing is the implicitly generated copy.

I get that we're trying to be terse, and also somewhat hierarchical in putting all the -Wdeprecated-copy-related warnings under -Wdeprecated-copy-foo-bar; but the current names are too far into the ungrammatical realm for me personally.

clang/include/clang/Basic/DiagnosticSemaKinds.td

583

This is the first time I've ever seen this idiom. As a person who often greps the wording of an error message to find out where it comes from, I would very strongly prefer to see the actual string literal here. But if this is established practice, then okay.

clang/lib/Sema/SemaDeclCXX.cpp

14030–14043

I wonder if this would be easier to read as

if (UserDeclaredOperation) {
    bool UDOIsUserProvided = UserDeclaredOperation->isUserProvided();
    bool UDOIsDestructor = isa<CXXDestructorDecl>(UserDeclaredOperation);
    bool IsCopyAssignment = !isa<CXXConstructorDecl>(CopyOp);
    unsigned DiagID =
        ( UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation_user_provided :
        ( UDOIsUserProvided && !UDOIsDestructor) ? diag::warn_deprecated_copy_operation_user_provided :
        (!UDOIsUserProvided &&  UDOIsDestructor) ? diag::warn_deprecated_copy_dtor_operation :
        diag::warn_deprecated_copy_operation;
    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
        << RD << /*copy assignment*/ IsCopyAssignment;

clang/test/SemaCXX/deprecated-copy.cpp

5

I'm fairly confident this should just be two different test files. Also, if a test file has an un-ifdeffed // expected-no-diagnostics plus an un-ifdeffed // expected-note ..., which one wins?

[xbolva00]

Thanks for initial comments.

clang/include/clang/Basic/DiagnosticGroups.td

168

Yeah, better names are welcome :)

clang/include/clang/Basic/DiagnosticSemaKinds.td

583

I think this is established way how to duplicated warning text strings.

clang/lib/Sema/SemaDeclCXX.cpp

14030–14043

I have been thinking about similar form, so +1.

clang/test/SemaCXX/deprecated-copy.cpp

5

ifdef is here

and ifNdef is below :)

[xbolva00]

clang/test/SemaCXX/deprecated-copy.cpp

5

and DEPRECATED_COPY_DTOR is in own "code block"

[• Quuxplusone]

clang/test/SemaCXX/deprecated-copy.cpp

5

Ah, you're right, I had missed that line 18 was still controlled by the #ifdef DEPRECATED_COPY_DTOR condition.
I still think this should be three(!) different files. Line 2 doesn't even look right to me: shouldn't it be -Wdeprecated-copy -Wno-deprecated-copy-user-provided?

I just did a git grep 'RUN:' | grep '[-]Wno-' | grep -v '[-]W[^n]' and found a massive number of tests that put -Wno-foo on the command line without putting -Wbar anywhere on the same line; I suspect many of these are bugs.

[xbolva00]

Updated. Thanks for review.

[xbolva00]

More tests

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticGroups.td

168

Do the current names match existing practice in GCC or anything?
I continue to opine that these options are poorly named. My best suggestion is
deprecated-copy, deprecated-copy-with-dtor, deprecated-copy-with-deleted-copy, deprecated-copy-with-deleted-dtor — operating on the (wrong?) assumption that absolutely the only difference between "user-declared" and "user-provided" corresponds to "user-declared as deleted."

Even if everything else remains the same, the internal identifier warn_deprecated_copy_dtor_operation_user_provided should certainly be warn_deprecated_copy_operation_dtor_user_provided — the phrase that goes together is "copy operation", not "dtor operation".

Your "deprecated-dtor-user-provided.cpp" passes -Wdeprecated-copy-dtor-user-provided, but the message text talks about "user-declared." Shouldn't the spelling of the option reflect the wording of the message text? This isn't important from the POV of someone who's just going to look at the message text and fix their code, but it's important from the POV of someone who's going to use -Wno- and wants to know exactly which bad situations they're ignoring. (Also, the name of the file should match the spelling of the option.)

clang/lib/Sema/SemaDeclCXX.cpp

14043

The /* copy assignment */ comment is probably no longer useful.

clang/test/SemaCXX/deprecated-copy.cpp

5

I've lost track of what I was trying to say here and whether I was right ;) but I assume we can consider this comment thread "resolved"?

clang/test/SemaCXX/deprecated-dtor-user-provided.cpp

6 ↗

(On Diff #280694)

Do you have a test case for when an operation is defaulted as deleted?
I actually don't understand Clang's/GCC's current behavior in this area.
https://godbolt.org/z/9h3qqP
Of these three situations, do any of them rely on deprecated behavior?
(And vice versa, what if we delete the copy constructor and try to use the implicit copy-assignment operator?)

clang/test/SemaCXX/deprecated.cpp

108

Nit: I would prefer to see the declarations of d1 and d2 on individual lines, both for style and to prove that d1's declaration triggers no warnings.

[xbolva00]

Updated, addressed review feedback.

[xbolva00]

Ping @Quuxplusone

clang/include/clang/Basic/DiagnosticGroups.td

168

Yeah, deprecated-copy and deprecated-copy-dtor is already defined by GCC.

I think deprecated-copy-user-provided-copy and deprecated-copy-user-provided-dtor could be good solution here, it is also quite easy to follow implementation of the checking logic in Sema with these names.

[• Quuxplusone]

The new individual test files are awesome. :)
The name of the -W options still aren't perfect IMHO, but maybe it will never be perfect, and meanwhile everything else looks great.

clang/include/clang/Basic/DiagnosticGroups.td

168

@xbolva00: I suggested -Wdeprecated-copy-with-user-provided-copy; you've got -Wdeprecated-copy-user-provided-copy. Could I persuade you to use the -with- versions?
I suppose where it really matters is -Wdeprecated-copy-dtor (what is a "copy destructor"?), but that's for GCC compatibility, so we can't change it. Right?

clang/test/SemaCXX/deprecated-copy-dtor.cpp

2 ↗

(On Diff #338336)

I think it's extremely unfortunate that -Wdeprecated-copy-dtor is not a subset of -Wdeprecated-copy. But again, GCC-compatibility binds our hands here AFAIK. https://godbolt.org/z/3r3ddvTfG

[xbolva00]

clang/include/clang/Basic/DiagnosticGroups.td

168

I can use with “-with” and create alias for -Wdeprecated-copy-dtor to keep gcc compatibility :)

[xbolva00]

Use suggested flag names.

[uabelho]

Hi,

With this commit I get failures in the following testcases when building check-runtimes on trunk:

Failed Tests (29):
  libc++ :: libcxx/debug/containers/db_sequence_container_iterators.pass.cpp
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/emplace.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_initializer_list.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/iterators.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/size.pass.cpp
  libc++ :: std/containers/sequences/vector/vector.cons/deduct.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ullong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp

They all get -Wdeprecated-copy warnigns and then with -Werror they fail.
Is this something you've seen or considered?

[xbolva00]

In D79714#2711871, @uabelho wrote:

Hi,

With this commit I get failures in the following testcases when building check-runtimes on trunk:

Failed Tests (29):
  libc++ :: libcxx/debug/containers/db_sequence_container_iterators.pass.cpp
  libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/assign_move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/copy_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/emplace.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/erase_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_initializer_list.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_iter_iter.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/insert_iter_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/iterators.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/move_alloc.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/resize_size_value.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
  libc++ :: std/containers/sequences/vector.bool/size.pass.cpp
  libc++ :: std/containers/sequences/vector/vector.cons/deduct.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/right_shift.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ullong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.members/to_ulong.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_and.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_not.pass.cpp
  libc++ :: std/utilities/template.bitset/bitset.operators/op_or.pass.cpp

They all get -Wdeprecated-copy warnigns and then with -Werror they fail.
Is this something you've seen or considered?

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

[uabelho]

In D79714#2711898, @xbolva00 wrote:

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

The above doesn't help but if I add it in the list after -Wextra the warning goes away. Can you submit something like that?
Thanks!

[xbolva00]

In D79714#2712393, @uabelho wrote:

In D79714#2711898, @xbolva00 wrote:

Can you try this fix?

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index ddf277dea246..abf712e78a61 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -12,6 +12,7 @@ from libcxx.test.features import _isMSVC
 _warningFlags = [
   '-Werror',
   '-Wall',
+  '-Wno-deprecated-copy',
   '-Wextra',
   '-Wshadow',
   '-Wundef',

The above doesn't help but if I add it in the list after -Wextra the warning goes away. Can you submit something like that?
Thanks!

Yes, I can. Thanks

[nemanjai]

This breaks Clang's ability to bootstrap LLVM with -Werror as evidenced by the buildbot failure https://lab.llvm.org/buildbot/#/builders/36/builds/7587.
Please revert or fix the googletest code that trips this warning. Pertinent file:

FAILED: unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o 
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/install/stage1/bin/clang++  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support -Iinclude -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -c /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp:12:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:62:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest-param-test.h:193:
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-param-util-generated.h:107:8: error: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const ValueArray2& other) = delete;
       ^

[xbolva00]

In D79714#2713311, @nemanjai wrote:

This breaks Clang's ability to bootstrap LLVM with -Werror as evidenced by the buildbot failure https://lab.llvm.org/buildbot/#/builders/36/builds/7587.
Please revert or fix the googletest code that trips this warning. Pertinent file:

FAILED: unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o 
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/install/stage1/bin/clang++  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support -Iinclude -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include -I/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/FormatVariadicTest.cpp.o -c /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/unittests/Support/FormatVariadicTest.cpp:12:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:62:
In file included from /home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest-param-test.h:193:
/home/buildbots/ppc64le-lld-multistage-test/ppc64le-lld-multistage-test/llvm-project/llvm/utils/unittest/googletest/include/gtest/internal/gtest-param-util-generated.h:107:8: error: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
  void operator=(const ValueArray2& other) = delete;
       ^

I pushed a fix to just disable this warning for googlemock/gtest.

[nemanjai]

In D79714#2713610, @xbolva00 wrote:

I pushed a fix to just disable this warning for googlemock/gtest.

There are still more of these. I will apply the same fix to utils/unittest/googlemock/include/gmock/gmock.h unless you think there's a problem with doing that.

[xbolva00]

In D79714#2713921, @nemanjai wrote:

In D79714#2713610, @xbolva00 wrote:

I pushed a fix to just disable this warning for googlemock/gtest.

There are still more of these. I will apply the same fix to utils/unittest/googlemock/include/gmock/gmock.h unless you think there's a problem with doing that.

Yeah, please go ahead :)

[nemanjai]

The fixes in https://reviews.llvm.org/D101214 are sufficient to bring the -Werror bootstrap back to green. Please review the patch.

[uabelho]

In D79714#2713610, @xbolva00 wrote:

I pushed a fix to just disable this warning for googlemock/gtest.

Unfortunately that fix breaks compiling llvm with clang8. I commented in
https://reviews.llvm.org/rG9658d045926545e62cc3f963fe611d7c5d0c9d98

[Abpostelnicu]

I'm seeing here something very strange, if the user provided copy assignment operator is provided but is = delete and the copy constructor is default this warning will still trigger. Is this something expected?
I'm referring at this issue from the mozilla code-base.

[• Quuxplusone]

In D79714#2735455, @Abpostelnicu wrote:

I'm seeing here something very strange, if the user provided copy assignment operator is provided but is = delete and the copy constructor is default this warning will still trigger. Is this something expected?
I'm referring at this issue from the mozilla code-base.

Yes, that implicit default is deprecated.
If you want your copy constructor to be defaulted, even in the presence of deleted SMFs, you should explicitly =default it. Here's an example: https://godbolt.org/z/GaGn1638E

clang/include/clang/Basic/DiagnosticGroups.td

 def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;
 
 def DeprecatedArrayCompare : DiagGroup<"deprecated-array-compare">;
 def DeprecatedAttributes : DiagGroup<"deprecated-attributes">;
 def DeprecatedCommaSubscript : DiagGroup<"deprecated-comma-subscript">;
-def DeprecatedCopy : DiagGroup<"deprecated-copy">;
-def DeprecatedCopyDtor : DiagGroup<"deprecated-copy-dtor">;
+def DeprecatedCopyWithUserProvidedCopy : DiagGroup<"deprecated-copy-with-user-provided-copy">;
+def DeprecatedCopyWithUserProvidedDtor : DiagGroup<"deprecated-copy-with-user-provided-dtor">;
+def DeprecatedCopy : DiagGroup<"deprecated-copy", [DeprecatedCopyWithUserProvidedCopy]>;
+def DeprecatedCopyWithDtor : DiagGroup<"deprecated-copy-with-dtor", [DeprecatedCopyWithUserProvidedDtor]>;
+// For compatibility with GCC.
+def : DiagGroup<"deprecated-copy-dtor", [DeprecatedCopyWithDtor]>;
 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", [DeprecatedAnonEnumEnumConversion,
                                           DeprecatedArrayCompare,
                                           DeprecatedAttributes,
                                           DeprecatedCommaSubscript,
                                           DeprecatedCopy,
-                                          DeprecatedCopyDtor,
+                                          DeprecatedCopyWithDtor,
                                           DeprecatedDeclarations,
                                           DeprecatedDynamicExceptionSpec,
                                           DeprecatedEnumCompare,
                                           DeprecatedEnumCompareConditional,
                                           DeprecatedEnumEnumConversion,
                                           DeprecatedEnumFloatConversion,
                                           DeprecatedIncrementBool,
                                           DeprecatedRegister,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_using_decl_redeclaration_expansion : Error<
   "using declaration pack expansion at block scope produces multiple values">;
 
 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<
+def warn_deprecated_copy : Warning<
   "definition of implicit copy %select{constructor|assignment operator}1 "
   "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<
+def warn_deprecated_copy_with_dtor : Warning<
   "definition of implicit copy %select{constructor|assignment operator}1 "
   "for %0 is deprecated because it has a user-declared destructor">,
-  InGroup<DeprecatedCopyDtor>, DefaultIgnore;
+  InGroup<DeprecatedCopyWithDtor>, DefaultIgnore;
+def warn_deprecated_copy_with_user_provided_copy: Warning<
+  "definition of implicit copy %select{constructor|assignment operator}1 "
+  "for %0 is deprecated because it has a user-provided copy "
+  "%select{assignment operator|constructor}1">,
+  InGroup<DeprecatedCopyWithUserProvidedCopy>, DefaultIgnore;
+def warn_deprecated_copy_with_user_provided_dtor : Warning<
+  "definition of implicit copy %select{constructor|assignment operator}1 "
+  "for %0 is deprecated because it has a user-provided destructor">,
+  InGroup<DeprecatedCopyWithUserProvidedDtor>, 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()) {
       if (I->isCopyAssignmentOperator()) {
         UserDeclaredOperation = I;
         break;
       }
     }
     assert(UserDeclaredOperation);
   }
 
-  if (UserDeclaredOperation && UserDeclaredOperation->isUserProvided()) {
-    S.Diag(UserDeclaredOperation->getLocation(),
-           isa<CXXDestructorDecl>(UserDeclaredOperation)
-               ? diag::warn_deprecated_copy_dtor_operation
-               : diag::warn_deprecated_copy_operation)
-        << RD << /*copy assignment*/ !isa<CXXConstructorDecl>(CopyOp);
+  if (UserDeclaredOperation) {
+    bool UDOIsUserProvided = UserDeclaredOperation->isUserProvided();
+    bool UDOIsDestructor = isa<CXXDestructorDecl>(UserDeclaredOperation);
+    bool IsCopyAssignment = !isa<CXXConstructorDecl>(CopyOp);
+    unsigned DiagID =
+        (UDOIsUserProvided && UDOIsDestructor)
+            ? diag::warn_deprecated_copy_with_user_provided_dtor
+        : (UDOIsUserProvided && !UDOIsDestructor)
+            ? diag::warn_deprecated_copy_with_user_provided_copy
+        : (!UDOIsUserProvided && UDOIsDestructor)
+            ? diag::warn_deprecated_copy_with_dtor
+            : diag::warn_deprecated_copy;
+    S.Diag(UserDeclaredOperation->getLocation(), DiagID)
+        << RD << IsCopyAssignment;
   }
 }
 
 void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
                                         CXXMethodDecl *CopyAssignOperator) {
   assert((CopyAssignOperator->isDefaulted() &&
           CopyAssignOperator->isOverloadedOperator() &&
           CopyAssignOperator->getOverloadedOperator() == OO_Equal &&

clang/test/SemaCXX/deprecated-copy-with-dtor.cpp

+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-dtor -verify
+
+class A {
+public:
+   ~A() = default; // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared destructor}}
+};
+
+void test() {
+  A a1;
+  A a2 = a1; // expected-note {{in implicit copy constructor for 'A' first required here}}
+}

clang/test/SemaCXX/deprecated-copy-with-user-provided-copy.cpp

+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-copy -verify
+
+struct A {
+  A &operator=(const A &); // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-provided copy assignment operator}}
+};
+
+void foo() {
+  A a1;
+  A a2(a1); // expected-note {{implicit copy constructor for 'A' first required here}}
+}

clang/test/SemaCXX/deprecated-copy-with-user-provided-dtor.cpp

+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-dtor -verify
+
+struct A {
+  ~A(); // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-provided destructor}}
+};
+
+void test() {
+  A a1;
+  A a2(a1); // expected-note {{implicit copy constructor for 'A' first required here}}
+}

clang/test/SemaCXX/deprecated-copy.cpp

+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
 // 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}}
+    A& operator=(const A&) = default; // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared copy assignment operator}}
 };
 
-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}}
+    B& operator=(const B&) = delete; // 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}}
+void test() {
+  A a1;
+  A a2(a1); // expected-note {{implicit copy constructor for 'A' first required here}}
+
+  B b1;
+  B b2(b1); // expected-note {{implicit copy constructor for 'B' first required here}}
 }
-#endif
+
+// PR45634
+struct S {
+    int i;
+    S& operator=(const S&) = delete; // expected-warning {{definition of implicit copy constructor for 'S' is deprecated because it has a user-declared copy assignment operator}}
+};
+
+S test(const S &s) { return S(s); } // expected-note {{implicit copy constructor for 'S' first required here}}

clang/test/SemaCXX/deprecated.cpp

 #else
   // expected-error@-4 {{ISO C++11 does not allow access declarations; use using declarations instead}}
 #endif
 };
 
 #if __cplusplus >= 201103L
 namespace DeprecatedCopy {
   struct Assign {
-    Assign &operator=(const Assign&); // expected-warning {{definition of implicit copy constructor for 'Assign' is deprecated because it has a user-declared copy assignment operator}}
+    Assign &operator=(const Assign&); // expected-warning {{definition of implicit copy constructor for 'Assign' is deprecated because it has a user-provided copy assignment operator}}
   };
   Assign a1, a2(a1); // expected-note {{implicit copy constructor for 'DeprecatedCopy::Assign' first required here}}
 
   struct Ctor {
     Ctor();
-    Ctor(const Ctor&); // expected-warning {{definition of implicit copy assignment operator for 'Ctor' is deprecated because it has a user-declared copy constructor}}
+    Ctor(const Ctor&); // expected-warning {{definition of implicit copy assignment operator for 'Ctor' is deprecated because it has a user-provided copy constructor}}
   };
   Ctor b1, b2;
   void f() { b1 = b2; } // expected-note {{implicit copy assignment operator for 'DeprecatedCopy::Ctor' first required here}}
 
   struct Dtor {
     ~Dtor();
-    // expected-warning@-1 {{definition of implicit copy constructor for 'Dtor' is deprecated because it has a user-declared destructor}}
-    // expected-warning@-2 {{definition of implicit copy assignment operator for 'Dtor' is deprecated because it has a user-declared destructor}}
+    // expected-warning@-1 {{definition of implicit copy constructor for 'Dtor' is deprecated because it has a user-provided destructor}}
+    // expected-warning@-2 {{definition of implicit copy assignment operator for 'Dtor' is deprecated because it has a user-provided destructor}}
   };
   Dtor c1, c2(c1); // expected-note {{implicit copy constructor for 'DeprecatedCopy::Dtor' first required here}}
   void g() { c1 = c2; } // expected-note {{implicit copy assignment operator for 'DeprecatedCopy::Dtor' first required here}}
 
   struct DefaultedDtor {
-    ~DefaultedDtor() = default;
-  };
-  DefaultedDtor d1, d2(d1);
-  void h() { d1 = d2; }
+    ~DefaultedDtor() = default; // expected-warning {{definition of implicit copy constructor for 'DefaultedDtor' is deprecated because it has a user-declared destructor}}
+  };                            // expected-warning@-1 {{definition of implicit copy assignment operator for 'DefaultedDtor' is deprecated because it has a user-declared destructor}}
+  DefaultedDtor d1;
+  DefaultedDtor d2(d1);         // expected-note {{in implicit copy constructor for 'DeprecatedCopy::DefaultedDtor' first required here}}
+  void h() { d1 = d2; }         // expected-note {{in implicit copy assignment operator for 'DeprecatedCopy::DefaultedDtor' first required here}}
 }
 #endif
 
 struct X {
   friend int operator,(X, X);
   void operator[](int);
 };
 void array_index_comma() {