Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make NotImplemented unusable in boolean context #79893

Closed
MojoVampire mannequin opened this issue Jan 10, 2019 · 29 comments
Closed

Make NotImplemented unusable in boolean context #79893

MojoVampire mannequin opened this issue Jan 10, 2019 · 29 comments
Labels
3.9 only security fixes type-feature A feature request or enhancement

Comments

@MojoVampire
Copy link
Mannequin

MojoVampire mannequin commented Jan 10, 2019

BPO 35712
Nosy @rhettinger, @terryjreedy, @stevendaprano, @ethanfurman, @serhiy-storchaka, @MojoVampire, @vedgar, @tirkarthi, @AlexWaygood
PRs
  • bpo-35712: Make using NotImplemented in a boolean context issue a dep… #13195
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-03-03.18:52:02.603>
    created_at = <Date 2019-01-10.21:55:06.172>
    labels = ['type-feature', '3.9']
    title = 'Make NotImplemented unusable in boolean context'
    updated_at = <Date 2021-09-20.16:34:34.784>
    user = 'https://github.com/MojoVampire'

    bugs.python.org fields:

    activity = <Date 2021-09-20.16:34:34.784>
    actor = 'AlexWaygood'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-03-03.18:52:02.603>
    closer = 'serhiy.storchaka'
    components = []
    creation = <Date 2019-01-10.21:55:06.172>
    creator = 'josh.r'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35712
    keywords = ['patch']
    message_count = 28.0
    messages = ['333421', '333426', '333433', '333440', '333483', '333508', '341909', '348529', '349203', '349215', '349303', '363293', '363956', '363961', '363969', '363973', '364065', '380596', '380603', '380604', '380605', '380613', '380614', '402246', '402248', '402253', '402256', '402259']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'terry.reedy', 'steven.daprano', 'ethan.furman', 'serhiy.storchaka', 'josh.r', 'veky', 'xtreak', 'AlexWaygood']
    pr_nums = ['13195']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue35712'
    versions = ['Python 3.9']

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Jan 10, 2019

    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.

    @MojoVampire MojoVampire mannequin added the type-bug An unexpected behavior, bug, or error label Jan 10, 2019
    @tirkarthi
    Copy link
    Member

    Seems related bpo-22978

    @stevendaprano
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member

    This is a common mistake. Even the default implementation of object.__ne__ had a bug, fixed only 4 years ago in bpo-21408. 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__.

    @gvanrossum
    Copy link
    Member

    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 bpo-21408. 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)

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy terryjreedy added 3.8 only security fixes type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 11, 2019
    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented May 8, 2019

    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.

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Jul 26, 2019

    Moving to 3.9 target, per Serhiy's request. PR has been rebased against master, including updating the What's New info to be in the 3.9 version, not 3.8.

    @MojoVampire MojoVampire mannequin added 3.9 only security fixes and removed 3.8 only security fixes labels Jul 26, 2019
    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Aug 7, 2019

    In the docs for my PR, I mention using NotImplemented in a boolean context is deprecated, raising DeprecationWarning, and will raise TypeError in a future version of Python. Serhiy has suggested the end state might be RuntimeWarning instead of TypeError. I'm in favor of it ending up as an error, but not rigid on it. Does anyone else have a strong opinion among the following options?

    1. DeprecationWarning, documented as eventual TypeError (current PR)
    2. DeprecationWarning, documented as eventual RuntimeWarning
    3. Immediate RuntimeWarning, no DeprecationWarning period (since a warning only causes stderr output by default, it doesn't prevent working code from working unless warnings are explicitly configured to become exceptions)

    @serhiy-storchaka
    Copy link
    Member

    I do not have a strong opinion, so I suggest to not mention neither TypeError, nor RuntimeWarning. After some time of using a DeprecationWarning we can have have some information about whether there is a code which can't be fixed if bool(NotImplemented) will raise an error.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Aug 9, 2019

    Another reason why current behavior is confusing: what do you think

        filter(2 .__eq__, 'text')

    should yield? :-o

    (Yes, I know this isn't the right way to do it, but

    (element for element in 'text' if element == 2)
    

    is twice longer.:)

    @serhiy-storchaka
    Copy link
    Member

    New changeset 469325c by MojoVampire in branch 'master':
    bpo-35712: Make using NotImplemented in a boolean context issue a deprecation warning (GH-13195)
    469325c

    @ethanfurman
    Copy link
    Member

    I know I'm late to the party, but if

      bool(NotImplemented)

    returned NotImplemented wouldn't that solve the problem?

        def __ge__(self, other):
            return not self.__lt__(other)

    then

    if __lt__ returns then __gt__ returns

     NotImplemented      NotImplemented
     True                False
     False               True
    

    Correct code (which checks for NotImplemented) would still work, and buggy code (which just returns the bool() of NotImplemented), would then be correct.

    @serhiy-storchaka
    Copy link
    Member

    No.

    First, it is impossible. nb_bool and PyObject_IsTrue() can return only three value: 1 for true, 0 for false, and -1 for error. It is not possible to represent NotImplemented without breaking all extensions.

    Currently

        if not a:
            b()
        else:
            c()

    is equivalent to

        if a:
            c()
        else:
            b()

    If a is NotImplemented, what branch be executed in every case?

    Second, it would not help. Because real-world examples are not always so trivial as "return not self.__lt__(other)". It may be a part of more complex expression, e.g.:

    return super().__eq__(other) and self.attr == other.attr
    

    So it may help in some examples, but make bugs in other examples even more mystical.

    @ethanfurman
    Copy link
    Member

    Serhiy:
    ------

    First, it is impossible. nb_bool and PyObject_IsTrue() can return
    only three value: 1 for true, 0 for false, and -1 for error.

    Huh. How is -1 interpreted? Does it become a TypeError?

    It is not possible to represent NotImplemented without breaking all
    extensions.

    Currently

    if not a:
        b()
    else:
        c()
    

    is equivalent to

    if a:
    c()
    else:
    b()

    If a is NotImplemented, what branch be executed in every case?

    Side-stepping to other __dunder__ methods for a moment: if, for example, both __add__ and __radd__ return NotImplemented then the interpreter will convert that into a TypeError.

    So my thinking is that when the interpreter gets the NotImplemented returned by either if a or by if not a that it would be converted to a TypeError, meaning none of the branches would be executed as an exception would be raised instead.

    Second, it would not help. Because real-world examples are not always so
    trivial as "return not self.__lt__(other)". It may be a part of more
    complex expression, e.g.:

    return super().\_\_eq__(other) and self.attr == other.attr
    

    I don't see the problem -- breaking it down:

    return super().__eq__(other) and self.attr == other.attr
    

    becomes

        return NotImplemented and ...

    and the and machinery sees it has a NotImplemented and raises a TypeError. The same would be true if the __eq__ operation returned NotImplemented.

    So to make that work, and and or would have to check if one of the operands is NotImplemented. Are there others that would need to have that check?

    The reason I would prefer this solution is that if it could behave as I described above, the TypeError would point at the user's line of code as being the problem, and not inside the dunder:

        class NoAdd:
            #
            def __add__(self, other):
                return NotImplemented
            #
            def __radd__(self, other):
                # fake a NotImplemented TypeError from bool(NotImplemented)
                raise TypeError("cannot boolean NotImplemented")
        # what we should see, since the error is in the users' code
        --> NoAdd() + 7
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: unsupported operand type(s) for +: 'NoAdd' and 'int'
    
        # what we will see -- a leaky implementation detail
        --> 7 + NoAdd()
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
          File "<stdin>", line 6, in __radd__
        TypeError: cannot boolean NotImplemented

    @serhiy-storchaka
    Copy link
    Member

    How is -1 interpreted? Does it become a TypeError?

    It is interpreted as error. It requires an exception be set. If you return -1 without setting an exception you will usually get a SystemError or crash. Oh, and this may happen during executing other code, so you will search a bug in wrong place.

    You cannot change this without breaking the C and Python API in hard way.

    So my thinking is that when the interpreter gets the NotImplemented returned by either if a or by if not a that it would be converted to a TypeError

    and the and machinery sees it has a NotImplemented and raises a TypeError.

    So you literally want NotImplemented raising a TypeError in boolean context except the "not" operator, and redefine not a from

    False if a else True
    

    to

    NotImplemented if a is NotImplemented else False if a else True
    

    You can do this, but this is a different issue, and I doubt that it will solve many problems.

    @ethanfurman
    Copy link
    Member

    Hmm. Okay, I'm happy with just raising a TypeError in NotImplemented.__bool__.

    @rhettinger
    Copy link
    Contributor

    One of the idioms for removing None no longer works:

    >>> L = [0, 23, 234, 89, None, 0, 35, 9]
    >>> list(filter(None.__ne__, L))
    Warning (from warnings module):
      File "<pyshell#1>", line 1
    DeprecationWarning: NotImplemented should not be used in a boolean context
    [0, 23, 234, 89, 0, 35, 9]

    @gvanrossum
    Copy link
    Member

    list(filter(None.__ne__, L))

    I assume you've been recommending this? To me it looks obfuscated. People should just use a comprehension, e.g.

    [x for x in L if x is not None]

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Nov 9, 2020

    ... as it probably should: look at https://bugs.python.org/msg349303

    Yes, filtering comprehensions are a frequently used niche, and too long in the "official" parlance. I seem to recall that

    [x in mylist if x is not None]
    

    (instead of triple-x version) was rejected because it was too hard to parse. Maybe now we can really implement it?

    @gvanrossum
    Copy link
    Member

    That's off topic for this issue -- you can go to python-ideas to propose that.

    @rhettinger
    Copy link
    Contributor

    I assume you've been recommending this?

    Not really, but it does come up and I've seen it in customer code more than once.

    I do show people this:

        >>> data = [10.5, 3.27, float('Nan'), 56.1]
        >>> list(filter(isfinite, data))
        [10.5, 3.27, 56.1]
        >>> list(filterfalse(isnan, data))
        [10.5, 3.27, 56.1]

    The question does arise about how to do this for None using functional programming. The answer is a bit awkward:

        >>> from operator import is_not
        >>> from functools import partial
        >>> data = [10.5, 3.27, None, 56.1]
        >>> list(filter(partial(is_not, None), data))
        [10.5, 3.27, 56.1]

    From a teaching point of view, the important part is to show that this code does not do what people typically expect:

        >>> data = [10.5, 0.0, float('NaN'), 3.27, None, 56.1]
        >>> list(filter(None, data))
        [10.5, nan, 3.27, 56.1]

    FWIW, this issue isn't important to me. Just wanted to note that one of the idioms no longer works. There are of course other ways to do it.

    @serhiy-storchaka
    Copy link
    Member

    I am sad that such code (as well as my former code in total_ordering) no longer works, but this is just a clever trick, and the code can be written in less clever but more explicit way.

    On other hand, the warning helps to catch common mistakes like

    not self.__lt__(other)
    self.__lt__(other) or self.__eq__(other)
    super().__eq__(other) and self.x == other.x

    Even non-beginners often make such kind of mistakes, and it is hard to catch them if you have not trained yourself specially. I suggest you to include lessons about writing complex comparison methods in your course for advanced users.

    @AlexWaygood
    Copy link
    Member

    The following code now leads to a DeprecationWarning, but I am unclear why it should.

    >>> from enum import Enum
    >>> 
    >>> class CardColour(Enum):
    ...     """Enumeration of the two colours in a pack of cards."""
    ... 
    ... 	BLACK = 'black'
    ... 	RED = 'red'
    ... 
    ... 	def other_colour(self):
    ...         """Given one colour, get the other one."""
    ... 	    return next(filter(self.__ne__, CardColour))
    ... 	
    >>> CardColour.BLACK.other_colour()
    <input>:5: DeprecationWarning: NotImplemented should not be used in a boolean context
    <CardColour.RED: 'red'>
    

    If I change the last line of other_colour to either return next(colour for colour in CardColour if colour != self) or return next(colour for colour in CardColour if colour is not self), the warning goes away.

    Is this intended behaviour?

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 20, 2021

    Please see the message https://bugs.python.org/issue35712#msg349303. Filtering with those dunder sesqui-dispatch methods really is a bug magnet.

    @AlexWaygood
    Copy link
    Member

    Thanks, Vedran. I read https://bugs.python.org/issue35712#msg349303 before adding my message, but am not quite clear why my snippet is the same situation.

    next(filter((2).__eq__, 'text')) surely returns 't' because (2).__eq__('t') returns NotImplemented, and NotImplemented is truthy. (Apologies if my understanding is wrong here.)

    I'm unclear, however, why x.__ne__(y) should ever return NotImplemented (or even have the possibility of returning NotImplemented) if it is known that both x and y are members of the same Enum. The documentation for Enums clearly states that equality comparisons between members of the same enum are defined (https://docs.python.org/3/library/enum.html#comparisons).

    If the argument is "filter should never be used with a predicate that could return NotImplemented in some situations", then I think that could usefully be added to the documentation for filter. Moreover, if that is the argument, then I don't understand why the following does not raise a DeprecationWarning:

    >>> next(filter((2).__eq__, (2, 3, 4)))
    2
    

    Again, apologies if I'm missing something here.

    @serhiy-storchaka
    Copy link
    Member

    Interesting, it is because object().__eq__(object()) returns NotImplemented instead of False.

    object.__eq__ could return False if arguments have same type (or in some other cases). I think it would not break anything, and it would fix your case. But I am not sure that it is worth to change this. Using bound methods __eq__ and __ne__ is an antipattern, and we should not encourage this, even if it is safe as in your case. If you want to discuss this more, it is better to do this on a mailing-list or Discuss.

    Your code can be rewritten as:

        def other_colour(self):
            for other in CardColour:
                if self != other:
                    return other
            assert False, "not reachable"

    @AlexWaygood
    Copy link
    Member

    Thanks, Serhiy, that makes sense. I'll consider raising this elsewhere, as you suggest.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @JelleZijlstra
    Copy link
    Member

    I've opened https://discuss.python.org/t/should-bool-notimplemented-become-an-error/51342 to discuss whether we should go through with the deprecation.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants