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

Fix equality checks for some types #81866

Closed
serhiy-storchaka opened this issue Jul 25, 2019 · 15 comments
Closed

Fix equality checks for some types #81866

serhiy-storchaka opened this issue Jul 25, 2019 · 15 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 37685
Nosy @ambv, @serhiy-storchaka, @MojoVampire, @vedgar, @pganssle, @miss-islington, @aeros
PRs
  • bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes. #14952
  • bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. #14996
  • [3.8] bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996) #15102
  • [3.7] bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996) #15104
  • bpo-37685: Use singletons ALWAYS_EQ and NEVER_EQ in more tests. #15167
  • [3.8] bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes. (GH-14952) #17836
  • 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-02-12.20:35:36.788>
    created_at = <Date 2019-07-25.21:07:46.625>
    labels = ['type-bug', 'library', '3.9']
    title = 'Fix equality checks for some types'
    updated_at = <Date 2020-02-12.20:35:36.786>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2020-02-12.20:35:36.786>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-02-12.20:35:36.788>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2019-07-25.21:07:46.625>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37685
    keywords = ['patch']
    message_count = 15.0
    messages = ['348452', '348514', '348516', '348517', '348518', '348522', '348552', '348977', '348978', '348982', '349218', '349219', '349220', '361876', '361912']
    nosy_count = 7.0
    nosy_names = ['lukasz.langa', 'serhiy.storchaka', 'josh.r', 'veky', 'p-ganssle', 'miss-islington', 'aeros']
    pr_nums = ['14952', '14996', '15102', '15104', '15167', '17836']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37685'
    versions = ['Python 3.9']

    @serhiy-storchaka
    Copy link
    Member Author

    The __eq__ implementation should return NotImplemented instead of False or raising an exception (like AttributeError or TypeError) when it does not support comparison with the other operand's type. It is so for most of implementations in the stdlib, but there are several exceptions. The proposed patch fixes these cases.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 25, 2019
    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Jul 26, 2019

    Wat?? Are we heading towards the world where 3.2 == 'something' is NotImplemented? I understand __lt__, of course, but in my opinion Python has always defined equality comparisons between different types. I think this is a huge regression.

    @pganssle
    Copy link
    Member

    @veky I can't be sure, but I think you may not understand what returning NotImplemented does - this makes comparisons more versatile, not less.

    The way it works is that when Python does, for example, x == y, it will first call x.__eq__(y) and check the return value of that. If that's True or False, it will return the value, but if it's NotImplemented, it will call y.__eq__(x) and return that value, unless the value is NotImplemented. If both comparisons return NotImplemented, it falls back on the default implementation for __eq__, which I think is "x is y". For __lt__ it's different fallback behavior, but the same general idea.

    Most of these changes are going from a situation where __lt__ or __eq__ was raising an exception or returning False for comparisons where the operation is not defined. This makes the distinction between "I don't know how to compare myself to that object" and "I know how to compare myself to that object and I am not equal to it".

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Jul 26, 2019

    @p-ganssle: Yup. If both sides return NotImplemented, __eq__ and __ne__ return a result based on an identity comparison; all other rich comparisons raise TypeError in that case. Code is here: https://github.com/python/cpython/blob/3.7/Objects/object.c#L679

    As you said, NotImplemented is never actually returned from syntax based comparisons; you only see it if you explicitly call __eq__ (and related __XX__ rich comparison methods) directly (usually because you're implementing one method in terms of another).

    AFAICT, about the only time you should ever be returning False rather than NotImplemented is when the other operand is an instance of the same class as you (or a subclass), allowing you to say with certainty that you know how to do the comparison (Python already handles the edge case of subclasses for you, by calling the comparison method on the subclass first, even if it's on the right hand side). In any other case, you return NotImplemented, and let the other side determine if they know how to do the comparison.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Jul 26, 2019

    Serhiy: Is there a reason not to use the functools.total_ordering decorator on TimerHandle, so you can get rid of __le__/ge/gt rather than fixing them individually? I notice at least one behavioral difference (total_ordering's le/ge methods use ==/!= for the fallback comparison, where TimerHandle explicitly calls __eq__) which this would eliminate, along with reducing the code to maintain.

    Specifically, the total_ordering fallbacks work if the provided operator (e.g. __lt__) works and either side provides a functional __eq__/ne, as appropriate (even if the other side *can't* provide the complementary inequality operator, e.g. __gt__). TimerHandle's behavior differs; it's all or nothing (so if __lt__ works, but __eq__ fails, then the success of __lt__ doesn't matter, the other side has to do everything).

    I'm not saying total_ordering is necessarily correct (it does seem a bit odd that A <= B can return True based on a mix of results, one from A.__lt__, one from B.__eq__), but constantly reimplementing these fallback operations has been a source of subtle bugs so often, that I'd rather depend on a single common implementation, and debate tweaks to it there, rather than having it reimplemented slightly differently in a million different places.

    I don't think import times for functools should be a problem; looks like at least four asyncio submodules import it, including asyncio.format_helpers (which asyncio.events takes a direct dependency on), so if you're using asyncio.events, you're already importing functools anyway.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Jul 26, 2019

    Thanks for the clarification. Yes, now I see how it really works, but I saw some comment about not wanting to backport it to 3.7, because it changes the semantics. Now that I understand the implementation, it seems to me that it really doesn't change the semantics, except in some weird cases like the objects "accidentally" having the sought attributes. But nevermind, I'm fine with that -- I'm just saying that's the reason I thought the change is much greater than it really is.

    @aeros
    Copy link
    Contributor

    aeros commented Jul 27, 2019

    Serhiy: Is there a reason not to use the functools.total_ordering decorator on TimerHandle, so you can get rid of __le__/ge/gt rather than fixing them
    individually?

    I strongly agree with this suggestion, unless there's some unique behavior with TimerHandle objects which would prevent this from being viable. IMO, the @total_ordering decorator should be used as much as practically possible. Individually specifying all of the rich comparisons when we don't have to adds a significant maintenance cost.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 17e5264 by Serhiy Storchaka in branch 'master':
    bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996)
    17e5264

    @miss-islington
    Copy link
    Contributor

    New changeset dde944f by Miss Islington (bot) in branch '3.8':
    bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996)
    dde944f

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 6ed20e5 by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996) (GH-15104)
    6ed20e5

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 662db12 by Serhiy Storchaka in branch 'master':
    bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes. (GH-14952)
    662db12

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 7d44e7a by Serhiy Storchaka in branch 'master':
    bpo-37685: Use singletons ALWAYS_EQ and NEVER_EQ in more tests. (GH-15167)
    7d44e7a

    @serhiy-storchaka
    Copy link
    Member Author

    Łukasz, are you fine with backporting PR 14952 to 3.8?

    @ambv
    Copy link
    Contributor

    ambv commented Feb 12, 2020

    Unfortunately, we released 3.8.0, 3.8.1, and 3.8.2rc1 without this change. It seems too late in the release cycle to introduce this change. I'd feel better seeing it in 3.9 only. Sorry that we didn't act in time to include this in 3.8.0.

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you Łukasz. I considered this as a bug fix, but was not sure that we should fix these bugs. They were here from the beginning.

    Josh, as for using total_ordering, I think it is a different issue. I did not want to change the behavior except fixing bugs (and raising an error or not supporting comparison with objects which support comparison when used at LHS looks like a bug). Feel free to open separate issues for using total_ordering or changing its semantic.

    @serhiy-storchaka serhiy-storchaka removed 3.7 (EOL) end of life 3.8 only security fixes labels Feb 12, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants