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 7 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
44 changes: 22 additions & 22 deletions guide/src/class.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ 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 ConcreteLayout = pyo3::PyCell<Self>;
type Initializer = pyo3::PyClassInitializer<Self>;

const NAME: &'static str = "MyClass";
Expand Down Expand Up @@ -109,35 +109,35 @@ fn mymodule(_py: Python, m: &PyModule) -> PyResult<()> {
You sometimes need to convert your `pyclass` into a Python object in Rust code (e.g., for testing it).

For getting *GIL-bounded* (i.e., with `'py` lifetime) references of `pyclass`,
you can use `PyClassShell<T>`.
you can use `PyCell<T>`.
Or you can use `Py<T>` directly, for *not-GIL-bounded* references.

### `PyClassShell`
`PyClassShell` represents the actual layout of `pyclass` on the Python heap.
### `PyCell`
`PyCell` represents the actual layout of `pyclass` on the Python heap.

If you want to instantiate `pyclass` in Python and get the reference,
you can use `PyClassShell::new_ref` or `PyClassShell::new_mut`.
you can use `PyCell::new_ref` or `PyCell::new_mut`.

```rust
# use pyo3::prelude::*;
# use pyo3::types::PyDict;
# use pyo3::PyClassShell;
# use pyo3::PyCell;
#[pyclass]
struct MyClass {
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();
let obj = PyCell::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
// You can treat a `&PyCell` 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();
// return &mut PyCell<MyClass>
let obj = PyCell::new_mut(py, MyClass { num: 3, debug: true }).unwrap();
obj.num = 5;
```

Expand Down Expand Up @@ -230,7 +230,7 @@ explicitly.

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

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

fn method2(self_: &PyClassShell<Self>) -> PyResult<usize> {
fn method2(self_: &PyCell<Self>) -> PyResult<usize> {
self_.get_super().method().map(|x| x * self_.val2)
}
}
Expand All @@ -279,7 +279,7 @@ impl SubSubClass {
.add_subclass(SubSubClass{val3: 20})
}

fn method3(self_: &PyClassShell<Self>) -> PyResult<usize> {
fn method3(self_: &PyCell<Self>) -> PyResult<usize> {
let super_ = self_.get_super();
SubClass::method2(super_).map(|x| x * self_.val3)
}
Expand All @@ -288,20 +288,20 @@ impl SubSubClass {

# let gil = Python::acquire_gil();
# let py = gil.python();
# let subsub = pyo3::PyClassShell::new_ref(py, SubSubClass::new()).unwrap();
# let subsub = pyo3::PyCell::new_ref(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 +761,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: &mut PyCell<Self>) -> PyResult<impl IntoPy<PyObject>>`
* `fn __next__(slf: &mut PyCell<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 +779,10 @@ struct MyIterator {

#[pyproto]
impl PyIterProtocol for MyIterator {
fn __iter__(slf: &mut PyClassShell<Self>) -> PyResult<Py<MyIterator>> {
fn __iter__(slf: &mut PyCell<Self>) -> PyResult<Py<MyIterator>> {
Ok(slf.into())
}
fn __next__(slf: &mut PyClassShell<Self>) -> PyResult<Option<PyObject>> {
fn __next__(slf: &mut PyCell<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_ref(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] },
}))
}
}
46 changes: 24 additions & 22 deletions pyo3-derive-backend/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,22 @@ pub fn process_functions_in_module(func: &mut syn::ItemFn) {
}

/// Transforms a rust fn arg parsed with syn into a method::FnArg
fn wrap_fn_argument<'a>(input: &'a syn::FnArg, name: &'a Ident) -> Option<method::FnArg<'a>> {
match input {
syn::FnArg::Receiver(_) => None,
syn::FnArg::Typed(ref cap) => {
let (mutability, by_ref, ident) = match *cap.pat {
syn::Pat::Ident(ref patid) => (&patid.mutability, &patid.by_ref, &patid.ident),
_ => panic!("unsupported argument: {:?}", cap.pat),
};
fn wrap_fn_argument<'a>(cap: &'a syn::PatType, name: &'a Ident) -> method::FnArg<'a> {
let (mutability, by_ref, ident) = match *cap.pat {
syn::Pat::Ident(ref patid) => (&patid.mutability, &patid.by_ref, &patid.ident),
_ => panic!("unsupported argument: {:?}", cap.pat),
kngwyu marked this conversation as resolved.
Show resolved Hide resolved
};

let py = crate::utils::if_type_is_python(&cap.ty);
let opt = method::check_arg_ty_and_optional(&name, &cap.ty);
Some(method::FnArg {
name: ident,
mutability,
by_ref,
ty: &cap.ty,
optional: opt,
py,
reference: method::is_ref(&name, &cap.ty),
})
}
let py = crate::utils::if_type_is_python(&cap.ty);
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed while we're here: I really wish this util function was just called type_is_python(), because

if type_is_python(ty) { ... }

reads nicer than

if if_type_is_python(ty) { ... }

let opt = method::check_arg_ty_and_optional(&name, &cap.ty);
method::FnArg {
name: ident,
mutability,
by_ref,
ty: &cap.ty,
optional: opt,
py,
reference: method::is_ref(&name, &cap.ty),
}
}

Expand Down Expand Up @@ -138,10 +133,16 @@ pub fn add_fn_to_module(
pyfn_attrs: Vec<pyfunction::Argument>,
) -> TokenStream {
let mut arguments = Vec::new();
let mut self_ = None;

for input in func.sig.inputs.iter() {
if let Some(fn_arg) = wrap_fn_argument(input, &func.sig.ident) {
arguments.push(fn_arg);
match input {
syn::FnArg::Receiver(recv) => {
self_ = Some(recv.mutability.is_some());
}
syn::FnArg::Typed(ref cap) => {
arguments.push(wrap_fn_argument(cap, &func.sig.ident));
}
}
}

Expand All @@ -160,6 +161,7 @@ pub fn add_fn_to_module(

let spec = method::FnSpec {
tp: method::FnType::Fn,
self_,
name: &function_wrapper_ident,
python_name,
attrs: pyfn_attrs,
Expand Down
Loading