update #[derive(FromPyObject)] to use extract_bound

[Icxolu]

In this PR I've converted the FromPyObject proc-macro to output the new extract_bound function. Most of this should be completely invisible to downstream, but one place where this leaks out is the #[pyo3(from_py_with = ...)] attribute.

So leaving this as is, would be a breaking change. I don't really see a good way of abstracting over two function types with different signatures.

I can see two options to avoid the breakage:

1. leave the custom extractor on a gil-ref API for now
2. introduce a new #[pyo3(from_py_with_bound = ...)] attribute

Probably option 2 is preferable, so that we can provide an upgrade path. But maybe there is another option which I don't see right now... 🙃

[adamreichold]

So leaving this as is, would be a breaking change. I don't really see a good way of abstracting over two function types with different signatures.

I think we want to abstract over the return value and are in macro context? So we could use autoref-based specialization like we do for iterator return values and other things? But admittedly I am not a fan of this and would personally prefer option 2.

[davidhewitt]

We might be able to get away without specialization and just use some type inference to wrap the closure at the points where we need it. I was playing with a similar trick for #[pymodule] in the second commit of #3744

[Icxolu]

I think we want to abstract over the return value and are in macro context?

I think we would need to abstract over the whole (user provided) closure type, no?

We might be able to get away without specialization and just use some type inference to wrap the closure at the points where we need it. I was playing with a similar trick for #[pymodule] in the second commit of #3744

I tried something like this, but wrapping closures is always a bit weird, because we can't name them. I also ran into problems with overlapping trait implementations for the conversions. (In principle there could be a single type which implements Fn multiple times, with different signature, i guess)

[Icxolu]

With function pointers we could do something like this:

pub enum Extractor<'a, 'py, T> {
    Bound(fn(&'a Bound<'py, PyAny>) -> PyResult<T>),
    GilRef(fn(&'a PyAny) -> PyResult<T>),
}

impl<'a, 'py, T> From<fn(&'a Bound<'py, PyAny>) -> PyResult<T>> for Extractor<'a, 'py, T> {
    fn from(value: fn(&'a Bound<'py, PyAny>) -> PyResult<T>) -> Self {
        Self::Bound(value)
    }
}

impl<'a, T> From<fn(&'a PyAny) -> PyResult<T>> for Extractor<'a, '_, T> {
    fn from(value: fn(&'a PyAny) -> PyResult<T>) -> Self {
        Self::GilRef(value)
    }
}

impl<'a, 'py, T> Extractor<'a, 'py, T> {
    fn call(self, obj: &'a Bound<'py, PyAny>) -> PyResult<T> {
        match self {
            Extractor::Bound(f) => f(obj),
            Extractor::GilRef(f) => f(obj.as_gil_ref()),
        }
    }
}

And since from_py_with has to be a ExprPath I think all uses have to real functions (or methods) anyway.

[codspeed-hq]

CodSpeed Performance Report

Merging #3828 will degrade performances by 15.02%

_{Comparing Icxolu:frompyobject-macro (90e62bf) with main (1279467)}

Summary

❌ 2 regressions
✅ 77 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	main	Icxolu:frompyobject-macro	Change
❌	list_via_downcast	157.2 ns	185 ns	-15.02%
❌	not_a_list_via_downcast	244.4 ns	272.2 ns	-10.2%

[davidhewitt]

Ah, nicely solved!

And since from_py_with has to be a ExprPath I think all uses have to real functions (or methods) anyway.

I agree with this thinking too.

I think this implementation will probably lead to some painful error messages for users if they get the type of the from_py_with function wrong. I suspect that it'd be hard to do much better and we can remove the Extractor wrapper in the future to get better type hints in errors again 👍