overflow evaluating requirement for `&pyo3::Bound<'_, _>: pyo3::IntoPyObject<'_>`

[davidhewitt]

Found while attempting to upgrade pydantic-core to test out PyO3 main.

Consider the following code from 0.22.

use pyo3::prelude::*;

pub trait Foo<'py>: ToPyObject {}

fn takes_foo<'py>(foo: impl Foo<'py>) {}

... this compiles fine, and is a useful pattern. Unfortunately, the upgrade path for this seems difficult in our current 0.23 draft.

Given the ToPyObject super trait (i.e. by-ref conversion), the simple thing I tried is:

use pyo3::prelude::*;

pub trait Foo<'py>
where
    for<'a> &'a Self: IntoPyObject<'py>,
{
}

fn takes_foo<'py>(foo: impl Foo<'py>) {}

Unfortunately, this immediately blows up the compiler with a recursion error:

error[E0275]: overflow evaluating the requirement `&pyo3::Bound<'_, _>: pyo3::IntoPyObject<'_>`
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`pyo3_scratch`)
  = note: required for `&&pyo3::Bound<'_, _>` to implement `pyo3::IntoPyObject<'_>`
  = note: 126 redundant requirements hidden
  = note: required for `&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&...` to implement `for<'a> pyo3::IntoPyObject<'py>`
  = note: the full name for the type has been written to '/Users/david/Dev/pyo3-scratch/target/debug/deps/pyo3_scratch.long-type-2493199481127865716.txt'
  = note: consider using `--verbose` to print the full type name to the console

[davidhewitt]

Interestingly, tweaking to add a "real" implementation of IntoPyObjectRef fixes the problem, though I think I would need to add a method to the IntoPyObjectRef trait (maybe into_pyobject_ref) to actually be able to access the conversion:

use pyo3::prelude::*;

pub trait IntoPyObjectRef<'py> {}

impl<'a, 'py, T: 'a> IntoPyObjectRef<'py> for T where &'a T: IntoPyObject<'py> {}

pub trait Foo<'py>: IntoPyObjectRef<'py> {}

pub fn takes_foo<'py>(foo: impl Foo<'py>) {}

[davidhewitt]

... this also makes me wonder, should we do more crazy things like un-deprecate ToPyObject? We could instead maybe have this blanket:

impl<T> IntoPyObject<'py> for &'_ T where T: ToPyObject { /* details */ }

The justification would be that if we need to have a real trait to be usable as a super-trait for "I can be converted by-reference to Python" like the IntoPyObjectRef above, ToPyObject already fills this need. If we ended up with the final pair being ToPyObject and IntoPyObject, that seems ok to me.

The idea would be that perhaps in 0.24 we would "break" ToPyObject to make it fallible etc.

I think I need to sleep on this...

[davidhewitt]

Maybe we could even make the changes ToPyObject immediately in 0.23. I think we'd know what we'd like the final state to be and it would avoid a weird intermediate state. It probably wouldn't be much different to whatever we're going to have to ask users to upgrade through when we change FromPyObject...

[Icxolu]

Uff, that seems tricky indeed. This seems like the correct approach to me. To be honest I think the error here is a compiler limitation and this should just work™️

use pyo3::prelude::*;

pub trait Foo<'py>
where
    for<'a> &'a Self: IntoPyObject<'py>,
{
}

fn takes_foo<'py>(foo: impl Foo<'py>) {}

It seems a bit unfortunate if we need to stick with two traits for this, but I agree that this looks like a useful pattern.

If we need to provide a blanket, my first thought would be to use the first one

impl<'a, 'py, T: 'a> IntoPyObjectRef<'py> for T where &'a T: IntoPyObject<'py> {}

and have IntoPyObjectRef (or ToPyObject if we want to reuse that name) Sealed, so that IntoPyObject is the primary tool for conversion and IntoPyObjectRef is "just" the workaround to make it usable for now.

It seems like with -Znext-solver this starts to work, we just need to repeat the bound on the impl, because only bounds with a literal Self on their left side are currently implied:

pub trait Foo<'py>
where
    for<'a> &'a Self: IntoPyObject<'py>,
{
}

fn takes_foo<'py, F>(foo: F)
where
    F: Foo<'py>,
    for<'a> &'a F: IntoPyObject<'py>,
{
}

[Icxolu]

Actually that last example also works on stable, so maybe the compiler just has a really awful error message to tell us to repeat the bound....

[davidhewitt]

Ah, great insights! Yes, perhaps in that case we can avoid doing anything rash here and keep things as-is for 0.23

I'll try to proceed with the pydantic-core update today and hopefully find a pattern that's good enough while we learn about next refinements 👍

[davidhewitt]

It looks like with -Znext-solver it is possible to at least see where the problematic uses of impl Trait are (maybe ~100 of them, so it's going to be a chunky diff), so that makes me feel more comfortable about this given that the result should hopefully then compile on stable. Using nightly as a one-off workaround to get feedback during painful upgrades seems acceptable, albeit unfortunate.

I will experiment briefly with the blanket idea too, I wonder if I can localise it to pydantic-core to learn without needing to impose changes here.

[davidhewitt]

Well, I found a different solution, which makes it more convenient to upgrade.

I transformed the trait from

pub trait Foo<'py>: ToPyObject {}

into

pub trait Foo<'py> {
    /// Convert this `Foo` to a Python object, by-reference
    #[inline]
    fn to_object<'a>(&'a self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
        Ok(self
            .py_converter()
            .into_pyobject(py)
            .map_err(Into::into)?
            .into_bound()
            .into_any())
    }

    /// GAT which allows conversion into Python objects, this is expected to be `&Self`
    type PyConverter<'a>: IntoPyObject<'py>
    where
        Self: 'a;

    /// Helper method to prepare for conversion into a Python object
    fn py_converter(&self) -> Self::PyConverter<'_>;
}

This made it so that callsites using impl Foo just needed to switch .to_object() calls to .to_object()?.unbind(), and everything then pretty much worked as it did before.

[Icxolu]

Nice trick, you might be able to get rid of the extra GAT by using fn py_converter(&self) -> impl IntoPyObject<'py>; if your using 1.75+