bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes.

They now return NotImplemented for unsupported type of the other operand.
python · serhiy-storchaka · Aug 8, 2019 · Jul 25, 2019 · Jul 25, 2019 · Jul 27, 2019
commit 4b4b2f1b849e448aabbf406b3092a57df6005bba
diff --git a/Lib/asyncio/events.py b/Lib/asyncio/events.py
@@ -119,18 +119,22 @@ def __hash__(self):
         return hash(self._when)
 
     def __lt__(self, other):
-        return self._when < other._when
+        if isinstance(other, TimerHandle):
+            return self._when < other._when
+        return NotImplemented
 
     def __le__(self, other):
-        if self._when < other._when:
+        if isinstance(other, TimerHandle) and self._when < other._when:
             return True
         return self.__eq__(other)
 
     def __gt__(self, other):
-        return self._when > other._when
+        if isinstance(other, TimerHandle):
+            return self._when > other._when
+        return NotImplemented
 
     def __ge__(self, other):
-        if self._when > other._when:
+        if isinstance(other, TimerHandle) and self._when > other._when:
             return True
         return self.__eq__(other)
 
@@ -142,10 +146,6 @@ def __eq__(self, other):
                     self._cancelled == other._cancelled)
         return NotImplemented
 
-    def __ne__(self, other):
-        equal = self.__eq__(other)
-        return NotImplemented if equal is NotImplemented else not equal
-
     def cancel(self):
         if not self._cancelled:
             self._loop._timer_handle_cancelled(self)

diff --git a/Lib/datetime.py b/Lib/datetime.py
@@ -739,25 +739,25 @@ def __le__(self, other):
         if isinstance(other, timedelta):
             return self._cmp(other) <= 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __lt__(self, other):
         if isinstance(other, timedelta):
             return self._cmp(other) < 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __ge__(self, other):
         if isinstance(other, timedelta):
             return self._cmp(other) >= 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __gt__(self, other):
         if isinstance(other, timedelta):
             return self._cmp(other) > 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def _cmp(self, other):
         assert isinstance(other, timedelta)
@@ -1316,25 +1316,25 @@ def __le__(self, other):
         if isinstance(other, time):
             return self._cmp(other) <= 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __lt__(self, other):
         if isinstance(other, time):
             return self._cmp(other) < 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __ge__(self, other):
         if isinstance(other, time):
             return self._cmp(other) >= 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def __gt__(self, other):
         if isinstance(other, time):
             return self._cmp(other) > 0
         else:
-            _cmperror(self, other)
+            return NotImplemented
 
     def _cmp(self, other, allow_mixed=False):
         assert isinstance(other, time)
@@ -2210,9 +2210,9 @@ def __getinitargs__(self):
         return (self._offset, self._name)
 
     def __eq__(self, other):
-        if type(other) != timezone:
-            return False
-        return self._offset == other._offset
+        if isinstance(other, timezone):
+            return self._offset == other._offset
+        return NotImplemented
 
     def __hash__(self):
         return hash(self._offset)

diff --git a/Lib/distutils/tests/test_version.py b/Lib/distutils/tests/test_version.py
@@ -45,6 +45,14 @@ def test_cmp_strict(self):
             self.assertEqual(res, wanted,
                              'cmp(%s, %s) should be %s, got %s' %
                              (v1, v2, wanted, res))
+            res = StrictVersion(v1)._cmp(v2)
+            self.assertEqual(res, wanted,
+                             'cmp(%s, %s) should be %s, got %s' %
+                             (v1, v2, wanted, res))
+            res = StrictVersion(v1)._cmp(object())
+            self.assertIs(res, NotImplemented,
+                          'cmp(%s, %s) should be NotImplemented, got %s' %
+                          (v1, v2, res))
 
 
     def test_cmp(self):
@@ -63,6 +71,14 @@ def test_cmp(self):
             self.assertEqual(res, wanted,
                              'cmp(%s, %s) should be %s, got %s' %
                              (v1, v2, wanted, res))
+            res = LooseVersion(v1)._cmp(v2)
+            self.assertEqual(res, wanted,
+                             'cmp(%s, %s) should be %s, got %s' %
+                             (v1, v2, wanted, res))
+            res = LooseVersion(v1)._cmp(object())
+            self.assertIs(res, NotImplemented,
+                          'cmp(%s, %s) should be NotImplemented, got %s' %
+                          (v1, v2, res))
 
 def test_suite():
     return unittest.makeSuite(VersionTestCase)

diff --git a/Lib/distutils/version.py b/Lib/distutils/version.py
@@ -166,6 +166,8 @@ def __str__ (self):
     def _cmp (self, other):
         if isinstance(other, str):
             other = StrictVersion(other)
