split PyCell and PyClassObject concepts
#3917
Conversation
davidhewitt
force-pushed
the
pyclass-object
branch
from
e9c4553 to
0b95215
Compare
There was a problem hiding this comment.
Ok, I tidied this up and pushed a third commit where I tried to get rid of a bit more code which was referencing PyCell.
@Icxolu, want to see what you think of this PR? I guess #3916 will conflict with this, as you note, hopefully in general though this PR will just remove a lot of the #[allow(deprcated)] calls which otherwise needed to be added in #3916.
There was a problem hiding this comment.
There are probably some module reorganisations worth considering, like why this is here rather than under the normal impl_ submodule.
But for now I wanted to focus on the actual diff and just relocated code which should be "internal" from src/pycell.rs into here.
| <<T::BaseType as PyClassImpl>::PyClassMutability as GetBorrowChecker<T::BaseType>>::borrow_checker(&cell.ob_base) | ||
| } | ||
| } | ||
|
|
||
| /// Base layout of PyClassObject. |
There was a problem hiding this comment.
This is all basically lifted from src/pycell.rs and renamed.
davidhewitt
force-pushed
the
pyclass-object
branch
from
bc6af44 to
ac28db3
Compare
There was a problem hiding this comment.
This definitely feels right to do. It makes a clean cut between internals and userfacing types and makes deprecating PyCell much nicer. After going through this I think I've also spotted the problem that you mentioned in #3915, good catch.
I found a few style nits, otherwise this looks good to me (but haven't really dealt with this low level CPython interface, so I can't really verify these interops).
I think it makes sense to wait with #3916 until after this, otherwise we would add (and then remove) alot of unnessesary #[allow(deprecated)]
| mem::forget(owned.try_borrow()?); | ||
| Ok(RefGuard(owned.clone().unbind())) | ||
| let bound = obj.downcast::<T>()?; | ||
| bound.get_class_object().borrow_checker().try_borrow()?; |
There was a problem hiding this comment.
Ah, I see the probem now, bound.try_borrow currently clones the Bound, so we leak that ref count increment. I guess this could be changed back, once we can store a &Bound in PyRef(mut), right?
There was a problem hiding this comment.
Yes exactly, and potentially we could change it back later. I wonder whether a better follow-up would be to instead change the way this borrow checker works to return a borrow "token" which then needs to be handed back to the borrow checker later (or otherwise panic / abort on drop, maybe just in debug mode). Then what I've written here would be robust and the justification to change back to forget a PyRef seems weaker.
Co-authored-by: Icxolu <[email protected]>
|
Thanks for the review! I've tidied up for all those points, and to unblock you with #3916 I will merge this now 👍 |
Motivated by #3915 (comment)
I was staring at that problem and it felt like a step in the right direction was to carry out the proposed refactoring to split the
PyCellGIL Ref andPyClassObjectclass object layout concepts. That should make it easier for us to get rid ofPyCelllater by decoupling PyO3's internal implementation from that type.This probably wants a little cleaning up and I'll rebase it after #3915, I just wanted to push it now so that we don't forget this for 0.21.
(The first commit extends a test case to make it fail on
mainbut pass after the cleaning up here.)