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

Add lifetime to PyAny etc #885

Closed
wants to merge 11 commits into from
Closed

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented May 1, 2020

This is a working implementation of PyAny<'a>.

By working - I mean tests pass on my machine (except doctests). I think the code is still heavily WIP, because I ended up making a few hacks to make things compile. I wanted to get to a point where we could have something to play with to assess ergonomics, and how it feels versus the new proposal in #679.

Some initial observations:

Owned, Borrowed and Lifetimes

  • Owned pointers are now obviously owned in docs - they are PyAny<'py>
  • Borrowed pointers are also very straightforward - they are &'a PyAny<'py>
    • The lifetime 'a is the lifetime of the PyAny we're borrowing from.

Because there are now more input lifetimes to functions, there are a few more cases where Rust asks for lifetime annotations. We might want to make the recommendation that users return Py<T> owned pointers always to avoid lifetime complexity.

Reference counting

As well as ownership above, incrementing and decrementing reference counts is done by PyAny::clone() and PyAny::drop() respectively.

PyAny deref

After making PyAny<'py>, I also was able to make e.g.

struct PyDict<'py>(PyAny<'py>);

This means that all native types now implement deref to PyAny. I think this provides an opportunity for us to remove ObjectProtocol and put all its methods directly on PyAny, which is a nice API simplification.

Py<T>

Py<PyTuple> broke by adding lifetimes, and not even Py<PyTuple<'static>> worked. To solve this problem, I added type markers, which are ZSTs without lifetimes which take on the role of type information for Py. Now you can write.

use pyo3::type_marker::Tuple;

struct Foo {
    t: Py<Tuple>
}

To avoid confusion with PyTuple and Py<Tuple> I think it would be wise to rename Py to PyRc (maybe not right now, but in the near future).

To avoid needing complex lifetime rules in the proc macros, I also had to change it so that you write #[extends=Dict] instead of #[extends=PyDict]. We might be able to undo this change with some careful implementation in the proc macros.

I rather hacked together the type marker idea in general because I wanted to get to a point where things compile. There's a lot of cleanup I think can be done here.

TODO:

  • Needs a lot of documentation updated
  • Benchmarking!
  • Much tidying up:
    • Remove GILPool's owned and borrowed tracking.
    • Can we remove lifetimes from any traits
    • Type marker idea can maybe be cleaned up / merged with pytuple.
    • What other traits become irrelevant in this new system?
    • Rename Py to PyRc?

src/pycell.rs Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented May 1, 2020

Amazing, thank you!

how it feels versus the new proposal in #679.

About this, I'll make a draft implementation. Please wait for a bit.

@davidhewitt
Copy link
Member Author

davidhewitt commented May 5, 2020

I'm going to close this PR; it's a big pile of changes which I think we decided we don't want right now (instead opting for #887 as fast enough for now).

This PR is so big it will fall out of date quickly and become hard to rebase.

Some things I think I learned from this PR:

  • PyAny<'py> interacts really badly with Py<PyAny>, because writing the lifetimes inside Py is strange and doesn't have an easy solution.
  • This PR might be easier in the future when Rust has working Generic Associated Types. I had to add lifetimes to a lot of traits for this PR which I think won't all need to change if we had GATs.
  • Making this change didn't give particularly massive performance boosts (compared to what we already got from New Native Types and Lighter GILPool #887), so for now if we have performance concerns we should focus on the rest of the pyO3 codebase.

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.

None yet

2 participants