+        elif not isinstance(other, StrictVersion):
+            return NotImplemented
 
         if self.version != other.version:
             # numeric versions don't match
@@ -331,6 +333,8 @@ def __repr__ (self):
     def _cmp (self, other):
         if isinstance(other, str):
             other = LooseVersion(other)
+        elif not isinstance(other, LooseVersion):
+            return NotImplemented
 
         if self.version == other.version:
             return 0

diff --git a/Lib/email/headerregistry.py b/Lib/email/headerregistry.py
@@ -97,8 +97,8 @@ def __str__(self):
         return self.addr_spec
 
     def __eq__(self, other):
-        if type(other) != type(self):
-            return False
+        if not isinstance(other, Address):
+            return NotImplemented
         return (self.display_name == other.display_name and
                 self.username == other.username and
                 self.domain == other.domain)
@@ -150,8 +150,8 @@ def __str__(self):
         return "{}:{};".format(disp, adrstr)
 
     def __eq__(self, other):
-        if type(other) != type(self):
-            return False
+        if not isinstance(other, Group):
+            return NotImplemented
         return (self.display_name == other.display_name and
                 self.addresses == other.addresses)
 

diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
@@ -371,7 +371,7 @@ def __eq__(self, other):
                     self.cached == other.cached and
                     self.has_location == other.has_location)
         except AttributeError:
-            return False
+            return NotImplemented
 
     @property
     def cached(self):

diff --git a/Lib/test/datetimetester.py b/Lib/test/datetimetester.py
@@ -16,6 +16,7 @@
 import re
 import struct
 import unittest
+from unittest.mock import ANY
 
 from array import array
 
@@ -353,6 +354,18 @@ def test_comparison(self):
         self.assertTrue(timezone(ZERO) != None)
         self.assertFalse(timezone(ZERO) ==  None)
 
+        tz = timezone(ZERO)
+        largest = support.LargestObject()
+            self.assertTrue(tz < largest)
+        self.assertFalse(tz > largest)
+        self.assertTrue(tz <= largest)
+        self.assertFalse(tz >= largest)
+        self.assertFalse(tz == largest)
+        self.assertTrue(tz != largest)
+
+        self.assertTrue(tz == ANY)
+        self.assertFalse(tz != ANY)
+
     def test_aware_datetime(self):
         # test that timezone instances can be used by datetime
         t = datetime(1, 1, 1)
@@ -414,10 +427,20 @@ def test_harmless_mixed_comparison(self):
 
         # Comparison to objects of unsupported types should return
         # NotImplemented which falls back to the right hand side's __eq__
-        # method. In this case, ComparesEqualClass.__eq__ always returns True.
-        # ComparesEqualClass.__ne__ always returns False.
-        self.assertTrue(me == ComparesEqualClass())
-        self.assertFalse(me != ComparesEqualClass())
+        # method. In this case, ANY.__eq__ always returns True.
+        # ANY.__ne__ always returns False.
+        self.assertTrue(me == ANY)
+        self.assertFalse(me != ANY)
+
+        # If the other class explicitly defines ordering
+        # relative to our class, it is allowed to do so
+        largest = support.LargestObject()
+        self.assertTrue(me < largest)
+        self.assertFalse(me > largest)
+        self.assertTrue(me <= largest)
+        self.assertFalse(me >= largest)
+        self.assertFalse(me == largest)
+        self.assertTrue(me != largest)
 
     def test_harmful_mixed_comparison(self):
         me = self.theclass(1, 1, 1)
@@ -1582,29 +1605,6 @@ class SomeClass:
         self.assertRaises(TypeError, lambda: our < their)
         self.assertRaises(TypeError, lambda: their < our)
 
-        # However, if the other class explicitly defines ordering
-        # relative to our class, it is allowed to do so
-
-        class LargerThanAnything:
-            def __lt__(self, other):
-                return False
-            def __le__(self, other):
-                return isinstance(other, LargerThanAnything)
-            def __eq__(self, other):
-                return isinstance(other, LargerThanAnything)
-            def __gt__(self, other):
-                return not isinstance(other, LargerThanAnything)
-            def __ge__(self, other):
-                return True
-
-        their = LargerThanAnything()
-        self.assertEqual(our == their, False)
-        self.assertEqual(their == our, False)
-        self.assertEqual(our != their, True)
-        self.assertEqual(their != our, True)
-        self.assertEqual(our < their, True)
-        self.assertEqual(their < our, False)
-
     def test_bool(self):
         # All dates are considered true.
         self.assertTrue(self.theclass.min)
@@ -3781,8 +3781,8 @@ def test_replace(self):
         self.assertRaises(ValueError, base.replace, microsecond=1000000)
 
     def test_mixed_compare(self):
-        t1 = time(1, 2, 3)
-        t2 = time(1, 2, 3)
+        t1 = self.theclass(1, 2, 3)
+        t2 = self.theclass(1, 2, 3)
         self.assertEqual(t1, t2)
         t2 = t2.replace(tzinfo=None)
         self.assertEqual(t1, t2)

diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py
@@ -3093,6 +3093,21 @@ def __fspath__(self):
             return self.path
 
 
+@functools.total_ordering
+class LargestObject:
+    def __eq__(self, other):
+        return isinstance(other, LargestObject)
+    def __lt__(self, other):
+        return False
+
+@functools.total_ordering
+class SmallestObject:
+    def __eq__(self, other):
+        return isinstance(other, SmallestObject)
+    def __gt__(self, other):
+        return False
+
+
 def maybe_get_event_loop_policy():
     """Return the global event loop policy if one is set, else return None."""
     return asyncio.events._event_loop_policy

diff --git a/Lib/test/test_asyncio/test_events.py b/Lib/test/test_asyncio/test_events.py
@@ -2364,6 +2364,28 @@ def callback(*args):
         self.assertIs(NotImplemented, h1.__eq__(h3))
         self.assertIs(NotImplemented, h1.__ne__(h3))
 
+        with self.assertRaises(TypeError):
+            h1 < ()
+        with self.assertRaises(TypeError):
+            h1 > ()
+        with self.assertRaises(TypeError):
+            h1 <= ()
+        with self.assertRaises(TypeError):
+            h1 >= ()
        self.assertFalse(h1 == ())
+        self.assertTrue(h1 != ())
+
+        largest = support.LargestObject()
+        self.assertTrue(h1 < largest)
+        self.assertFalse(h1 > largest)
+        self.assertTrue(h1 <= largest)
+        self.assertFalse(h1 >= largest)
+        self.assertFalse(h1 == largest)
+        self.assertTrue(h1 != largest)
+
+        self.assertTrue(h1 == mock.ANY)
+        self.assertFalse(h1 != mock.ANY)
+
 
 class AbstractEventLoopTests(unittest.TestCase):
 

diff --git a/Lib/test/test_email/test_headerregistry.py b/Lib/test/test_email/test_headerregistry.py
@@ -1,6 +1,7 @@
 import datetime
 import textwrap
 import unittest
+from unittest.mock import ANY
 from email import errors
 from email import policy
 from email.message import Message
@@ -1525,6 +1526,24 @@ def test_set_message_header_from_group(self):
         self.assertEqual(m['to'], 'foo bar:;')
         self.assertEqual(m['to'].addresses, g.addresses)
 
+    def test_address_comparison(self):
+        a = Address('foo', 'bar', 'example.com')
+        self.assertEqual(Address('foo', 'bar', 'example.com'), a)
+        self.assertNotEqual(Address('baz', 'bar', 'example.com'), a)
+        self.assertNotEqual(Address('foo', 'baz', 'example.com'), a)
+        self.assertNotEqual(Address('foo', 'bar', 'baz'), a)
+        self.assertFalse(a == object())
+        self.assertTrue(a == ANY)
+
+    def test_group_comparison(self):
+        a = Address('foo', 'bar', 'example.com')
+        g = Group('foo bar', [a])
+        self.assertEqual(Group('foo bar', (a,)), g)
+        self.assertNotEqual(Group('baz', [a]), g)
+        self.assertNotEqual(Group('foo bar', []), g)
+        self.assertFalse(g == object())
+        self.assertTrue(g == ANY)
+
 
 class TestFolding(TestHeaderBase):
 

diff --git a/Lib/test/test_ipaddress.py b/Lib/test/test_ipaddress.py
@@ -12,6 +12,7 @@
 import pickle
 import ipaddress
 import weakref
+from test.support import LargestObject, SmallestObject
 
 
 class BaseTestCase(unittest.TestCase):
@@ -673,20 +674,6 @@ def test_ip_network(self):
         self.assertFactoryError(ipaddress.ip_network, "network")
 
 
-@functools.total_ordering
-class LargestObject:
-    def __eq__(self, other):
-        return isinstance(other, LargestObject)
-    def __lt__(self, other):
-        return False
-
-@functools.total_ordering
-class SmallestObject:
-    def __eq__(self, other):
-        return isinstance(other, SmallestObject)
-    def __gt__(self, other):
-        return False
-
 class ComparisonTests(unittest.TestCase):
 
     v4addr = ipaddress.IPv4Address(1)