[GR-53764] Migrate to sq_item and mp_item slots.

PullRequest: graalpython/3372

Author: steve-s

graalpython/com.oracle.graal.python.annotations/src/com/oracle/graal/python/annotations/Slot.java

@@ -94,7 +94,9 @@
     enum SlotKind {
         nb_bool,
         sq_length,
+        sq_item,
         mp_length,
+        mp_subscript,
         tp_descr_get,
         tp_descr_set,
         tp_getattro,

graalpython/com.oracle.graal.python.hpy.test/src/hpytest/test_slots.py

@@ -1,6 +1,6 @@
 # MIT License
 # 
-# Copyright (c) 2020, 2023, Oracle and/or its affiliates.
+# Copyright (c) 2020, 2024, Oracle and/or its affiliates.
 # Copyright (c) 2019 pyhandle
 # 
 # Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -633,7 +633,10 @@ def test_sq_item_and_sq_length(self):
         assert len(p) == 1234
         assert p[4] == 8
         assert p[21] == 42
-        assert p[-1] == 1233 * 2
+        assert p[-1] == 1233 * 2, str(p[-1])
+        assert p.__getitem__(4) == 8
+        assert p.__getitem__(21) == 42
+        assert p.__getitem__(-1) == 1233 * 2
 
     def test_sq_ass_item(self):
         import pytest

graalpython/com.oracle.graal.python.processor/src/com/oracle/graal/python/processor/SlotsMapping.java

@@ -52,6 +52,8 @@ static String getSlotBaseClass(Slot s) {
         return switch (s.value()) {
             case nb_bool -> "TpSlotInquiry.TpSlotInquiryBuiltin";
             case sq_length, mp_length -> "TpSlotLen.TpSlotLenBuiltin" + getSuffix(s.isComplex());
+            case sq_item -> "TpSlotSizeArgFun.TpSlotSizeArgFunBuiltin";
+            case mp_subscript -> "TpSlotBinaryFunc.TpSlotMpSubscript";
             case tp_getattro -> "TpSlotGetAttr.TpSlotGetAttrBuiltin";
             case tp_descr_get -> "TpSlotDescrGet.TpSlotDescrGetBuiltin" + getSuffix(s.isComplex());
             case tp_descr_set -> "TpSlotDescrSet.TpSlotDescrSetBuiltin";
@@ -64,6 +66,8 @@ static String getSlotNodeBaseClass(Slot s) {
             case tp_descr_get -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotDescrGet.DescrGetBuiltinNode";
             case nb_bool -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotInquiry.NbBoolBuiltinNode";
             case sq_length, mp_length -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotLen.LenBuiltinNode";
+            case sq_item -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotSizeArgFun.SqItemBuiltinNode";
+            case mp_subscript -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotBinaryFunc.MpSubscriptBuiltinNode";
             case tp_getattro -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotGetAttr.GetAttrBuiltinNode";
             case tp_descr_set -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotDescrSet.DescrSetBuiltinNode";
             case tp_setattro -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotSetAttr.SetAttrBuiltinNode";
@@ -75,22 +79,24 @@ static String getUncachedExecuteSignature(SlotKind s) {
             case nb_bool -> "boolean executeUncached(Object self)";
             case tp_descr_get -> "Object executeUncached(Object self, Object obj, Object type)";
             case sq_length, mp_length -> "int executeUncached(Object self)";
-            case tp_getattro, tp_descr_set, tp_setattro ->
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript ->
                 throw new AssertionError("Should not reach here: should be always complex");
         };
     }
 
     static boolean supportsComplex(SlotKind s) {
         return switch (s) {
             case nb_bool -> false;
-            case sq_length, mp_length, tp_getattro, tp_descr_get, tp_descr_set, tp_setattro -> true;
+            case sq_length, mp_length, tp_getattro, tp_descr_get, tp_descr_set,
+                            tp_setattro, sq_item, mp_subscript ->
+                true;
         };
     }
 
     static boolean supportsSimple(SlotKind s) {
         return switch (s) {
             case nb_bool, sq_length, mp_length, tp_descr_get -> true;
-            case tp_getattro, tp_descr_set, tp_setattro -> false;
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript -> false;
         };
     }
 
@@ -99,7 +105,7 @@ static String getUncachedExecuteCall(SlotKind s) {
             case nb_bool -> "executeBool(null, self)";
             case sq_length, mp_length -> "executeInt(null, self)";
             case tp_descr_get -> "execute(null, self, obj, type)";
-            case tp_getattro, tp_descr_set, tp_setattro ->
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript ->
                 throw new AssertionError("Should not reach here: should be always complex");
         };
     }

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/builtin/objects/TpSlotsTests.java

@@ -40,11 +40,14 @@
  */
 package com.oracle.graal.python.test.builtin.objects;
 
+import java.util.EnumSet;
+
 import org.junit.Assert;
 import org.junit.Test;
 
 import com.oracle.graal.python.builtins.objects.type.TpSlots;
 import com.oracle.graal.python.builtins.objects.type.TpSlots.Builder;
+import com.oracle.graal.python.builtins.objects.type.TpSlots.TpSlotGroup;
 import com.oracle.graal.python.builtins.objects.type.TpSlots.TpSlotMeta;
 import com.oracle.graal.python.builtins.objects.type.slots.TpSlot;
 import com.oracle.graal.python.builtins.objects.type.slots.TpSlot.TpSlotNative;
@@ -57,7 +60,7 @@ public void testBuilderBasic() {
         for (TpSlotMeta def : TpSlotMeta.VALUES) {
             // Use the TpSlotMeta as dummy "callable" object to verify that the slot values were
             // properly assigned to the right fields of TpSlots record
-            builder.set(def, new TpSlotNative(def));
+            builder.set(def, TpSlotNative.createCExtSlot(def));
         }
 
         TpSlots slots = builder.build();
@@ -72,9 +75,9 @@ public void testBuilderBasic() {
     @Test
     public void testBuilderOptimizations1() {
         Builder builder = TpSlots.newBuilder();
-        builder.set(TpSlotMeta.MP_LENGTH, new TpSlotNative(TpSlotMeta.MP_LENGTH));
-        builder.set(TpSlotMeta.TP_GETATTR, new TpSlotNative(TpSlotMeta.TP_GETATTR));
-        builder.set(TpSlotMeta.TP_SETATTR, new TpSlotNative(TpSlotMeta.TP_SETATTR));
+        builder.set(TpSlotMeta.MP_LENGTH, TpSlotNative.createCExtSlot(TpSlotMeta.MP_LENGTH));
+        builder.set(TpSlotMeta.TP_GETATTR, TpSlotNative.createCExtSlot(TpSlotMeta.TP_GETATTR));
+        builder.set(TpSlotMeta.TP_SETATTR, TpSlotNative.createCExtSlot(TpSlotMeta.TP_SETATTR));
 
         TpSlots slots = builder.build();
         verifySlots(slots, def -> def == TpSlotMeta.MP_LENGTH || def == TpSlotMeta.TP_GETATTR || def == TpSlotMeta.TP_SETATTR);
@@ -88,7 +91,7 @@ public void testBuilderOptimizations1() {
     @Test
     public void testBuilderOptimizations2() {
         Builder builder = TpSlots.newBuilder();
-        builder.set(TpSlotMeta.SQ_LENGTH, new TpSlotNative(TpSlotMeta.SQ_LENGTH));
+        builder.set(TpSlotMeta.SQ_LENGTH, TpSlotNative.createCExtSlot(TpSlotMeta.SQ_LENGTH));
 
         TpSlots slots = builder.build();
         verifySlots(slots, def -> def == TpSlotMeta.SQ_LENGTH);
@@ -99,14 +102,25 @@ public void testBuilderOptimizations2() {
     }
 
     private static void verifySlots(TpSlots slots, Function<TpSlotMeta, Boolean> checkNonNullValue) {
+        var groupsSeen = EnumSet.noneOf(TpSlotGroup.class);
         for (TpSlotMeta def : TpSlotMeta.VALUES) {
             TpSlot slotValue = def.getValue(slots);
             if (checkNonNullValue.apply(def)) {
                 checkSlotValue(def, slotValue);
+                groupsSeen.add(def.getGroup());
             } else {
                 Assert.assertNull(def.name(), slotValue);
             }
         }
+        for (TpSlotGroup group : TpSlotGroup.values()) {
+            switch (group) {
+                case NO_GROUP -> {
+                }
+                case AS_NUMBER -> Assert.assertEquals(slots.has_as_number(), groupsSeen.contains(group));
+                case AS_SEQUENCE -> Assert.assertEquals(slots.has_as_sequence(), groupsSeen.contains(group));
+                case AS_MAPPING -> Assert.assertEquals(slots.has_as_mapping(), groupsSeen.contains(group));
+            }
+        }
     }
 
     private static void checkSlotValue(TpSlotMeta def, TpSlot slotValue) {

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

@@ -40,6 +40,7 @@
 import array
 import collections
 import sys
+import mmap
 
 from . import CPyExtTestCase, CPyExtFunction, CPyExtType, unhandled_error_compare
 
@@ -177,6 +178,8 @@ def istypeof(obj, types):
 def _reference_getitem(args):
     seq = args[0]
     idx = args[1]
+    if isinstanceof(seq, [mmap.mmap]):
+        return seq[idx:(idx+1)]
     if not hasattr(seq, '__getitem__'):
         raise TypeError
     return seq.__getitem__(idx)
@@ -208,6 +211,13 @@ def wrapped_fun(args):
         return fun(args)
     return wrapped_fun
 
+
+def create_anon_mmap(data):
+    mmap_object = mmap.mmap(-1, len(data) + 1)
+    mmap_object.write(data)
+    return mmap_object
+
+
 class NoNumber():
     pass
 
@@ -1175,13 +1185,14 @@ class TestAbstract(CPyExtTestCase):
             (DummyListSubclass(), 1),
             ('hello', 1),
             (DictSubclassWithSequenceMethods(), 1),
+            (create_anon_mmap(b'hello world!'), 3),
         ),
         resultspec="O",
         argspec='On',
         arguments=["PyObject* sequence", "Py_ssize_t idx"],
         cmpfunc=unhandled_error_compare
     )
-    
+
     test_PySequence_GetSlice = CPyExtFunction(
         _wrap_slice_fun(lambda args: args[0][args[1]:args[2]]),
         lambda: (
@@ -1331,6 +1342,7 @@ def _reference_delslice(args):
             (['a', 'b', 'c'], 2),
             ([None], 0),
             ('hello', 0),
+            (create_anon_mmap(b'hello world!'), 3),
         ),
         resultspec="O",
         argspec='On',

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

@@ -644,10 +644,7 @@ def test_tp_as_buffer_pickle(self):
 
 
 class TestNativeSubclass(unittest.TestCase):
-    def test_builtins(self):
-        b = BytesSubclass(b"hello")
-        assert is_native_object(b)
-        assert type(b) == BytesSubclass
+    def _verify_common(self, b):
         assert b
         assert not BytesSubclass(b'')
         assert len(b) == 5
@@ -660,13 +657,11 @@ def test_builtins(self):
         assert b + b" world" == b"hello world"
         assert b * 2 == b"hellohello"
         assert list(b) == [ord('h'), ord('e'), ord('l'), ord('l'), ord('o')]
-        assert repr(b) == "b'hello'"
         assert b.index(ord('l')) == 2
         assert b.count(ord('l')) == 2
         assert b.decode('ascii') == 'hello'
         assert BytesSubclass(b'hello ').strip() == b'hello'
         assert BytesSubclass(b',').join([b'a', BytesSubclass(b'b')]) == b'a,b'
-        assert hash(b) == hash(b'hello')
         assert BytesSubclass(b'(%s)') % b'a' == b'(a)'
         assert b.startswith(b'h')
         assert b.endswith(b'o')
@@ -676,8 +671,43 @@ def test_builtins(self):
         assert b.upper() == b'HELLO'
         assert BytesSubclass(b'a,b').split(BytesSubclass(b',')) == [b'a', b'b']
 
-    def test_managed_subclass(self):
+    def _verify_bytes(self, b):
+        self._verify_common(b)
+        assert is_native_object(b)
+        assert repr(b) == "b'hello'", repr(b)
+        assert hash(b) == hash(b'hello')
+
+    def _verify_bytearray(self, b):
+        self._verify_common(b)
+        # assert is_native_object(b) # TODO: BuiltinConstructors.ByteArrayNode doesnt allocate native objects
+        self.assertRaises(TypeError, lambda: hash(b))
+
+    def test_bytes_subclass(self):
+        b = BytesSubclass(b"hello")
+        assert type(b) == BytesSubclass
+        self._verify_bytes(b)
+
+    def test_managed_bytes_subclass(self):
         class ManagedSubclass(BytesSubclass):
             pass
 
-        assert is_native_object(ManagedSubclass(b"hello"))
+        b = ManagedSubclass(b"hello")
+        assert type(b) == ManagedSubclass
+        self._verify_bytes(b)
+
+    def test_bytearray_subclass(self):
+        b = ByteArraySubclass(b"hello")
+        assert type(b) == ByteArraySubclass
+        # TODO: if TypeNode.GetNameNode is equivalent to _PyType_Name, it should transform tp_name "abc.xzy" to "xyz"
+        # assert repr(b) == "ByteArraySubclass(b'hello')", repr(b)
+        self._verify_bytearray(b)
+
+    def test_managed_bytearray_subclass(self):
+        class ArrManagedSubclass(ByteArraySubclass):
+            pass
+
+        b = ArrManagedSubclass(b"hello")
+        assert type(b) == ArrManagedSubclass
+        # TODO: if TypeNode.GetNameNode is equivalent to _PyType_Name, it should transform tp_name "abc.xzy" to "xyz"
+        # assert repr(b) == "ArrManagedSubclass(b'hello')", repr(b)
+        self._verify_bytearray(b)

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

@@ -1312,7 +1312,8 @@ def test_slot_precedence(self):
         # 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 len(obj) == 111
+        assert obj['hello'] == 'mp_subscript', obj['hello']
         assert obj + 'hello' == 'mas_nb_add'
 
     def test_take_ownership(self):

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

@@ -379,3 +379,62 @@ def test_type_not_ready():
     """, "NotReadyType")
     not_ready = module.create_not_ready()
     assert not_ready.foo == 'foo'
+
+
+def test_sq_len_and_item():
+    MySqLenItem = CPyExtType("MySqLenItem",
+                             '''
+                             Py_ssize_t my_sq_len(PyObject* a) { return 10; }
+                             PyObject* my_sq_item(PyObject* self, Py_ssize_t index) { return PyLong_FromSsize_t(index); }
+                             ''',
+                             sq_length="&my_sq_len",
+                             sq_item="my_sq_item")
+    MySqItemAddDunderLen = CPyExtHeapType("MySqItemAddDunderLen",
+                              code = '''
+                                        PyObject* my_sq_item(PyObject* self, Py_ssize_t index) { return PyLong_FromSsize_t(index); }
+                                        ''',
+                              slots=[
+                                  '{Py_sq_item, &my_sq_item}',
+                              ])
+    MySqItemAddDunderLen.__len__ = lambda self: 10
+
+    def verify(x):
+        assert x[5] == 5
+        assert x.__getitem__(5) == 5
+        assert x[-1] == 9
+        assert x.__getitem__(-1) == 9
+        assert x[-20] == -10
+        assert x.__getitem__(-20) == -10
+
+    verify(MySqLenItem())
+    verify(MySqItemAddDunderLen())
+
+
+def test_mp_len_and_sq_item():
+    MyMpLenSqItem = CPyExtType("MyMpLenSqItem",
+                             '''
+                             Py_ssize_t my_mp_len(PyObject* a) { return 10; }
+                             PyObject* my_sq_item(PyObject* self, Py_ssize_t index) { return PyLong_FromSsize_t(index); }
+                             ''',
+                             mp_length="&my_mp_len",
+                             sq_item="my_sq_item")
+    MyMpLenSqItemHeap = CPyExtHeapType("MyMpLenSqItemHeap",
+                                       code = '''
+                                        Py_ssize_t my_mp_len(PyObject* a) { return 10; }
+                                        PyObject* my_sq_item(PyObject* self, Py_ssize_t index) { return PyLong_FromSsize_t(index); }
+                                        ''',
+                                       slots=[
+                                           '{Py_mp_length, &my_mp_len}',
+                                           '{Py_sq_item, &my_sq_item}',
+                                       ])
+    def verify(x):
+        assert x[5] == 5
+        assert x.__getitem__(5) == 5
+        # no sq_length, negative index is just passed to sq_item
+        assert x[-1] == -1
+        assert x.__getitem__(-1) == -1
+        assert x[-20] == -20
+        assert x.__getitem__(-20) == -20
+
+    verify(MyMpLenSqItem())
+    verify(MyMpLenSqItemHeap())

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

@@ -246,9 +246,7 @@ class TestPyTuple(CPyExtTestCase):
 
 
 class TestNativeSubclass(unittest.TestCase):
-    def test_builtins(self):
-        t = TupleSubclass(1, 2, 3)
-        assert type(t) == TupleSubclass
+    def _verify(self, t):
         assert is_native_object(t)
         assert t
         assert len(t) == 3
@@ -265,8 +263,16 @@ def test_builtins(self):
         assert t.index(2) == 1
         assert t.count(2) == 1
 
+    def test_builtins(self):
+        t = TupleSubclass(1, 2, 3)
+        assert type(t) == TupleSubclass
+        self._verify(t)
+
     def test_managed_sublclass(self):
         class ManagedSubclass(TupleSubclass):
             pass
 
-        assert is_native_object(ManagedSubclass(1, 2, 3))
+        t = ManagedSubclass(1, 2, 3)
+        assert is_native_object(t)
+        assert type(t) == ManagedSubclass
+        self._verify(t)