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

Rename PyClassShell with PyCell and do mutability checking #770

Merged
merged 29 commits into from
Mar 2, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a2408ca
Rename PyClassShell with PyCell
kngwyu Feb 3, 2020
5d4e737
Introduce PyDownCastImpl and Change PyTryFrom and FromPyPointer
kngwyu Feb 9, 2020
c43fb9f
Prototype Implementation of RefCell-like PyCell
kngwyu Feb 9, 2020
4a5f219
New AsPyRef
kngwyu Feb 10, 2020
da4ed39
Fix PyCell to share BorrowFlag with parents
kngwyu Feb 15, 2020
8f8b425
Fix PySelf and AsPyRef
kngwyu Feb 15, 2020
2fd2185
Merge branch 'master' into pycell
kngwyu Feb 16, 2020
daca04e
Update class.md and Change super API
kngwyu Feb 17, 2020
98d810e
Apply suggestions from davidhewitt's review
kngwyu Feb 18, 2020
4b746af
Rename unchecked_refmut -> unchecked_mut
kngwyu Feb 18, 2020
1f5cb83
Add tests for mutability checking
kngwyu Feb 18, 2020
3d0ee2a
Use AsRef/AsMut instead of as_super/as_super_mut
kngwyu Feb 21, 2020
043b130
Write docs for PyCell, PyRef, PyRefMut
kngwyu Feb 21, 2020
c2a40fb
Modify CallbackConverter so that it can deal with try_borrow well
kngwyu Feb 22, 2020
d3d61c6
Remove all usages of unguarded
kngwyu Feb 22, 2020
0e3f7cb
More documents for PyCell
kngwyu Feb 22, 2020
68a3b15
Use PyBorrowFlagLayout to ensure the baseclass has a borrow flag
kngwyu Feb 22, 2020
9bc4192
More documents for AsPyRef and PyRef
kngwyu Feb 25, 2020
1895715
Add tests for inheriting class with dict or weakref
kngwyu Feb 25, 2020
6c652df
Merge branch 'master' into pycell
kngwyu Feb 25, 2020
bab146a
Refactor set_item
kngwyu Feb 25, 2020
ff1ae98
Fix class.md
kngwyu Feb 25, 2020
0515941
Update CHANGELOG
kngwyu Feb 25, 2020
6a64806
Address clippy warnings
kngwyu Feb 26, 2020
dcac5ad
Simplify PyLayout
kngwyu Feb 26, 2020
461b32a
More docs
kngwyu Feb 28, 2020
399e4bf
Apply suggestions from code review
kngwyu Mar 1, 2020
ca6227c
Address review comments
kngwyu Mar 1, 2020
ee0c178
Remove ObjectProtocol::get_base and fix class.md
kngwyu Mar 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 65 additions & 44 deletions guide/src/class.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Specifically, the following implementation is generated:

```rust
use pyo3::prelude::*;
use pyo3::types::PyAny;

/// Class for demonstration
struct MyClass {
Expand All @@ -33,9 +34,10 @@ impl pyo3::pyclass::PyClassAlloc for MyClass {}

unsafe impl pyo3::PyTypeInfo for MyClass {
type Type = MyClass;
type BaseType = pyo3::types::PyAny;
type ConcreteLayout = pyo3::PyClassShell<Self>;
type Initializer = pyo3::PyClassInitializer<Self>;
type BaseType = PyAny;
type BaseLayout = pyo3::pycell::PyCellBase<PyAny>;
type Layout = PyCell<Self>;
type Initializer = PyClassInitializer<Self>;

const NAME: &'static str = "MyClass";
const MODULE: Option<&'static str> = None;
Expand All @@ -53,6 +55,7 @@ unsafe impl pyo3::PyTypeInfo for MyClass {
impl pyo3::pyclass::PyClass for MyClass {
type Dict = pyo3::pyclass_slots::PyClassDummySlot;
type WeakRef = pyo3::pyclass_slots::PyClassDummySlot;
type BaseNativeType = PyAny;
}

impl pyo3::IntoPy<PyObject> for MyClass {
Expand Down Expand Up @@ -105,47 +108,58 @@ fn mymodule(_py: Python, m: &PyModule) -> PyResult<()> {
}
```

## Get Python objects from `pyclass`
You sometimes need to convert your `pyclass` into a Python object in Rust code (e.g., for testing it).
## PyCell and interior mutability
You sometimes need to convert your `pyclass` into a Python object and access it
from Rust code (e.g., for testing it).
`PyCell` is our primary interface for that.

For getting *GIL-bounded* (i.e., with `'py` lifetime) references of `pyclass`,
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>`.
Copy link
Member

Choose a reason for hiding this comment

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

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


### `PyClassShell`
`PyClassShell` represents the actual layout of `pyclass` on the Python heap.
Thus, to mutate data behind `&PyCell` safely, we employ the
[Interior Mutability Pattern](https://doc.rust-lang.org/book/ch15-05-interior-mutability.html)
like [std::cell::RefCell](https://doc.rust-lang.org/std/cell/struct.RefCell.html).

If you want to instantiate `pyclass` in Python and get the reference,
you can use `PyClassShell::new_ref` or `PyClassShell::new_mut`.
Users who are familiar with `RefCell` can use `PyCell` just like `RefCell`.

For users who doesn't know `RefCell` well, we repeat the Rust's borrowing rule here:
- At any given time, you can have either (but not both of) one mutable reference or any number of immutable references.
- References must always be valid.
`PyCell` ensures these borrowing rules by managing a reference counter.

TODO: link to the API document

```rust
# use pyo3::prelude::*;
# use pyo3::types::PyDict;
# use pyo3::PyClassShell;
# use pyo3::PyCell;
#[pyclass]
struct MyClass {
#[pyo3(get)]
num: i32,
debug: bool,
}
let gil = Python::acquire_gil();
let py = gil.python();
let obj = PyClassShell::new_ref(py, MyClass { num: 3, debug: true }).unwrap();
// You can use deref
assert_eq!(obj.num, 3);
let dict = PyDict::new(py);
// You can treat a `&PyClassShell` as a normal Python object
dict.set_item("obj", obj).unwrap();

// return &mut PyClassShell<MyClass>
let obj = PyClassShell::new_mut(py, MyClass { num: 3, debug: true }).unwrap();
obj.num = 5;
let obj = PyCell::new(py, MyClass { num: 3, debug: true }).unwrap();
{
let obj_ref = obj.borrow(); // Get PyRef
assert_eq!(obj_ref.num, 3);
// You cannot get PyRefMut unless all PyRefs are dropped
assert!(obj.try_borrow_mut().is_err());
}
{
let mut obj_mut = obj.borrow_mut(); // Get PyRefMut
obj_mut.num = 5;
// You cannot get any other refs until the PyRefMut is dropped
assert!(obj.try_borrow().is_err());
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved
}
// You can convert `&PyCell` to Python object
pyo3::py_run!(py, obj, "assert obj.num == 5")
```

### `Py`

`Py` is an object wrapper which stores an object longer than the GIL lifetime.

You can use it to avoid lifetime problems.
`&PyCell<T>` is bouded by the same lifetime as `GILGuard`.
kngwyu marked this conversation as resolved.
Show resolved Hide resolved
To avoid this you can use `Py<T>`, which stores an object longer than the GIL lifetime.
```rust
# use pyo3::prelude::*;
#[pyclass]
Expand All @@ -159,7 +173,9 @@ fn return_myclass() -> Py<MyClass> {
}
let gil = Python::acquire_gil();
let obj = return_myclass();
assert_eq!(obj.as_ref(gil.python()).num, 1);
let cell = obj.as_ref(gil.python()); // AsPyRef::as_ref returns &PyCell
let obj_ref = cell.borrow(); // Get PyRef<T>
assert_eq!(obj_ref.num, 1);
```

## Customizing the class
Expand Down Expand Up @@ -228,9 +244,12 @@ baseclass of `T`.
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`,
Copy link
Member

Choose a reason for hiding this comment

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

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`,

or `PyRefMut<T>` instead of `&mut self`.

```rust
# use pyo3::prelude::*;
use pyo3::PyClassShell;
use pyo3::PyCell;

#[pyclass]
struct BaseClass {
Expand Down Expand Up @@ -261,8 +280,9 @@ impl SubClass {
(SubClass{ val2: 15}, BaseClass::new())
}

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_super(); // Get &BaseClass
super_.method().map(|x| x * self_.val2)
}
}

Expand All @@ -279,29 +299,30 @@ impl SubSubClass {
.add_subclass(SubSubClass{val3: 20})
}

fn method3(self_: &PyClassShell<Self>) -> PyResult<usize> {
let super_ = self_.get_super();
SubClass::method2(super_).map(|x| x * self_.val3)
fn method3(self_: PyRef<Self>) -> PyResult<usize> {
let v = self_.val3;
let super_ = self_.into_super(); // Get PyRef<SubClass>
SubClass::method2(super_).map(|x| x * v)
}
}


