classification
Title: Fix equality checks for some types
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: aeros, josh.r, lukasz.langa, miss-islington, p-ganssle, serhiy.storchaka, veky
Priority: normal Keywords: patch

Created on 2019-07-25 21:07 by serhiy.storchaka, last changed 2019-08-08 05:48 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 14952 merged serhiy.storchaka, 2019-07-25 21:14
PR 14996 merged serhiy.storchaka, 2019-07-28 19:32
PR 15102 merged miss-islington, 2019-08-04 09:39
PR 15104 merged serhiy.storchaka, 2019-08-04 11:00
PR 15167 merged serhiy.storchaka, 2019-08-07 18:44
Messages (13)
msg348452 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-07-25 21:07
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.
msg348514 - (view) Author: Vedran Čačić (veky) * Date: 2019-07-26 17:55
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.
msg348516 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2019-07-26 18:10
@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".
msg348517 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-07-26 18:31
@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.
msg348518 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-07-26 18:56
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.
msg348522 - (view) Author: Vedran Čačić (veky) * Date: 2019-07-26 19:56
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.
msg348552 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2019-07-27 17:30
> 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.
msg348977 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-04 09:38
New changeset 17e52649c0e7e9389f1cc2444a53f059e24e6bca by Serhiy Storchaka in branch 'master':
bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996)
https://github.com/python/cpython/commit/17e52649c0e7e9389f1cc2444a53f059e24e6bca
msg348978 - (view) Author: miss-islington (miss-islington) Date: 2019-08-04 10:02
New changeset dde944f9df8dea28c07935ebd6de06db7e575c12 by Miss Islington (bot) in branch '3.8':
bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996)
https://github.com/python/cpython/commit/dde944f9df8dea28c07935ebd6de06db7e575c12
msg348982 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-04 12:28
New changeset 6ed20e54e4c110e9adcfb70aba85310625e3edb4 by Serhiy Storchaka in branch '3.7':
[3.7] bpo-37685: Fixed comparisons of datetime.timedelta and datetime.timezone. (GH-14996) (GH-15104)
https://github.com/python/cpython/commit/6ed20e54e4c110e9adcfb70aba85310625e3edb4
msg349218 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-08 05:42
New changeset 662db125cddbca1db68116c547c290eb3943d98e by Serhiy Storchaka in branch 'master':
bpo-37685: Fixed __eq__, __lt__ etc implementations in some classes. (GH-14952)
https://github.com/python/cpython/commit/662db125cddbca1db68116c547c290eb3943d98e
msg349219 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-08 05:43
New changeset 7d44e7a4563072d0fad00427b76b94cad61c38ae by Serhiy Storchaka in branch 'master':
bpo-37685: Use singletons ALWAYS_EQ and NEVER_EQ in more tests. (GH-15167)
https://github.com/python/cpython/commit/7d44e7a4563072d0fad00427b76b94cad61c38ae
msg349220 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-08 05:48
Łukasz, are you fine with backporting PR 14952 to 3.8?
History
Date User Action Args
2019-08-08 05:48:15serhiy.storchakasetnosy: + lukasz.langa
messages: + msg349220
2019-08-08 05:43:21serhiy.storchakasetmessages: + msg349219
2019-08-08 05:42:57serhiy.storchakasetmessages: + msg349218
2019-08-07 18:44:56serhiy.storchakasetpull_requests: + pull_request14899
2019-08-04 12:28:25serhiy.storchakasetmessages: + msg348982
2019-08-04 11:00:06serhiy.storchakasetpull_requests: + pull_request14846
2019-08-04 10:02:00miss-islingtonsetnosy: + miss-islington
messages: + msg348978
2019-08-04 09:39:04miss-islingtonsetpull_requests: + pull_request14844
2019-08-04 09:38:49serhiy.storchakasetmessages: + msg348977
2019-07-28 19:32:34serhiy.storchakasetpull_requests: + pull_request14762
2019-07-27 17:30:37aerossetnosy: + aeros
messages: + msg348552
2019-07-26 19:56:18vekysetmessages: + msg348522
2019-07-26 18:56:56josh.rsetmessages: + msg348518
2019-07-26 18:31:14josh.rsetnosy: + josh.r
messages: + msg348517
2019-07-26 18:10:22p-gansslesetnosy: + p-ganssle
messages: + msg348516
2019-07-26 17:55:12vekysetnosy: + veky
messages: + msg348514
2019-07-25 21:14:58serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request14721
2019-07-25 21:07:46serhiy.storchakacreate