add second lifetime to FromPyObject

[Icxolu]

This adds the second lifetime to FromPyObject and removes FromPyObjectBound. I hope I adjusted all the bounds correctly. I believe the equivalent to our current implementation is use HRTB for the input lifetime on any generic bound. But this should only be necessary for containers that create temporary Python references during extraction. For wrapper types it should be possible to just forward the relaxed bound.

For easier trait bounds FromPyObjectOwned is introduced. This is blanket implemented for any FromPyObject type that does not depend on the input lifetime. It is intended to be used as a trait bound, the idea is inspired by serde (Deserialize <=> DeserializeOwned)

I tried to document different cases in the migration guide, but it can probably still be extended. Changelog entry is still missing, will write that tomorrow.

Breaking changes:

• the second lifetime
• the extract method without default
• the addition lifetime bound on extract_bound to make the default work

[mejrs]

I propose that we add a FromPyObjectOwned trait similar to serde's DeserializeOwned. It's much easier to explain "just use this trait if the output doesn't borrow from Python" rather than "you need higher ranked trait bounds".

[Icxolu]

Thanks! I think that's a great idea. It's essentially the same thing, but it's much easier to grasp by giving it a distinct name, plus we can better document it in the API docs. I will adapt this to make use of that.

[davidhewitt]

Thanks for pushing on with this! Overall it looks good to me, and I only have a few docs suggestions.

Before we definitely commit to this, I want to take a brief moment just to reflect on the choice of Borrowed over &Bound. There's at least two good technical reasons, which is the lifetime constraint and performance by avoiding pointer-to-pointer. The codspeed branch does show a few slight perf improvements in the ~4% range on tuple extraction and we have further possibilities like PyRef containing Borrowed once we do this.

