Repository navigation
Py_TYPE(lhs)->tp_as_mapping->mp_subscript is not the same as Dict_Type.tp_as_mapping->mp_subscript when it should be #132284
Description
Activity
Apparently,
update_one_slot()in typeobject.c always sets the slot, whether or not it is already set to a perfectly acceptable value, like in the case ofdict/Counter. That seems like something worth improving.FWIW, this happened for
Counterbut not fordictbecause the former comes from a class definition and the latter is a static type.update_one_slot()is called byfixup_slot_dispatchers(), which is only called bytype_new_impl(), which is only called bytype_new()(the value ofPyType_Type.tp_new). That is called bytype()when a class definition is turned into a class, like in the case ofCounter. It is not called for static types. Thusmp_subscriptis updated byupdate_one_slot()forCounterbutdictkeeps its originalmp_subscript.The following small change leaves previous C method slots unchanged in subclasses if not overridden (including
Counter.__getitem__):diff --git a/Objects/typeobject.c b/Objects/typeobject.c index b92eaefc90d..69bcc32e7c0 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11184,7 +11184,14 @@ update_one_slot(PyTypeObject *type, pytype_slotdef *p) } else { use_generic = 1; - generic = p->function; + if (generic == NULL && Py_IS_TYPE(descr, &PyMethodDescr_Type) && + *ptr == ((PyMethodDescrObject *)descr)->d_method->ml_meth) + { + generic = *ptr; + } + else { + generic = p->function; + } if (p->function == slot_tp_call) { /* A generic __call__ is incompatible with vectorcall */ type_clear_flags(type, Py_TPFLAGS_HAVE_VECTORCALL);
Subclass
__getitem__()performance even increases a bit from:$ ./python -m timeit -s $'class d(dict): pass\ni = d({1:2})' "i[1]" 10000000 loops, best of 5: 28.5 nsec per loopto:
$ ./python -m timeit -s $'class d(dict): pass\ni = d({1:2})' "i[1]" 10000000 loops, best of 5: 21.6 nsec per loopRationale for the checks is as follows:
generic == NULLbecause not sure of behavior with slots that loop several times liketp_getattr,tp_setattrandtp_richcompare.Py_IS_TYPE(descr, &PyMethodDescr_Type)because only if setting pre-existing builtin C method.*ptr == ((PyMethodDescrObject *)descr)->d_method->ml_methchecks both*ptr != NULLand stuff like:
class C: __new__ = type.__dict__['__subclasscheck__']Passes all the tests but I don't have too much experience in this part of the code so there may be some other check or two still needed?
Do you want this as a PR?
That makes sense. Go ahead and make a PR.
- added a commit that references this issue
on Apr 17, 2025 I marked it as a bug but I don't know if we want to backport this as it could be an important behavior change for some users.
I marked it as a bug but I don't know if we want to backport this as it could be an important behavior change for some users.
Would backporting give any benefit? And I don't know if I would classify this as a bug as it doesn't really break anything.
I may have misunderstood the issue then. Let's consider it as a feature.
A dict subclass which does not override
__getitem__(likecollections.Counter) should haveDict_Type.tp_as_mapping->mp_subscriptas itsPy_TYPE(lhs)->tp_as_mapping->mp_subscript, but they are not the same.CC @ericsnowcurrently .
Linked PRs