Add `#[classattr]` methods to define Python class attributes by scalexm · Pull Request #905 · PyO3/pyo3

scalexm · 2020-05-05T20:13:33Z

Here is my approach to #662: we add an attribute on methods defined in #[pymethods] impl blocks to allow defining class attributes (also named class variables).

A class attribute method cannot take arguments, and must return impl IntoPy<PyObject>.

For example, given the following Python code:

class Foo:
    foo = 5
    BAR = 'bar'

a translation in Rust with pyo3 could be given by:

#[pyclass]
struct Foo { }

#[pymethods]
impl Foo {
    #[classattr]
    fn foo() -> i32 {
        5
    }

    #[classattr]
    #[name = "BAR"]
    fn bar() -> String {
        "bar".to_string()
    }
}

davidhewitt · 2020-05-05T22:40:02Z

Thanks for this PR, a way to add class attributes like this would be great. Couple questions (disclaimer, I haven't read the code yet):

Bike-shedding on the name of the attribute (currently #[attribute]) is welcome. Also, I'm not used to the Python C API so it's possible that my code has memory errors.

Can these things ever be settable? Maybe a pair of names like classgetter / classsetter? Though maybe these functions run only once during type object creation?

Note that in the latter case, returning an Err value will crash the Python interpreter

Did you explore mechanisms to prevent returning PyResult from these functions? It would be nice to make it impossible to compile code that's guaranteed to cause runtime crash, if that's viable.

Also, I wonder whether we could use syntax like associated constants instead of functions. But I think they're only stable for traits, not structs?

programmerjake · 2020-05-06T01:53:15Z

It would be nice if we could also use associated constants (which are stable, i8::MAX (not to be confused with std::i8::MAX) is a good example):

#[pymethods]
impl MyClass {
    #[attribute]
    pub const MY_CONST: &str = "blah";

    #[attribute]
    pub fn my_const2(_py: Python) -> Vec<u8> {
        vec![1, 2, 3]
    }
}

kngwyu · 2020-05-06T02:36:59Z

Interesting, thank you @scalexm!

It would be nice if we could also use associated constants

~~How about using const fn instead of constants?~~

kngwyu

Left some comments, but generally looks good.

pyo3-derive-backend/src/pymethod.rs

src/pyclass.rs

tests/test_class_attributes.rs

src/class/methods.rs

pganssle · 2020-05-06T03:07:24Z

Haven't dug into the implementation, but it's not clear - can you modify these at runtime? Either way I would add a test for the expected behavior if you try to assign a value to it.

Also, it's not immediately clear: Is the function evaluated when the class is constructed, or every time it is accessed?

kngwyu · 2020-05-06T03:12:42Z

Haven't dug into the implementation, but it's not clear - can you modify these at runtime? Either way I would add a test for the expected behavior if you try to assign a value to it.
Also, it's not immediately clear: Is the function evaluated when the class is constructed, or every time it is accessed?

In the current implementation, we just do type(PyClass).__dict__["name"] = var once when the type object is created.

About returning an error
Is there any use case where we need to return an error from attribute? If not, I think we should restrict the return type to IntoPy<PyObject>.
About naming
attribute is not so bad, but aren't they called class variable in Python?
How about #[classvar]?
Can these things ever be settable?

They are settable in Python.
But since in Rust, we need to use Cell/RefCell to share mutable variables and it's not a common pattern. So I don't think we have any strong reason to support setter for it.

It would be nice if we could also use associated constants

Agreed.

scalexm · 2020-05-06T06:06:15Z

Thanks for the reviews.

How about #[classvar]

Yes, that’s probably better. Always called that class attributes but indeed the official Python documentation calls them class variables :)

Is there any use case where we need to return an error from attribute

Python does support it, in which case an exception is raised when the module is imported. It’s indeed not clear how useful it is, so I guess we can disable it for now (only accept IntoPy<PyObject>) and think about it later if someone comes up with a use case.

About using associated constants, I can open a follow-up PR.

davidhewitt · 2020-05-06T06:57:14Z

It would be nice if we could also use associated constants (which are stable, i8::MAX (not to be confused with std::i8::MAX) is a good example):

Nice, I was so sure they were stable but then when looking at documentation to confirm this last night I got bogged down in the docs for traits xD

They are settable in Python.

If they're settable in Python, but we don't want to allow setting these in Rust, I suggest we make it so that an exception is raised if a user attempts to set these attributes from Python. Otherwise the original Rust value and current Python value may become out of sync?

pganssle · 2020-05-06T12:25:29Z

For comparison, here's what happens with a Cython cdef class defined like so:

cdef class MyClass:
    attr = 3

>>> from mymodule._backend import MyClass
>>> MyClass.attr
3
>>> MyClass.attr = 4
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't set attributes of built-in/extension type 'mymodule._backend.MyClass'
>>> MyClass().attr
3
>>> MyClass().attr = 4
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'mymodule._backend.MyClass' object attribute 'attr' is read-only

So it seems like they are called "attributes" and that it is reasonable for them to be read-only in both Python and Rust.

I think #[attribute] or #[classattr] or #[classattribute] are all reasonable spellings. I'm not terribly fond of the idea of #[classvar]. Class scoping in Python is super weird and there may be a distinction between "class variable" and "attribute" that I don't understand.

pganssle · 2020-05-06T12:28:10Z

tests/test_class_attributes.rs

+    }
+}
+
+#[test]


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 examples/rustapi_module tests, which are written in Python and where you can use pytest.raises.

