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

Implement get/set all on pyclass #2692

Merged
merged 3 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions guide/pyclass_parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
| <span style="white-space: pre">`extends = BaseType`</span> | Use a custom baseclass. Defaults to [`PyAny`][params-1] |
| <span style="white-space: pre">`freelist = N`</span> | Implements a [free list][params-2] of size N. This can improve performance for types that are often created and deleted in quick succession. Profile your code to see whether `freelist` is right for you. |
| <span style="white-space: pre">`frozen`</span> | Declares that your pyclass is immutable. It removes the borrowchecker overhead when retrieving a shared reference to the Rust struct, but disables the ability to get a mutable reference. |
| `get_all` | Generates getters for all fields of the pyclass. |
| `mapping` | Inform PyO3 that this class is a [`Mapping`][params-mapping], and so leave its implementation of sequence C-API slots empty. |
| <span style="white-space: pre">`module = "module_name"`</span> | Python code will see the class as being defined in this module. Defaults to `builtins`. |
| <span style="white-space: pre">`name = "python_name"`</span> | Sets the name that Python sees this class as. Defaults to the name of the Rust struct. |
| `sequence` | Inform PyO3 that this class is a [`Sequence`][params-sequence], and so leave its C-API mapping length slot empty. |
| `set_all` | Generates setters for all fields of the pyclass. |
| `subclass` | Allows other Python classes and `#[pyclass]` to inherit from this class. Enums cannot be subclassed. |
| <span style="white-space: pre">`text_signature = "(arg1, arg2, ...)"`</span> | Sets the text signature for the Python class' `__new__` method. |
| `unsendable` | Required if your struct is not [`Send`][params-3]. Rather than using `unsendable`, consider implementing your struct in a threadsafe way by e.g. substituting [`Rc`][params-4] with [`Arc`][params-5]. By using `unsendable`, your class will panic when accessed by another thread.|
Expand Down
1 change: 1 addition & 0 deletions newsfragments/2692.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The `#[pyclass]` macro can now take `get_all` and `set_all` to create getters and setters for every field.
2 changes: 2 additions & 0 deletions pyo3-macros-backend/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ pub mod kw {
syn::custom_keyword!(frozen);
syn::custom_keyword!(gc);
syn::custom_keyword!(get);
syn::custom_keyword!(get_all);
syn::custom_keyword!(item);
syn::custom_keyword!(mapping);
syn::custom_keyword!(module);
syn::custom_keyword!(name);
syn::custom_keyword!(pass_module);
syn::custom_keyword!(sequence);
syn::custom_keyword!(set);
syn::custom_keyword!(set_all);
syn::custom_keyword!(signature);
syn::custom_keyword!(subclass);
syn::custom_keyword!(text_signature);
Expand Down
150 changes: 103 additions & 47 deletions pyo3-macros-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,14 @@ pub struct PyClassPyO3Options {
pub krate: Option<CrateAttribute>,
pub dict: Option<kw::dict>,
pub extends: Option<ExtendsAttribute>,
pub get_all: Option<kw::get_all>,
pub freelist: Option<FreelistAttribute>,
pub frozen: Option<kw::frozen>,
pub mapping: Option<kw::mapping>,
pub module: Option<ModuleAttribute>,
pub name: Option<NameAttribute>,
pub sequence: Option<kw::sequence>,
pub set_all: Option<kw::set_all>,
pub subclass: Option<kw::subclass>,
pub text_signature: Option<TextSignatureAttribute>,
pub unsendable: Option<kw::unsendable>,
Expand All @@ -80,10 +82,12 @@ enum PyClassPyO3Option {
Extends(ExtendsAttribute),
Freelist(FreelistAttribute),
Frozen(kw::frozen),
GetAll(kw::get_all),
Mapping(kw::mapping),
Module(ModuleAttribute),
Name(NameAttribute),
Sequence(kw::sequence),
SetAll(kw::set_all),
Subclass(kw::subclass),
TextSignature(TextSignatureAttribute),
Unsendable(kw::unsendable),
Expand All @@ -105,6 +109,8 @@ impl Parse for PyClassPyO3Option {
input.parse().map(PyClassPyO3Option::Freelist)
} else if lookahead.peek(attributes::kw::frozen) {
input.parse().map(PyClassPyO3Option::Frozen)
} else if lookahead.peek(attributes::kw::get_all) {
input.parse().map(PyClassPyO3Option::GetAll)
} else if lookahead.peek(attributes::kw::mapping) {
input.parse().map(PyClassPyO3Option::Mapping)
} else if lookahead.peek(attributes::kw::module) {
Expand All @@ -113,6 +119,8 @@ impl Parse for PyClassPyO3Option {
input.parse().map(PyClassPyO3Option::Name)
} else if lookahead.peek(attributes::kw::sequence) {
input.parse().map(PyClassPyO3Option::Sequence)
} else if lookahead.peek(attributes::kw::set_all) {
input.parse().map(PyClassPyO3Option::SetAll)
} else if lookahead.peek(attributes::kw::subclass) {
input.parse().map(PyClassPyO3Option::Subclass)
} else if lookahead.peek(attributes::kw::text_signature) {
Expand Down Expand Up @@ -165,10 +173,12 @@ impl PyClassPyO3Options {
PyClassPyO3Option::Extends(extends) => set_option!(extends),
PyClassPyO3Option::Freelist(freelist) => set_option!(freelist),
PyClassPyO3Option::Frozen(frozen) => set_option!(frozen),
PyClassPyO3Option::GetAll(get_all) => set_option!(get_all),
PyClassPyO3Option::Mapping(mapping) => set_option!(mapping),
PyClassPyO3Option::Module(module) => set_option!(module),
PyClassPyO3Option::Name(name) => set_option!(name),
PyClassPyO3Option::Sequence(sequence) => set_option!(sequence),
PyClassPyO3Option::SetAll(set_all) => set_option!(set_all),
PyClassPyO3Option::Subclass(subclass) => set_option!(subclass),
PyClassPyO3Option::TextSignature(text_signature) => set_option!(text_signature),
PyClassPyO3Option::Unsendable(unsendable) => set_option!(unsendable),
Expand Down Expand Up @@ -212,7 +222,7 @@ pub fn build_py_class(
For an explanation, see https://pyo3.rs/latest/class.html#no-generic-parameters"
);

let field_options = match &mut class.fields {
let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = match &mut class.fields {
syn::Fields::Named(fields) => fields
.named
.iter_mut()
Expand All @@ -230,18 +240,54 @@ pub fn build_py_class(
})
.collect::<Result<_>>()?,
syn::Fields::Unit => {
if let Some(attr) = args.options.set_all {
return Err(syn::Error::new_spanned(attr, UNIT_SET));
};
if let Some(attr) = args.options.get_all {
return Err(syn::Error::new_spanned(attr, UNIT_GET));
};
// No fields for unit struct
Vec::new()
}
};

if let Some(attr) = args.options.get_all {
for (_, FieldPyO3Options { get, .. }) in &mut field_options {
if let Some(old_get) = get.replace(Annotated::Struct(attr)) {
return Err(syn::Error::new(old_get.span(), DUPE_GET));
}
}
}

if let Some(attr) = args.options.set_all {
for (_, FieldPyO3Options { set, .. }) in &mut field_options {
if let Some(old_set) = set.replace(Annotated::Struct(attr)) {
return Err(syn::Error::new(old_set.span(), DUPE_SET));
}
}
}

impl_class(&class.ident, &args, doc, field_options, methods_type, krate)
}

enum Annotated<X, Y> {
Field(X),
Struct(Y),
}

impl<X: Spanned, Y: Spanned> Spanned for Annotated<X, Y> {
fn span(&self) -> Span {
match self {
Self::Field(x) => x.span(),
Self::Struct(y) => y.span(),
}
}
}

/// `#[pyo3()]` options for pyclass fields
struct FieldPyO3Options {
get: bool,
set: bool,
get: Option<Annotated<kw::get, kw::get_all>>,
set: Option<Annotated<kw::set, kw::set_all>>,
name: Option<NameAttribute>,
}

Expand Down Expand Up @@ -269,33 +315,27 @@ impl Parse for FieldPyO3Option {
impl FieldPyO3Options {
fn take_pyo3_options(attrs: &mut Vec<syn::Attribute>) -> Result<Self> {
let mut options = FieldPyO3Options {
get: false,
set: false,
get: None,
set: None,
name: None,
};

for option in take_pyo3_options(attrs)? {
match option {
FieldPyO3Option::Get(kw) => {
ensure_spanned!(
!options.get,
kw.span() => "`get` may only be specified once"
);
options.get = true;
if options.get.replace(Annotated::Field(kw)).is_some() {
return Err(syn::Error::new(kw.span(), UNIQUE_GET));
}
}
FieldPyO3Option::Set(kw) => {
ensure_spanned!(
!options.set,
kw.span() => "`set` may only be specified once"
);
options.set = true;
if options.set.replace(Annotated::Field(kw)).is_some() {
return Err(syn::Error::new(kw.span(), UNIQUE_SET));
}
}
FieldPyO3Option::Name(name) => {
ensure_spanned!(
options.name.is_none(),
name.span() => "`name` may only be specified once"
);
options.name = Some(name);
if options.name.replace(name).is_some() {
return Err(syn::Error::new(options.name.span(), UNIQUE_NAME));
}
}
}
}
Expand Down Expand Up @@ -664,39 +704,42 @@ fn descriptors_to_items(
field_options: Vec<(&syn::Field, FieldPyO3Options)>,
) -> syn::Result<Vec<MethodAndMethodDef>> {
let ty = syn::parse_quote!(#cls);
field_options
.into_iter()
.enumerate()
.flat_map(|(field_index, (field, options))| {
let name_err = if options.name.is_some() && !options.get && !options.set {
Some(Err(err_spanned!(options.name.as_ref().unwrap().span() => "`name` is useless without `get` or `set`")))
} else {
None
};
let mut items = Vec::new();
for (field_index, (field, options)) in field_options.into_iter().enumerate() {
if let FieldPyO3Options {
name: Some(name),
get: None,
set: None,
} = options
{
return Err(syn::Error::new_spanned(name, USELESS_NAME));
}

let getter = if options.get {
Some(impl_py_getter_def(&ty, PropertyType::Descriptor {
if options.get.is_some() {
let getter = impl_py_getter_def(
&ty,
PropertyType::Descriptor {
field_index,
field,
python_name: options.name.as_ref()
}))
} else {
None
};
python_name: options.name.as_ref(),
},
)?;
items.push(getter);
}

let setter = if options.set {
Some(impl_py_setter_def(&ty, PropertyType::Descriptor {
if options.set.is_some() {
let setter = impl_py_setter_def(
&ty,
PropertyType::Descriptor {
field_index,
field,
python_name: options.name.as_ref()
}))
} else {
None
};

name_err.into_iter().chain(getter).chain(setter)
})
.collect::<syn::Result<_>>()
python_name: options.name.as_ref(),
},
)?;
items.push(setter);
};
}
Ok(items)
}

fn impl_pytypeinfo(
Expand Down Expand Up @@ -1085,3 +1128,16 @@ fn define_inventory_class(inventory_class_name: &syn::Ident) -> TokenStream {
_pyo3::inventory::collect!(#inventory_class_name);
}
}

const UNIQUE_GET: &str = "`get` may only be specified once";
const UNIQUE_SET: &str = "`set` may only be specified once";
const UNIQUE_NAME: &str = "`name` may only be specified once";

const DUPE_SET: &str = "useless `set` - the struct is already annotated with `set_all`";
const DUPE_GET: &str = "useless `get` - the struct is already annotated with `get_all`";
const UNIT_GET: &str =
"`get_all` on an unit struct does nothing, because unit structs have no fields";
const UNIT_SET: &str =
"`set_all` on an unit struct does nothing, because unit structs have no fields";

const USELESS_NAME: &str = "`name` is useless without `get` or `set`";
1 change: 1 addition & 0 deletions tests/test_compile_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ fn _test_compile_errors() {
t.compile_fail("tests/ui/not_send.rs");
t.compile_fail("tests/ui/not_send2.rs");
t.compile_fail("tests/ui/not_send3.rs");
t.compile_fail("tests/ui/get_set_all.rs");
}

#[rustversion::before(1.63)]
Expand Down
31 changes: 31 additions & 0 deletions tests/test_getter_setter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,34 @@ fn tuple_struct_getter_setter() {
py_assert!(py, inst, "inst.num == 20");
});
}

#[pyclass(get_all, set_all)]
struct All {
num: i32,
}

#[test]
fn get_set_all() {
Python::with_gil(|py| {
let inst = Py::new(py, All { num: 10 }).unwrap();

py_run!(py, inst, "assert inst.num == 10");
py_run!(py, inst, "inst.num = 20; assert inst.num == 20");
});
}

#[pyclass(get_all)]
struct All2 {
#[pyo3(set)]
num: i32,
}

#[test]
fn get_all_and_set() {
Python::with_gil(|py| {
let inst = Py::new(py, All2 { num: 10 }).unwrap();

py_run!(py, inst, "assert inst.num == 10");
py_run!(py, inst, "inst.num = 20; assert inst.num == 20");
});
}
21 changes: 21 additions & 0 deletions tests/ui/get_set_all.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use pyo3::prelude::*;

#[pyclass(set_all)]
struct Foo;

#[pyclass(set_all)]
struct Foo2{
#[pyo3(set)]
field: u8,
}

#[pyclass(get_all)]
struct Foo3;

#[pyclass(get_all)]
struct Foo4{
#[pyo3(get)]
field: u8,
}

fn main() {}
23 changes: 23 additions & 0 deletions tests/ui/get_set_all.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
error: `set_all` on an unit struct does nothing, because unit structs have no fields
--> tests/ui/get_set_all.rs:3:11
|
3 | #[pyclass(set_all)]
| ^^^^^^^

error: useless `set` - the struct is already annotated with `set_all`
--> tests/ui/get_set_all.rs:8:12
|
8 | #[pyo3(set)]
| ^^^

error: `get_all` on an unit struct does nothing, because unit structs have no fields
--> tests/ui/get_set_all.rs:12:11
|
12 | #[pyclass(get_all)]
| ^^^^^^^

error: useless `get` - the struct is already annotated with `get_all`
--> tests/ui/get_set_all.rs:17:12
|
17 | #[pyo3(get)]
| ^^^
2 changes: 1 addition & 1 deletion tests/ui/invalid_property_args.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,4 @@ error: `name` is useless without `get` or `set`
--> tests/ui/invalid_property_args.rs:40:33
|
40 | struct NameWithoutGetSet(#[pyo3(name = "value")] i32);
| ^^^^
| ^^^^^^^^^^^^^^
4 changes: 2 additions & 2 deletions tests/ui/invalid_pyclass_args.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `sequence`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `sequence`, `set_all`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
--> tests/ui/invalid_pyclass_args.rs:3:11
|
3 | #[pyclass(extend=pyo3::types::PyDict)]
Expand Down Expand Up @@ -34,7 +34,7 @@ error: expected string literal
18 | #[pyclass(module = my_module)]
| ^^^^^^^^^

error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `mapping`, `module`, `name`, `sequence`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
error: expected one of: `crate`, `dict`, `extends`, `freelist`, `frozen`, `get_all`, `mapping`, `module`, `name`, `sequence`, `set_all`, `subclass`, `text_signature`, `unsendable`, `weakref`, `gc`
--> tests/ui/invalid_pyclass_args.rs:21:11
|
21 | #[pyclass(weakrev)]
Expand Down