DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN detects mis-uses

[jwnimmer-tri]

Advice to users to adjust to these breaking changes:

If this commit induces use of deleted function: operator= errors in your code, you can adjust in the following ways:

• If the class was not supposed to be copyable anyway, then change DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN to read DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN instead. See [fixed_fem] DirichletBoundaryCondition is not assignable #15904 for an example.
• If the class was supposed to be conditionally copyable (i.e., to reflect whatever it's templated members can support), then remove the macro entirely (so that C++ implicit defaults take over). See [common] SortedPair is consistent with the wrapper type's operators #15907 for an example.
• If there is a more complicated situation in play, you could manually transcribe the contents of the macro into your code (substituting the Classname) at its point of use, taking only the parts of the macro (i.e., the specific defaults) that you want.

Any of these fixes can be committed to your own code immediately, i.e., prior any Drake upgrade that requires them.

---

Additional commit details:

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

This commit also contains a demonstration of related changes:

• Fix name_value to correctly document its (pre-existing) non-copyable behavior.
• Remove useless unit test for a defaulted copy constructor.

---

Other PRs have fixed various faulty code that is broken by the new detection:

• [fixed_fem] DirichletBoundaryCondition is not assignable #15904
• [solvers] MinimalDistanceFromSphereProblem is not assignable #15905
• [geometry] Bvh uses default copy/move/assign operations #15906
• [common] SortedPair is consistent with the wrapper type's operators #15907

Also on a related topic:

• [yaml] Clarify unit test for node move operations #15910

---

This change is 

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

---

common/drake_copyable.h, line 63 at r1 (raw file):

then check for that as well.  Any tests beyond those two cases woulld usually
be unnecessary bloat to the unit test.
</pre>

Working

Check if this advice still holds true for templated classes.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

---

common/drake_copyable.h, line 63 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Check if this advice still holds true for templated classes.

\CC @SeanCurtis-TRI

Here is an example;

template <class T>
class Foo {
 public:
  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Foo);
  Foo() = default;
 private:
  const T x_{};
};

template class Foo<double>;

In this case, it is only in the presence of the final line (the explicit template instantiation) that the compilation error is surfaced. Without that final line, there is no diagnostic.

I'll work on amending the advice here to explain this case, and recommend the instantiation.

Even aside from this macro, we should anyway have a GSG rule that every templated class should have at least one explicit instantiation in a cc file. Otherwise, there is no easy way to guarantee that any given function is able to be compiled at all ever. Having an instantiation in its cc file guarantees nicely-local error messages during its direct build rule, instead of deferred until its firs point-of-use much further down the road.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

---

common/drake_copyable.h, line 63 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

\CC @SeanCurtis-TRI

Here is an example;

template <class T>
class Foo {
 public:
  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(Foo);
  Foo() = default;
 private:
  const T x_{};
};

template class Foo<double>;

In this case, it is only in the presence of the final line (the explicit template instantiation) that the compilation error is surfaced. Without that final line, there is no diagnostic.

I'll work on amending the advice here to explain this case, and recommend the instantiation.

Even aside from this macro, we should anyway have a GSG rule that every templated class should have at least one explicit instantiation in a cc file. Otherwise, there is no easy way to guarantee that any given function is able to be compiled at all ever. Having an instantiation in its cc file guarantees nicely-local error messages during its direct build rule, instead of deferred until its firs point-of-use much further down the road.

Done.

[SeanCurtis-TRI]

Take a look at this branch:

https://github.com/SeanCurtis-TRI/drake/blob/TEST_copy_move_test/common/test/copy_move_test.cc

Specifically, this file which can be run as:

bazel run //common:copy_move

Things to observe:

1. Straight out of the box it fails to compile. It will complain that Foo<NoCopyAssign> is trying to access a deleted function.
2. However, it will not complain about Foo<NoCopyConstruct>
3. Toggle the #if 1 to #if 0 on line 84
   1. In addition to the DRAKE_DEFAULT... complaining about Foo<NoCopyAssign> you will now get a complaint about Foo<NoCopyConstruct>.
   2. You'll also get a redundant complaint about Foo<NoCopyAssign>.
4. There are some "NoMove*" variants that are there just to show that all of move is perfectly happy even without move, as it were.

I've included the implication of this down in the comments below.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

common/drake_copyable.h, line 58 at r2 (raw file):

When unit testing uses of this macro:
- It is usually sufficient to test only one of the functions; usually the copy

nit: Based on my comment above, I'd suggest:

When unit testing uses of this macro:
- There must always be an explicit test of the copy constructor.
- The built-in "...CAN_COMPILE" failsafe will ensure that copy assignment also
  works, but will not be compiled when Classname is a templated class; in that
  case, either use an explicit template instantiation, or add a test for
  assignment.
- If it is important for performance that the move functions actually move
  (instead of making a copy), then test for that as well.

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

common/drake_copyable.h, line 58 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Based on my comment above, I'd suggest:

