Use return_value_policy::reference for mutable lvalues (#1200).

[EricCousineau-TRI]

This addresses #1200 (and #1231) by ensuring that argument casting is more C++-ish: Do not default to copying for mutable lvalue references.

The problem in the above issues is that in casting a Python function to a C++ function and calling it directly on a C++ object, there's a possibility that the C++ object will have not been seen by pybind. Because of this, the prior policy was to copy the previously unseen object.

This PR only changes function argument casting to alleviate this. It keeps the behavior of cast<>() the same, for reasons stated in test_methods_and_attributes.py unittest.

\cc @uentity @benEnsta

[jagerman]

This seems like a backwards-compatibility-breaking change, which makes it a non-starter for 2.x.

Regarding #1200, I'll comment in that issue.

[EricCousineau-TRI]

This seems like a backwards-compatibility-breaking change, which makes it a non-starter for 2.x.

I'd say it's a potential breaking change. However, this behavior was not captured by any of the other unittests (at least as seen on my system), and required zero changes in those unittests to make them pass.

TBH, I couldn't imagine a scenario where this is desired functionality, and I would deem the current behavior as a bug. Copying completely makes sense for const lvalue (no modification) and rvalue (terminal lifetime implicit ownership transfer), but not for mutable lvalues.

[wjakob]

Agreed with @jagerman. I don't think we'll want to change something so fundamental about return value policies (and I quite frankly don't see the benefit here).

[EricCousineau-TRI]

Gotcha, thank y'all for looking at it!

Regarding value, I personally have value in it in that (a) it is closer to C++'s behavior and seems expected (since I and at least two others were surprised by it) and (b) the alternative is to manually annotate input arguments (per @jagerman's comment in #1200), which seems like this might grow rather verbose.

If the route for (b) were taken and I wanted to eventually abandon our fork, I'd probably end up writing wrappers around all overloads for def, def_static, cpp_function, etc., to automatically infer these flags (because, at least at this time of writing, I'd never want the original behavior), which seems like a lot more work than the 6 lines of code added to the codebase to realize the change.

If backwards-incompatibility is the main issue, is it something that could be considered for 3.x?
(Is there a roadmap or something for that?)

[jagerman]

The biggest reason for defaulting to copy is that it's a safe operation (from a Python caller's perspective), while reference isn't, and we generally try to make potentially unsafe things opt-in. Requiring you to be explicit with reference is essentially making sure you take that extra step to say "yeah, I know this is potentially unsafe, but I know what I'm doing." For example:

class Element { /* ... */ };
class HasElement {
private:
    int i_;
public:
    int &get() { return i_; }
};

In C++, I need to know at the point of calling whether the returned value can be referenced or not. If I do it wrong, I get into trouble:

    Element &e = HasElement().get(); // bad
    Element e = HasElement().get(); // No problem (invokes a copy)

Now in the Python interface for something like this, you'd normally bind it with reference_internal, so that the equivalent becomes safe:

    e = HasElement().get() // Safe; e holds a reference to the temporary

or you make sure the Python code only ever gets references that can't be deleted.

As an Python __call__ argument (which is really where both these issues are coming up via std::function), there's a similar non-safety that can come up with you try to use reference with an argument that has temporary lifetime: namely that Python code might try to keep a reference, which might not be valid. For example:

def set_element(e):
    global last_element
    last_element = e
foo(set_element)

combined with:

m.def("foo", [](py::function f) { f(HasElement().get()); });

Since right now the default is automatic_reference, that ends up copying. You can tell it to reference with:

m.def("foo", [](py::function f) { f<py::return_value_policy::reference>(HasElement().get()); });
// (I suspect that probably needs a `template` and/or `operator ()` to actually compile)

but again, that's only something you do if you know that the returned lvalue reference is safe.

TL;DR: I do think we should let a reference policy be specified for how std::function casts its arguments; it's mainly a matter of how to make it work nicely.

[EricCousineau-TRI]

I don't think I agree with you, as it seems like you're wanting to by-default redirect / break a C++ mechanism (mutable lvalue references) with pybind (defaulting to copy) rather than maintain it (reference) or augment it (reference_internal). (By break, I mean that you're not guaranteed to be modifying the instance you were passed for callback arguments, since they not may be registered in pybind, yadda yadda.).

I see what you mean by your example for a callback [](py::function f) { f(HasElement().get()); }, where a user may invoke a callback which is impure, and maintains a reference to a soon-to-be-invalidated temporary.

However, that seems like a very unusual edge case: the callback-invoking method uses a temporary value in C++, calls a method directly using py::function which is directly passed the return of a method returning a lvalue reference.

What I would see as more usual is that the user accepts a callback from Python, but then casts it to a std::function<...>, and then passes that to another, pure C++ callsite, where that callsite may have the temporary lvalue reference because, hey, it's pure C++. In this case, if the callback does not intend on modifying the value, then argument will be T or const T&, and then all the copies will be had on the pybind side, no harm no foul.
But, if the callback does modify the value, then it will be the same exact problem: The developer will have to explicitly dictate that they want a reference in the py::cpp_function, a downstream user will pass an impure callback that stores a global reference, and they'll be back at square one with the same segfault (with extra work on top of that). In this case, there is no saving grace: The value's a temporary, so it will always go out of scope (no keep_alive option), but it must be modified to be valid for C++. (If it were made a return value from the callback, then that'd be better; or if it were a shared_ptr, that'd work too.)
(Additionally, if there is a segfault (at some point), it may be clearer that something has gone awry with referencing, rather than running the code to find out that your binding has not worked as expected. May that's just the masochist in me speaking, though...)

(As a minor aside, wouldn't int& be copied anyways, since it's a primitive / built-in pybind type? I know it's not super relevant to the discussion as it's a conceptual example, but I actually don't know.)

All that being said, ultimately a smaller workaround for me would be to wrap whatever methods, and replace any mutable rvalue references (arguments and return values) with pointers (possibly with a non-null check), as that'd be the behavior I'd want.

Thank you for the explanations and taking time to respond - sorry for the stubborness, just throwing in my 2 cents!

[EricCousineau-TRI]

Oh goodness, I feel rather silly. After focusing on the issue posted here, I realized my problem was actually this and one more thing: the default behavior of return_value_policy::copy for const lvalue references, for non-copyable, non-movable classes.

I will post more about this in a follow-up issue.

That being said, I still stand by my original statements. However, they now have much less value to me, so I'd say I'd be fine with closing this PR :)