[GR-55197] Fix inheritance of tp_hash in native types

PullRequest: graalpython/3400

Author: msimacek

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

@@ -6291,7 +6291,6 @@ type_ready_inherit(PyTypeObject *type)
 }
 
 
-#if 0 // GraalPy change
 /* Hack for tp_hash and __hash__.
    If after all that, tp_hash is still NULL, and __hash__ is not in
    tp_dict, set tp_hash to PyObject_HashNotImplemented and
@@ -6304,21 +6303,22 @@ type_ready_set_hash(PyTypeObject *type)
         return 0;
     }
 
-    int r = PyDict_Contains(type->tp_dict, &_Py_ID(__hash__));
+    // GraalPy change: cannot use CPython's current _Py_ID mechanism
+    int r = _PyDict_ContainsId(type->tp_dict, &PyId___hash__);
     if (r < 0) {
         return -1;
     }
     if (r > 0) {
         return 0;
     }
 
-    if (PyDict_SetItem(type->tp_dict, &_Py_ID(__hash__), Py_None) < 0) {
+    // GraalPy change: cannot use CPython's current _Py_ID mechanism
+    if (_PyDict_SetItemId(type->tp_dict, &PyId___hash__, Py_None) < 0) {
         return -1;
     }
     type->tp_hash = PyObject_HashNotImplemented;
     return 0;
 }
-#endif // GraalPy change
 
 
 /* Link into each base class's list of subclasses */
@@ -6476,11 +6476,10 @@ type_ready(PyTypeObject *type)
      * when the C API is not loaded, we need to do that later.
      */
     GraalPyTruffle_AddInheritedSlots(type);
-#if 0 // GraalPy change
+
     if (type_ready_set_hash(type) < 0) {
         return -1;
     }
-#endif // GraalPy change
     if (type_ready_add_subclasses(type) < 0) {
         return -1;
     }

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

@@ -37,7 +37,7 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
 import sys
-from . import CPyExtType, CPyExtHeapType, compile_module_from_string
+from . import CPyExtType, CPyExtHeapType, compile_module_from_string, assert_raises
 
 SlotsGetter = CPyExtType("SlotsGetter",
                          """
@@ -437,4 +437,79 @@ def verify(x):
         assert x.__getitem__(-20) == -20
 
     verify(MyMpLenSqItem())
-    verify(MyMpLenSqItemHeap())
+    verify(MyMpLenSqItemHeap())
+
+
+def test_tp_hash():
+    TypeWithHash = CPyExtType(
+        "TypeWithHash",
+        '''
+        static PyObject* richcompare(PyObject* self, PyObject* other, int cmp) {
+            Py_RETURN_NOTIMPLEMENTED;
+        }
+        static Py_hash_t hash(PyObject* self) {
+            return 123;
+        }
+        ''',
+        tp_richcompare='richcompare',
+        tp_hash='hash',
+    )
+    assert TypeWithHash.__hash__
+    assert hash(TypeWithHash()) == 123
+
+    class InheritsHash(TypeWithHash):
+        pass
+
+    assert InheritsHash.__hash__
+    assert hash(InheritsHash()) == 123
+
+    TypeWithoutHash = CPyExtType(
+        "TypeWithoutHash",
+        '''
+        static PyObject* richcompare(PyObject* self, PyObject* other, int cmp) {
+            Py_RETURN_NOTIMPLEMENTED;
+        }
+        static PyObject* has_hash_not_implemented(PyObject* unused, PyObject* obj) {
+            return PyBool_FromLong(Py_TYPE(obj)->tp_hash == PyObject_HashNotImplemented);
+        }
+        ''',
+        tp_richcompare='richcompare',
+        tp_methods='{"has_hash_not_implemented", (PyCFunction)has_hash_not_implemented, METH_STATIC | METH_O, ""}',
+    )
+
+    def assert_has_no_hash(obj):
+        assert_raises(TypeError, hash, obj)
+        assert type(obj).__hash__ is None
+        assert TypeWithoutHash.has_hash_not_implemented(obj)
+
+    assert_has_no_hash(TypeWithoutHash())
+
+    class OverridesEq1:
+        def __eq__(self, other):
+            return self is other
+
+    assert_has_no_hash(OverridesEq1())
+
+    class OverridesEq2(TypeWithHash):
+        def __eq__(self, other):
+            return self is other
+
+    assert_has_no_hash(OverridesEq2())
+
+    class DisablesHash1:
+        __hash__ = None
+
+    assert_has_no_hash(DisablesHash1())
+
+    class DisablesHash2(TypeWithHash):
+        __hash__ = None
+
+    assert_has_no_hash(DisablesHash2())
+
+    # TODO GR-55196
+    # TypeWithoutHashExplicit = CPyExtType(
+    #     "TypeWithoutHashExplicit",
+    #     tp_hash='PyObject_HashNotImplemented',
+    # )
+    #
+    # assert_has_no_hash(TypeWithoutHashExplicit())

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/NativeCAPISymbol.java

@@ -158,6 +158,7 @@ public enum NativeCAPISymbol implements NativeCExtSymbol {
     FUN_MMAP_INIT_BUFFERPROTOCOL("mmap_init_bufferprotocol", ArgDescriptor.Void, PyTypeObject),
     FUN_PY_TRUFFLE_CDATA_INIT_BUFFER_PROTOCOL("PyTruffleCData_InitBufferProtocol", ArgDescriptor.Void, PyTypeObject),
     FUN_TRUFFLE_CHECK_TYPE_READY("truffle_check_type_ready", ArgDescriptor.Void, PyTypeObject),
+    FUN_PYOBJECT_HASH_NOT_IMPLEMENTED("PyObject_HashNotImplemented", ArgDescriptor.Py_hash_t, PyObject),
 
     /* PyDateTime_CAPI */
 

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/LookupNativeSlotNode.java

@@ -42,6 +42,8 @@
 
 import com.oracle.graal.python.builtins.objects.PNone;
 import com.oracle.graal.python.builtins.objects.cext.PythonAbstractNativeObject;
+import com.oracle.graal.python.builtins.objects.cext.capi.CApiContext;
+import com.oracle.graal.python.builtins.objects.cext.capi.NativeCAPISymbol;
 import com.oracle.graal.python.builtins.objects.cext.capi.SlotMethodDef;
 import com.oracle.graal.python.builtins.objects.cext.structs.CStructAccess;
 import com.oracle.graal.python.builtins.objects.type.PythonAbstractClass;
@@ -138,6 +140,9 @@ private static Object readSlot(SlotMethodDef slot, PythonAbstractClass currentTy
             }
             Object value = readNode.execute(currentType, slot.methodName);
             if (value != PNone.NO_VALUE) {
+                if (slot == SlotMethodDef.TP_HASH && value == PNone.NONE) {
+                    return CApiContext.getNativeSymbol(null, NativeCAPISymbol.FUN_PYOBJECT_HASH_NOT_IMPLEMENTED);
+                }
                 return wrapManagedMethod(slot, (PythonManagedClass) currentType, value);
             }
         }