-
Notifications
You must be signed in to change notification settings - Fork 777
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
Add #[classattr]
methods to define Python class attributes
#905
Changes from all commits
7b5a076
8f22d10
d3d68ea
e3d9544
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
//! `PyClass` trait | ||
use crate::class::methods::{PyMethodDefType, PyMethodsImpl}; | ||
use crate::class::methods::{PyClassAttributeDef, PyMethodDefType, PyMethodsImpl}; | ||
use crate::conversion::{IntoPyPointer, ToPyObject}; | ||
use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; | ||
use crate::type_object::{type_flags, PyLayout}; | ||
use crate::types::PyDict; | ||
use crate::{class, ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; | ||
use std::ffi::CString; | ||
use std::os::raw::c_void; | ||
|
@@ -165,13 +167,23 @@ where | |
// buffer protocol | ||
type_object.tp_as_buffer = to_ptr(<T as class::buffer::PyBufferProtocolImpl>::tp_as_buffer()); | ||
|
||
let (new, call, mut methods, attrs) = py_class_method_defs::<T>(); | ||
|
||
// normal methods | ||
let (new, call, mut methods) = py_class_method_defs::<T>(); | ||
if !methods.is_empty() { | ||
methods.push(ffi::PyMethodDef_INIT); | ||
type_object.tp_methods = Box::into_raw(methods.into_boxed_slice()) as *mut _; | ||
} | ||
|
||
// class attributes | ||
if !attrs.is_empty() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering a recursive case(e.g., #[pymethods]
impl MyClass {
#[classattr]
fn foo() -> MyClass { ... }
} ), could you please move this code after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was precisely one of my use cases indeed. But I think it’s still possible to return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, but I'm not sure there's no corner case 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it safe to modify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized that it cannot be a problem since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ll add a few « recursive » test cases to be sure that we don’t break that assumption in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
no, it's not safe, according to the C API docs |
||
let dict = PyDict::new(py); | ||
for attr in attrs { | ||
dict.set_item(attr.name, (attr.meth)(py))?; | ||
} | ||
type_object.tp_dict = dict.to_object(py).into_ptr(); | ||
} | ||
|
||
// __new__ method | ||
type_object.tp_new = new; | ||
// __call__ method | ||
|
@@ -219,8 +231,10 @@ fn py_class_method_defs<T: PyMethodsImpl>() -> ( | |
Option<ffi::newfunc>, | ||
Option<ffi::PyCFunctionWithKeywords>, | ||
Vec<ffi::PyMethodDef>, | ||
Vec<PyClassAttributeDef>, | ||
) { | ||
let mut defs = Vec::new(); | ||
let mut attrs = Vec::new(); | ||
let mut call = None; | ||
let mut new = None; | ||
|
||
|
@@ -243,6 +257,9 @@ fn py_class_method_defs<T: PyMethodsImpl>() -> ( | |
| PyMethodDefType::Static(ref def) => { | ||
defs.push(def.as_method_def()); | ||
} | ||
PyMethodDefType::ClassAttribute(def) => { | ||
attrs.push(def); | ||
} | ||
_ => (), | ||
} | ||
} | ||
|
@@ -265,7 +282,7 @@ fn py_class_method_defs<T: PyMethodsImpl>() -> ( | |
|
||
py_class_async_methods::<T>(&mut defs); | ||
|
||
(new, call, defs) | ||
(new, call, defs, attrs) | ||
} | ||
|
||
fn py_class_async_methods<T>(defs: &mut Vec<ffi::PyMethodDef>) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
use pyo3::prelude::*; | ||
|
||
mod common; | ||
|
||
#[pyclass] | ||
struct Foo { | ||
#[pyo3(get)] | ||
x: i32, | ||
} | ||
|
||
#[pyclass] | ||
struct Bar { | ||
#[pyo3(get)] | ||
x: i32, | ||
} | ||
|
||
#[pymethods] | ||
impl Foo { | ||
#[classattr] | ||
fn a() -> i32 { | ||
5 | ||
} | ||
|
||
#[classattr] | ||
#[name = "B"] | ||
fn b() -> String { | ||
"bar".to_string() | ||
} | ||
|
||
#[classattr] | ||
fn foo() -> Foo { | ||
Foo { x: 1 } | ||
} | ||
|
||
#[classattr] | ||
fn bar() -> Bar { | ||
Bar { x: 2 } | ||
} | ||
} | ||
|
||
#[pymethods] | ||
impl Bar { | ||
#[classattr] | ||
fn foo() -> Foo { | ||
Foo { x: 3 } | ||
} | ||
} | ||
|
||
#[test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned this in my comment, but adding an in-line note so it doesn't get lost: I think it would be a good idea to add tests for the error cases, to ensure that they raise the correct exception. Could be done here or it could be done in the |
||
fn class_attributes() { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
let foo_obj = py.get_type::<Foo>(); | ||
py_assert!(py, foo_obj, "foo_obj.a == 5"); | ||
py_assert!(py, foo_obj, "foo_obj.B == 'bar'"); | ||
} | ||
|
||
#[test] | ||
fn class_attributes_are_immutable() { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
let foo_obj = py.get_type::<Foo>(); | ||
py_expect_exception!(py, foo_obj, "foo_obj.a = 6", TypeError); | ||
} | ||
|
||
#[test] | ||
fn recursive_class_attributes() { | ||
let gil = Python::acquire_gil(); | ||
let py = gil.python(); | ||
let foo_obj = py.get_type::<Foo>(); | ||
let bar_obj = py.get_type::<Bar>(); | ||
py_assert!(py, foo_obj, "foo_obj.foo.x == 1"); | ||
py_assert!(py, foo_obj, "foo_obj.bar.x == 2"); | ||
py_assert!(py, bar_obj, "bar_obj.foo.x == 3"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be helpful to optionally allow one argument, of type
Python
, for those who want to create python types as class attributes. Seepymethod/impl_wrap_getter
as an example of how we allow this for getters.One caveat I am not sure about though: as we're currently in the middle of creating a type object, is it safe to run arbitrary Python code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this code can cause SIGSEGV.Oh I was wrong. This code works. Still investigating this can cause some odd errors, though.
I'll open a small PR to prevent this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kngwyu is your conclusion that this is safe?