pydrake AcrobotPlant.get_mutable_state makes a copy

[jwnimmer-tri]

The pydrake function AcrobotPlant.get_mutable_state makes a copy, instead of returning a pointer. This means it's not mutating the context value!

Example failure:

from pydrake.examples.acrobot import (AcrobotPlant) 
acrobot = AcrobotPlant()   
context = acrobot.CreateDefaultContext() 
state = acrobot.get_mutable_state(context)
state.set_theta1(1.)
state.set_theta1dot(2.)
state.set_theta2(3.)
state.set_theta2dot(4.)
print(acrobot.get_mutable_state(context))

Printing out the id() of the repeated calls to get_mutable_state shows it differing; should be the same.

Work-around:

from pydrake.examples.acrobot import (AcrobotPlant, AcrobotState)
acrobot = AcrobotPlant()   
context = acrobot.CreateDefaultContext() 
print(acrobot.get_state(context))

new_state = AcrobotState()
new_state.set_theta1(1.)
new_state.set_theta1dot(2.)
new_state.set_theta2(3.)
new_state.set_theta2dot(4.)
context.get_mutable_continuous_state().SetFromVector(new_state.value())

print(acrobot.get_state(context))

[ggould-tri]

FYI at first glance I think pendulum is likely to have the same issue.

[jwnimmer-tri]

Yup! One of things I want us to do here is to figure out how to fix the whole category of these bugs.

Hopefully in C++ bindings affordances that fail at compile-time, but at worst we need to unit test every reference-returning function that:

assertEqual(id(dut.get_mutable_anything()), id(dut.get_mutable_anything())

[jwnimmer-tri]

@EricCousineau-TRI do you have any suggestions on strategies to use here, to try to find this category of bug before users reach it?

[EricCousineau-TRI]

Acutely: Unittests should have used mutation and observed the changes though alternate path. Doing id() checks would do it as well.

Unclear what ask is via assignment, though. I'll file acute fix, but will not perform sweeping analysis / fixes anytime soon.

[jwnimmer-tri]

The issue assignment was merely per https://drake.mit.edu/issues.html#component for triage.

Asking developers to remember to unit test this all the time seems like a tall hill?

I wonder how practical it would be to disable return_value_policy::automatic and instead require that every non-void function always writes out a specific policy. That would at least get the semantics onto the screen during review, in which case I bet very few of these bugs would escape us. If get_mutable_foo was annotated py_rvp::copy, I think we'd notice that immediately.

[EricCousineau-TRI]

Gotcha! (fwiw the assignment removal was me fat-fingering alt+tab - soz for that noise)

Asking developers to remember to unit test this all the time seems like a tall hill?

If it's just an identity check, maybe not?

I wonder how practical it would be to disable return_value_policy::automatic [...]

Perhaps! That, or maybe fixing #11531 if this is a bug of sorts.

Re: the bug itself, I'm observing some oddities in 7d1279f061 Something smells weird. Will look more into it on Monday.

[jwnimmer-tri]

Asking developers to remember to unit test this all the time seems like a tall hill?

If it's just an identity check, maybe not?

Indeed, the test code would be small and simple; it's the "remembering to think about it" part that I'm worried about. I worry about having too many rules without any tooling to help us succeed in following them.