# let gil = Python::acquire_gil();
# let py = gil.python();
# let subsub = pyo3::PyClassShell::new_ref(py, SubSubClass::new()).unwrap();
# let subsub = pyo3::PyCell::new(py, SubSubClass::new()).unwrap();
# pyo3::py_run!(py, subsub, "assert subsub.method3() == 3000")
```

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()`
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@kngwyu kngwyu Mar 2, 2020

Choose a reason for hiding this comment

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

Oops, this was left unchanged... thanks

- `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.
Copy link
Member

@Alexander-N Alexander-N Mar 1, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍



If `SubClass` does not provide a baseclass initialization, the compilation fails.
```compile_fail
# use pyo3::prelude::*;
use pyo3::PyClassShell;
use pyo3::PyCell;

#[pyclass]
struct BaseClass {
Expand Down Expand Up @@ -761,16 +782,16 @@ struct GCTracked {} // Fails because it does not implement PyGCProtocol
Iterators can be defined using the
[`PyIterProtocol`](https://docs.rs/pyo3/latest/pyo3/class/iter/trait.PyIterProtocol.html) trait.
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>>`
Copy link
Member

Choose a reason for hiding this comment

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

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

* `fn __next__(slf: PyRefMut<Self>) -> PyResult<Option<impl IntoPy<PyObject>>>`

Returning `Ok(None)` from `__next__` indicates that that there are no further items.

Example:

```rust
use pyo3::prelude::*;
use pyo3::{PyIterProtocol, PyClassShell};
use pyo3::{PyIterProtocol, PyCell};

#[pyclass]
struct MyIterator {
Expand All @@ -779,10 +800,10 @@ struct MyIterator {

#[pyproto]
impl PyIterProtocol for MyIterator {
fn __iter__(slf: &mut PyClassShell<Self>) -> PyResult<Py<MyIterator>> {
fn __iter__(mut slf: PyRefMut<Self>) -> PyResult<Py<MyIterator>> {
Ok(slf.into())
}
fn __next__(slf: &mut PyClassShell<Self>) -> PyResult<Option<PyObject>> {
fn __next__(mut slf: PyRefMut<Self>) -> PyResult<Option<PyObject>> {
Ok(slf.iter.next())
}
}
Expand Down
4 changes: 2 additions & 2 deletions guide/src/python_from_rust.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ your Python extensions quickly.

```rust
use pyo3::prelude::*;
use pyo3::{PyClassShell, PyObjectProtocol, py_run};
use pyo3::{PyCell, PyObjectProtocol, py_run};
# fn main() {
#[pyclass]
struct UserData {
Expand All @@ -61,7 +61,7 @@ let userdata = UserData {
id: 34,
name: "Yu".to_string(),
};
let userdata = PyClassShell::new_ref(py, userdata).unwrap();
let userdata = PyCell::new(py, userdata).unwrap();
let userdata_as_tuple = (34, "Yu");
py_run!(py, userdata userdata_as_tuple, r#"
assert repr(userdata) == "User Yu(id: 34)"
Expand Down
67 changes: 46 additions & 21 deletions pyo3-derive-backend/src/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@ pub enum FnType {
FnCall,
FnClass,
FnStatic,
PySelf(syn::TypeReference),
// self_: &PyCell<Self>,
PySelfRef(syn::TypeReference),
// self_: PyRef<Self> or PyRefMut<Self>
Copy link
Member

Choose a reason for hiding this comment

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

Forgotten comments?

Copy link
Member Author

@kngwyu kngwyu Mar 2, 2020

Choose a reason for hiding this comment

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

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

PySelfPath(syn::TypePath),
}

#[derive(Clone, PartialEq, Debug)]
pub struct FnSpec<'a> {
pub tp: FnType,
pub self_: Option<bool>,
// Rust function name
pub name: &'a syn::Ident,
// Wrapped python name. This should not have any leading r#.
Expand All @@ -54,6 +58,14 @@ pub fn get_return_info(output: &syn::ReturnType) -> syn::Type {
}

impl<'a> FnSpec<'a> {
/// Generate the code for borrowing self
pub(crate) fn borrow_self(&self) -> TokenStream {
let is_mut = self
.self_
.expect("impl_borrow_self is called for non-self fn");
crate::utils::borrow_self(is_mut, true)
}

/// Parser function signature and function attributes
pub fn parse(
sig: &'a syn::Signature,
Expand All @@ -67,19 +79,19 @@ impl<'a> FnSpec<'a> {
mut python_name,
} = parse_method_attributes(meth_attrs, allow_custom_name)?;

let mut has_self = false;
let mut self_ = None;
let mut arguments = Vec::new();
for input in sig.inputs.iter() {
match input {
syn::FnArg::Receiver(_) => {
has_self = true;
syn::FnArg::Receiver(recv) => {
self_ = Some(recv.mutability.is_some());
}
syn::FnArg::Typed(syn::PatType {
ref pat, ref ty, ..
}) => {
// skip first argument (cls)
if fn_type == FnType::FnClass && !has_self {
has_self = true;
if fn_type == FnType::FnClass && self_.is_none() {
self_ = Some(false);
continue;
}

Expand Down Expand Up @@ -114,18 +126,18 @@ impl<'a> FnSpec<'a> {

let ty = get_return_info(&sig.output);

if fn_type == FnType::Fn && !has_self {
if fn_type == FnType::Fn && self_.is_none() {
if arguments.is_empty() {
return Err(syn::Error::new_spanned(
name,
"Static method needs #[staticmethod] attribute",
));
}
let tp = match arguments.remove(0).ty {
syn::Type::Reference(r) => replace_self(r)?,
fn_type = match arguments.remove(0).ty {
syn::Type::Reference(r) => FnType::PySelfRef(replace_self_in_ref(r)?),
syn::Type::Path(p) => FnType::PySelfPath(replace_self_in_path(p)),
x => return Err(syn::Error::new_spanned(x, "Invalid type as custom self")),
};
fn_type = FnType::PySelf(tp);
}

// "Tweak" getter / setter names: strip off set_ and get_ if needed
Expand Down Expand Up @@ -158,9 +170,11 @@ impl<'a> FnSpec<'a> {
};

let text_signature = match &fn_type {
FnType::Fn | FnType::PySelf(_) | FnType::FnClass | FnType::FnStatic => {
utils::parse_text_signature_attrs(&mut *meth_attrs, name)?
}
FnType::Fn
| FnType::PySelfRef(_)
| FnType::PySelfPath(_)
| FnType::FnClass
| FnType::FnStatic => utils::parse_text_signature_attrs(&mut *meth_attrs, name)?,
FnType::FnNew => parse_erroneous_text_signature(
"text_signature not allowed on __new__; if you want to add a signature on \
__new__, put it on the struct definition instead",
Expand All @@ -174,6 +188,7 @@ impl<'a> FnSpec<'a> {

Ok(FnSpec {
tp: fn_type,
self_,
name,
python_name,
attrs: fn_attrs,
Expand Down Expand Up @@ -514,17 +529,24 @@ fn parse_method_name_attribute(
}

// Replace &A<Self> with &A<_>
fn replace_self(refn: &syn::TypeReference) -> syn::Result<syn::TypeReference> {
fn infer(span: proc_macro2::Span) -> syn::GenericArgument {
syn::GenericArgument::Type(syn::Type::Infer(syn::TypeInfer {
underscore_token: syn::token::Underscore { spans: [span] },
}))
}
fn replace_self_in_ref(refn: &syn::TypeReference) -> syn::Result<syn::TypeReference> {
let mut res = refn.to_owned();
let tp = match &mut *res.elem {
syn::Type::Path(p) => p,
_ => return Err(syn::Error::new_spanned(refn, "unsupported argument")),
};
replace_self_impl(tp);
res.lifetime = None;
Ok(res)
}

fn replace_self_in_path(tp: &syn::TypePath) -> syn::TypePath {
let mut res = tp.to_owned();
replace_self_impl(&mut res);
res
}

fn replace_self_impl(tp: &mut syn::TypePath) {
for seg in &mut tp.path.segments {
if let syn::PathArguments::AngleBracketed(ref mut g) = seg.arguments {
let mut args = syn::punctuated::Punctuated::new();
Expand All @@ -546,6 +568,9 @@ fn replace_self(refn: &syn::TypeReference) -> syn::Result<syn::TypeReference> {
g.args = args;
}
}
res.lifetime = None;
Ok(res)
fn infer(span: proc_macro2::Span) -> syn::GenericArgument {
syn::GenericArgument::Type(syn::Type::Infer(syn::TypeInfer {
underscore_token: syn::token::Underscore { spans: [span] },
}))
}
}
Loading