Py_TYPE(lhs)->tp_as_mapping->mp_subscript is not the same as Dict_Type.tp_as_mapping->mp_subscript when it should be

[iritkatriel]

A dict subclass which does not override __getitem__ (like collections.Counter) should have Dict_Type.tp_as_mapping->mp_subscript as its Py_TYPE(lhs)->tp_as_mapping->mp_subscript, but they are not the same.

CC @ericsnowcurrently .

Linked PRs

• gh-132284: Don't wrap base PyCFunction slots on class creation if not overridden #132329

[ericsnowcurrently]

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 of dict/Counter. That seems like something worth improving.

FWIW, this happened for Counter but not for dict because the former comes from a class definition and the latter is a static type. update_one_slot() is called by fixup_slot_dispatchers(), which is only called by type_new_impl(), which is only called by type_new() (the value of PyType_Type.tp_new). That is called by type() when a class definition is turned into a class, like in the case of Counter. It is not called for static types. Thus mp_subscript is updated by update_one_slot() for Counter but dict keeps its original mp_subscript.

[tom-pytel]

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 loop

to:

$ ./python -m timeit -s $'class d(dict): pass\ni = d({1:2})' "i[1]"
10000000 loops, best of 5: 21.6 nsec per loop

Rationale for the checks is as follows:

• generic == NULL because not sure of behavior with slots that loop several times like tp_getattr, tp_setattr and tp_richcompare.
• Py_IS_TYPE(descr, &PyMethodDescr_Type) because only if setting pre-existing builtin C method.
• *ptr == ((PyMethodDescrObject *)descr)->d_method->ml_meth checks both *ptr != NULL and 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?

[ericsnowcurrently]

That makes sense. Go ahead and make a PR.