Issue #16453: Fix equality testing of dead weakref objects.

Also add tests for ordering and hashing.

3 files changed, 93 insertions, 24 deletions

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index 74b9a87..92a4713 100644
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -32,6 +32,27 @@ def create_bound_method():
     return C().method
 
 
+class Object:
+    def __init__(self, arg):
+        self.arg = arg
+    def __repr__(self):
+        return "<Object %r>" % self.arg
+    def __eq__(self, other):
+        if isinstance(other, Object):
+            return self.arg == other.arg
+        return NotImplemented
+    def __lt__(self, other):
+        if isinstance(other, Object):
+            return self.arg < other.arg
+        return NotImplemented
+    def __hash__(self):
+        return hash(self.arg)
+
+class RefCycle:
+    def __init__(self):
+        self.cycle = self
+
+
 class TestBase(unittest.TestCase):
 
     def setUp(self):
@@ -692,6 +713,69 @@ class ReferencesTestCase(TestBase):
         self.assertEqual(a(), None)
         self.assertEqual(l, [a])
 
+    def test_equality(self):
+        # Alive weakrefs defer equality testing to their underlying object.
+        x = Object(1)
+        y = Object(1)
+        z = Object(2)
+        a = weakref.ref(x)
+        b = weakref.ref(y)
+        c = weakref.ref(z)
+        d = weakref.ref(x)
+        # Note how we directly test the operators here, to stress both
+        # __eq__ and __ne__.
+        self.assertTrue(a == b)
+        self.assertFalse(a != b)
+        self.assertFalse(a == c)
+        self.assertTrue(a != c)
+        self.assertTrue(a == d)
+        self.assertFalse(a != d)
+        del x, y, z
+        gc.collect()
+        for r in a, b, c:
+            # Sanity check
+            self.assertIs(r(), None)
+        # Dead weakrefs compare by identity: whether `a` and `d` are the
+        # same weakref object is an implementation detail, since they pointed
+        # to the same original object and didn't have a callback.
+        # (see issue #16453).
+        self.assertFalse(a == b)
+        self.assertTrue(a != b)
+        self.assertFalse(a == c)
+        self.assertTrue(a != c)
+        self.assertEqual(a == d, a is d)
+        self.assertEqual(a != d, a is not d)
+
+    def test_ordering(self):
+        # weakrefs cannot be ordered, even if the underlying objects can.
+        ops = [operator.lt, operator.gt, operator.le, operator.ge]
+        x = Object(1)
+        y = Object(1)
+        a = weakref.ref(x)
+        b = weakref.ref(y)
+        for op in ops:
+            self.assertRaises(TypeError, op, a, b)
+        # Same when dead.
+        del x, y
+        gc.collect()
+        for op in ops:
+            self.assertRaises(TypeError, op, a, b)
+
+    def test_hashing(self):
+        # Alive weakrefs hash the same as the underlying object
+        x = Object(42)
+        y = Object(42)
+        a = weakref.ref(x)
+        b = weakref.ref(y)
+        self.assertEqual(hash(a), hash(42))
+        del x, y
+        gc.collect()
+        # Dead weakrefs:
+        # - retain their hash is they were hashed when alive;
+        # - otherwise, cannot be hashed.
+        self.assertEqual(hash(a), hash(42))
+        self.assertRaises(TypeError, hash, b)
+
 
 class SubclassableWeakrefTestCase(TestBase):
 
@@ -796,27 +880,6 @@ class SubclassableWeakrefTestCase(TestBase):
         self.assertEqual(self.cbcalled, 0)
 
 
-class Object:
-    def __init__(self, arg):
-        self.arg = arg
-    def __repr__(self):
-        return "<Object %r>" % self.arg
-    def __eq__(self, other):
-        if isinstance(other, Object):
-            return self.arg == other.arg
-        return NotImplemented
-    def __lt__(self, other):
-        if isinstance(other, Object):
-            return self.arg < other.arg
-        return NotImplemented
-    def __hash__(self):
-        return hash(self.arg)
-
-class RefCycle:
-    def __init__(self):
-        self.cycle = self
-
-
 class MappingTestCase(TestBase):
 
     COUNT = 10
diff --git a/Misc/NEWS b/Misc/NEWS
index 2099109..d537df8 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -12,6 +12,8 @@ What's New in Python 3.3.1?
 Core and Builtins
 -----------------
 
+- Issue #16453: Fix equality testing of dead weakref objects.
+
 - Issue #9535: Fix pending signals that have been received but not yet
   handled by Python to not persist after os.fork() in the child process.
 
diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c
index e07adb2..d3a4dd5 100644
--- a/Objects/weakrefobject.c
+++ b/Objects/weakrefobject.c
@@ -198,9 +198,13 @@ weakref_richcompare(PyWeakReference* self, PyWeakReference* other, int op)
     }
     if (PyWeakref_GET_OBJECT(self) == Py_None
         || PyWeakref_GET_OBJECT(other) == Py_None) {
-        PyObject *res = self==other ? Py_True : Py_False;
-        Py_INCREF(res);
-        return res;
+        int res = (self == other);
+        if (op == Py_NE)
+            res = !res;
+        if (res)
+            Py_RETURN_TRUE;
+        else
+            Py_RETURN_FALSE;
     }
     return PyObject_RichCompare(PyWeakref_GET_OBJECT(self),
                                 PyWeakref_GET_OBJECT(other), op);