Adding pybind11::cast overload for rvalue references

[YannickJadoul]

py::cast(std::move(move_only_holder));does not seem to work in the current version of pybind11. This PR adds a function template to support this.

I have not figured out where to add tests, but the current test suite seems to pass with this addition.

[EricCousineau-TRI]

Nice!

I did something similar in #1237 (but with a much broader scope - your changes are definitely useful, and could hopefully be merged in well before mine):
https://github.com/pybind/pybind11/pull/1237/files#diff-e0d8df3cdf4b37acbf5e8712fb6ce80cR1720

The tests I modified were here, so perhaps yours can go in this same file?
https://github.com/pybind/pybind11/pull/1237/files#diff-7bb675d9a6bf3ae55b71328911840179R334
Feel free to rip out some of those contents, modifying those tests to be return-value only.

[YannickJadoul]

@EricCousineau-TRI Thanks for the suggestion! However, the test there seems a bit heavy for this PR and I don't want to get you into merge problems afterwards either. So I found a nice spot in the existing tests in test_smart_ptr where I could py::cast a move-only holder type before returning it as py::object to pybind. (After all, the test is mainly to have the call to py::cast with an rvalue reference tested somewhere.)

Next to that, I collapsed the two py::cast variants into a single function template with a universal reference, as that gets rid of the extra SFINAE construct I had before.

[EricCousineau-TRI]

You're welcome, and good find on collapsing the overloads!

[wjakob]

It never occurred to me that this isn't already working. 👍 for the simplicity of the change.

Let's merge this to master and see if folks experience problems (potentially we'll need to roll back.)

[lgritz]

I'm getting build breaks from this commit. I tried to pass a call to a method returning a const Thing& as the argument to py::cast<Thing>(). Did this patch, by changing from cast(const T& value) to cast(T&& value), lose the ability to cast from non-moveable (but copyable) objects?

[lgritz]

I was able to avoid the error messages on my side by changing the call from

x = py::cast<Thing>(y()); // y() returns a const Thing&

to

x = py::cast<Thing>(Thing(y())); // force a copy to a temp object that is lvalue?

[YannickJadoul]

id this patch, by changing from cast(const T& value) to cast(T&& value), lose the ability to cast from non-moveable (but copyable) objects?

Not that I'm aware of, but it's always possible that I overlooked something, of course.
Do you perhaps have some (full) code that reproduces the error? Then I'll immediately check if I can reproduce it and why it fails.

[lgritz]

Let me see if I can generate a failure case in just a few lines of code...

[YannickJadoul]

Ooooh, wait, yes. I see what's going on!

By specifying py::cast<Thing> rather than just py::cast, you disable the type deduction of the template parameter, and the T && becomes a rvalue reference (that can only bind to temporary/std::moved objects), rather than a universal reference (that can bind to everything).

Is there a reason you are writing py::cast<Thing>(y()) rather than py::cast(y())?

@wjakob Is this problematic? I could just revert back to have two overloads (the old one for lvalue references, and a new one only for rvalue references).

[lgritz]

Oh, let me try that. I don't know why I was using cast<Thing>, very possibly it was redundant or just the result of a misunderstanding on my part (maybe years ago).

...

Yes, that seems fine. That's a totally acceptable solution for me. I don't know why I was explicit with the cast previously.

[YannickJadoul]

No problem. Quite an obscure difference, indeed. If more people run into this problem we could decide to revert the commit that unified the two original overloads (see commit history of this PR).

Thanks for reporting!

[bstaletic]

By specifying py::cast rather than just py::cast, you disable the type deduction of the template parameter, and the T && becomes a rvalue reference

In the words of STL "do not help the compiler". The thing @lgritz did is one of the things STL really hammered home as one of those that you do not want to do.

[YannickJadoul]

Alright, thanks @bstaletic! Another vote to keep things as they are, for now, then! :-)