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

fraction.Fraction does not implement __int__. #88713

Closed
mamrhein mannequin opened this issue Jul 1, 2021 · 16 comments
Closed

fraction.Fraction does not implement __int__. #88713

mamrhein mannequin opened this issue Jul 1, 2021 · 16 comments
Labels
3.11 stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@mamrhein
Copy link
Mannequin

mamrhein mannequin commented Jul 1, 2021

BPO 44547
Nosy @rhettinger, @mdickinson, @mamrhein, @ambv, @serhiy-storchaka, @MojoVampire
PRs
  • bpo-44547: Make Fractions objects instances of typing.SupportsInt #27851
  • 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 2021-10-21.22:10:31.682>
    created_at = <Date 2021-07-01.19:52:28.462>
    labels = ['type-feature', 'library', '3.11']
    title = 'fraction.Fraction does not implement __int__.'
    updated_at = <Date 2021-10-21.22:10:31.681>
    user = 'https://github.com/mamrhein'

    bugs.python.org fields:

    activity = <Date 2021-10-21.22:10:31.681>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-10-21.22:10:31.682>
    closer = 'lukasz.langa'
    components = ['Library (Lib)']
    creation = <Date 2021-07-01.19:52:28.462>
    creator = 'mamrhein'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44547
    keywords = ['patch']
    message_count = 15.0
    messages = ['396827', '396840', '396847', '396856', '396858', '396861', '396864', '396868', '396876', '397022', '397033', '399958', '400060', '404682', '404683']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'mark.dickinson', 'mamrhein', 'lukasz.langa', 'serhiy.storchaka', 'josh.r']
    pr_nums = ['27851']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue44547'
    versions = ['Python 3.11']

    @mamrhein
    Copy link
    Mannequin Author

    mamrhein mannequin commented Jul 1, 2021

    While int, float, complex and Decimal implement __int__, Fraction does not. Thus, checking for typing.SupportsInt for fractions fails, although int(<fraction>) succeeds, because Fraction implements __trunc__.
    This looks inconsistent.
    Easiest fix seems to be: Fraction.__int__ = Fraction.__trunc__

    @mamrhein mamrhein mannequin added type-bug An unexpected behavior, bug, or error 3.7 3.8 3.9 3.10 3.11 stdlib Python modules in the Lib dir labels Jul 1, 2021
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Jul 2, 2021

    Seems like an equally reasonable solution would be to make class's with __trunc__ but not __int__ automatically generate a __int__ in terms of __trunc__ (similar to __str__ using __repr__ when the latter is defined but not the former). The inconsistency is in both methods existing, but having the equivalence implemented in int() rather than in the type (thereby making SupportsInt behave unexpectedly, even though it's 100% true that obj.__int__() would fail).

    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 2, 2021

    FWIW, there's some history here: there's a good reason that fractions.Fraction didn't originally implement __int__.

    Back in the Bad Old Days, many Python functions that expected an integer would accept anything whose type implemented __int__ instead, and call __int__ to get the required integer. For example:

    Python 3.7.10 (default, Jun  1 2021, 23:43:35) 
    [Clang 11.0.3 (clang-1103.0.32.62)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from decimal import Decimal
    >>> chr(Decimal("45.67"))
    '-'

    Effectively, int was being used to mean two different things: (1) this value can be used as an integer, and (2) this value can be truncated to an integer. The solution was to introduce two new dedicated magic methods index and trunc for these two different meanings. See PEP-357 and PEP-3141 for some of the details. So adding int to fractions.Fraction would have made things like chr(Fraction("5/2")) possible, too.

    The behaviour above is still present (with a deprecation warning) in Python 3.9, and chr(Decimal("45.67")) has only finally been made a TypeError in Python 3.10.

    We may now finally be in a state where ill-advised uses of __int__ in internal functions have all been deprecated and removed, so that it's safe to re-add __int__ methods.

    But still, this seems mostly like an issue with the typing library. What is typing.SupportsInt intended to indicate? That an object can be used _as_ an integer, or that an object can be _truncated_ to an integer?

    @mdickinson mdickinson added type-feature A feature request or enhancement and removed 3.7 3.8 3.9 3.10 type-bug An unexpected behavior, bug, or error labels Jul 2, 2021
    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 2, 2021

    I'm actually struggling to think of situations where typing.SupportsInt would be useful in its current form: if I'm writing a function that wants to do a duck-typed acceptance of integer-like things (for example because I want my function to work with NumPy integers as well as plain old Python ints) then I want an __index__ check rather than an __int__ check. If I'm writing a function that allows general numeric inputs, then I'm not sure why I'd be calling 'int' on those inputs.

    As another data point, complex supporting __int__ is a little bit of an oddity, since all that __int__ method does is raise a TypeError.

    @michael: are you in a position to share the use-case that motivated opening the issue? I'd be interested to see any concrete uses of typing.SupportsInt.

    Maybe typing.SupportsIndex (or typing.UsableAsInt, or ... --- naming things is hard) is what we actually need?

    On this particular issue: I'm not opposed to adding __int__ to fractions.Fraction purely for the sake of consistency, but it's not yet clear to me that it solves any real issue.

    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 2, 2021

    Maybe typing.SupportsIndex

    Apologies: that already exists, of course. It was introduced in bpo-36972.

    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 2, 2021

    As another data point, complex supporting __int__ is a little bit of an oddity, since all that __int__ method does is raise a TypeError.

    This was fixed in 3.10: bpo-41974

    @mamrhein
    Copy link
    Mannequin Author

    mamrhein mannequin commented Jul 2, 2021

    The background is an implementation of __pow__ for a fixed-point decimal number:

    SupportsIntOrFloat = Union[SupportsInt, SupportsFloat]
    
    def __pow__(self, other: SupportsIntOrFloat, mod: Any = None) -> Complex:
        if isinstance(other, SupportsInt):
            exp = int(other)
            if exp == other:
                ... handle integer exponent
        if isinstance(other, SupportsFloat):
            # fractional exponent => fallback to float
            return float(self) ** float(other)
        return NotImplemented

    I came across SupportsInt and SupportsFloat, because they are used in typeshed as param types for int and float.

    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 2, 2021

    Thanks, that's helpful. I guess what you _really_ want there is a duck-typed "tell me whether this value is integral and if so give me the corresponding Python int", but that's not currently easily available, so I suppose x == int(x) is the next-best thing. Possibly the "right" way from the point of view of PEP-3141 is to be testing x == math.trunc(x) instead and asking for typing.SupportsTrunc, but it seems to me that __trunc__ never did really take off the way it was intended to.

    tl;dr: I agree it would make sense to add __int__ to fractions.Fraction for 3.11.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jul 2, 2021

    In ideal world we would use __int__ for (1), and __trunc__ for (2). But for some historical reasons __index__ was introduced for (1) and __int__ is only used in the int constructor, although it falls back to __trunc__.

    I am wondering whether one of __int__ or __trunc__ should be deprecated.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 5, 2021

    I am wondering whether one of __int__ or __trunc__ should be deprecated.

    I would not miss __trunc__.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jul 6, 2021

    On other hand, there are classes which define __int__ but not __trunc__: UUID and IP4Address. So it makes sense to keep separate __int__ and __trunc__.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 20, 2021

    Fraction.__int__ = Fraction.__trunc__ may not work because __trunc__() can return any object with __index__, while __int__ should return an exact int (not even an int subclass).

    @mdickinson
    Copy link
    Member

    mdickinson commented Aug 22, 2021

    I think there's no reason not to keep __trunc__ and math.trunc - they're natural counterparts to floor and ceil, and there's probably at least some code out there already using math.trunc.

    It's the involvement of __trunc__ in the int() builtin that I'd quite like to deprecate and eventually remove. I think at this point it complicates the object model for no particularly good reason. But this is getting off-topic for this issue; I'll open a new one.

    @ambv
    Copy link
    Contributor

    ambv commented Oct 21, 2021

    New changeset d1b2477 by Mark Dickinson in branch 'main':
    bpo-44547: Make Fractions objects instances of typing.SupportsInt (GH-27851)
    d1b2477

    @ambv
    Copy link
    Contributor

    ambv commented Oct 21, 2021

    Thanks for the patch, Mark! 🍰

    @ambv ambv closed this as completed Oct 21, 2021
    @ambv ambv closed this as completed Oct 21, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @MojoVampire
    Copy link
    Contributor

    MojoVampire commented May 3, 2022

    The background is an implementation of pow for a fixed-point decimal number:

    SupportsIntOrFloat = Union[SupportsInt, SupportsFloat]
    
    def __pow__(self, other: SupportsIntOrFloat, mod: Any = None) -> Complex:
        if isinstance(other, SupportsInt):
            exp = int(other)
            if exp == other:
                ... handle integer exponent
        if isinstance(other, SupportsFloat):
            # fractional exponent => fallback to float
            return float(self) ** float(other)
        return NotImplemented

    I came across SupportsInt and SupportsFloat, because they are used in typeshed as param types for int and float.

    I'm going to note, this code is subtly confusing/wrong. Since float itself is considered an instance of SupportsInt (and I believe has done so for as long as isinstance has worked with typing.SupportsInt), actual floats will be treated as int if they happen to have no decimal component (the equality test at least avoids the worst case where they silently lose data), so the values of your arguments change the type of your result, which is... not great.

    In practice, you really want to be testing for SupportsIndex (which means something is logically equivalent to an integer and can be converted to an int losslessly) and using operator.index(other) for the conversion (int(operator.index(other)) if it must produce an exact int, not a subclass thereof), or, with slightly different semantics (that should never differ on properly implemented integral types, but many types aren't properly implemented), testing isinstance(other, numbers.Integral) (at which point it usually doesn't matter if you use int() or operator.index() since it supports both, and at worst using index allows you to get an int subclass as a result, where int() guarantees a true int).

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants