◐ Shell
reader mode source ↗
Skip to content

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

Merged
serhiy-storchaka merged 7 commits into
python:masterfrom
serhiy-storchaka:convert-index-to-number
Jun 1, 2019
Merged

bpo-20092. Use __index__ in constructors of int, float and complex.#13108
serhiy-storchaka merged 7 commits into
python:masterfrom
serhiy-storchaka:convert-index-to-number

Conversation

@serhiy-storchaka

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

Copy link
Copy Markdown
Member

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

remilapeyre commented May 6, 2019

Copy link
Copy Markdown

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

serhiy-storchaka commented May 7, 2019

Copy link
Copy Markdown
Member Author

Yes, converting integers to float is not always reversible.

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

This is expected behavior.

@jdemeyer

jdemeyer commented May 16, 2019

Copy link
Copy Markdown
Contributor

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

serhiy-storchaka commented May 31, 2019

Copy link
Copy Markdown
Member Author

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

@mdickinson mdickinson self-requested a review June 1, 2019 18:51
mdickinson
mdickinson approved these changes Jun 1, 2019
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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

mdickinson commented Jun 1, 2019

Copy link
Copy Markdown
Member

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

@mdickinson

mdickinson commented Jun 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 suggested changes Jun 1, 2019
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

@bedevere-bot

bedevere-bot commented Jun 1, 2019

Copy link
Copy Markdown

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 June 1, 2019 21:05
@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

1 more reviewer

@mdickinson mdickinson mdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@serhiy-storchaka @remilapeyre @jdemeyer @mdickinson @bedevere-bot @the-knights-who-say-ni