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

int() and math.trunc don't accept objects that only define __index__ #77220

Closed
ncoghlan opened this issue Mar 10, 2018 · 10 comments
Closed

int() and math.trunc don't accept objects that only define __index__ #77220

ncoghlan opened this issue Mar 10, 2018 · 10 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 33039
Nosy @mdickinson, @ncoghlan, @serhiy-storchaka, @appeltel, @csabella, @remilapeyre, @tirkarthi
PRs
  • bpo-20092: Make __int__ defaults to __index__ #13106
  • Superseder
  • bpo-20092: type() constructor should bind int to index when index is defined and int is not
  • 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 2019-05-06.14:29:29.151>
    created_at = <Date 2018-03-10.08:57:10.957>
    labels = ['3.8', 'type-feature', '3.7', 'docs']
    title = "int() and math.trunc don't accept objects that only define __index__"
    updated_at = <Date 2019-05-06.14:29:29.150>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2019-05-06.14:29:29.150>
    actor = 'serhiy.storchaka'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2019-05-06.14:29:29.151>
    closer = 'serhiy.storchaka'
    components = ['Documentation']
    creation = <Date 2018-03-10.08:57:10.957>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33039
    keywords = ['patch']
    message_count = 10.0
    messages = ['313515', '313522', '332819', '333546', '336044', '336201', '336234', '340453', '341505', '341508']
    nosy_count = 8.0
    nosy_names = ['mark.dickinson', 'ncoghlan', 'docs@python', 'serhiy.storchaka', 'Eric Appelt', 'cheryl.sabella', 'remi.lapeyre', 'xtreak']
    pr_nums = ['13106']
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '20092'
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33039'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @ncoghlan
    Copy link
    Contributor Author

    (Note: I haven't categorised this yet, as I'm not sure how it *should* be categorised)

    Back when the __index__/nb_index slot was added, the focus was on allowing 3rd party integer types to be used in places where potentially lossy conversion with __int__/nb_int *wasn't* permitted.

    However, this has led to an anomaly where the lossless conversion method *isn't* tried implicitly for the potentially lossy int() and math.trunc() calls, but is tried automatically in other contexts:

    >>> import math
    >>> class MyInt:
    ...     def __index__(self):
    ...         return 42
    ... 
    >>> int(MyInt())
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: int() argument must be a string, a bytes-like object or a number, not 'MyInt'
    >>> math.trunc(MyInt())
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: type MyInt doesn't define __trunc__ method
    >>> hex(MyInt())
    '0x2a'
    >>> len("a" * MyInt())
    42
    
    

    Supporting int() requires also setting __int__:

    >>> MyInt.__int__ = MyInt.__index__
    >>> int(MyInt())
    42
    

    Supporting math.trunc() requires also setting __trunc__:

    >>> MyInt.__trunc__ = MyInt.__index__
    >>> math.trunc(MyInt())
    42
    

    (This anomaly was noticed by Eric Appelt while updating the int() docs to cover the fallback to trying __trunc__ when __int__ isn't defined: #6022 (comment))

    @ncoghlan
    Copy link
    Contributor Author

    Marking this as a documentation enhancement request for now, but I think we should also consider changing the type creation behaviour in 3.8 to implicitly add __int__ and __trunc__ definitions when __index__ is defined, but they aren't.

    That way, no behaviour will change for classes that explicitly define __int__ or __trunc__, but classes that only define __index__ without defining the other methods will behave more intuitively.

    @ncoghlan ncoghlan added docs Documentation in the Doc dir 3.7 (EOL) end of life 3.8 only security fixes labels Mar 10, 2018
    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label Mar 10, 2018
    @remilapeyre
    Copy link
    Mannequin

    remilapeyre mannequin commented Dec 31, 2018

    I think we should also consider changing the type creation behaviour in 3.8

    @ncoghlan is this what's being done in PyTypeReady?

    @ncoghlan
    Copy link
    Contributor Author

    @rémi Aye, filling out derived slots is one of the things PyType_Ready does.

    This would need a discussion on python-dev before going ahead and doing it though, as the closest equivalent we have to this right now is the "negative" derivation, where overriding __eq__ without overriding __hash__ implicitly marks the derived class as unhashable (look for "type->tp_hash = PyObject_HashNotImplemented;").

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-33039.

    @serhiy-storchaka
    Copy link
    Member

    Is not this a duplicate of bpo-20092?

    @remilapeyre
    Copy link
    Mannequin

    remilapeyre mannequin commented Feb 21, 2019

    Yes it is. Thanks for finding that @serhiy.

    Since nobody objected to the change on the mailing list and people seem to agree in bpo-20092:

    [R. David Murray]
    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.
    

    I will post my patch tonight.

    @csabella
    Copy link
    Contributor

    Rémi,

    Are you still working on the patch for this? Thanks!

    @remilapeyre
    Copy link
    Mannequin

    remilapeyre mannequin commented May 6, 2019

    Hi Cheryl,

    thanks for the ping.

    I wasn't sure my patch was correct but reading typeobject.c:add_operators(), it is actually more straight-forward than I thought.

    Serhiy Storchaka: This is indeed a duplicate of bpo-20092. I believe the solution proposed by Nick Coghlan is better than the one of Amitava Bhattacharyya, "adding a call to nb_index (if that slot exists) in _PyLong_FromNbInt" though.

    One thing to note regarding the proposed patch: the following stops to work and raises a RecursionError since __index__ == __int__:

            class MyInt(int):
                def __index__(self):
                    return int(self) + 1

    I changed test_int_subclass_with_index() as int(self) + 1 is the same thing as self + 1 for int subclasses. I don't think this sort of code should appear in the wild but if you think it is important not to break compatibility here, I think I could check for number subclasses before overriding index.

    @serhiy-storchaka
    Copy link
    Member

    Then let to continue the discussion on the older issue which has larger discussion.

    @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.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants