From ac20e0f98d6727ba97a9575bfa2a11b2f6247c35 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 31 Jul 2018 09:18:24 +0300 Subject: bpo-1617161: Make the hash and equality of methods not depending on the value of self. (GH-7848) * The hash of BuiltinMethodType instances no longer depends on the hash of __self__. It depends now on the hash of id(__self__). * The hash and equality of ModuleType and MethodWrapperType instances no longer depend on the hash and equality of __self__. They depend now on the hash and equality of id(__self__). * MethodWrapperType instances no longer support ordering. --- Lib/test/test_class.py | 38 +++++++--- Lib/test/test_descr.py | 83 +++++++++++++++------- .../2018-06-21-21-42-15.bpo-1617161.tSo2yM.rst | 7 ++ Objects/classobject.c | 14 ++-- Objects/descrobject.c | 33 ++++----- Objects/methodobject.c | 10 +-- 6 files changed, 112 insertions(+), 73 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-06-21-21-42-15.bpo-1617161.tSo2yM.rst diff --git a/Lib/test/test_class.py b/Lib/test/test_class.py index 841cac9..998452a 100644 --- a/Lib/test/test_class.py +++ b/Lib/test/test_class.py @@ -534,6 +534,16 @@ class ClassTests(unittest.TestCase): else: self.fail("attribute error for I.__init__ got masked") + def assertNotOrderable(self, a, b): + with self.assertRaises(TypeError): + a < b + with self.assertRaises(TypeError): + a > b + with self.assertRaises(TypeError): + a <= b + with self.assertRaises(TypeError): + a >= b + def testHashComparisonOfMethods(self): # Test comparison and hash of methods class A: @@ -544,24 +554,30 @@ class ClassTests(unittest.TestCase): def g(self): pass def __eq__(self, other): - return self.x == other.x + return True def __hash__(self): - return self.x + raise TypeError class B(A): pass a1 = A(1) - a2 = A(2) - self.assertEqual(a1.f, a1.f) - self.assertNotEqual(a1.f, a2.f) - self.assertNotEqual(a1.f, a1.g) - self.assertEqual(a1.f, A(1).f) + a2 = A(1) + self.assertTrue(a1.f == a1.f) + self.assertFalse(a1.f != a1.f) + self.assertFalse(a1.f == a2.f) + self.assertTrue(a1.f != a2.f) + self.assertFalse(a1.f == a1.g) + self.assertTrue(a1.f != a1.g) + self.assertNotOrderable(a1.f, a1.f) self.assertEqual(hash(a1.f), hash(a1.f)) - self.assertEqual(hash(a1.f), hash(A(1).f)) - self.assertNotEqual(A.f, a1.f) - self.assertNotEqual(A.f, A.g) - self.assertEqual(B.f, A.f) + self.assertFalse(A.f == a1.f) + self.assertTrue(A.f != a1.f) + self.assertFalse(A.f == A.g) + self.assertTrue(A.f != A.g) + self.assertTrue(B.f == A.f) + self.assertFalse(B.f != A.f) + self.assertNotOrderable(A.f, A.f) self.assertEqual(hash(B.f), hash(A.f)) # the following triggers a SystemError in 2.4 diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index 0e7728e..b96d35c 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -4350,38 +4350,71 @@ order (MRO) for bases """ else: self.fail("did not test __init__() for None return") + def assertNotOrderable(self, a, b): + with self.assertRaises(TypeError): + a < b + with self.assertRaises(TypeError): + a > b + with self.assertRaises(TypeError): + a <= b + with self.assertRaises(TypeError): + a >= b + def test_method_wrapper(self): # Testing method-wrapper objects... # did not support any reflection before 2.5 - - # XXX should methods really support __eq__? - l = [] - self.assertEqual(l.__add__, l.__add__) - self.assertEqual(l.__add__, [].__add__) - self.assertNotEqual(l.__add__, [5].__add__) - self.assertNotEqual(l.__add__, l.__mul__) + self.assertTrue(l.__add__ == l.__add__) + self.assertFalse(l.__add__ != l.__add__) + self.assertFalse(l.__add__ == [].__add__) + self.assertTrue(l.__add__ != [].__add__) + self.assertFalse(l.__add__ == l.__mul__) + self.assertTrue(l.__add__ != l.__mul__) + self.assertNotOrderable(l.__add__, l.__add__) self.assertEqual(l.__add__.__name__, '__add__') - if hasattr(l.__add__, '__self__'): - # CPython - self.assertIs(l.__add__.__self__, l) - self.assertIs(l.__add__.__objclass__, list) - else: - # Python implementations where [].__add__ is a normal bound method - self.assertIs(l.__add__.im_self, l) - self.assertIs(l.__add__.im_class, list) + self.assertIs(l.__add__.__self__, l) + self.assertIs(l.__add__.__objclass__, list) self.assertEqual(l.__add__.__doc__, list.__add__.__doc__) - try: - hash(l.__add__) - except TypeError: - pass - else: - self.fail("no TypeError from hash([].__add__)") + # hash([].__add__) should not be based on hash([]) + hash(l.__add__) - t = () - t += (7,) - self.assertEqual(t.__add__, (7,).__add__) - self.assertEqual(hash(t.__add__), hash((7,).__add__)) + def test_builtin_function_or_method(self): + # Not really belonging to test_descr, but introspection and + # comparison on seems not + # to be tested elsewhere + l = [] + self.assertTrue(l.append == l.append) + self.assertFalse(l.append != l.append) + self.assertFalse(l.append == [].append) + self.assertTrue(l.append != [].append) + self.assertFalse(l.append == l.pop) + self.assertTrue(l.append != l.pop) + self.assertNotOrderable(l.append, l.append) + self.assertEqual(l.append.__name__, 'append') + self.assertIs(l.append.__self__, l) + # self.assertIs(l.append.__objclass__, list) --- could be added? + self.assertEqual(l.append.__doc__, list.append.__doc__) + # hash([].append) should not be based on hash([]) + hash(l.append) + + def test_special_unbound_method_types(self): + # Testing objects of ... + self.assertTrue(list.__add__ == list.__add__) + self.assertFalse(list.__add__ != list.__add__) + self.assertFalse(list.__add__ == list.__mul__) + self.assertTrue(list.__add__ != list.__mul__) + self.assertNotOrderable(list.__add__, list.__add__) + self.assertEqual(list.__add__.__name__, '__add__') + self.assertIs(list.__add__.__objclass__, list) + + # Testing objects of ... + self.assertTrue(list.append == list.append) + self.assertFalse(list.append != list.append) + self.assertFalse(list.append == list.pop) + self.assertTrue(list.append != list.pop) + self.assertNotOrderable(list.append, list.append) + self.assertEqual(list.append.__name__, 'append') + self.assertIs(list.append.__objclass__, list) def test_not_implemented(self): # Testing NotImplemented... diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-06-21-21-42-15.bpo-1617161.tSo2yM.rst b/Misc/NEWS.d/next/Core and Builtins/2018-06-21-21-42-15.bpo-1617161.tSo2yM.rst new file mode 100644 index 0000000..bd19944 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-06-21-21-42-15.bpo-1617161.tSo2yM.rst @@ -0,0 +1,7 @@ +The hash of :class:`BuiltinMethodType` instances (methods of built-in classes) +now depends on the hash of the identity of *__self__* instead of its value. +The hash and equality of :class:`ModuleType` and :class:`MethodWrapperType` +instances (methods of user-defined classes and some methods of built-in classes +like ``str.__add__``) now depend on the hash and equality of the identity +of *__self__* instead of its value. :class:`MethodWrapperType` instances no +longer support ordering. diff --git a/Objects/classobject.c b/Objects/classobject.c index 095b0ca..a193ada 100644 --- a/Objects/classobject.c +++ b/Objects/classobject.c @@ -225,13 +225,9 @@ method_richcompare(PyObject *self, PyObject *other, int op) b = (PyMethodObject *)other; eq = PyObject_RichCompareBool(a->im_func, b->im_func, Py_EQ); if (eq == 1) { - if (a->im_self == NULL || b->im_self == NULL) - eq = a->im_self == b->im_self; - else - eq = PyObject_RichCompareBool(a->im_self, b->im_self, - Py_EQ); + eq = (a->im_self == b->im_self); } - if (eq < 0) + else if (eq < 0) return NULL; if (op == Py_EQ) res = eq ? Py_True : Py_False; @@ -274,11 +270,9 @@ method_hash(PyMethodObject *a) { Py_hash_t x, y; if (a->im_self == NULL) - x = PyObject_Hash(Py_None); + x = _Py_HashPointer(Py_None); else - x = PyObject_Hash(a->im_self); - if (x == -1) - return -1; + x = _Py_HashPointer(a->im_self); y = PyObject_Hash(a->im_func); if (y == -1) return -1; diff --git a/Objects/descrobject.c b/Objects/descrobject.c index dfad1ec..b1bee90 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1038,38 +1038,35 @@ wrapper_dealloc(wrapperobject *wp) static PyObject * wrapper_richcompare(PyObject *a, PyObject *b, int op) { - PyWrapperDescrObject *a_descr, *b_descr; + wrapperobject *wa, *wb; + int eq; assert(a != NULL && b != NULL); /* both arguments should be wrapperobjects */ - if (!Wrapper_Check(a) || !Wrapper_Check(b)) { + if ((op != Py_EQ && op != Py_NE) + || !Wrapper_Check(a) || !Wrapper_Check(b)) + { Py_RETURN_NOTIMPLEMENTED; } - /* compare by descriptor address; if the descriptors are the same, - compare by the objects they're bound to */ - a_descr = ((wrapperobject *)a)->descr; - b_descr = ((wrapperobject *)b)->descr; - if (a_descr == b_descr) { - a = ((wrapperobject *)a)->self; - b = ((wrapperobject *)b)->self; - return PyObject_RichCompare(a, b, op); + wa = (wrapperobject *)a; + wb = (wrapperobject *)b; + eq = (wa->descr == wb->descr && wa->self == wb->self); + if (eq == (op == Py_EQ)) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; } - - Py_RETURN_RICHCOMPARE(a_descr, b_descr, op); } static Py_hash_t wrapper_hash(wrapperobject *wp) { Py_hash_t x, y; - x = _Py_HashPointer(wp->descr); - if (x == -1) - return -1; - y = PyObject_Hash(wp->self); - if (y == -1) - return -1; + x = _Py_HashPointer(wp->self); + y = _Py_HashPointer(wp->descr); x = x ^ y; if (x == -1) x = -2; diff --git a/Objects/methodobject.c b/Objects/methodobject.c index 9606768..a7042ca 100644 --- a/Objects/methodobject.c +++ b/Objects/methodobject.c @@ -251,16 +251,8 @@ static Py_hash_t meth_hash(PyCFunctionObject *a) { Py_hash_t x, y; - if (a->m_self == NULL) - x = 0; - else { - x = PyObject_Hash(a->m_self); - if (x == -1) - return -1; - } + x = _Py_HashPointer(a->m_self); y = _Py_HashPointer((void*)(a->m_ml->ml_meth)); - if (y == -1) - return -1; x ^= y; if (x == -1) x = -2; -- cgit v0.12