-
Notifications
You must be signed in to change notification settings - Fork 776
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.
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
overflow evaluating requirement for &pyo3::Bound<'_, _>: pyo3::IntoPyObject<'_>
#4702
Comments
Interestingly, tweaking to add a "real" implementation of 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>) {} |
... this also makes me wonder, should we do more crazy things like un-deprecate 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 The idea would be that perhaps in 0.24 we would "break" I think I need to sleep on this... |
Maybe we could even make the changes |
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™️
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 It seems like with 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>,
{
} |
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.... |
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 |
It looks like with I will experiment briefly with the blanket idea too, I wonder if I can localise it to |
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 |
Nice trick, you might be able to get rid of the extra GAT by using |
Found while attempting to upgrade
pydantic-core
to test out PyO3main
.Consider the following code from 0.22.
... 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:Unfortunately, this immediately blows up the compiler with a recursion error:
The text was updated successfully, but these errors were encountered: