gh-115231: fill __module__ for built-in class/staticmethods #115232
Conversation
skirpichev
commented
Feb 10, 2024
•
skirpichev
commented
- Issue: Empty __module__ attribute for built-in static methods #115231
Objects/typeobject.c
Outdated
| @@ -6653,7 +6653,9 @@ type_add_method(PyTypeObject *type, PyMethodDef *meth) | |||
| descr = PyDescr_NewClassMethod(type, meth); | |||
| } | |||
| else if (meth->ml_flags & METH_STATIC) { | |||
| PyObject *cfunc = PyCFunction_NewEx(meth, (PyObject*)type, NULL); | |||
| PyObject *mod = PyObject_GetAttr((PyObject*)type, &_Py_ID(__module__)); | |||
There was a problem hiding this comment.
This might theoretically return NULL and raise AttributeError.
There was a problem hiding this comment.
Hmm, thanks. Probably we could just clear the error indicator on mod == NULL.
There was a problem hiding this comment.
Also take a look at GetOptionalAttr
There was a problem hiding this comment.
I think we want ignore here not just AttributeError's. I did similar fix for class methods and got a failure in the test.test_types.UnionTests.test_or_type_operator_with_bad_module().
There was a problem hiding this comment.
It is better to use type_module() for consistency with the rest of this file. It will not give a surprise if Exceptions other than AttributeError should not be ignored.
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
skirpichev
force-pushed
the
static-bltin-__module__-115231
branch
from
8d2f05d to
be1c7ef
Compare
skirpichev
changed the title
skirpichev
force-pushed
the
static-bltin-__module__-115231
branch
from
28eec6f to
c4dec10
Compare
|
See also #82062: we could fix that issue in a similar approach, using >>> int.to_bytes.__module__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'method_descriptor' object has no attribute '__module__'. Did you mean: '__reduce__'? |
Objects/typeobject.c
Outdated
| @@ -6653,7 +6653,9 @@ type_add_method(PyTypeObject *type, PyMethodDef *meth) | |||
| descr = PyDescr_NewClassMethod(type, meth); | |||
| } | |||
| else if (meth->ml_flags & METH_STATIC) { | |||
| PyObject *cfunc = PyCFunction_NewEx(meth, (PyObject*)type, NULL); | |||
| PyObject *mod = PyObject_GetAttr((PyObject*)type, &_Py_ID(__module__)); | |||
There was a problem hiding this comment.
It is better to use type_module() for consistency with the rest of this file. It will not give a surprise if Exceptions other than AttributeError should not be ignored.
| @@ -130,7 +130,11 @@ classmethod_get(PyObject *self, PyObject *obj, PyObject *type) | |||
| if (descr->d_method->ml_flags & METH_METHOD) { | |||
| cls = descr->d_common.d_type; | |||
| } | |||
| return PyCMethod_New(descr->d_method, type, NULL, cls); | |||
| PyObject *mod = PyObject_GetAttr((PyObject*)type, &_Py_ID(__module__)); | |||
There was a problem hiding this comment.
This will add an overhead for use of class methods. We should try to find other solution.
There was a problem hiding this comment.
overhead for use of class methods
You meant for adding static/class methods?
We should try to find other solution.
Unlikely e.g. __name__, it's a member of PyCFunctionObject. I think we can set one only in PyCMethod_New and there is no more cheapest solution.
There was a problem hiding this comment.
PR updated to use type_module().
For classmethods, I doubt there is another solution. Here is a micro-benchmark:
| Benchmark | ref | patch |
|---|---|---|
| int.from_bytes | 289 ns | 446 ns: 1.54x slower |
@serhiy-storchaka, does the rest (static methods) looks sane for you? If so, I'll create a separate pr just for that case.
import pyperf
runner = pyperf.Runner()
n = 'from_bytes'
runner.bench_func('int.from_bytes', getattr, int, n)
