From 469325c30e147680543b2f5118b83fd95055a499 Mon Sep 17 00:00:00 2001 From: MojoVampire Date: Tue, 3 Mar 2020 18:50:17 +0000 Subject: [PATCH] bpo-35712: Make using NotImplemented in a boolean context issue a deprecation warning (GH-13195) --- Doc/library/constants.rst | 7 ++++++- Doc/reference/datamodel.rst | 9 +++++++-- Doc/whatsnew/3.9.rst | 6 ++++++ Lib/functools.py | 4 ++++ Lib/ipaddress.py | 6 +++--- Lib/test/test_buffer.py | 2 +- Lib/test/test_builtin.py | 14 +++++++++++++ Lib/test/test_descr.py | 6 +++--- .../2019-05-08-11-11-45.bpo-35712.KJthus.rst | 2 ++ Objects/object.c | 20 +++++++++++++++++-- 10 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-05-08-11-11-45.bpo-35712.KJthus.rst diff --git a/Doc/library/constants.rst b/Doc/library/constants.rst index 501715980f9..f17e1a37875 100644 --- a/Doc/library/constants.rst +++ b/Doc/library/constants.rst @@ -31,7 +31,7 @@ A small number of constants live in the built-in namespace. They are: etc.) to indicate that the operation is not implemented with respect to the other type; may be returned by the in-place binary special methods (e.g. :meth:`__imul__`, :meth:`__iand__`, etc.) for the same purpose. - Its truth value is true. + It should not be evaluated in a boolean context. .. note:: @@ -50,6 +50,11 @@ A small number of constants live in the built-in namespace. They are: even though they have similar names and purposes. See :exc:`NotImplementedError` for details on when to use it. + .. versionchanged:: 3.9 + Evaluating ``NotImplemented`` in a boolean context is deprecated. While + it currently evaluates as true, it will emit a :exc:`DeprecationWarning`. + It will raise a :exc:`TypeError` in a future version of Python. + .. index:: single: ...; ellipsis literal .. data:: Ellipsis diff --git a/Doc/reference/datamodel.rst b/Doc/reference/datamodel.rst index 5b3b669c5eb..8be432d465b 100644 --- a/Doc/reference/datamodel.rst +++ b/Doc/reference/datamodel.rst @@ -156,13 +156,18 @@ NotImplemented object is accessed through the built-in name ``NotImplemented``. Numeric methods and rich comparison methods should return this value if they do not implement the operation for the operands provided. (The interpreter will then try the - reflected operation, or some other fallback, depending on the operator.) Its - truth value is true. + reflected operation, or some other fallback, depending on the operator.) It + should not be evaluated in a boolean context. See :ref:`implementing-the-arithmetic-operations` for more details. + .. versionchanged:: 3.9 + Evaluating ``NotImplemented`` in a boolean context is deprecated. While + it currently evaluates as true, it will emit a :exc:`DeprecationWarning`. + It will raise a :exc:`TypeError` in a future version of Python. + Ellipsis .. index:: diff --git a/Doc/whatsnew/3.9.rst b/Doc/whatsnew/3.9.rst index f49575d89da..9a8a484fc8f 100644 --- a/Doc/whatsnew/3.9.rst +++ b/Doc/whatsnew/3.9.rst @@ -452,6 +452,12 @@ Deprecated of Python. For the majority of use cases, users can leverage the Abstract Syntax Tree (AST) generation and compilation stage, using the :mod:`ast` module. +* Using :data:`NotImplemented` in a boolean context has been deprecated, + as it is almost exclusively the result of incorrect rich comparator + implementations. It will be made a :exc:`TypeError` in a future version + of Python. + (Contributed by Josh Rosenberg in :issue:`35712`.) + * The :mod:`random` module currently accepts any hashable type as a possible seed value. Unfortunately, some of those types are not guaranteed to have a deterministic hash value. After Python 3.9, diff --git a/Lib/functools.py b/Lib/functools.py index 535fa046c18..e230175e56c 100644 --- a/Lib/functools.py +++ b/Lib/functools.py @@ -96,6 +96,8 @@ def _gt_from_lt(self, other, NotImplemented=NotImplemented): def _le_from_lt(self, other, NotImplemented=NotImplemented): 'Return a <= b. Computed by @total_ordering from (a < b) or (a == b).' op_result = self.__lt__(other) + if op_result is NotImplemented: + return op_result return op_result or self == other def _ge_from_lt(self, other, NotImplemented=NotImplemented): @@ -136,6 +138,8 @@ def _lt_from_gt(self, other, NotImplemented=NotImplemented): def _ge_from_gt(self, other, NotImplemented=NotImplemented): 'Return a >= b. Computed by @total_ordering from (a > b) or (a == b).' op_result = self.__gt__(other) + if op_result is NotImplemented: + return op_result return op_result or self == other def _le_from_gt(self, other, NotImplemented=NotImplemented): diff --git a/Lib/ipaddress.py b/Lib/ipaddress.py index 9c47405ce8d..702433953a1 100644 --- a/Lib/ipaddress.py +++ b/Lib/ipaddress.py @@ -1398,7 +1398,7 @@ class IPv4Interface(IPv4Address): def __eq__(self, other): address_equal = IPv4Address.__eq__(self, other) - if not address_equal or address_equal is NotImplemented: + if address_equal is NotImplemented or not address_equal: return address_equal try: return self.network == other.network @@ -2096,7 +2096,7 @@ class IPv6Interface(IPv6Address): def __eq__(self, other): address_equal = IPv6Address.__eq__(self, other) - if not address_equal or address_equal is NotImplemented: + if address_equal is NotImplemented or not address_equal: return address_equal try: return self.network == other.network @@ -2109,7 +2109,7 @@ class IPv6Interface(IPv6Address): def __lt__(self, other): address_less = IPv6Address.__lt__(self, other) if address_less is NotImplemented: - return NotImplemented + return address_less try: return (self.network < other.network or self.network == other.network and address_less) diff --git a/Lib/test/test_buffer.py b/Lib/test/test_buffer.py index 6178ffde7a5..2ddca06b8b5 100644 --- a/Lib/test/test_buffer.py +++ b/Lib/test/test_buffer.py @@ -2528,7 +2528,7 @@ class TestBufferProtocol(unittest.TestCase): values = [INT(9), IDX(9), 2.2+3j, Decimal("-21.1"), 12.2, Fraction(5, 2), [1,2,3], {4,5,6}, {7:8}, (), (9,), - True, False, None, NotImplemented, + True, False, None, Ellipsis, b'a', b'abc', bytearray(b'a'), bytearray(b'abc'), 'a', 'abc', r'a', r'abc', f, lambda x: x] diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index 5c553a92b97..e50c2735ec2 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -1666,6 +1666,20 @@ class BuiltinTest(unittest.TestCase): self.assertRaises(TypeError, tp, 1, 2) self.assertRaises(TypeError, tp, a=1, b=2) + def test_warning_notimplemented(self): + # Issue #35712: NotImplemented is a sentinel value that should never + # be evaluated in a boolean context (virtually all such use cases + # are a result of accidental misuse implementing rich comparison + # operations in terms of one another). + # For the time being, it will continue to evaluate as truthy, but + # issue a deprecation warning (with the eventual intent to make it + # a TypeError). + self.assertWarns(DeprecationWarning, bool, NotImplemented) + with self.assertWarns(DeprecationWarning): + self.assertTrue(NotImplemented) + with self.assertWarns(DeprecationWarning): + self.assertFalse(not NotImplemented) + class TestBreakpoint(unittest.TestCase): def setUp(self): diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index d2e121820ea..96cc8de2d98 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -2526,9 +2526,9 @@ order (MRO) for bases """ except TypeError: pass - # Two essentially featureless objects, just inheriting stuff from - # object. - self.assertEqual(dir(NotImplemented), dir(Ellipsis)) + # Two essentially featureless objects, (Ellipsis just inherits stuff + # from object. + self.assertEqual(dir(object()), dir(Ellipsis)) # Nasty test case for proxied objects class Wrapper(object): diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-05-08-11-11-45.bpo-35712.KJthus.rst b/Misc/NEWS.d/next/Core and Builtins/2019-05-08-11-11-45.bpo-35712.KJthus.rst new file mode 100644 index 00000000000..3a68632883f --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2019-05-08-11-11-45.bpo-35712.KJthus.rst @@ -0,0 +1,2 @@ +Using :data:`NotImplemented` in a boolean context has been deprecated. Patch +contributed by Josh Rosenberg. diff --git a/Objects/object.c b/Objects/object.c index 81de3b82530..9c74e07eddc 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1673,6 +1673,22 @@ notimplemented_dealloc(PyObject* ignore) Py_FatalError("deallocating NotImplemented"); } +static int +notimplemented_bool(PyObject *v) +{ + if (PyErr_WarnEx(PyExc_DeprecationWarning, + "NotImplemented should not be used in a boolean context", + 1) < 0) + { + return -1; + } + return 1; +} + +static PyNumberMethods notimplemented_as_number = { + .nb_bool = notimplemented_bool, +}; + PyTypeObject _PyNotImplemented_Type = { PyVarObject_HEAD_INIT(&PyType_Type, 0) "NotImplementedType", @@ -1683,8 +1699,8 @@ PyTypeObject _PyNotImplemented_Type = { 0, /*tp_getattr*/ 0, /*tp_setattr*/ 0, /*tp_as_async*/ - NotImplemented_repr, /*tp_repr*/ - 0, /*tp_as_number*/ + NotImplemented_repr, /*tp_repr*/ + ¬implemented_as_number, /*tp_as_number*/ 0, /*tp_as_sequence*/ 0, /*tp_as_mapping*/ 0, /*tp_hash */