convert &'py PyList -> Py2<'py, PyList>

[davidhewitt]

This is a kind of "what if" experiment to run with the idea of a GIL<'py, PyAny> smart pointer as discussed in #3205 (comment)

Here I've called it Py2<'py, PyAny> instead of GIL<'py, PyAny> because ultimately I think if we go down this route it should be called Py<'py, PyAny> and we should rename the existing "gil-free" Py to something like PyUngil or PyDetached.

The main challenge with these Py2 smart-pointer APIs as noted in #3205 (comment) is that without arbitrary_self_types we cannot have inherent methods for T which accept &Py2<T> as the receiver.

The way I propose we would workaround this problem is by putting the type's methods behind a trait, and then including that trait in pyo3::prelude. For example, there is PyAny and impl PyAnyMethods for Py2<'py, PyAny>. This PR also adds PyList and impl PyListMethods for Py2<'py, PyAny>. While this split looks a bit unusual at first, it's actually quite easy to document and a repeatable convention that downstream crates like rust_numpy could also follow (potentially with their own rust_numpy::prelude if they wanted to bundle all their trades).

This also has the great property that I expect a future PyO3 with an MSRV with stable arbitrary_self_types to require no changes to user code, because we'd just fold the traits into inherent methods and delete the trait imports from the prelude.

I'm quite pumped about this possibility for a pool-free API. Even better, I think because this doesn't change the fundamental types at all (PyAny, PyDict, PyList etc all continue to exist with no definition changes) I think it's possible we could ship a pyo3-pool crate with its own pyo3_pool::prelude and macro compatibility e.g. #[pyfunction(crate = "pyo3_pool")] for users to use during their upgrade cycle to get fully backwards-compatible behaviour while removing the pool from the main API.

Personally, I now like this significantly more than the corresponding experiment in #3253 with PyList<'py>.

[alex]

Maybe a dumb question, but why do we need the traits, why isn't impl<'py> Py2<'p, PyList> sufficient? e.g. https://rust.godbolt.org/z/brnx3vYn5

[adamreichold]

Maybe a dumb question, but why do we need the traits, why isn't impl<'py> Py2<'p, PyList> sufficient? e.g. https://rust.godbolt.org/z/brnx3vYn5

IIRC, the main issue is the documentation. All methods of all types would end up in the one documentation page of Py2.

[alex]

Ah yeah, that'd be painful.

[adamreichold]

The trait appears to be a nice trick to work around the documentation issues of inherent implementations on Py2<'py, PyList>. Maybe discoverability could be improved using the #[doc(alias = "PyList")] feature of Rustdoc?

Two things I wonder though: Is there a cost in compile time for this additional indirection? Also, should we keep the traits open or seal them to be able to make additional assumptions in their definition and/or implementation?

[alex]

Personally I think sealing them makes sense, seems like only trouble could be had by users implementing them.

[davidhewitt]

Added a commit to push doc aliases, that seems like a good idea and easy to implement.

Regarding the further questions:

• For the additional indirection: I think although it's likely to be a more complex lookup than inherent methods, the is hopefully negligible. We use trait methods like .to_string() and .clone() all over Rust code so I think if it was not efficient to resolve these names there would be wide recognition of a problem. Or was there a different concern you had wrt compile times?
• Sealing these traits: yes I think it cannot hurt and can always be reversed later more easily than sealing later.

[davidhewitt]

Yes the documentation was my primary concern with just doing inherent methods on Py2. Also I think numpy and other downstream crates would not be able to implement further inherent methods on Py2. That's a lesser concern, because they could always do tricks like these traits here, but it felt more fair if PyO3 itself is implemented with a pattern which downstream can mirror.

Overall what are folks' reactions to this? Does this seem better or worse to you compared to PyList<'py>?

These seem like the two main API options to me if we want to commit to removing the pool. There could be something we've so far missed, though I've been mulling on this problem for a while. So until proven that there's a third (better) way, the choice to me seems to be between Py2<'py, PyList> or PyList<'py>. For reasons written above I think I prefer Py2<'py, PyList> (bikeshed name).

[alex]

I don't have a strong feeling about Py2<'py, PyList>/Unbound<PyList> vs. PyList<'py>/Unbound<PyList<'static>>, which I understand to be the choices at play. I'm happy for either of them.

[adamreichold]

I think that not having to explain the 'static lifetime in Py<PyList<'static>> and having a syntax-compatible migration path towards arbitrary_self_types makes this the strictly superior solution and we should pursue this instead.

From an implementor's perspective, the additional indirection via the traits does feel weird and it does still add some friction to the consumption of the documentation, but I from the most important point of view, that of our users, this is so far the best solution among those we discussed.

(I would strongly argue for sealing and take a few minutes to think whether this can introduce issues with type inference and/or method resolution though. I cannot think of any right now, but it seems like a possibility we should at least consider.)

[alex]

Do the traits mean that users will need to import the traits in order to call the methods?

…

On Wed, Aug 2, 2023, 12:33 PM Adam Reichold ***@***.***> wrote: I think that not having to explain the 'static lifetime in Py<PyList<'static>> and having a syntax-compatible migration path towards arbitrary_self_types makes this the strictly superior solution and we should pursue this instead. From an implementor's perspective, the additional indirection via the traits does feel weird and it does still add some friction to the consumption of the documentation, but I from the most important point of view, that of our users, this is so far the best solution among those we discussed. (I would strongly argue for sealing and take a few minutes to think whether this can introduce issues with type inference and/or method resolution though. I cannot think of any right now, but it seems like a possibility we should at least consider.) — Reply to this email directly, view it on GitHub <#3361 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBETJZP5GM2QECFC47DXTJ6NRANCNFSM6AAAAAA27JCGKU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[adamreichold]

Do the traits mean that users will need to import the traits in order to
call the methods?

As David suggested above, we put those traits into our prelude and so users continue as they do now, they import e.g. use pyo3::prelude::*; use pyo3::types::PyList; and the traits are always "primed", i.e. ready to be applied as soon as the underlying type is used.

People like me who have to avoid glob imports for religious reasons will need to make a few more imports, but we already made that choice and twice as many type names does not change the big-O growth so that's fine. 🤪

[alex]

Nice to know I'm not alone in pathological aversion to glob imports :-)

At any rate, you've convinced me that this approach makes sense.

[adamreichold]

I think this PR is by now superseded by the more comprehensive Py2 rework or do we want to recycle this for PyListMethods? I'd say otherwise it can be closed.

[davidhewitt]

Yes this was a good test balloon, but let's close this one now. I'll try to push a refreshed one tomorrow.