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

type() constructor should bind __int__ to __index__ when __index__ is defined and __int__ is not #64291

Open
ethanfurman opened this issue Dec 28, 2013 · 21 comments
Labels
3.8 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ethanfurman
Copy link
Member

ethanfurman commented Dec 28, 2013

BPO 20092
Nosy @terryjreedy, @mdickinson, @benjaminp, @tpn, @bitdancer, @serhiy-storchaka, @MojoVampire
PRs
  • bpo-20092. Use __index__ in constructors of int, float and complex. #13108
  • bpo-20092: Make __int__ defaults to __index__ #13106
  • Files
  • test_index_not_int.py
  • 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 = None
    created_at = <Date 2013-12-28.19:56:32.757>
    labels = ['interpreter-core', 'type-feature', '3.8']
    title = 'type() constructor should bind __int__ to __index__ when __index__ is defined and __int__ is not'
    updated_at = <Date 2021-11-19.13:14:29.675>
    user = 'https://github.com/ethanfurman'

    bugs.python.org fields:

    activity = <Date 2021-11-19.13:14:29.675>
    actor = 'patrick.yang.1248'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2013-12-28.19:56:32.757>
    creator = 'ethan.furman'
    dependencies = []
    files = ['35734']
    hgrepos = []
    issue_num = 20092
    keywords = ['patch']
    message_count = 21.0
    messages = ['207048', '207050', '207051', '207065', '207068', '207072', '207075', '207084', '207085', '207263', '221310', '221339', '221453', '221459', '221474', '221510', '313287', '341509', '344206', '344234', '406580']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'mark.dickinson', 'benjamin.peterson', 'trent', 'Arfrever', 'r.david.murray', 'serhiy.storchaka', 'josh.r', 'amitava.b', 'patrick.yang.1248']
    pr_nums = ['13108', '13106']
    priority = 'low'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20092'
    versions = ['Python 3.8']

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Dec 28, 2013

    In order to create a coherent integer type class both __int__ and __index__ must be defined or the resulting instances will behave inconsistently in different places.

    For example, if __index__ is not defined then the class cannot be used in slices, and if __int__ is not defined then int(integer_type) will fail.

    At this point the programmer must remember to define both, but since they should return the same value we can have the type constructor automatically assign __int__ to __index__ when the latter is defined and the former is not. This would be similar to how we currently treat __ne__ when __eq__ is defined.

    @ethanfurman ethanfurman added the type-feature A feature request or enhancement label Dec 28, 2013
    @benjaminp
    Copy link
    Contributor

    benjaminp commented Dec 28, 2013

    __ne__ is not "bound" to __eq__. The comparison code simply falls back onto __eq__ if __ne__ is not defined.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Dec 28, 2013

    True. I meant similar in that Python will use what's available to fill in the blank:

    class has __eq__ but user tried != ? use __eq__ and invert result

    class has __index__ but user tried int() ? use __index__ as-is

    @bitdancer
    Copy link
    Member

    bitdancer commented Dec 29, 2013

    It is not clear to me that that would be correct, though. Isn't the whole point of __index__ that some types can act as indicies even though they are *not* integers? Shouldn't it be up to the type to decide if converting them to int is a sensible thing to do? Maybe that's being silly/pendantic, though. On the gripping hand, it feels like this is another case of what you have pointed out elsewhere, that it is not clear that we actually have a consistent API here...

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Dec 29, 2013

    In bpo-19995, in msg206339, Guido exhorted:
    --------------------------------------------

    > [Ethan claimed] it is possible to want a type that can be used as an
    > index or slice but that is still not a number

    I'm sorry, but this requirement is absurd. An index *is* a number. You
    have to make up your mind. (I know, in the context of the example that
    started this, this is funny, but I still stand by it.)

    Finally, the correct name should perhaps have been __integer__ but I don't
    see enough reason to change it now.

    The de facto API that is forming is that if an actual int is needed from an actual integer type (not float, not complex, not etc.), then __index__ is used. If __index__ is not defined by some numeric type then it will not be considered a true int in certain key places in Python, such as as indices, arguments to hex(), etc.

    Making the change suggested in the title would help solidify the API.

    @bitdancer
    Copy link
    Member

    bitdancer commented Dec 29, 2013

    Ah, I see. A link to that issue would have been helpful :).

    To summarize for anyone like me who didn't follow that issue: __index__ means the object can be losslessly converted to an int (is a true int), while __int__ may be an approximate conversion. Thus it makes sense for an object to have an __int__ but not __index__, but vice-versa does not make sense.

    Is someone updating the docs to reflect this, or should that be spun off as a separate issue as well?

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Dec 29, 2013

    I have the following as part of the patch for that issue:
    ---------------------------------------------------------

    diff -r b668c409c10a Doc/reference/datamodel.rst
    --- a/Doc/reference/datamodel.rst	Sat Dec 28 20:37:58 2013 +0100
    +++ b/Doc/reference/datamodel.rst	Sun Dec 29 06:55:11 2013 -0800
    @@ -2073,23 +2073,31 @@ left undefined.
           builtin: float
           builtin: round
     
        Called to implement the built-in functions :func:`complex`,
        :func:`int`, :func:`float` and :func:`round`.  Should return a value
        of the appropriate type.
     
     
     .. method:: object.__index__(self)
     
    -   Called to implement :func:`operator.index`.  Also called whenever Python needs
    -   an integer object (such as in slicing, or in the built-in :func:`bin`,
    -   :func:`hex` and :func:`oct` functions). Must return an integer.
    +   Called to implement :func:`operator.index`, and whenever Python needs to
    +   losslessly convert the numeric object to an integer object (such as in
    +   slicing, or in the built-in :func:`bin`, :func:`hex` and :func:`oct`
    +   functions). Presence of this method indicates that the numeric object is
    +   an integer type.  Must return an integer.
    +
    +   .. note::
    +
    +      When :meth:`__index__` is defined, :meth:`__int__` should also be defined,
    +      and both shuld return the same value, in order to have a coherent integer
    +      type class.

    If for some reason that patch doesn't make it into 3.4 I'll split the doc change off to its own issue, unless you think it should be split off anyway?

    @bitdancer
    Copy link
    Member

    bitdancer commented Dec 29, 2013

    Nah, splitting it doesn't seem worth it unless you think the patch won't make it in.

    (Not that I looked at it earlier, but he patch on the issue doesn't look like what you just posted here...and here there's a typo: shuld).

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Dec 29, 2013

    I updated it as I liked your wording better. :) Doing more testing to see if anything else needs fixing before I make the next patch for the tracker on that issue.

    @terryjreedy
    Copy link
    Member

    terryjreedy commented Jan 4, 2014

    I believe this is my suggestion 2) in msg206715 of bpo-19995, and I think it better than 3) (and 1) alone). Thanks for moving this forward. I believe what Guido said is that indexes (integers in the broad sense) are (or should be) a subset of things convertible to ints. I concur completely.

    @amitavab
    Copy link
    Mannequin

    amitavab mannequin commented Jun 22, 2014

    I started working on this issue as part of the Bloomberg Python Sprint (https://etherpad.mozilla.org/LjZPQ55oZs).

    Ethan, can you confirm if the attached test case summarizes the issue correctly?

    I tried to hook _PyLong_FromNbInt to check nb_index but didn't make progress there. Will need to load in a debugger to see what's going on.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 23, 2014

    Yes, that test should pass when this issue is resolved.

    I can't tell from your comment what you are trying to do to resolve it, but the course of action I had in mind was to have the type meta-class make a final examination of the type it was creating, and if that type had index but not int then bind int to index.

    @amitavab
    Copy link
    Mannequin

    amitavab mannequin commented Jun 24, 2014

    Hi Ethan, I tried adding a call to nb_index (if that slot exists) in _PyLong_FromNbInt, but that didn't work. Based on your comment it seems the fix would be to patch this at object creation. I will check where that happens (bear with me while I familiarize myself with the code base :) ).

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 24, 2014

    Thank you for your efforts, Amitava. Please also sign a Contributor's License Agreement so we can actually use your code. :)

    http://www.python.org/psf/contrib/
    

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 24, 2014

    There will be a corporate agreement from Bloomberg sometime soon (and that will be required in this case, not an individual agreement).

    @amitavab
    Copy link
    Mannequin

    amitavab mannequin commented Jun 24, 2014

    I did fill in the contributor agreement form and e-signed it, maybe it takes some time for my profile to be updated? But I guess I need to wait for the corporate agreement. :)

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Mar 5, 2018

    Pingback from bpo-33002, which is caused by the fact that parts of the CPython code base that use PyNumber_Index for type conversion still pre-check for valid types with PyNumber_Check, meaning that a type with __index__ and not __int__ gets erroneously rejected by the pre-check.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented May 6, 2019

    See also the discussion on the duplicated bpo-33039.

    Few months ago I wrote the PR that makes constructors of int, float and complex to fall back to __index__ if corresponding special methods __int__, __float__ and __complex__ are not defined. I did not exposed it to public because binding __int__ to __index__ looks better to me. But perhaps some tests from that PR can be used in an alternate PR.

    @serhiy-storchaka serhiy-storchaka added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) 3.8 labels May 6, 2019
    @mdickinson
    Copy link
    Member

    mdickinson commented Jun 1, 2019

    I like the delegation of float and complex to use __index__ that was introduced in #57317; it would be nice to have that regardless of which solution is decided on for __int__ and __index__.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jun 1, 2019

    New changeset bdbad71 by Serhiy Storchaka in branch 'master':
    bpo-20092. Use __index__ in constructors of int, float and complex. (GH-13108)
    bdbad71

    @patrickyang1248
    Copy link
    Mannequin

    patrickyang1248 mannequin commented Nov 19, 2021

    I ended up in this issue after I learnt the following from the Python Library Reference Manual.

    ----
    float(..).

    For a general Python object x, float(x) delegates to x.__float__(). If __float__() is not defined then it falls back to __index__().
    ----

    The discussion on __int__() and __index__() was very interesting but I still didn't get the answer I wanted.

    If __int__() is assumed to be a possibly approximate conversion and it's possible that __int__() may exist while __index__() doesn't, shouldn't __int__() be used as a fall back before __index__()?

    The downside would be that the resulting float may not be "very close" to the original object because __int__() is only an approximation while __index__() guarantees exact, but loss of precision is acceptable during type conversion, isn't it? (i.e. int(3.14) -> 3). Perhaps it's not acceptable if the conversion is a widening conversion and that's why __int__() is skipped?

    @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.8 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants