Skip to content
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

convert &'py PyList -> PyList<'py> #3253

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

This is a branch to continue the conversation from #3205 (comment)

It adds an (internal for now) PyAnyOwned<'py>, which could eventually become PyAny<'py>, and attempts to gauge fallout by replacing &'py PyList with PyList<'py> implemented on top of PyAnyOwned<'py>.

Because of the scale of the churn, I think it's more realistic to implement migration of each type in individual PRs. I think a single branch and PR to rebuild the entirety of PyO3 would be unreviewable. Probably &PyAny needs to be migrated first, although I want us to understand what changes look like for the core collection types (list, dict, tuple) before we commit to this road for definite.

Doesn't compile yet because a lot of the conversion traits aren't implemented properly for PyList<'py>. I might try to refine further later if we think this is worth pursuing.

Already we see the following migration efforts:

  • Ownership changes (not a suprise)
  • Proliferation of from_owned_ptr methods etc.
    • These can probably be refactored into a trait or a macro
  • List index operators have to be removed if the pool is removed, because the index operators can't return owned values
    • This would be true of any other solution which removes the pool.
  • Addition of the lifetime to PyList<'py> has interesting consequences for is_instance and similar generic methods.
    • x.is_instance_of::<PyList<'_>>()
    • x.downcast::<PyList<'_>>()

Of these, I think only the fourth bullet needs further consideration. If users don't have the elided_lifetimes_in_paths lint enabled, then I believe they can continue writing x.is_instance_of<PyList>() without the lifetime. However, the papercut is still notable. I think this would just have to be noted as part of syntactical drawbacks which we could describe as things which would go away if arbitrary_self_types stabilised (any PyO3 migrated to it).

@adamreichold
Copy link
Member

Probably &PyAny needs to be migrated first, although I want us to understand what changes look like for the core collection types (list, dict, tuple) before we commit to this road for definite.

One thing that I would like to state explicitly to make sure that know everyone's assumptions: In my personal experience writing application-level code using PyO3, I have rarely interacted directly with native types like PyList, PyDict, etc. Rather, I have usually interacted with them only indirectly via the FromPyObject impls for Vec, HashMap, etc.

IMO, this implies that even if there is churn in their interfaces, it does not affect the majority of application-level code. But rather library and performance-critical code, the owners of which should be in a good position to fix that churn. (Of course, the still won't like us for making them do those changes. But the performance gains of removing the pool should be pretty persuasive to that crowd.)


/// Used by `PyList::iter()`.
pub struct PyListIterator<'a> {
list: &'a PyList,
pub struct PyListIterator<'py> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is not a #[pyclass], wouldn't the double indirection &'a PyList<'py> be closer to the usual Rust pattern for iterators borrowing the container? I think the iterator could still yield PyAny<'py> items though?

@davidhewitt
Copy link
Member Author

Closing in favour of #3361

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants