Rename PyClassShell with `PyCell` and do mutability checking by kngwyu · Pull Request #770 · PyO3/pyo3

kngwyu · 2020-02-16T15:27:00Z

TODO

Fix documents
Fix documentation in th guide
Add documentation about interior mutability.
Now protocol implementations uses try_borrow_unguarded so fix them to use try_borrow
Add documents to all new APIs
Rethink Py::new
- I just marked it as soft-duplicated
Add tests for inheriting classes with weakref/dict slots

TL; DR

Now we can take multiple mutable references of PyClass.
This PR fixes it by runtime reference counting like RefCell.
We can use PyCell like RefCell. We can get PyRef and PyRefMut from &PyCell, with interior mutability checking.
The implementation is done via overhauling our PyClass and PyClassShell, resulting in renaming PyClassShell with PyCell.

Notation

BorrowFlag it the reference counter that PyCell has. It can also represent mutably borrowed.

Implementation

So, what's the problem of implementing mutability checking for PyClasses?
The answer is inheritation.
Our PyClass can inherit other class by #[pyclass(extends=OtherClass)] syntax.
In such cases, we should share BorrowFlag between child and parent classes.
So the following object layout is required:

pub struct PyCell {
    ob_base: ffi::PyObject,
    borrow_flag: BorrowFlag,
    value1: ParentClass,
    value2: ChildClass,
}

It is somewhat difficult since PyCell is defined recursively using the layout of base object, but I resolved this by using

#[repr(C)]
pub struct PyCellBase<T: PyTypeInfo> {
    ob_base: T::Layout,
    borrow_flag: Cell<BorrowFlag>,
}

as the farthest baseclasses for all PyClasses.
This needs some helper types like derive_utils::PyBaseTypeUtils.

Changed APIs

PyTryFrom: Now returns &PyCell<T> instead of &T and &mut T
FromPyPointer: Same as above.
AsPyRef: Now has an associated type Target. Returns &PyCell<T> from Py<T>.
(TODO: make the complete list)

New APIs

PyRef
PyRefMut
(hidden) PyDowncastImpl
(TODO: make the complete list)

davidhewitt · 2020-02-16T15:29:46Z

Awesome! There's a lot here and I'll try to give it all a thorough review, though this will take me a few days...

davidhewitt

This is awesome stuff, really excited to see this! I've given this a really deep read and asked a lot of questions - hopefully useful comments.

In such cases, we should share BorrowFlag between child and parent classes.

Can you explain in a bit more detail why this has to be the case? I would think that as the Rust objects are separate, it should be safe to have &mut T::Base and &T simultaneously.

src/type_object.rs

src/class/macros.rs

guide/src/class.md

src/derive_utils.rs

src/instance.rs

src/objectprotocol.rs

davidhewitt · 2020-02-17T19:27:34Z

src/pycell.rs

