[GR-45310] Fix slot precedence wrt installed attributes.

PullRequest: graalpython/2710

Author: fangerer

graalpython/com.oracle.graal.python.cext/src/typeobject.c

@@ -538,6 +538,12 @@ int PyType_Ready(PyTypeObject* cls) {
     type_ready_set_new(cls, dict, base);
 
     /* fill dict */
+
+    /*
+     * NOTE: ADD_SLOT_CONV won't overwrite existing attributes, so the order is crucial and must
+     * reflect CPython's 'slotdefs' array.
+     */
+
     // add special methods defined directly on the type structs
     ADD_SLOT_CONV("__dealloc__", cls->tp_dealloc, -1, JWRAPPER_DIRECT);
     // https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_getattr
@@ -583,20 +589,7 @@ int PyType_Ready(PyTypeObject* cls) {
     ADD_SLOT_CONV("__del__", cls->tp_del, -1, JWRAPPER_DIRECT);
     ADD_SLOT_CONV("__finalize__", cls->tp_finalize, -1, JWRAPPER_DIRECT);
 
-    PySequenceMethods* sequences = PyTypeObject_tp_as_sequence(cls);
-    if (sequences) {
-    	// sequence functions first, so that the number functions take precendence
-        ADD_SLOT_CONV("__len__", sequences->sq_length, -1, JWRAPPER_LENFUNC);
-        ADD_SLOT_CONV("__add__", sequences->sq_concat, -2, JWRAPPER_BINARYFUNC);
-        ADD_SLOT_CONV("__mul__", sequences->sq_repeat, -2, JWRAPPER_SSIZE_ARG);
-        ADD_SLOT_CONV("__getitem__", sequences->sq_item, -2, JWRAPPER_GETITEM);
-        ADD_SLOT_CONV("__setitem__", sequences->sq_ass_item, -3, JWRAPPER_SETITEM);
-        ADD_SLOT_CONV("__delitem__", sequences->sq_ass_item, -3, JWRAPPER_DELITEM);
-        ADD_SLOT_CONV("__contains__", sequences->sq_contains, -2, JWRAPPER_OBJOBJPROC);
-        ADD_SLOT_CONV("__iadd__", sequences->sq_inplace_concat, -2, JWRAPPER_BINARYFUNC);
-        ADD_SLOT_CONV("__imul__", sequences->sq_inplace_repeat, -2, JWRAPPER_SSIZE_ARG);
-    }
-
+    // 'tp_as_number' takes precedence over 'tp_as_mapping' and 'tp_as_sequence' !
     PyNumberMethods* numbers = PyTypeObject_tp_as_number(cls);
     if (numbers) {
         ADD_SLOT_CONV("__add__", numbers->nb_add, -2, JWRAPPER_BINARYFUNC_L);
@@ -650,6 +643,7 @@ int PyType_Ready(PyTypeObject* cls) {
         ADD_SLOT_CONV("__imatmul__", numbers->nb_inplace_matrix_multiply, -2, JWRAPPER_BINARYFUNC_L);
     }
 
+    // 'tp_as_mapping' takes precedence over 'tp_as_sequence' !
     PyMappingMethods* mappings = PyTypeObject_tp_as_mapping(cls);
     if (mappings) {
         ADD_SLOT_CONV("__len__", mappings->mp_length, -1, JWRAPPER_LENFUNC);
@@ -658,6 +652,20 @@ int PyType_Ready(PyTypeObject* cls) {
         ADD_SLOT_CONV("__delitem__", mappings->mp_ass_subscript, -3, JWRAPPER_MP_DELITEM);
     }
 
+    PySequenceMethods* sequences = PyTypeObject_tp_as_sequence(cls);
+    if (sequences) {
+        // sequence functions first, so that the number functions take precendence
+        ADD_SLOT_CONV("__len__", sequences->sq_length, -1, JWRAPPER_LENFUNC);
+        ADD_SLOT_CONV("__add__", sequences->sq_concat, -2, JWRAPPER_BINARYFUNC);
+        ADD_SLOT_CONV("__mul__", sequences->sq_repeat, -2, JWRAPPER_SSIZE_ARG);
+        ADD_SLOT_CONV("__getitem__", sequences->sq_item, -2, JWRAPPER_GETITEM);
+        ADD_SLOT_CONV("__setitem__", sequences->sq_ass_item, -3, JWRAPPER_SETITEM);
+        ADD_SLOT_CONV("__delitem__", sequences->sq_ass_item, -3, JWRAPPER_DELITEM);
+        ADD_SLOT_CONV("__contains__", sequences->sq_contains, -2, JWRAPPER_OBJOBJPROC);
+        ADD_SLOT_CONV("__iadd__", sequences->sq_inplace_concat, -2, JWRAPPER_BINARYFUNC);
+        ADD_SLOT_CONV("__imul__", sequences->sq_inplace_repeat, -2, JWRAPPER_SSIZE_ARG);
+    }
+
     PyAsyncMethods* async = PyTypeObject_tp_as_async(cls);
     if (async) {
         ADD_SLOT_CONV("__await__", async->am_await, -1, JWRAPPER_DIRECT);

graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_object.py

@@ -741,6 +741,42 @@ class X(_A, B):
         x = X()
         assert x.foo == "foo"
 
+    def test_slot_precedence(self):
+        MapAndSeq = CPyExtType("MapAndSeq",
+                               '''
+                               static PyObject * mas_nb_add(PyObject *self, PyObject *other) {
+                                   return PyUnicode_FromString("mas_nb_add");
+                               }
+                               static Py_ssize_t mas_sq_length(PyObject *self) {
+                                   return 111;
+                               }
+                               static PyObject *mas_sq_item(PyObject *self, Py_ssize_t idx) {
+                                   return PyUnicode_FromString("sq_item");
+                               }
+                               static PyObject * mas_sq_concat(PyObject *self, PyObject *other) {
+                                   return PyUnicode_FromString("mas_sq_concat");
+                               }
+                               static Py_ssize_t mas_mp_length(PyObject *self) {
+                                   return 222;
+                               }
+                               static PyObject * mas_mp_subscript(PyObject *self, PyObject *key) {
+                                   return PyUnicode_FromString("mp_subscript");
+                               }
+                               ''',
+                               nb_add='mas_nb_add',
+                               sq_length='mas_sq_length',
+                               sq_item='mas_sq_item',
+                               sq_concat='mas_sq_concat',
+                               mp_length='mas_mp_length',
+                               mp_subscript='mas_mp_subscript',
+                               )
+        obj = MapAndSeq()
+        # Note: len(obj) uses 'PyObject_Lenght' which does not use the attribute but first tries
+        # 'sq_length' and falls back to 'mp_length'. Therefore, we just look at '__len__' here.
+        assert obj.__len__() == 222
+        assert obj['hello'] == 'mp_subscript'
+        assert obj + 'hello' == 'mas_nb_add'
+
 
 class CBytes: 
     def __bytes__(self):