kngwyu · 2020-05-06T13:51:23Z

I think #[attribute] or #[classattr] or #[classattribute] are all reasonable spellings. I'm not terribly fond of the idea of #[classvar]. Class scoping in Python is super weird and there may be a distinction between "class variable" and "attribute" that I don't understand.

Thank you for the feedback.

I suggest we make it so that an exception is raised if a user attempts to set these attributes from Python.

I checked the code that Cython produced and what it does is the same as this PR. They set attr to tp_dict.

  if (PyDict_SetItem((PyObject *)__pyx_ptype_4demo_MyClass->tp_dict, __pyx_n_s_attr, __pyx_int_3) < 0) __PYX_ERR(1, 2, __pyx_L1_error)

But I don't understand how it makes the attr read-only 🤔 ...

programmerjake · 2020-05-06T17:18:03Z

But I don't understand how it makes the attr read-only ...

Because Python enforces that, for classes defined using the C API that PyO3 uses, type objects can't be changed (I'm assuming since they're shared across all interpreters in the program) and tp_dict is considered part of that type object.

davidhewitt · 2020-05-06T17:22:22Z

+1 for #[classattr] as my favourite of the names suggested.

Because Python enforces that, for classes defined using the C API that PyO3 uses, type objects can't be changed (I'm assuming since they're shared across all interpreters in the program) and tp_dict is considered part of that type object.

Nice. I guess should make sure there are tests in this PR to verify that trying to modify a classattr raises an exception as described.

scalexm · 2020-05-06T18:18:23Z

Current status:

applied @kngwyu comments: it's cleaner that way, thanks
removed the possibility to return a PyResult: this can always be added later if someone has a use case for it
added a test to showcase that class variables from an extension module are indeed immutable

Next steps:

decide on the name: I personally don't have a strong opinion
add a paragraph to the guide
allow associated constants to define class attributes: follow-up PR Allow #[classattr] on associated constants #914

kngwyu · 2020-05-07T05:01:31Z

tests/test_class_attributes.rs

+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let typeobj = py.get_type::<Foo>();
+    py_expect_exception!(py, typeobj, "typeobj.foo = 6", TypeError);


kngwyu · 2020-05-07T05:06:50Z

Thank you for the update!

decide on the name: I personally don't have a strong opinion

Me too, so the top candidate is #[classattr]?(+1 by @davidhewitt)

add a paragraph to the guide

Yeah, please place some docs in class.md.

allow associated constants to define class attributes: I'd prefer to open a separate follow-up PR for that one

👍

programmerjake · 2020-05-07T05:55:45Z

For naming, I'm happy with any of the options other than #[classvar] since the attributes act like constants, not variables.

davidhewitt · 2020-05-07T07:42:26Z

pyo3-derive-backend/src/method.rs

