-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Demo: return_value_policy::copy
ignored
#4591
base: master
Are you sure you want to change the base?
Conversation
…:reference_internal` already created a Python object. Back-ported from pywrapcc test_class_sh_property_stl.cpp,py
Hi @wjakob, what do you think about the existing behavior? It's not very likely to be a problem in real life, but if it bites, it could be very surprising and extremely difficult to debug, especially in a large system. See
With those lines swapped the test fails:
|
I think (but have not verified) that the existing behavior goes back to this pybind11/include/pybind11/detail/type_caster_base.h Lines 524 to 526 in 5bbcba5
pre-empting this pybind11/include/pybind11/detail/type_caster_base.h Lines 546 to 562 in 5bbcba5
|
Hi @rwgk — I agree, this is super-dangerous and something I did not think of when coding up the original logic a long time ago. I introduced a change here in nanobind by always prioritizing This goes deeper by the way: what about the other return value policies, when returning an object that is already registered with the binding tool? All of them are currently ignored.
I don't do anything special for these other cases in nanobind, because I wasn't 100% sure what to do about them. It would be be good to decide on a way of resolving all of these situations, and then implementing & documenting them consistently in both binding tools. |
Thanks Wenzel! That's very useful background information.
What I usually do in this situation is experimenting: run global testing with different ideas. Reviewing the results usually gives me a much more certainty than going with just thinking and guessing. It might take me a while to get back here. The problem didn't come up repeatedly, and we're down to a very tiny fraction of failing tests, therefore I came to look at it as a fringe case. But it would definitely be good to get rid of this trap. If you or anyone wants to try things out (changes in pybind11), please let me know. I can offer running global tests for experimentation. It's not a lot of work for me (only for the machines). |
Description
Demo:
return_value_policy::copy
ignored after a Python object was created already withreturn_value_policy::reference_internal
. Back-ported from pywrapcc test_class_sh_property_stl.cpp,pySuggested changelog entry: