Make #wrap_pyfunction return a Bound value

[snuderl]

Part of Part of #3684

This is a tehnically a breaking change. The diff is pretty simple though which hopefully means not many users would be impacted in practice. Thoughts?

[codspeed-hq]

CodSpeed Performance Report

Merging #3808 will degrade performances by 12.82%

_{Comparing snuderl:wrap_pyfunction-bound (0e43071) with main (030a618)}

Summary

⚡ 5 improvements
❌ 1 regressions
✅ 73 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	snuderl:wrap_pyfunction-bound	Change
⚡	not_a_list_via_downcast	242.8 ns	215 ns	+12.92%
⚡	mapping_from_dict	327.8 ns	272.2 ns	+20.41%
❌	f64_from_pyobject	377.8 ns	433.3 ns	-12.82%
⚡	sequence_from_list	355.6 ns	300 ns	+18.52%
⚡	extract_str_downcast_fail	266.1 ns	238.3 ns	+11.66%
⚡	extract_int_downcast_fail	266.1 ns	238.3 ns	+11.66%

[Icxolu]

There might be an option to avoid the breakage for PyModule::add_function. I think we could return a "private" wrapper type around Bound<PyCFunction> in _wrap_pyfunction, which implements From conversions for both Bound and gil-ref, and then perform the Into conversion inside the wrap_pyfunction macro.

[davidhewitt]

It would definitely be nice to avoid breaking existing calls to add_function. I wonder if the below idea might work?

@Icxolu The downside I could see to adding some type deduction inside wrap_pyfunction would be that I think existing assignments like this become ambiguous:

let f = wrap_pyfunction!(...)

Maybe there are breakages caused by what I suggest too?

[davidhewitt]

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.

[davidhewitt]

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

[adamreichold]

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)?

[davidhewitt]

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?

[snuderl]

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?

[davidhewitt]

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... 😢

[davidhewitt]

I think we resolved this with new tricks in #3897. Thanks @snuderl for kicking this off and sorry we ended up going in a different direction...