classification
Title: Make NotImplemented unusable in boolean context
Type: enhancement Stage: patch review
Components: Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, josh.r, serhiy.storchaka, steven.daprano, terry.reedy, xtreak
Priority: normal Keywords: patch

Created on 2019-01-10 21:55 by josh.r, last changed 2019-05-08 16:44 by josh.r.

Pull Requests
URL Status Linked Edit
PR 13195 open josh.r, 2019-05-08 16:20
Messages (7)
msg333421 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-10 21:55
I don't really expect this to go anywhere until Python 4 (*maybe* 3.9 after a deprecation period), but it seems like it would have been a good idea to make NotImplementedType's __bool__ explicitly raise a TypeError (rather than leaving it unset, so NotImplemented evaluates as truthy). Any correct use of NotImplemented per its documented intent would never evaluate it in a boolean context, but rather use identity testing, e.g. back in the Py2 days, the canonical __ne__ delegation to __eq__ for any class should be implemented as something like:

    def __ne__(self, other):
        equal = self.__eq__(other)
        return equal if equal is NotImplemented else not equal

Problem is, a lot of folks would make mistakes like doing:

    def __ne__(self, other):
        return not self.__eq__(other)

which silently returns False when __eq__ returns NotImplemented, rather than returning NotImplemented and allowing Python to check the mirrored operation. Similar issues arise when hand-writing the other rich comparison operators in terms of each other.

It seems like, given NotImplemented is a sentinel value that should never be evaluated in a boolean context, at some point it might be nice to explicitly prevent it, to avoid errors like this.

Main argument against it is that I don't know of any other type/object that explicitly makes itself unevaluable in a boolean context, so this could be surprising if someone uses NotImplemented as a sentinel unrelated to its intended purpose and suffers the problem.
msg333426 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-01-10 22:54
Seems related issue22978
msg333433 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2019-01-11 01:44
> the canonical __ne__ delegation to __eq__ for any class should be implemented as something like

I disagree that your code snippet is the canonical way to write __ne__. I'd write it like this:

    def __ne__(self, other):
        return not (self == other)

Or just not write it at all. Python will automatically call __eq__ going back to Python 2.4 (and possibly older).

Delegating to __eq__ directly is the wrong way to do it.

> NotImplemented is a sentinel value that should never be evaluated in a boolean context

I don't see why not. I can't think of any use-cases where I would want to *specifically* use NotImplemented in a boolean context, but I see no good reason why I would want it to fail if one happened to do so.
msg333440 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-11 07:24
This is a common mistake. Even the default implementation of object.__ne__ had a bug, fixed only 4 years ago in issue21408. There were several errors in stdlib. I am sure there is a lot of occurrences of this errors in a third party code.

So I like this idea. This can help to fix hidden errors in existing code. But I share also Josh's concerns.

There is related common mistake. In C code, the result of PyObject_IsTrue() often is treated as just a boolean, without checking for errors. Fortunately, in the current CPython code it is handled properly, but I am sure this error still is occurred in third-party extensions.

When these two errors (using NotImplemented in the boolean context and ignoring the error in PyObject_IsTrue() in the C code) meet, this can lead to manifestation of more weird bugs than treating NotImplemented as true: from a crash in debug build to raising an exception in the following unrelated code.

I suggest to start emitting a DeprecationWarning or a FutureWarning in NotImplemented.__bool__.
msg333483 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2019-01-11 16:07
I agree.

On Thu, Jan 10, 2019 at 11:24 PM Serhiy Storchaka <report@bugs.python.org>
wrote:

>
> Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment:
>
> This is a common mistake. Even the default implementation of object.__ne__
> had a bug, fixed only 4 years ago in issue21408. There were several errors
> in stdlib. I am sure there is a lot of occurrences of this errors in a
> third party code.
>
> So I like this idea. This can help to fix hidden errors in existing code.
> But I share also Josh's concerns.
>
> There is related common mistake. In C code, the result of
> PyObject_IsTrue() often is treated as just a boolean, without checking for
> errors. Fortunately, in the current CPython code it is handled properly,
> but I am sure this error still is occurred in third-party extensions.
>
> When these two errors (using NotImplemented in the boolean context and
> ignoring the error in PyObject_IsTrue() in the C code) meet, this can lead
> to manifestation of more weird bugs than treating NotImplemented as true:
> from a crash in debug build to raising an exception in the following
> unrelated code.
>
> I suggest to start emitting a DeprecationWarning or a FutureWarning in
> NotImplemented.__bool__.
>
> ----------
> nosy: +gvanrossum, serhiy.storchaka
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue35712>
> _______________________________________
>
-- 
--Guido (mobile)
msg333508 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-01-11 21:49
I consider it a nice feature of Python that all builtin objects, and, AFAIK (and Josh, apparently), all stdlib class instances, have a boolean value.  (I am aware of numpy's element-wise behavior.)  I hate to give this up.  This is part of Python's general avoidance of singular exceptions and exceptions to exceptions.  This proposal would be the latter: "An object is truthy, unless its class makes it false, unless it is NotImplemented and a TypeError."

If this exception is made, I expect that there will be proposals to extend the exception to other objects, such as Ellipsis.
msg341909 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-05-08 16:44
I've submitted a PR for this. I did discover a use case for boolean evaluation while doing this in the functools.total_ordering helpers. While it's legitimate, it *looks* wrong (the code looks like it didn't consider the possibility of NotImplemented even though it did), and really only applies to the case total_ordering handles on behalf of most folks (implementing one comparator in terms of two others).

The specific case was:

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)
     return op_result or self == other

(with a similar implementation for _ge_from_gt). It happens to work because the return value of __lt__ is used directly for both True and NotImplemented cases, with only False delegating to equality. It's not at all clear that that's correct at first glance though, and the fix to avoid the warning is simple, matching the other 10 comparator helpers by explicit checking for NotImplemented via:

    if op_result is NotImplemented:
        return NotImplemented

That's about the strongest case I can find for "legitimate" use of NotImplemented in a boolean context, so I figured I'd point it out explicitly so people knew it existed.

If that makes an eventual TypeError a non-starter, I'd still like that usage to emit a warning (e.g. a RuntimeWarning) simply because the utility of that one little shortcut pales in comparison to the damage done by the *many* misuses that occur.
History
Date User Action Args
2019-05-08 16:44:03josh.rsetmessages: + msg341909
2019-05-08 16:20:46josh.rsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request13106
2019-01-11 21:49:58terry.reedysetversions: + Python 3.8
nosy: + terry.reedy

messages: + msg333508

type: behavior -> enhancement
stage: test needed
2019-01-11 16:07:42gvanrossumsetmessages: + msg333483
2019-01-11 07:24:07serhiy.storchakasetnosy: + gvanrossum, serhiy.storchaka
messages: + msg333440
2019-01-11 01:44:01steven.dapranosetnosy: + steven.daprano
messages: + msg333433
2019-01-10 22:54:18xtreaksetnosy: + xtreak
messages: + msg333426
2019-01-10 21:55:06josh.rcreate