Don't allow keep_alive or call_guard on properties#5533
Conversation
The def_property family blindly ignore the keep_alive and call_guard arguments passed to them making them confusing to use. This adds a static_assert if either is passed to make it clear it doesn't work. I would prefer this to be a compiler warning but I can't find a way to do that. Is that even possible?
|
(random timing, I'm cleaning up my email backlog) Looks good, thanks! — 'll get back here a few days after #5542 is merged, but before the v3.0.0 release. |
|
Looks like this got missed |
|
Oh, sorry. Could you please update this branch and tag me if you see that the CI passed? |
|
I think the android runner is broken. This is also an issue in #5896 |
|
Ignoring, after two unsuccessful reruns: CIBW / Android wheel ubuntu-latest (pull_request) (pull_request)Failing after 10m |
|
@gentlegiantJGC Thank you so much for the guard rails! Just to check the update path for this: should we use a syntax with |
The only cases this change should break are cases where |
error: static assertion failed: def_property family does not currently support call_guard. Use a py::cpp_function instead. See pybind/pybind11#5533
See also [PR #5533](pybind/pybind11#5533). PiperOrigin-RevId: 886714838 Change-Id: I8c98461098ccf4958fb18bdcb5acc293c988c258
pybind11 prohibits keep_alive inside def_property_readonly. The correct equivalent for 'return reference + keep parent alive' is py::return_value_policy::reference_internal. Fixes google#5140 See: pybind/pybind11#5533
* set install directories depending on target type * set rpath for tests * set rpath for python bindings * set install dirs of the C lib * set install dirs of the fortran lib * update python and library paths in the CI * set c mpi lib install path * fix python binary module install paths and rpaths * update PYTHONPATH for the windows CI * Revert "update PYTHONPATH for the windows CI" This reverts commit 4e452ba. * debug print * remove direct compiler flags specifying the cpp standard * update pybind11 to version 3.0.4 * ignore clangd configuration * pybind11::module => pybind11::module_ * wrap python bindings of iterator ranges into pybind11::cpp_functions (see pybind/pybind11#5533) * explicit template argument for pybind11 holder type * debug * indicate which version of python is used for running tests on rocky * / => \ on windows * install on rocky before running tests * point PYTHONPATH to the install dir on windows * the rocky docker image relies on /etc/bashrc?? * special treatment for pybind modules on windows * kratos' rocky image needs /etc/local/bin, try sourcing /etc/profile instead of /etc/bashrc * sh => bash * give up on sourcing the fucking profile and extend PATH manually * maybe the rocky image got silently updated?w * yes, the rocky image did get silently updated, and python3.8 was replaced with python3.10, instead of installing the dev package for the system's default python3. wow. I'm never going to touch this piece of shit image again. If Altair updates their docker image, it's their job to fix this crap. * drop intel mpi from the CI * trigger CI


Description
The def_property family blindly ignore the keep_alive and call_guard arguments passed to them making them confusing to use.
This adds a static_assert if either is passed to make it clear it doesn't work.
I would prefer this to be a compiler warning but I can't find a way to do that. Is that even possible?
This does not fix - #4236 or #5046 but makes it clear that they don't work.
Workaround
If your property returns a reference, pybind11 will automatically link the lifespan of self to the return value.
See
return_value_policy::reference_internaldoc for more info..def_property("foo", &Obj::getFoo, &Obj::setFoo).def_readwrite("foo", &Obj::foo)If you need a custom
keep_aliveorcall_guardthese can be passed intopy::cpp_function.def_property( "foo", py::cpp_function(&Obj::getFoo, py::call_guard<py::gil_scoped_release>()), py::cpp_function(&Obj::setFoo, py::call_guard<py::gil_scoped_release>()))Suggested changelog entry: