-
Notifications
You must be signed in to change notification settings - Fork 677
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
Experiment: Fallible conversion trait #4060
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #4060 will not alter performanceComparing Summary
|
I've played around with this some more, and added more conversions for experimentation. (I will remove most of the impls again from here, once we reached some consensus, since many of them warrant their own PRs) I have the feeling that an associated type is the more appropriate choice:
For these points I switched the generic for an associated type. The only type that's a bit special is |
I just wanted to comment that although I haven't managed to actually reply or give thought in detail here, I'm really excited for this and want to circle back to it ASAP. I think at this point it might be that this is too big of a change for landing in 0.22, but I'd be keen to see it land in 0.23 if we can make it happen! |
I'm excited too! I hope this can both enable new use cases, that we previously just didn't support, while also reducing the complexity to implement Python conversions. Having a single trait in each direction is hopefully making it much clearer how everything is wired together.
Yes, I agree. Given how close we are to releasing 0.22 I think it makes sense to target 0.23 here 🤞I think there is also some more work following this, I included a lot of new impls in here for demontration purposes, but in general I think it would make sense to land them seperately, so we can discuss them individually and make reviewing easier. So no need to rush 😄 |
allow error type to be convertible into `PyErr` instead of requiring `PyErr`
This implements a proof of concept of a new fallible conversion trait.
Design
IntoPyObject
This introduces a new trait
IntoPyObject
to perform the conversion of implemented type into Python.Currently the trait is generic so that the same type can have multiple conversions to different Python types. In this version I used it to provide an additional typed conversion for theI changed the generic to an associated type as outlined in #4060 (comment)chrono
types when the unlimited API is enabled. There might be more situations where this could be useful.The
Error
is specified as an associated type, since every conversion should have a single applicable error (orInfallible
if it can't fail).The return type is specified to
Bound<T>
to guarantee that we convert to a Python type.IntoPyObjectExtAdditionally to
IntoPyObject
this also addsIntoPyObjectExt
. The sole purpose of this trait is to allow specifying the desired output type using the turbo fish syntax:This makes it much nicer to work with types that implement multiple conversions and need type hinting. This would probably be the API most users would interact with.This is only beneficial in the case of a generic trait.Macros
To use the new conversion on return values of
#[pyfunction]
and friends without breaking current code, a variant ofautorefderef specialization is implemented. It prefers the implementation ofIntoPyObject
overIntoPy<PyObject>
. (Similar to the current approach the conversion to) This means there should be no changes required by users. The macros will pick up the falliblePyAny
needs to be available.IntoPyObject
implementation automatically, if available. For the migration we could then just deprecateIntoPy
(andToPyObject
) and advise to implementIntoPyObject
instead, removing the old conversions in a future release. Additionally there is another layer of specialization for the()
return type in#[pymethods]
and#[pyfunctions]
. This allows converting them intoPyNone
as part of the macro code, because that's the Python equivalent of a function returning "nothing", while still usingPyTuple
as theIntoPyObject::Target
.Open Questions
IntoPyObject
be generic, or would an associated type also work? Maybe implementing it on some more types will give us more insight whether the generic is needed.From my initial implementation I have the feeling that the generic version introduces a lot of complexity while not really providing a big benefit.
IntoPy
andToPyObject
? I assume we need to introduce new variants that are generic overIntoPyObject
, similar to what we did with the_bound
constructors.There are probably some edge cases that don't work correctly, but I thought I'll gather some feedback first whether this goes into the right direction before I chase those down.
Xref #4041