-
Notifications
You must be signed in to change notification settings - Fork 783
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
restrict IntoPyObject::Error
to convert into PyErr
#4489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, thanks! A couple of brief thoughts, which might be better for follow ups...
{ | ||
type Target = PyTuple; | ||
type Output = Bound<'py, Self::Target>; | ||
type Error = PyErr; | ||
|
||
fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> { | ||
Ok(array_into_tuple(py, [$(self.$n.into_pyobject(py)?.into_any().unbind()),+]).into_bound(py)) | ||
Ok(array_into_tuple(py, [$(self.$n.into_pyobject(py).map_err(Into::into)?.into_any().unbind()),+]).into_bound(py)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's value in having a trait like
trait IntoPyObjectExt<'py>: IntoPyObject<'py> {
fn try_into_pyobject(self, py: Python<'py>) -> PyResult<Self::Output>;
fn try_into_pyobject_any(self, py: Python<'py>) -> PyResult<Bound<'py, PyAny>>;
}
... which is sealed and has a single blanket impl for all T: IntoPyObject
.
I don't know if I like those exact names, but the point is there's a lot of repeated patterns which I see us using (and I think users might experience the same).
We could make the trait crate-private in the short term while we figure out what feels good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting thought, we can definitely experiment with that. But this is really only effects generic code. Normal user code can usually just use the ?
, because they get a concrete type and all PyAnyMethods
are accessible via Deref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, though I think we have a lot of generic code internally and I think users probably do write generic code from time to time, so it'd be nice to have a long term solution to make it easier.
Also I wonder if having such methods will help keep generic code bloat down by reusing common chunks, but that's just speculation until measured.
Either way, no need to block merge on this.
cde0e7c
to
4278c9e
Compare
This restricts the error type of
IntoPyObject
to be convertible intoPyErr
. While not strictly necessary this makes writing generic code much nicer, because we don't need to repeat a bound on the associated type all the time. It's also potentially less surprising to users because the error with an incompatible error will occur at implementation site instead of usage site. The usage without this bound are extremely limited, since the macros and nearly all the PyO3 API and containers require it, so it could only be called manually anyway.XRef: #4480 (review)