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

Make PyObject a type alias of Py<PyAny> (& remove FromPy) #1063

Merged
merged 4 commits into from Aug 9, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jul 22, 2020

This is two related simplifications of the PyO3 public API:

1. PyObject has been replaced by type PyObject = Py<PyAny>;

Not much to say about this one. I think many of us have wanted to see us make this change for a long time.

To avoid breaking as much code as possible, all methods from PyObject have been naively moved Py<T> for now, though I plan next to clean up and deduplicate methods on Py<T>.

2. FromPy has been removed.

I have been thinking about making this change for a while. In the end, while attempting the PyObject simplification, I realised that there was a fundamental complication caused by FromPy:

impl FromPy<T> for T { ... }

This blanket implementation, central to FromPy's design, conflicts with the following blanket implementation once PyObject is replaced with Py<PyAny>:

impl<T> IntoPy<PyObject> for Py<T> { ... }

Therefore I realised that making change (1) was basically impossible without making alterations to the FromPy trait.

Here are a few reasons why I think making this breaking change (removal of FromPy) is okay:

  • IntoPy is the main trait we use in all PyO3 APIs to describe the "into python" direction of conversions. FromPy, while closely related, is barely used.
  • Previously there were two ways to define the to-python conversion: implement FromPy<T> for PyObject, or implement IntoPy<PyObject> for T. Now there's just one (always implement IntoPy).
  • The name FromPy, to me, implies "conversion from Python to Rust". I don't think I'm the first person who has been confused by this name.
  • While it is a breaking change, migration of FromPy to IntoPy should be easy for most projects.

Because this is a breaking change to the PyO3 API, I'd like to gather feedback from as many people as possible before I continue ahead with updating documentation as well as the migration guide.

I'd like to stress that I overwhelmingly think this simplification is worth it. There are so many types and conversion traits in PyO3 that it's tough for users to understand it all, and hard for us to refactor / improve APIs. This is a minimally breaking removal of one of each, which seems like great opportunity to make the library leaner.


TODO:

  • Update documentation
  • Write migration guide
  • Check all methods transplanted from PyObject onto Py<T> - ensure names are consistent, deprecate duplicates & inconsistent names.
  • (optional) Remove AsPyRef trait with .as_ref() member on Py<T>?

@gilescope
Copy link
Contributor

Will try it out at work this week but all in favour of less types and it being simpler.

@davidhewitt
Copy link
Member Author

As all feedback so far has been in favour of this change, I'm going to wait one more week. If nobody has reasons why we shouldn't merge this by then, I'm going to finish up documentation and aim to merge soon after.

@kngwyu
Copy link
Member

kngwyu commented Aug 1, 2020

I think this change can improve usability a bit, but honestly, I don't know this is so meaningful.
Please choose the way you like.

@birkenfeld
Copy link
Member

I guess it's pretty meh for the old guard who is familiar with all the types already, but IMO will make it much easier for newcomers.

@kngwyu
Copy link
Member

kngwyu commented Aug 1, 2020

but IMO will make it much easier for newcomers.

So it can be much easier for now but anyway I want to remove Py as I commented.

@davidhewitt
Copy link
Member Author

davidhewitt commented Aug 1, 2020

Oh - it wasn't clear to me from your comment there that you wanted to remove Py completely. So your proposal will be that PyAny<'a> replaces what we currently use as &'a PyAny, and PyAnyRaw replaces all of Py<T> and PyObject?

(Maybe best to discuss that in the other thread.)

@kngwyu
Copy link
Member

kngwyu commented Aug 1, 2020

So your proposal will be

Yeah, but you don't have to consider it seriously for this PR. I'm sorry for my off-topic words.

I agree with what you're going to do in this PR, but what I'm not sure is how conversion traits should be changed.

@davidhewitt
Copy link
Member Author

davidhewitt commented Aug 1, 2020

Ok. I agree design improvements for the conversion traits can still be worked out, though I am confident of the opinion that FromPy should not be in the final design. 😄

I actually have some notes on some thinking I did about a (future, would currently be nightly-only) design for the conversion traits which I think is nice. But because it is unstable for now I haven't rushed to write them up. I'll try to do this another time soon.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks.

I was thinking about the FromPy, and now I think it's OK to remove it.
Into and From pair is good in that it compatible with stdlib, but current FromPy is actually not so useful.

src/types/datetime.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Cool! I will rebase this PR and write docs in the near future!

@davidhewitt
Copy link
Member Author

I've now updated the documentation and migration guide on this branch.

I also did the tidy up of Py methods. Mostly I just rearranged them so the docs are in a better order. The only method I marked deprecated in the end is PyObject::from_owned_ptr_or_panic, which is now the same as Py::from_owned_ptr.

If someone is willing to review the documentation for typos and suggestions, then I'd like to get this merged!

@davidhewitt
Copy link
Member Author

(rebased to resolve merge conflict)

@davidhewitt
Copy link
Member Author

(Rebased again to resolve merge conflicts. I'm thinking I'll merge this tonight if nobody raises any objections by then. We can always improve / rewrite the documentation further in future PRs.)

@davidhewitt davidhewitt merged commit bcb9077 into PyO3:master Aug 9, 2020
@davidhewitt davidhewitt deleted the remove-pyobject branch December 24, 2021 02:05
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

4 participants