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

Jump to bottom

bpo-20092. Use __index__ in constructors of int, float and complex. #13108

Merged
merged 7 commits into from Jun 1, 2019
Merged

bpo-20092. Use __index__ in constructors of int, float and complex. #13108

merged 7 commits into from Jun 1, 2019

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 6, 2019 •
edited by bedevere-bot

Constructors of int, float and complex will now fall back to __index__ if corresponding special methods __int__, __float__ and __complex__ are not defined.

https://bugs.python.org/issue20092

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label May 6, 2019
@remilapeyre
Copy link
Contributor

remilapeyre commented May 6, 2019

I'm not sure PyLong_AsDouble will always raise an exception for the conversion from int to float like Steven D'Aprano showed in https://mail.python.org/pipermail/python-dev/2019-March/156687.html.

Here the example given by Steven D'Aprano:

>>> class Test:
...     def __index__(self):
...             return 2**64 + 1
... 
>>> 2**64 + 1 == int(float(Test()))
False

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented May 7, 2019

Yes, converting integers to float is not always reversible.

>>> 2**64 + 1 == int(float(2**64 + 1))
False

This is expected behavior.

@jdemeyer
Copy link
Contributor

jdemeyer commented May 16, 2019

For __index__, I prefer the idea of #13106: that will work in all cases where __int__ is used, not only in those 4 particular constructors.

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented May 31, 2019

The only cases where __int__ is used are those particular constructors.

@mdickinson mdickinson self-requested a review Jun 1, 2019
mdickinson
mdickinson approved these changes Jun 1, 2019
View changes
Copy link
Member

@mdickinson mdickinson left a comment

LGTM: code looks good, and I agree that this is a desirable change.

The one fly in the ointment is that index can return something that's not an exact int, which means that there's now another code-path by which int can return something that's not of exact type int.

>>> class A:
...     def __index__(self): return True
... 
>>> int(A())
<stdin>:1: DeprecationWarning: __index__ returned non-int (type bool).  The ability to return an instance of a strict subclass of int is deprecated, and may be removed in a future version of Python.
1

I'd love to finally turn that DeprecationWarning into an error, but fear we've run out of time for that for 3.8, and we may want to wait for 3.9 anyway to make sure that everyone's had a chance to react to that warning. (Do you remember which version it got introduced in?)

@mdickinson
Copy link
Member

mdickinson commented Jun 1, 2019

Ah, sorry; apparently we are converting to exact int already. (Yay!)

@mdickinson
Copy link
Member

mdickinson commented Jun 1, 2019 •
edited

Hmm: bug found during manual testing:

Python 3.8.0a4+ (remotes/serhiy-storchaka/convert-index-to-number:5f980e9716, Jun  1 2019, 19:59:) 
[Clang 10.0.1 (clang-1001.0.46.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A:
...     def __index__(self): return 2**1024
... 
>>> float(A())
OverflowError: int too large to convert to float

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: <class 'float'> returned a result with an error set

mdickinson
mdickinson reviewed Jun 1, 2019
View changes
Objects/abstract.c Show resolved Hide resolved
mdickinson
mdickinson requested changes Jun 1, 2019
View changes
Copy link
Member

@mdickinson mdickinson left a comment

Updating my approval to "request changes": there's a missing error check in PyNumber_Float.

@bedevere-bot
Copy link

bedevere-bot commented Jun 1, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@serhiy-storchaka serhiy-storchaka merged commit bdbad71 into python:master Jun 1, 2019
@serhiy-storchaka serhiy-storchaka deleted the convert-index-to-number branch Jun 1, 2019
@remilapeyre remilapeyre mentioned this pull request Jun 8, 2019
Closed
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@serhiy-storchaka @DinoV
bpo-20092. Use __index__ in constructors of int, float and complex. (p…
a121230 
…ythonGH-13108)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdickinson mdickinson

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@serhiy-storchaka @remilapeyre @jdemeyer @mdickinson @bedevere-bot @the-knights-who-say-ni