-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] Generate __setattr__ wrapper #19937
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
Conversation
return None | ||
|
||
self_reg = builder.accept(expr.args[0]) if is_object_callee else builder.self() | ||
ir = builder.get_current_class_ir() |
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.
What if this is a non-native class? Do we need to anything different?
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.
i think in general we can still translate object.__setattr__
calls because the underlying implementation of __setattr__
in cpython just calls the same function that we translate to.
for super().__setattr__
there might be issues when a non-native class inherits from another non-native class that defines __setattr__
as the translation would skip that inherited definition. but if it inherits only from object then we should be fine because we're back to object.__setattr__
. i've changed the conditions for translating super().__setattr__
to reflect this.
nevermind, the translation doesn't play well with the fact that the non-native class is really a couple of python objects.
super().__setattr__(key, val) | ||
|
||
@mypyc_attr(native_class=False) | ||
class SetAttrNonNative: |
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.
Test also non-native class that overrides __setattr__
and calls super().__setattr__
.
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.
doesn't this test already do this?
def __setattr__(self, key: str, val: object) -> None:
if key == "regular_attr":
super().__setattr__("regular_attr", val)
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.
LGTM now!
Thanks for adding this feature, it is really useful especially to emulate immutable types. One thing I would like to mention is that different code is being generated if I early bind __setattr__ = object.__setattr__
class Example:
def __init__(self, **kwargs):
for name, value in kwargs.items():
__setattr__(self, name, value) # ...
cpy_r_r20 = CPyStatic_annotable___globals;
cpy_r_r21 = CPyStatics[44]; /* '__setattr__' */
cpy_r_r22 = CPyDict_GetItem(cpy_r_r20, cpy_r_r21);
if (unlikely(cpy_r_r22 == NULL)) {
CPy_AddTraceback("coerce/annotable.py", "__new__", 51, CPyStatic_annotable___globals);
goto CPyL22;
}
PyObject *cpy_r_r23[3] = {cpy_r_self, cpy_r_name, cpy_r_r18};
cpy_r_r24 = (PyObject **)&cpy_r_r23;
cpy_r_r25 = PyObject_Vectorcall(cpy_r_r22, cpy_r_r24, 3, 0);
# ... whereas using class Example:
def __init__(self, **kwargs):
for name, value in kwargs.items():
object.__setattr__(self, name, value) uses generic setattr properly: cpy_r_name = cpy_r_r19;
cpy_r_r20 = CPyObject_GenericSetAttr(cpy_r_self, cpy_r_name, cpy_r_r18);
CPy_DECREF(cpy_r_name);
CPy_DECREF(cpy_r_r18); I'm not sure whether mypyc should be able to backtrack the symbol or not, but such a rewrite would be nice to have. |
In general I think we can't because It would only be possible to rewrite it if the variable is declared as But I'm not sure if that's a common use-case. It seems to me that if someone uses a variable like this then they intend to change its value at some point. |
Sounds fair enough. |
Generate wrapper function for
__setattr__
and set it as thetp_setattro
slot. The wrapper doesn't have to do anything because interpreted python runs the defined__setattr__
on every attribute assignment. So the wrapper only reports errors on unsupported uses of__setattr__
and makes the function signature match with the slot.Since
__setattr__
should run on every attribute assignment, native classes with__setattr__
generate calls to this function on attribute assignment instead of direct assignment to the underlying C struct.Native classes generally don't have
__dict__
which makes implementing dynamic attributes more challenging. The native class has to manage its own dictionary of names to values and handle assignment to regular attributes specially.With
__dict__
, assigning values to regular and dynamic attributes can be done by simply assigning the value in__dict__
, ie.self.__dict__[name] = value
orobject.__setattr__(self, name, value)
. With a custom attribute dictionary, assigning withname
being a regular attribute doesn't work because it would only update the value in the custom dictionary, not the actual attribute.On the other hand, the
object.__setattr__
call doesn't work for dynamic attributes and raises anAttributeError
without__dict__
.So something like this has to be implemented as a work-around:
To make this efficient in native classes, calls to
object.__setattr__
or equivalentsuper().__setattr__
are transformed to direct C struct assignments when the name literal matches an attribute name.