[py systems] Adjust scalar conversion to avoid unique_ptr

[jwnimmer-tri]

Towards #21968.

• Requires [pydrake] Factor out make_shared_ptr_from_py_object for reuse #22478.
• Requires [framework] Fix diagram scalar conversion losing external constraints #22490.

---

This change is 

[jwnimmer-tri]

+@rpoyner-tri for feature review, please.

My aim is to hit Sherm for platform review (on Thursday during his on call, or else next week as a special assignment).

[rpoyner-tri]

my head hurts.

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, needs at least two assigned reviewers

---

systems/framework/system_base.h line 1102 at r1 (raw file):

  /** (Internal use only) Declares that `parent_service` is the service
  interface of the Diagram that owns this subsystem. Throws if the parent
  service has already been set and parent_service is non-null. */

nit maybe add the back ticks here, since the distinction between the internal state and the argument is a bit subtle in English text.

Suggestion:

`parent_service`

---

systems/framework/test/wrapped_system_test.cc line 16 at r1 (raw file):

using symbolic::Expression;

GTEST_TEST(WrappedSystemTset, Basic) {

typo

Suggestion:

WrappedSystemTest

---

systems/framework/test/wrapped_system_test.cc line 23 at r1 (raw file):

  EXPECT_EQ(dut.num_input_ports(), 2);
  EXPECT_EQ(dut.num_output_ports(), 1);
  EXPECT_TRUE(dynamic_cast<const Adder<double>*>(&dut.unwrap()));

nit I don't know why I find this weird (implicit bool conversion for nullptr check). Did we go through a fashion of forbidding this and then later give up on it?

---

systems/framework/diagram.cc line 1395 at r1 (raw file):

    // WrappedSystem was originally injected into this control flow.
    if (auto* wrapped =
            dynamic_cast<internal::WrappedSystem<NewType>*>(new_system.get())) {

nit I don't know why I find this weird (implicit bool conversion for nullptr check). Did we go through a fashion of forbidding this and then later give up on it?

---

systems/framework/diagram.cc line 1399 at r1 (raw file):

      const std::string name = new_system->get_name();
      DRAKE_DEMAND(wrapped->registered_systems_.size() == 1);
      std::shared_ptr<System<NewType>> inner = wrapped->registered_systems_[0];

nit It seems a little strange to me that this "extract" surgery is not a method on WrappedSystem, but I guess we only do it once? 🤷

[jwnimmer-tri]

+@sherm1 for platform review per schedule (tomorrow), please. Also please check as feature-expert for systems/framework stuff.

... my head hurts.

I did have a whole "Yo, dawg" meme post ready to go, but I decided it was probably a bridge too far.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform)

---

systems/framework/diagram.cc line 1395 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit I don't know why I find this weird (implicit bool conversion for nullptr check). Did we go through a fashion of forbidding this and then later give up on it?

Yes, we disprefer if (foo) { by itself, preferring if (foo != nullptr) to clarify that we're branching on null instead of some other magic.

I've not been extending that to if-with-new-named-variables but maybe we should? I've done that here now.

---

systems/framework/diagram.cc line 1399 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit It seems a little strange to me that this "extract" surgery is not a method on WrappedSystem, but I guess we only do it once? 🤷

The logic and invariants here (the registered systems and the parent service) seem to me more strongly coupled with the internals of Diagram than the internals of WrappedSystem.

[rpoyner-tri]

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)

[rpoyner-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)

[sherm1]

Whew! My hair fell out. I'm glad I didn't have to feature review the Python stuff, thanks Rico.
Platform

Reviewed 10 of 18 files at r1, 7 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

---

systems/framework/wrapped_system.h line 20 at r3 (raw file):

@tparam_default_scalar */
template <typename T>
class WrappedSystem final : public Diagram<T> {

BTW implementing as a 1-system Diagram is clever, but please consider adding some documentation here explaining that since it is an unusual twist.

[rpoyner-tri]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform)