[common] DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN detects mis-uses (Rob…

…otLocomotion#15884)

Add unit test advice to complement the new built-in checking.

This commit also contains a demonstration of related changes:
* Add (positive) unit test for this macro.
* Fix name_value to correctly document its (pre-existing) non-copyable behavior.
* Remove useless unit test for a defaulted copy constructor.

common/BUILD.bazel

@@ -779,6 +779,13 @@ alias(
     actual = "//tools/cc_toolchain:print_host_settings",
 )
 
+drake_cc_googletest(
+    name = "drake_copyable_test",
+    deps = [
+        ":essential",
+    ],
+)
+
 drake_cc_googletest(
     name = "is_cloneable_test",
     deps = [

common/drake_copyable.h

@@ -43,8 +43,8 @@ copy-assignment defaults are well-formed.  Note that the defaulted move
 functions could conceivably still be ill-formed, in which case they will
 effectively not be declared or used -- but because the copy constructor exists
 the type will still be MoveConstructible.  Drake's Doxygen is customized to
-render the functions in detail, with appropriate comments.  Invoke this macro
-in the public section of the class declaration, e.g.:
+render the functions in detail, with appropriate comments.  Typically, you
+should invoke this macro in the public section of the class declaration, e.g.:
 <pre>
 class Foo {
  public:
@@ -53,14 +53,38 @@ class Foo {
   // ...
 };
 </pre>
+
+However, if Foo has a virtual destructor (i.e., is subclassable), then
+typically you should use either DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN in the
+public section or else DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN in the
+protected section, to prevent
+<a href="https://en.wikipedia.org/wiki/Object_slicing">object slicing</a>.
+
+The macro contains a built-in self-check that copy-assignment is well-formed.
+This self-check proves that the Classname is CopyConstructible, CopyAssignable,
+MoveConstructible, and MoveAssignable (in all but the most arcane cases where a
+member of the Classname is somehow CopyAssignable but not CopyConstructible).
+Therefore, classes that use this macro typically will not need to have any unit
+tests that check for the presence nor correctness of these functions.
+
+However, the self-check does not provide any checks of the runtime efficiency
+of the functions.  If it is important for performance that the move functions
+actually move (instead of making a copy), then you should consider capturing
+that in a unit test.
 */
 #define DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Classname)       \
   Classname(const Classname&) = default;                        \
   Classname& operator=(const Classname&) = default;             \
   Classname(Classname&&) = default;                             \
   Classname& operator=(Classname&&) = default;                  \
-  /* Fails at compile-time if default-copy doesn't work. */     \
-  static void DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE() {        \
-    (void) static_cast<Classname& (Classname::*)(               \
-        const Classname&)>(&Classname::operator=);              \
-  }
+  /* Fails at compile-time if copy-assign doesn't compile. */   \
+  /* Note that we do not test the copy-ctor here, because  */   \
+  /* it will not exist when Classname is abstract.         */   \
+  static void DrakeDefaultCopyAndMoveAndAssign_DoAssign(        \
+      Classname* a, const Classname& b) { *a = b; }             \
+  static_assert(                                                \
+      &DrakeDefaultCopyAndMoveAndAssign_DoAssign ==             \
+      &DrakeDefaultCopyAndMoveAndAssign_DoAssign,               \
+      "This assertion is never false; its only purpose is to "  \
+      "generate 'use of deleted function: operator=' errors "   \
+      "when Classname is a template.");

common/name_value.h

@@ -60,7 +60,7 @@ namespace drake {
 template <typename T>
 class NameValue {
  public:
-  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(NameValue)
+  DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(NameValue)
 
   /// Type of the referenced value.
   typedef T value_type;

common/test/drake_copyable_test.cc

@@ -0,0 +1,41 @@
+#include "drake/common/drake_copyable.h"
+
+#include <gtest/gtest.h>
+
+namespace drake {
+namespace {
+
+// When a class has a user-defined destructor, the implicit generation of the
+// copy constructor and copy-assignment operator is deprecated as of C++11.
+//
+// Under Drake's Clang configuration, relying on the deprecated operator
+// produces an error under -Werror=deprecated.
+//
+// Therefore, if DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN failed to default those
+// operations, then this unit test would no longer compile (under Clang).
+class Example {
+ public:
+  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Example);
+  Example() = default;
+  ~Example() {}
+};
+
+GTEST_TEST(DrakeCopyableTest, CopyConstruct) {
+  Example foo;
+  Example bar(foo);
+}
+
+GTEST_TEST(DrakeCopyableTest, CopyAssign) {
+  Example foo, bar;
+  bar = foo;
+}
+
+// Confirm that private (and by association, protected) access for this macro
+// is permitted.  If not, this class would fail to compile.
+class ExamplePrivate {
+ private:
+  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(ExamplePrivate);
+};
+
+}  // namespace
+}  // namespace drake

common/yaml/test/yaml_node_test.cc

@@ -31,17 +31,6 @@ GTEST_TEST(YamlNodeTest, DefaultConstructor) {
   EXPECT_EQ(dut.GetScalar(), "");
 }
 
-// Sanity check of defaulted operators.  We don't need to test them
-// exhaustively, because they are defaulted.
-GTEST_TEST(YamlNodeTest, DefaultCopy) {
-  Node dut = Node::MakeScalar("foo");
-
-  // Copy constructor.
-  Node foo(dut);
-  EXPECT_EQ(dut.GetScalar(), "foo");
-  EXPECT_EQ(foo.GetScalar(), "foo");
-}
-
 // Parameterize the remainder of the tests across the three possible types.
 using Param = std::tuple<NodeType, std::string_view>;
 class YamlNodeParamaterizedTest : public testing::TestWithParam<Param> {

tools/workspace/pybind11/mkdoc.py

@@ -103,7 +103,7 @@
 
 # 'Broadphase' culling; do not recurse inside these symbols.
 SKIP_RECURSE_NAMES = [
-    'DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE',
+    'DrakeDefaultCopyAndMoveAndAssign_DoAssign',
     'Eigen',
     'detail',
     'dev',