[systems] Add DiagramBuilder overloads for shared_ptr

[jwnimmer-tri]

Follow-up from #14387, and towards #5842 and #21968. We must stop passing unique_ptr as an argument from Python down to C++... passing unique_ptr in an argument of a call from Python into C++ is made possible by Drake-specific hacks to pybind11, and we need to get rid of the hacks.

---

This change is 

[jwnimmer-tri]

+@rpoyner-tri and +@sherm1 for all reviews, please. (Rico with feature hat on pydrake changes, Sherm with feature hat on non-pydrake changes.)

Reviewed 15 of 15 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),sherm1(platform)

[sherm1]

Feature for the non-pybind stuff

Reviewed 10 of 15 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform)

---

systems/framework/test/diagram_builder_test.cc line 135 at r2 (raw file):

}

// Integration test of the Digram <=> System <=> SystemBase interaction when the

typo: Digram -> Diagram

[rpoyner-tri]

Feature pybind with some documentation whingeing.

Reviewed 10 of 15 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 6 unresolved discussions

---

-- commits line 14 at r4:
nit I don't understand this talk of customizing cleanup -- does this represent some (squashed out) intermediate changes that culminated in the aliasing shared_ptr ctor?

---

-- commits line 15 at r4:
typo

Suggestion:

unique_ptr 

---

bindings/pydrake/systems/controllers_py.cc line 192 at r4 (raw file):

                          int plant_input_port_index) {
          return std::make_unique<Class>(
              // The ref_cycle is responsible for object lifetime, so we'll give

minor Having stared at (the many copies of) this comment for a while, I will say I find it misleading. While the ref_cycle plays a role in the overall memory management, the reason we can/must use the aliasing constructor here is a bit more local. We accept the plant (in this case) via alias (reference), so we must not pretend to grant (even shared) ownership to the c++ machinery. All we have is an alias, so that's all that we can pass on.

In the "good old days", we took ownership from the pybind wrapper of plant by custom-fork unique_ptr argument code, so we could pass it to c++.

The fact that the ownership chain ultimately terminates in the ref_cycle is not unimportant, but seems like a side issue.

---

bindings/pydrake/systems/controllers_py.cc line 192 at r4 (raw file):

                          int plant_input_port_index) {
          return std::make_unique<Class>(
              // The ref_cycle is responsible for object lifetime, so we'll give

nit There are 7 copies (I think) of this comment and its accompanying documentation. Is it worth factoring out, just so we can anchor the documentation in one place?

---

systems/framework/test/diagram_builder_test.cc line 135 at r4 (raw file):

}

// Integration test of the Diagram <=> System <=> SystemBase interaction when

nit maybe <==> instead of <=> to avoid confusion with spaceship?

---

systems/framework/diagram_builder.h line 106 at r4 (raw file):

  /// shared_ptr overload.}
  template<class S>
  S* AddSystem(std::unique_ptr<S> system) {

nit Why did we retain a unique_ptr overload here but not elsewhere?

Clearly promoting unique to shared on an argument is not a breaking change (go go implicit conversion ctors).

[jwnimmer-tri]

Reviewed 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions

---

-- commits line 14 at r4:

... intermediate changes ...

Right! In earlier drafts I had deleters that captured the py::object until I realized that existing annotations (ref_cycle or keep_alive) were better.

---

systems/framework/diagram_builder.h line 106 at r4 (raw file):

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

nit Why did we retain a unique_ptr overload here but not elsewhere?

Clearly promoting unique to shared on an argument is not a breaking change (go go implicit conversion ctors).

I was worried about overload resolution.

For all of the other edited functions with overloads, selecting which overload to use is fully determined by the other arguments (either their types like double vs Eigen, or their arity).

In this case, we have the crappy Args... overloads also in play, which I was worried would lead to ambiguity (build errors) under certain conditions.

I'll play around a bit more and see if I can convince myself it's 100% backcompat or not.

---

bindings/pydrake/systems/controllers_py.cc line 192 at r4 (raw file):

All we have is an alias, so that's all that we can pass on.

Not quite true. Inside the lambda we could recover a py::object handle to the plant which would then convey (shared) ownership to us, and then we could capture that owned py::object as the managed object of the shared_ptr<> that we pass down to C++, thus completing the ownership chain.

That would be correctly, but an uncollectable cycle. Because we are is-a Diagram, we must use the ref_cycle to align with how Diagram and DiagramBuilder treat lifetimes. Once we have that in place, then it's safe to not give any more ownership hints down to C++.

So the reasoning is very Diagram-specific.

I'll see if I can find a better phrasing, but suggestions are welcome, too.

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions

---

systems/framework/diagram_builder.h line 106 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I was worried about overload resolution.

For all of the other edited functions with overloads, selecting which overload to use is fully determined by the other arguments (either their types like double vs Eigen, or their arity).

In this case, we have the crappy Args... overloads also in play, which I was worried would lead to ambiguity (build errors) under certain conditions.

I'll play around a bit more and see if I can convince myself it's 100% backcompat or not.

Yup. If we remove the unique_ptr overloads, there are tons of compile errors right out of the gate. There might be a way to use concepts or something to fix the overload ambiguity, but for now I'm okay with leave it alone here.

[jwnimmer-tri]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: 3 unresolved discussions

---

bindings/pydrake/systems/controllers_py.cc line 192 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

All we have is an alias, so that's all that we can pass on.

Not quite true. Inside the lambda we could recover a py::object handle to the plant which would then convey (shared) ownership to us, and then we could capture that owned py::object as the managed object of the shared_ptr<> that we pass down to C++, thus completing the ownership chain.

That would be correctly, but an uncollectable cycle. Because we are is-a Diagram, we must use the ref_cycle to align with how Diagram and DiagramBuilder treat lifetimes. Once we have that in place, then it's safe to not give any more ownership hints down to C++.

So the reasoning is very Diagram-specific.

I'll see if I can find a better phrasing, but suggestions are welcome, too.

Better now?

---

bindings/pydrake/systems/controllers_py.cc line 192 at r4 (raw file):

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

nit There are 7 copies (I think) of this comment and its accompanying documentation. Is it worth factoring out, just so we can anchor the documentation in one place?

Better now?

[rpoyner-tri]

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

---

systems/framework/diagram_builder.h line 106 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yup. If we remove the unique_ptr overloads, there are tons of compile errors right out of the gate. There might be a way to use concepts or something to fix the overload ambiguity, but for now I'm okay with leave it alone here.

Ah, I missed the whole overloads with templates fiesta here. Yup, good enough.