+            if self_.is_some() || !arguments.is_empty() {
+                return Err(syn::Error::new_spanned(
+                    name,
+                    "Class attribute methods cannot take arguments",


It may be helpful to optionally allow one argument, of type Python, for those who want to create python types as class attributes. See pymethod/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?

is it safe to run arbitrary Python code?

~~Maybe this code can cause SIGSEGV.~~
Oh I was wrong. This code works. Still investigating this can cause some odd errors, though.

#[pymethods] impl MyClass { #[classattr] fn foo() -> MyClass { ... } }

~~I'll open a small PR to prevent this.~~

@kngwyu is your conclusion that this is safe?

kngwyu · 2020-05-07T08:35:47Z

src/pyclass.rs

    }

+    // class attributes
+    if !attrs.is_empty() {


Considering a recursive case(e.g.,

#[pymethods] impl MyClass { #[classattr] fn foo() -> MyClass { ... } }

), could you please move this code after PyType_Ready?
Then an incomplete type object is never used.

That was precisely one of my use cases indeed. But I think it’s still possible to return MyOtherClass where MyOtherClass has not yet been initialized?

But I think it’s still possible to return MyOtherClass where MyOtherClass has not yet been initialized?

Yes, but I'm not sure there's no corner case 🤔

Is it safe to modify type_object after PyType_Ready ?

I realized that it cannot be a problem since PyCell::new doesn't use the type object at all.
Thanks @scalexm

I’ll add a few « recursive » test cases to be sure that we don’t break that assumption in the future.

Is it safe to modify type_object after PyType_Ready ?

no, it's not safe, according to the C API docs

It cannot be a problem

kngwyu · 2020-05-08T02:24:38Z

LGTM, thank you for the update!

kngwyu approved these changes May 6, 2020

View reviewed changes

pyo3-derive-backend/src/pymethod.rs Outdated Show resolved Hide resolved

src/pyclass.rs Outdated Show resolved Hide resolved

tests/test_class_attributes.rs Outdated Show resolved Hide resolved

src/class/methods.rs Outdated Show resolved Hide resolved

pganssle reviewed May 6, 2020

View reviewed changes

scalexm force-pushed the master branch from 86ddfbc to c41402d Compare May 6, 2020 18:06

kngwyu reviewed May 7, 2020

View reviewed changes

davidhewitt reviewed May 7, 2020

View reviewed changes

kngwyu previously requested changes May 7, 2020

View reviewed changes

scalexm force-pushed the master branch from 9333c61 to 318b1cd Compare May 7, 2020 18:36

scalexm changed the title ~~Add #[attribute] methods to define Python class attributes~~ Add #[classattr] methods to define Python class attributes May 7, 2020

scalexm added 4 commits May 7, 2020 21:23

Add #[classattr] methods to define Python class attributes

7b5a076

Add a test showing that class attrs are immutable

8f22d10

Add a test with class attrs returning PyClass instances

d3d68ea

Add a paragraph to the guide about #[classattr]

e3d9544

scalexm force-pushed the master branch from 318b1cd to e3d9544 Compare May 7, 2020 19:25

kngwyu merged commit 8aeae6c into PyO3:master May 8, 2020

This was referenced May 8, 2020

Allow #[classattr] on associated constants #914

Merged

Support creating class member variables (not instance members) #662

Closed

davidhewitt mentioned this pull request Jun 15, 2020

Add pyo3::once_cell::GILOnceCell #975

Merged

+                  }
+              }
+              #[test]

Conversation

scalexm commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhewitt commented May 5, 2020

Uh oh!

programmerjake commented May 6, 2020

Uh oh!

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

Uh oh!

Uh oh!

kngwyu 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pganssle commented May 6, 2020

Uh oh!

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

Uh oh!

Uh oh!

scalexm commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhewitt commented May 6, 2020

Uh oh!

pganssle commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

programmerjake commented May 6, 2020

Uh oh!

davidhewitt commented May 6, 2020

Uh oh!

scalexm commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu commented May 7, 2020

Uh oh!

programmerjake commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kngwyu May 7, 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!

scalexm May 7, 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 May 7, 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 May 8, 2020

Uh oh!

Reviewers

Assignees

scalexm commented May 5, 2020 •

edited

Loading

kngwyu commented May 6, 2020 •

edited

Loading

kngwyu left a comment •

edited

Loading

kngwyu commented May 6, 2020 •

edited

Loading

scalexm commented May 6, 2020 •

edited

Loading

kngwyu commented May 6, 2020 •

edited

Loading

scalexm commented May 6, 2020 •

edited

Loading

programmerjake commented May 7, 2020 •

edited

Loading

kngwyu May 7, 2020 •

edited

Loading

scalexm May 7, 2020 •

edited

Loading

kngwyu May 7, 2020 •

edited

Loading