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

Clarify whether cdef class attributes must have a cdef #3154

Open
rebecca-palmer opened this issue Sep 22, 2019 · 3 comments

Comments

@rebecca-palmer
Copy link

commented Sep 22, 2019

The documentation states that cdef classes (by default, i.e. without an explicit __dict__) have the restriction "All attributes must be pre-declared at compile-time" / "The set of attributes is fixed at compile time".

Does this mean "all attributes used must exist in the class-level source" or "all attributes used must have a cdef in the class-level source"? i.e. is "Maybe" below allowed?

cdef class Allowed:
    cdef public object a=0 # or something more specific if 'a' is performance critical
    def f1(self):
        self.a=1

cdef class Maybe:
    a=0
    def f1(self):
        self.a=1

cdef class NotAllowed:
    def f1(self):
        self.a=1

One real-world instance of this is pandas _Timedelta.__array_priority__. This class works most of the time, but this crash bug looks like Python trying to access an instance dict that C didn't allocate space for, which plausibly might be a consequence of it not being allowed.

Please state explicitly whether this is allowed or not.

(It would be even better to either make it a compile-time error or treat it as 'cdef public object', but that might require an unreasonably large amount of work.)

@rebecca-palmer

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

cdef attributes can't have default values (though that at least is a compile-time error and not mystery crashes), so my above "Allowed" actually isn't, and just adding 'cdef public object' to them all doesn't fix pandas. Adding 'cdef dict dict' (in the .pxd if there is one) to enable dynamic attributes probably will, but at a performance cost.

However, my original request to document whether "Maybe:" is allowed remains open.

@da-woods

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

"Maybe" defines a class attribute (i.e. a single value shared between all instances, rather than one value per instance) - It's allowed but it doesn't do what you think. It's mostly consistent with what would happen if you put a=0 in an ordinary class definition (the self.a assignment doesn't work though, understandably). It's unlikely to be the source of your bug.

@rebecca-palmer

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Thanks; I plan to send a documentation patch later.

(I now think the pandas crash is #2823, i.e. something that is already fixed here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.