-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] Generate __getattr__ wrapper #19909
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
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.
Not a full review, just a few comments.
mypyc/primitives/generic_ops.py
Outdated
return_type=object_rprimitive, | ||
c_function_name="CPyObject_GenericGetAttr", | ||
error_kind=ERR_NEVER, | ||
is_borrowed=True, |
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 that the function returns a strong new reference, not a borrowed reference. Can you double check 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.
yes, you're right. it calls PyDict_GetItemRef
internally which returns a strong reference. i've changed the function description but without borrowing, mypyc would always insert a dec_ref
that would crash if CPyObject_GenericGetAttr
returned null. i've changed the refcount logic to insert xdec_ref
in this case since a null returned here is not an error.
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.
This concludes my review. Looks good overall, mostly some suggestions about things to test.
mypyc/irbuild/function.py
Outdated
tp_getattro slot is inherited by subclasses and if the subclass overrides __getattr__, | ||
the override would be ignored in our wrapper. TODO: To support this, the wrapper would | ||
have to resolve "__getattr__" against the type at runtime and call the returned method, | ||
like _Py_slot_tp_getattr_hook in cpython. |
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.
Can the wrapper check the type of self, and only use the slow path if the runtime type is unexpected?
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.
yes, that was my thinking as well. updated the comment to mention this.
from typing import Optional, Tuple | ||
|
||
class GetAttr: | ||
class_var = "x" |
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 a "real" class variable that is annotated using ClassVar[t]
.
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.
obtaining the value through the wrapper works (since i've also added this to the run tests) but when the variable is annotated like this, mypyc seems to always call CPyObject_GetAttr
instead of accessing it directly. not a regression since it works like this without __getattr__
too but maybe it could be optimized in the future.
Fixes mypyc/mypyc#887
Generate a wrapper function for
__getattr__
in classes where it's defined and put it into thetp_getattro
slot.At runtime, this wrapper function is called for every attribute access, so to match behavior of interpreted python it needs to first check if the given name is present in the type dictionary and only call the user-defined
__getattr__
when it's not present.Checking the type dictionary is implemented using a cpython function
_PyObject_GenericGetAttrWithDict
which is also used in the default attribute access handler in cpython.In compiled code, the wrapper will only be called when the attribute name cannot be statically resolved. When it can be resolved, the attribute will be accessed directly in the underlying C struct, or the generated function will be directly called in case of resolving method names. No change from existing behavior.
When the name cannot be statically resolved, mypyc generates calls to
PyObject_GetAttr
which internally callstp_getattro
.In interpreted code that uses compiled classes, attribute access will always result in calls to
PyObject_GetAttr
so there's always a dict look-up in the wrapper to find the attribute. But that's the case already, the dict look-up happens in the default attribute access handler in cpython. So the wrapper should not bring any negative performance impact.