That said, we deliberately kept the Borrowed smart pointer out of the way as much as possible in the original 0.21 design to try to keep new concepts down. Is it fine to increase its use? I think the upsides justify this, though we might want to increase the quality of documentation and examples on Borrowed (which I'd left quite minimal so far).

[davidhewitt]

It will be an interesting question for later if we ever support the 'a lifetime in our #[derive(FromPyObject)]. Definitely not for now 😂

[Icxolu]

Yeah, something to experiment with in the future, but out of scope for now for sure 😅

[Icxolu]

Thanks for the review! You are right, the choice of Borrowed over &Bound is definitely something to discuss about. I used it here because it was the final form of FromPyObjectBound that we landed on. Looking back, the change to Borrowed happened in #3959, which was related to the gil-refs API, so it might not be strictly necessary anymore (haven't checked). The points you mentioned seem like good justification why it still makes sense to introduce additional complexity here.

I think the upsides justify this, though we might want to increase the quality of documentation and examples on Borrowed (which I'd left quite minimal so far).

I agree, if we put Borrowed in such a core part of the infrastructure it deserves a documentation overhaul. We might also want to take a look at the current (public) API and additionally expose some helpers. Something like to_any comes to mind, which is currently internal only.

[mejrs]

IMO the recently added FromPyObject documentation is too "implementation detail"-y.

I'd prefer if this documentation is kept short and concise, one paragraph max.

For example:

Depending on the python version and implementation some `FromPyObject`
 implementations may produce Rust types that point into the Python type.
For example `PyString` and `PyBytes` may convert into `str` and `[u8]` references
 that borrow from the original Python object.

Types that do not borrow from the input should use `FromPyObjectOwned` instead.

[Icxolu]

Thanks for the feedback! I took another pass over it and tried to make it a bit more concise. I do however think that it's worth to talk about the individual lifetimes at play here, since this trait is right in the center of PyO3. I moved the paragraph about collections into the FromPyObjectOwned docs, because it works much better there with the code example, and replaced it with a smaller note for more info. I reworded my examples a bit and moved them under a # Details section, maybe this works better?

Let me know what you think about this.

[davidhewitt]

Thanks very much for keeping this branch alive, I guess let's move forward with this before it gets too painful to keep updating.

I had yet another reflection on &Bound vs Borrowed and I'm feeling reasonably comfortable that Borrowed is the correct choice.

[davidhewitt]

Just for the record and the decision to stick with Borrowed, the reason is that if we used &'a Bound here for the input argument, then tuple extraction would be forced to restrict elements to FromPyObjectOwned.

That maybe wouldn't be a terrible thing, as this is currently a special case when compared with every other container, which is forced to use FromPyObjectOwned due to their mutability.

But I think it would be a breaking change, so it's probably not a good change overall. (E.g. users would lose the ability to extract tuples containing &str, &Bound and Borrowed)

I think as a secondary effect, by using Borrowed here we might be able to change extract_argument.rs to make more use of Borrowed, which IIRC is a possible performance win.

[Icxolu]

I believe this now also finishes the _bound methods deprecations 🎉

[davidhewitt]

Fantastic, thank you so much for your help on pushing that over the line. When I get some time I'll try to write up a list of what we might have left for 0.23 (hopefully not much).

[davidhewitt]

Ah, I just started playing with this locally and found one additional finding: having Borrowed<'a, 'py, T> as the input type means that it's impossible to implement FromPyObject<'a, 'py> for &'a Bound<'py, T> - because the input borrowed can't be cast into a &'a Bound<'py, T> for a sufficient lifetime (pointer goes in, pointer-to-pointer goes out).

Is that a problem? I think not, because that's what .downcast() is for (and it's potentially nice to separate them like this), however just worth a brief double-check that others agree.

[davidhewitt]

Oh, and one more case which comes up in local testing. At the moment from_py_with extractors take &Bound<'py, PyAny>. Should we be considering changing those to Borrowed too, for consistency?

[davidhewitt]

Should we be considering changing those to Borrowed too, for consistency?

It feels to me like we might want to consider macro tricks such that users can use both &Bound and Borrowed.

[Icxolu]

having Borrowed<'a, 'py, T> as the input type means that it's impossible to implement FromPyObject<'a, 'py> for &'a Bound<'py, T>

That's true, but as you mention below, I actually think this a good thing and makes a clearer use case between .extract() and .downcast().

Oh, and one more case which comes up in local testing. At the moment from_py_with extractors take &Bound<'py, PyAny>. Should we be considering changing those to Borrowed too, for consistency?

Hmm, good question. I guess that is part of the question about how prominent we want Borrowed to be. Providing some flexibility via macro magic as you suggested sounds like a good compromise. I can explore that as a followup.

[davidhewitt]

Yes agreed, it's definitely a follow-up to make changes to expand from_py_with.

I wonder, are there any other options we have here? E.g. does it work to have a trait with both extract and extract,_borroeed, maybe with a default implementation of extract,_borrowed? I suspect that might not work because of the same reason we can't currently have &Bound as an output.

I am still feeling a little uneasy about the choice to commit Borrowed more directly into the face of users. I think it is probably the right choice still, as long as we improve the documentation and API on borrowed. Maybe I will quickly try to get the perf wins out of extract_argument.rs later today to add another data point.

[davidhewitt]

And (sorry to keep dangling ideas on here), inspired by the new IntoPyObject, I have two further perf related thoughts while we are making breaking changes here:

• In the past we have noted that the PyResult return type forced conversion of errors to PyErr and made .extract() a performance footgun compared to .downcast(). There is still this problem for FromPyObject for Bound<T>. Should we add a type Error in the same way we are adding for IntoPyObject?
• It is possible to call .extract() on e.g. Bound<PyString> to get a String, but this comes at the cost of a redundant Python type check because FromPyObject always works on PyAny. Could we add a type Source, and add a way to make Bound<T> directly offer extract() where U: FromPyObject<Source = T>?

Both changes would significantly complicate the trait. I wonder, is there a way that we can support a simple trait like FromPyObject and a more advanced FromPyObjectImpl, which has a blanket from FromPyObject, so users can just implement the simple trait if that's good enough?

All food for thought and I think worth discussing while we're committing to breaking changes here.

[Icxolu]

And (sorry to keep dangling ideas on here), inspired by the new IntoPyObject, I have two further perf related thoughts while we are making breaking changes here:

No problem 😄

• In the past we have noted that the PyResult return type forced conversion of errors to PyErr and made .extract() a performance footgun compared to .downcast(). There is still this problem for FromPyObject for Bound<T>. Should we add a type Error in the same way we are adding for IntoPyObject?

If we're willing to do more breaking changes here, that sounds like a good idea to me. The only downsides I can see

• sadly we can't provide a default type
• similar to IntoPyObject we would want FromPyObject::Error: Into<PyErr>, so this has the same effect on generic code and may tie into the same proposal you raised in restrict IntoPyObject::Error to convert into PyErr #4489 (comment)

• It is possible to call .extract() on e.g. Bound<PyString> to get a String, but this comes at the cost of a redundant Python type check because FromPyObject always works on PyAny. Could we add a type Source, and add a way to make Bound<T> directly offer extract() where U: FromPyObject<Source = T>?

This would be really cool indeed. I guess we would maybe somehow need to take advantage of method precedence between trait and inherent methods, to make the fall through to PyAny work...

[davidhewitt]

Thanks for all the work here, I think this is pretty much ready to go! I had a few minor comments, mostly I think documentation related.

I guess there are a few follow-ups we should probably throw into issues so we don't forget to complete for 0.27:

• Better documentation on borrowed
• We should explore if we can make some more error types to delay PyErr creation, e.g. on the set extraction it looks like the error is essentially a downcast error or the key extraction error. I guess there's a tradeoff here of API complexity vs performance.
• I think the functions in extract_argument.rs probably can take Borrowed rather than &Bound but I think that's probably going to involve macro changes which would preferably not be a part of this diff.

[davidhewitt]

I wonder if we should have a section to this doc "Implementing FromPyObject" which lists the choices available:

• #[derive(FromPyObject)]
• manual implementation for types without a lifetime (i.e. impl FromPyObject<'_, '_> for X)
• manual implementation for types with a lifetime

[Icxolu]

Good idea, I added a section with these. Do you have a suggestion for an example type with lifetimes?

[davidhewitt]

Looking great!

For the type with lifetimes, I suggest using either &'a str or Bound<'py, T> and saying "this is roughly how this is implemented in PyO3", I think just choosing one of the two is good enough.

[Icxolu]

Done 👍

[davidhewitt]

Oh, and I think there's a note in the guide about performance of .cast() vs .extract() which is presumably not true once we've landed this change?

[Icxolu]

Thanks for the review ❤️ . I've opened some sub-issues in #5390 to track various followups from here.

[davidhewitt]

Subject to final documentation fixups, let's merge!

Thank you for all the iteration and R&D done here 🚀

[davidhewitt]

Looking great!

For the type with lifetimes, I suggest using either &'a str or Bound<'py, T> and saying "this is roughly how this is implemented in PyO3", I think just choosing one of the two is good enough.

[Icxolu]

Great, all green 🎉 I'm going to send this now. We can handle any touchups that are left in followups.