Improved handling of cdef class properties #4021
Conversation
With everything being moved around a bit it needed moving too
Cython/Compiler/Nodes.py
Outdated
| @@ -5191,6 +5191,10 @@ def analyse_declarations(self, env): | |||
| scope.doc = embed_position(self.pos, self.doc) | |||
|
|
|||
| if has_body: | |||
| properties = {} | |||
| for stat in getattr(self.body, 'stats', []): | |||
There was a problem hiding this comment.
Calling getattr() always has a code smell. Is the problem here that self.body might have been wrapped? Because if that is the case, then it seems wrong to ignore the body.
There was a problem hiding this comment.
I can't immediately find why was there (it seems to pass all tests with "prop" in the name without it). I've tried removing it (and will either leave it removed or insert an appropriate comment if it is needed)
Cython/Compiler/Nodes.py
Outdated
| @@ -5538,6 +5547,7 @@ class PropertyNode(StatNode): | |||
| # body StatListNode | |||
|
|
|||
| child_attrs = ["body"] | |||
| created_from_decorator = 0 # just used in a deprecation warning | |||
There was a problem hiding this comment.
| created_from_decorator = 0 # just used in a deprecation warning | |
| created_from_decorator = False # just used in a deprecation warning |
Regarding the comment, I'm not sure if this definition should know about the usages. They might change at some point, and the comment is unlikely to get updated then. I'd leave this to the commit comment.
| self.scope_type = scope_type | ||
| self.scope_node = scope_node | ||
| self.current_directives = current_directives | ||
| self.properties = {} # a dict o |
| self._properties.append({}) | ||
| if self._properties_node_sets is None: | ||
| self._properties_node_sets = [] | ||
| id_props_transform = _IdentifyPropertiesTransform(self.context, |
There was a problem hiding this comment.
Does this require a new transform? Can it not be done along the way?
There was a problem hiding this comment.
The reason for doing it separately was for things like:
cdef class C:
@property
def x(self):
return self._x
@x.setter
@some_other_decorator
def x(self, value):
self._x = value
Because of some_other_decorator x can't be a cdef property (but can be handled by the python property function). It isn't possible to know this at the point that the getter is evaluated though.
Therefore I think it does need two passes - one to collect the properties and one clear any functions that have been changed to properties.
They should both be pretty shallow transforms though, since cdef classes can't be nested so there's no need to actually go into the functions.
| if not node.created_from_decorator: | ||
| level = 2 if isinstance(node.pos[0], str) else 0 | ||
| warning(node.pos, "'property %s:' syntax is deprecated, use '@property'" % node.name, level) |
There was a problem hiding this comment.
I wonder if the parser wouldn't be a better place to emit the warning. It knows for sure what syntax was used.
There was a problem hiding this comment.
Done - that lets me get rid of created_from_decorator too.
I'm not sure isinstance(node.pos[0], str) was working right anyway - it's not a FileSourceDescriptor. So we may not have been showing the warning to anyone for a while.
There was a problem hiding this comment.
I'm not sure
isinstance(node.pos[0], str)was working right anyway - it's not aFileSourceDescriptor. So we may not have been showing the warning to anyone for a while.
This might have been meant to exclude internal utility code, for which the source is a string, something like "<string>" or so.
There was a problem hiding this comment.
Yes - it definitely was meant to exclude internal utility code. It now looks like a choice between StringSourceDescriptor (for the internal code) and FileSourceDescriptor for user code. But I'm not sure what it would have been in the past.
Co-authored-by: scoder <stefan_ml@behnel.de>
I can't see why it's needed and it doesn't seem to be on any of the "prop*" tests
Note - has some tests disabled since they require #3966 to work completely. It probably isn't worth looking at before then.
Relax some restriction on cdef class properties. Where it isn't able to do the efficient thing it falls back to calling the builtin method property rather than using the getset slots.
Properties are only special-cased where the property decorator is the only decorator. Having mismatching function names now generates a warning, not an error, and follows Python 3 (which is slightly counter-intuitive but fairly easily tested). Python 2 behaviour appears to be different. (Mismatched names was the cause of #1905.)
Fixes #2181
This is part 3 of trying to split #3383 into more manageable chunks.