Skip to content
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.

Sign up for GitHub

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

Make #wrap_pyfunction return a Bound value #3808

Closed
wants to merge 1 commit into from
+11 −7
Closed

Make #wrap_pyfunction return a Bound value #3808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/impl_/pyfunction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::{derive_utils::PyFunctionArguments, types::PyCFunction, PyResult};

pub use crate::impl_::pymethods::PyMethodDef;
use crate::Bound;
use crate::{derive_utils::PyFunctionArguments, types::PyCFunction, PyResult};

pub fn _wrap_pyfunction<'a>(
method_def: &PyMethodDef,
py_or_module: impl Into<PyFunctionArguments<'a>>,
) -> PyResult<&'a PyCFunction> {
PyCFunction::internal_new(method_def, py_or_module.into()).map(|x| x.into_gil_ref())
) -> PyResult<Bound<'a, PyCFunction>> {
PyCFunction::internal_new(method_def, py_or_module.into())
}
1 change: 1 addition & 0 deletions src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ impl GILOnceCell<Py<PyType>> {
/// ```
/// use pyo3::intern;
/// # use pyo3::{pyfunction, types::PyDict, wrap_pyfunction, PyResult, Python};
/// use crate::pyo3::prelude::PyAnyMethods;
///
/// #[pyfunction]
/// fn create_dict(py: Python<'_>) -> PyResult<&PyDict> {
Expand Down
2 changes: 1 addition & 1 deletion src/types/module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ impl PyModule {
///
/// [1]: crate::prelude::pyfunction
/// [2]: crate::wrap_pyfunction
pub fn add_function<'a>(&'a self, fun: &'a PyCFunction) -> PyResult<()> {
pub fn add_function<'a>(&'a self, fun: Bound<'a, PyCFunction>) -> PyResult<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here we could just take fun: impl ToPyObject? That would avoid breaking existing direct calls, and we plan to remove this function anyway.

We could add a note to document the strange generic signature is for backwards compatibility while this API is on its way out.

Might make the implementation a little messier, but I'm sure it could work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, by changing this function I think we need a 3808.changed.md newsfragment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here we could just take fun: impl ToPyObject? That would avoid breaking existing direct calls, and we plan to remove this function anyway.

impl trait in argument position is a bit of a smell for library API because callers have a hard time defining the implicit type argument (only via casts and and such, potentially requiring a additional closure around the call). But adding a generic argument would strictly speaking not be backwards compatible.

I am not sure yet why we do not just have add_function and add_function_bound (or add_bound_function)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is what do to with wrap_pyfunction!, and whether we want to change it.

At the moment we have:

#[pymodule]
fn my_module(py: Python, m: &PyModule) {
    m.add_function(wrap_pyfunction!(foo, m)?)?;
}

If we don't change wrap_pyfunction, and go for wrap_pyfunction_bound, then we end up with this:

#[pymodule]
fn my_module(py: Python, m: &Bound<PyModule>) {
    m.add_function(wrap_pyfunction_bound!(foo, m)?)?;
}

Which creates a bigger diff the more functions the user has. I guess it's not too bad, though.

If we change wrap_pyfunction! return type like like we changed intern!, then the only thing users would need to change is the module type:

#[pymodule]
fn my_module(py: Python, m: &Bound<PyModule>) {
    m.add_function(wrap_pyfunction!(foo, m)?)?;
}

... but maybe we're making this more complicated than it needs to be, and we should just add wrap_pyfunction_bound? I guess unlike intern!, most wrap_pyfunction! calls will all be located in one place in the #[pymodule]. So it's not a very hard transformation.

... @snuderl what do you think of this? I guess adding wrap_pyfunction_bound! creates a bigger diff, but maybe it's not too bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah adding wrap_pyfunction_bound! should be pretty trivial it just hopped it could be avoided :)

There are 174 usages in the PyO3 repo, I guess we want to switch all of them right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, probably adding wrap_pyfunction_bound gets very messy without the second commit I have in #3744, which allowed &Bound<PyModule> in #[pymodule]. Because I think we would want to have have wrap_pyfunction_bound! used in #[pymodule] which use &Bound<PyModule>, and then maybe we keep just one test which uses the deprecated &PyModule to

I really need to tidy that PR up and get it ready for review... 😢

self.as_borrowed().add_function(&fun.as_borrowed())
}
}
Expand Down
5 changes: 4 additions & 1 deletion tests/test_coroutine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ fn test_coroutine_qualname() {
assert coro.__name__ == name and coro.__qualname__ == qualname
"#;
let locals = [
("my_fn", wrap_pyfunction!(my_fn, gil).unwrap().deref()),
(
"my_fn",
wrap_pyfunction!(my_fn, gil).unwrap().into_gil_ref().deref(),
),
("MyClass", gil.get_type::<MyClass>()),
]
.into_py_dict(gil);
Expand Down
2 changes: 1 addition & 1 deletion tests/test_wrap_pyfunction_deduction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use pyo3::{prelude::*, types::PyCFunction};
#[pyfunction]
fn f() {}

pub fn add_wrapped(wrapper: &impl Fn(Python<'_>) -> PyResult<&PyCFunction>) {
pub fn add_wrapped(wrapper: &impl Fn(Python<'_>) -> PyResult<Bound<'_, PyCFunction>>) {
let _ = wrapper;
}

Expand Down
Loading