When unit testing uses of this macro:
- There must always be an explicit test of the copy constructor.
- The built-in "...CAN_COMPILE" failsafe will ensure that copy assignment also
  works, but will not be compiled when Classname is a templated class; in that
  case, either use an explicit template instantiation, or add a test for
  assignment.
- If it is important for performance that the move functions actually move
  (instead of making a copy), then test for that as well.

BTW Actually, on the copy-assignment front, it might be worth having language that suggests, "When in doubt, test it. In some circumstances it might not be necessary, but testing it in the cases where it is redundant is not defective."

Or even stronger, simply say, "Always test copy-construct and copy-assign and only test move-semantics if you want to confirm data movement." That's probably the simplest possible formula to cargo cult.

[jwnimmer-tri]

I rewrote this PR entirely. I promoted the check to fire whether or not the class is templated, which means that we don't need unit testing in most cases. If someone is writing code with a member field type that has adversarial implementations of its operators, then we could imagine adding more unit tests in that case. However, in the common case where the default implementations are never going to fail in practice, we should not waste time writing unit tests that offer no useful feedback to future developers.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

common/drake_copyable.h, line 58 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Actually, on the copy-assignment front, it might be worth having language that suggests, "When in doubt, test it. In some circumstances it might not be necessary, but testing it in the cases where it is redundant is not defective."

Or even stronger, simply say, "Always test copy-construct and copy-assign and only test move-semantics if you want to confirm data movement." That's probably the simplest possible formula to cargo cult.

OK I rewrote this section entirely now.

---

common/drake_copyable.h, line 80 at r3 (raw file):

      &DrakeDefaultCopyAndMoveAndAssign_DoAssign ==             \
      &DrakeDefaultCopyAndMoveAndAssign_DoAssign,               \
      "The given Classname is not default copy-asignable.");

Working

It's probably easy to add an assertion for copy-construction as well. We should probably do that? I have never come across a class that is copy-assignable but not copy-constructible, but if checking for it is easy we might as well do it.

---

geometry/proximity/bvh.h, line 161 at r3 (raw file):

    // TODO(jwnimmer-tri) We should use copyable_unique_ptr here, but the
    // dependency cycle of BvNode <=> NodeChildren somehow defeats us.

Working

Possibly we should try to root-cause and fix the problem here, so we don't need this ugly work-around.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

geometry/proximity/bvh.h, line 161 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Possibly we should try to root-cause and fix the problem here, so we don't need this ugly work-around.

Done (and will be part of a different PR, anyway.)

[SeanCurtis-TRI]

I agree. I love having the guarantees built into the utility.

Reviewed 1 of 8 files at r3.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

common/drake_copyable.h, line 80 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

It's probably easy to add an assertion for copy-construction as well. We should probably do that? I have never come across a class that is copy-assignable but not copy-constructible, but if checking for it is easy we might as well do it.

Agreed.

[SeanCurtis-TRI]

Incidentally, as revealed by the related patches, it is decidedly possible for downstream uses of DRAKE_DEFAULT... to suddenly have their code not build after updating Drake. What are your thoughts on this?

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

[jwnimmer-tri]

-(status: do not merge)

Tagging all of the platform team to review this now:

+@EricCousineau-TRI
+@ggould-tri
+@sammy-tri
+@RussTedrake

You only need to badge on the big picture, which is:

(1)DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN is only to be used for types that actually support all of those concepts. It does not merely mean "Use C++ implicit defaults" which in some cases would mean that the operators are missing (e.g., if a member variable were const, the class would not be assignable). This was supposed to already be the case, but now we are re-affirming it more clearly in documentation, and also enforcing it on templated classes.

(2) The unit testing advice. Please read the updated text in the header file, but I'll call it out here as well:

Classes that use this macro typically will not need to have any unit tests that check for the presence nor correctness of these functions.

In short, do not de-jure defect code for lacking copy/move/assign unit test cases on the defaulted operators. If there is something acutely unusual for the class, then go ahead, but don't do it by rote.

Also note the caveat in the advice:

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.

Reviewable status: LGTM missing from assignees EricCousineau-TRI(platform),ggould-tri(platform),sammy-tri(platform),RussTedrake(platform), labeled "do not merge" (waiting on @EricCousineau-TRI, @ggould-tri, @RussTedrake, @sammy-tri, @SeanCurtis-TRI, and @sherm1)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Sorry for the delay. I'll defer to you on deprecation. The documentation looks good.

OK I think I'll keep it breaking. I've added advice about the recovery atop the PR description. We can point users who have trouble to there.

I do have a few mechanisms in mind where I could inject deprecation messages, but they all rely on SFINAE and it's really tricky do that that ODR-soundly in a way that I could be confident would not have false positives on correct code.

---

---

common/drake_copyable.h, line 87 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW My one regret is that it's a shame that the static_assert message has language that is suggestive of a message that would be seen at compile time, even though it's known not to be. It's so strongly suggestive, that it has to be contradicted with a comment.

Why not simply put the comment in the string?

Done. Hadn't occurred to me, but I do like it better.

[rpoyner-tri]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),EricCousineau-TRI(platform),RussTedrake(platform) (waiting on @EricCousineau-TRI, @ggould-tri, @RussTedrake, and @sammy-tri)

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),EricCousineau-TRI(platform),RussTedrake(platform) (waiting on @EricCousineau-TRI, @ggould-tri, @RussTedrake, and @sammy-tri)

[EricCousineau-TRI]

with minor nits

Reviewed 1 of 4 files at r2, 1 of 9 files at r4, 1 of 9 files at r5, 1 of 1 files at r9, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),RussTedrake(platform) (waiting on @ggould-tri, @jwnimmer-tri, @RussTedrake, and @sammy-tri)

a discussion (no related file):
nit I can't find a self-contained unittest drake_copyable_test.cc to test the positive parts of in a focused fashion (nor can I quickly test negative aspects).

Is it possible to include something like Sean's example in master?

---

---

common/drake_copyable.h, line 85 at r9 (raw file):

  static void DrakeDefaultCopyAndMoveAndAssign_DoAssign(        \
      Classname* a, const Classname& b) { *a = b; }             \
  static_assert(                                                \

nit Is it possible to completely remove this symbol from the class and defer it to outside of the class? e.g.

namespace drake::internal {
template <typename T>
struct maybe_generate_error {
  void DoAssign(T* a, const T& b) { *a = b; }
  constexpr inline bool value = (&DoAssign == &DoAssign);
};
}  // namespace drake::internal

...
#define MACRO(Classname) \ ...
    static_assert(
       ::drake::internal::maybe_generate_error<Classname>::value,
       "...");

[jwnimmer-tri]

Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),RussTedrake(platform) (waiting on @EricCousineau-TRI, @ggould-tri, @rpoyner-tri, @RussTedrake, @sammy-tri, @SeanCurtis-TRI, and @sherm1)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit I can't find a self-contained unittest drake_copyable_test.cc to test the positive parts of in a focused fashion (nor can I quickly test negative aspects).

Is it possible to include something like Sean's example in master?

Done.

---

---

common/drake_copyable.h, line 85 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Is it possible to completely remove this symbol from the class and defer it to outside of the class? e.g.

namespace drake::internal {
template <typename T>
struct maybe_generate_error {
  void DoAssign(T* a, const T& b) { *a = b; }
  constexpr inline bool value = (&DoAssign == &DoAssign);
};
}  // namespace drake::internal

...
#define MACRO(Classname) \ ...
    static_assert(
       ::drake::internal::maybe_generate_error<Classname>::value,
       "...");

Nope -- if the operators do not have public access, then an external function will not work. The guard needs to remain inline as part of the class definition, so that it has appropriate access.

[rpoyner-tri]

Reviewed 1 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: LGTM missing from assignees ggould-tri(platform),sammy-tri(platform),RussTedrake(platform) (waiting on @ggould-tri, @RussTedrake, and @sammy-tri)

[ggould-tri]

; I think this is a really nice check.

Reviewed 1 of 4 files at r2, 1 of 9 files at r4, 1 of 9 files at r5, 1 of 1 files at r9, 1 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: LGTM missing from assignees sammy-tri(platform),RussTedrake(platform) (waiting on @RussTedrake and @sammy-tri)

[sammy-tri]

Reviewed 1 of 4 files at r2, 1 of 9 files at r4, 1 of 9 files at r5, 1 of 1 files at r9, 1 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: LGTM missing from assignee RussTedrake(platform) (waiting on @RussTedrake)

[EricCousineau-TRI]

Reviewed 1 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: LGTM missing from assignees sammy-tri(platform),RussTedrake(platform) (waiting on @RussTedrake and @sammy-tri)

---

common/drake_copyable.h, line 85 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Nope -- if the operators do not have public access, then an external function will not work. The guard needs to remain inline as part of the class definition, so that it has appropriate access.

OK Ah, gotcha!

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform) (waiting on @jwnimmer-tri and @RussTedrake)

---

common/drake_copyable.h, line 70 at r11 (raw file):

tests that check for the presence nor correctness of these functions.

However, the self-check does not provide any checks of the runtime efficiency

BTW One last thought, I believe it would be worth while communicating what the failed check will look like. It's not necessarily intuitive.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee RussTedrake(platform) (waiting on @EricCousineau-TRI, @ggould-tri, @rpoyner-tri, @RussTedrake, and @sammy-tri)

---

common/drake_copyable.h, line 85 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK Ah, gotcha!

Oops, I should have made a unit test for that. Done now.

[jwnimmer-tri]

-@RussTedrake to get this merged before the next release.

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),EricCousineau-TRI(platform),sherm1(platform) (waiting on @jwnimmer-tri)

[sammy-tri]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform),sammy-tri(platform),SeanCurtis-TRI(platform),EricCousineau-TRI(platform),sherm1(platform) (waiting on @jwnimmer-tri)

[jwnimmer-tri]

The macOS build has passed, just waiting for a while to cough up CDash hairball before the lamp goes green. Merging now.