+#[repr(C)]
+pub struct PyCellBase<T: PyTypeInfo> {
+    ob_base: T::Layout,
+    borrow_flag: Cell<BorrowFlag>,


Is it worth making an assertion that this doesn't need to be thread-safe (i.e. not AtomicXyz) because the of the GIL?

This is already not Send nor Sync.
What kind of assertion do you have in mind?

Even if it's not Send or Sync in Rust, the python interpreter will happily share this data between threads. Perhaps just a comment explaining why Cell is enough?

Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

kngwyu · 2020-02-18T05:04:45Z

Thank you for the lots of reviews!
Enjoying addressing them 😃

Can you explain in a bit more detail why this has to be the case? I would think that as the Rust objects are separate, it should be safe to have &mut T::Base and &T simultaneously.

If we allow having PyRefMut<T> and PyRefMut<T::BaseType> safely, get_super APIs should be (roughly) as follows:

impl<T> PyCell<T> {
    pub fn get_super(&self) -> &PyCell<T::BaseType> { ... }
}
impl<'p, T> PyRef<'p, T>
{
    pub fn get_super(&self) ->  &PyCell<T::BaseType> { ... }
}
...

I think this has some downsides:

Simply not convenient. I feel PyRefMut<T> -> PyRefMut<T::BaseType> style API is easier to use.
If T::BaseType have weakref or dict flag we can't return PyCell<T::BaseType>(just an implementation detail, but can be hard to fix)

davidhewitt · 2020-02-18T08:37:08Z

Ah, that makes sense, thanks. Let's see what the gitter thread prefers. I'm warming to your design. It would be strange to try to .super() an object and then get a .PyCell.

src/types/mod.rs

davidhewitt · 2020-02-18T09:25:37Z

An idea: what if we had a type PyBorrowed<'a, T> ?

Then the API for get_super could be something like

impl<T> PyBorrowed<`_, T> {
    fn get_super(&self) -> PyBorrowed<T::BaseLayout>
}

Along with:

PyBorrowed<T> derefs to T
PyRef<T> derefs to PyBorrowed<T>
PyBorrowed<PyDict> etc. may also be useful for Why object creation is slow in PyO3 and how to enhance it #679, if we make PyDict<'py, T> an owned pointer.

I wrote this in a rush, sorry if it doesn't make much sense, or it has other problems!

kngwyu · 2020-02-22T06:07:38Z

An idea: what if we had a type PyBorrowed<'a, T> ?

Sounds good for the future design, but I don't think we should add it now.

davidhewitt · 2020-02-25T22:48:14Z

Excited to see all the progress on this. I'm hoping to find time to give this another review in the next day or two. 👍

birkenfeld · 2020-02-26T11:45:41Z

I can't contribute at the moment, but just wanted to say that this is very cool. Making the API safe is very much appreciated!

kngwyu · 2020-02-28T10:11:08Z

@Alexander-N
Could you please take a glance at class.md and documents for PyCell?
Thanks.

davidhewitt

I've managed to give this another read through. Really pleased with how this is shaping up!

Given the similarity between PyCellBase and RefCell, I wonder if we could simplify the code and make PyCell a wrapper around RefCell rather than implementing the unsafe primitives ourselves. I appreciate the way this is laid out it might be hard, but in my opinion if we can do it and reduce the amount of unsafe non-ffi code in pyo3, it's worth it.

CHANGELOG.md

guide/src/class.md

pyo3-derive-backend/src/module.rs

src/instance.rs

src/pycell.rs

src/pyclass.rs

src/type_object.rs

Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

kngwyu · 2020-03-01T04:04:09Z

@davidhewitt
Thank you for lots of reviews 😃

I wonder if we could simplify the code and make PyCell a wrapper around RefCell

RefCell<T> is a pair (T, BorrowFlag).
However, to enable extends= feature, PyCell can be a triplet (Base, Sub, BorrowFlag) or larger tuple. This nature conceptually makes it difficult to use RefCell here.

davidhewitt

Few small tidy-ups still possible; in general this is looking great to me.

RefCell is a pair (T, BorrowFlag).
However, to enable extends= feature, PyCell can be a triplet (Base, Sub, BorrowFlag) or larger tuple. This nature conceptually makes it difficult to use RefCell here.

That makes total sense. Let's keep it as-is.

Alexander-N

This seems like a great step, very excited to see this! At the moment I can't look at it in detail, but I tried to at least give some feedback, hope it helps.

Alexander-N · 2020-03-01T17:47:38Z

guide/src/class.md

-you can use `PyClassShell<T>`.
-Or you can use `Py<T>` directly, for *not-GIL-bounded* references.
+`PyCell<T: PyClass>` is always allocated in the Python heap, so we don't have the ownership of it.
+We can get `&PyCell<T>`, not `PyCell<T>`.


Suggested change

We can get `&PyCell<T>`, not `PyCell<T>`.

We can only get `&PyCell<T>`, not `PyCell<T>`.

Alexander-N · 2020-03-01T17:49:41Z

guide/src/class.md

 But for more deeply nested inheritance, you have to return `PyClassInitializer<T>`
 explicitly.

+To get a parent class from child, use `PyRef<T>` instead of `&self`,


Suggested change

To get a parent class from child, use `PyRef<T>` instead of `&self`,

To get a parent class from a child, use `PyRef<T>` instead of `&self`,

Alexander-N · 2020-03-01T18:02:13Z

guide/src/class.md

-   fn method2(self_: &PyClassShell<Self>) -> PyResult<usize> {
-      self_.get_super().method().map(|x| x * self_.val2)
+   fn method2(self_: PyRef<Self>) -> PyResult<usize> {
+       let super_ = self_.as_ref();  // Get &BaseClass


For me this is surprising, while get_super was very clear.

It was initially named as_super but changed to use AsRef since wasm-bindgen does so, after some discussions in gitter.
I prefer to as_super but, in the long run, it would be beneficial to use the same API convention with a major library.

Alexander-N · 2020-03-01T18:02:44Z

guide/src/class.md


 To access the super class, you can use either of these two ways:
- Use `self_: &PyClassShell<Self>` instead of `self`, and call `get_super()`
+- Use `self_: &PyCell<Self>` instead of `self`, and call `get_super()`


I find it a bit confusing that only into_super was used in the example.

Oops, this was left unchanged... thanks

Alexander-N · 2020-03-01T18:04:18Z

guide/src/class.md

+- Use `self_: &PyCell<Self>` instead of `self`, and call `get_super()`
 - `ObjectProtocol::get_base`
-We recommend `PyClassShell` here, since it makes the context much clearer.
+We recommend `PyCell` here, since it makes the context much clearer.


Maybe we can simply remove get_base since I remember there was a problem with this method #381

Alexander-N · 2020-03-01T18:09:11Z

guide/src/class.md

 It includes two methods `__iter__` and `__next__`:
-  * `fn __iter__(slf: &mut PyClassShell<Self>) -> PyResult<impl IntoPy<PyObject>>`
-  * `fn __next__(slf: &mut PyClassShell<Self>) -> PyResult<Option<impl IntoPy<PyObject>>>`
+  * `fn __iter__(slf: PyRefMut<Self>) -> PyResult<impl IntoPy<PyObject>>`


I liked that PyRefMut was replaced with PyClassShell since I find PyRef/PyRefMut unintuitive for reasons mentioned in #356

Alexander-N · 2020-03-01T18:10:24Z

pyo3-derive-backend/src/method.rs

-    PySelf(syn::TypeReference),
+    // self_: &PyCell<Self>,
+    PySelfRef(syn::TypeReference),
+    // self_: PyRef<Self> or PyRefMut<Self>


Forgotten comments?

They are comments for developers.
I rewrote them as doc comments.

kngwyu · 2020-03-02T04:08:58Z

@Alexander-N
Thank you for reviews!

kngwyu added 7 commits February 8, 2020 15:05

Rename PyClassShell with PyCell

a2408ca

Introduce PyDownCastImpl and Change PyTryFrom and FromPyPointer

5d4e737

Prototype Implementation of RefCell-like PyCell

c43fb9f

New AsPyRef

4a5f219

Fix PyCell to share BorrowFlag with parents

da4ed39

Fix PySelf and AsPyRef

8f8b425

Merge branch 'master' into pycell

2fd2185

Update class.md and Change super API

daca04e

kngwyu force-pushed the pycell branch from bd4082a to daca04e Compare February 17, 2020 15:20

davidhewitt reviewed Feb 17, 2020

View reviewed changes

Apply suggestions from davidhewitt's review

98d810e

Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

kngwyu force-pushed the pycell branch from 1da2601 to 5e28f7c Compare February 18, 2020 03:53

Rename unchecked_refmut -> unchecked_mut

4b746af

kngwyu force-pushed the pycell branch from 5e28f7c to 4b746af Compare February 18, 2020 03:55

Add tests for mutability checking

1f5cb83

kngwyu marked this pull request as ready for review February 18, 2020 08:24

davidhewitt reviewed Feb 18, 2020

View reviewed changes

src/types/mod.rs Outdated Show resolved Hide resolved

kngwyu mentioned this pull request Feb 21, 2020

Buffer protocol updates to support object references, custom release method #772

Merged

Use AsRef/AsMut instead of as_super/as_super_mut

3d0ee2a

kngwyu added 2 commits February 22, 2020 16:21

Write docs for PyCell, PyRef, PyRefMut

043b130

Modify CallbackConverter so that it can deal with try_borrow well

c2a40fb

kngwyu force-pushed the pycell branch from b0b8245 to c2a40fb Compare February 22, 2020 11:01

kngwyu added 3 commits February 22, 2020 20:26

Remove all usages of unguarded

d3d61c6

More documents for PyCell

0e3f7cb

Use PyBorrowFlagLayout to ensure the baseclass has a borrow flag

68a3b15

kngwyu added 6 commits February 25, 2020 19:56

More documents for AsPyRef and PyRef

9bc4192

Add tests for inheriting class with dict or weakref

1895715

Merge branch 'master' into pycell

6c652df

Refactor set_item

bab146a

Fix class.md

ff1ae98

Update CHANGELOG

0515941

kngwyu added 2 commits February 26, 2020 17:10

Address clippy warnings

6a64806

Simplify PyLayout

dcac5ad

kngwyu added this to the 0.9.0 milestone Feb 27, 2020

More docs

461b32a

kngwyu requested a review from Alexander-N February 28, 2020 10:09

davidhewitt reviewed Feb 28, 2020

View reviewed changes

kngwyu and others added 2 commits March 1, 2020 12:43

Apply suggestions from code review

399e4bf

Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Address review comments

ca6227c

davidhewitt approved these changes Mar 1, 2020

View reviewed changes

davidhewitt mentioned this pull request Mar 1, 2020

Simplify FromPyPointer #781

Closed

Alexander-N approved these changes Mar 1, 2020

View reviewed changes

Remove ObjectProtocol::get_base and fix class.md

ee0c178

kngwyu merged commit fee755a into PyO3:master Mar 2, 2020

Alexander-N mentioned this pull request Mar 2, 2020

Inheritance example does not compile #381

Closed

birkenfeld mentioned this pull request Mar 3, 2020

Porting to 0.9 after #770 #785

Closed

This was referenced Mar 3, 2020

A safe API for upcasting #787

Open

Unsoundness: &T and &mut T to same object #749

Closed

davidhewitt mentioned this pull request May 3, 2020

A soundness issue with GIL-bound references #687

Closed

davidhewitt mentioned this pull request Jul 18, 2025

introduce PyClassGuard(Mut) as future replacement for PyRef(Mut) #5233

Merged

	We can get `&PyCell<T>`, not `PyCell<T>`.
	We can only get `&PyCell<T>`, not `PyCell<T>`.

	To get a parent class from child, use `PyRef<T>` instead of `&self`,
	To get a parent class from a child, use `PyRef<T>` instead of `&self`,

Conversation

kngwyu commented Feb 16, 2020 • edited by davidhewitt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

TL; DR

Notation

Implementation

Changed APIs

New APIs

Uh oh!

davidhewitt commented Feb 16, 2020

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu commented Feb 18, 2020

Uh oh!

davidhewitt commented Feb 18, 2020

Uh oh!

Uh oh!

davidhewitt commented Feb 18, 2020

Uh oh!

kngwyu commented Feb 22, 2020

Uh oh!

davidhewitt commented Feb 25, 2020

Uh oh!

birkenfeld commented Feb 26, 2020

Uh oh!

kngwyu commented Feb 28, 2020

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kngwyu commented Mar 1, 2020

Uh oh!

davidhewitt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alexander-N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

kngwyu commented Feb 16, 2020 •

edited by davidhewitt

Loading

kngwyu Feb 18, 2020 •

edited

Loading

davidhewitt left a comment •

edited

Loading

kngwyu Mar 2, 2020 •

edited

Loading

kngwyu Mar 2, 2020 •

edited

Loading

Alexander-N Mar 1, 2020 •

edited

Loading

kngwyu Mar 2, 2020 •

edited

Loading