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

fractions.Fraction with 3 arguments: error passes silently #72019

Closed
vedgar mannequin opened this issue Aug 23, 2016 · 11 comments
Closed

fractions.Fraction with 3 arguments: error passes silently #72019

vedgar mannequin opened this issue Aug 23, 2016 · 11 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vedgar
Copy link
Mannequin

vedgar mannequin commented Aug 23, 2016

BPO 27832
Nosy @rhettinger, @mdickinson, @stevendaprano, @serhiy-storchaka, @MojoVampire, @vedgar
Files
  • fraction_from_normalized.patch
  • 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 = 'https://github.com/mdickinson'
    closed_at = <Date 2016-08-23.15:18:31.267>
    created_at = <Date 2016-08-23.08:39:41.189>
    labels = ['type-feature', 'library']
    title = 'fractions.Fraction with 3 arguments: error passes silently'
    updated_at = <Date 2016-08-23.15:58:52.107>
    user = 'https://github.com/vedgar'

    bugs.python.org fields:

    activity = <Date 2016-08-23.15:58:52.107>
    actor = 'veky'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2016-08-23.15:18:31.267>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2016-08-23.08:39:41.189>
    creator = 'veky'
    dependencies = []
    files = ['44197']
    hgrepos = []
    issue_num = 27832
    keywords = ['patch']
    message_count = 11.0
    messages = ['273422', '273423', '273427', '273432', '273439', '273440', '273454', '273463', '273465', '273466', '273471']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'mark.dickinson', 'steven.daprano', 'python-dev', 'serhiy.storchaka', 'josh.r', 'veky']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27832'
    versions = ['Python 3.6']

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 23, 2016

    During the discussion about http://bugs.python.org/issue27539, we found the following weird behavior:

        >>> from fractions import Fraction as F
        >>> F(2, 3, 4)
        Fraction(2, 3)

    It looks like the third argument is simply disregarded, but it's not so: if it's false, the fraction is not normalized, and it breaks a lot of arithmetic assumptions.

        >>> F(4, 2, 0)
        Fraction(4, 2)
        >>> _ == 2
        False

    The secret is additional argument "_normalize" to Fraction.__new__, which was added for optimization in various cases (where we know that numerator and denominator are relatively prime, so the gcd needn't be computed). That argument was obviously meant for internal use only (its name starts with '_'), so accepting the above forms can be viewed as a bug. Although cases can be made for being able to set that argument from the outside, surely in most cases people passing 3 arguments are doing it by accident, not on purpose.

    That bug is easily fixed, by declaring the argument as keyword only. The line 84 in lib/fractions.py should be changed from

    • def \_\_new__(cls, numerator=0, denominator=None, \_normalize=True):
      

    to

    + def __new__(cls, numerator=0, denominator=None, *, _normalize=True):

    That way, those who know what they are doing, can still pass the _normalize argument, but it's much harder to accidentally pass it for people who don't know nor care about it.

    Of course, all the code from that file already passes _normalize by keyword, so no other code should be changed.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Aug 23, 2016

    +1; obviously, this could only be changed on Python 3.

    Given the third argument is underscore prefixed, I think it can be safely changed without a deprecation period, it's not a public part of the interface, right?

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 23, 2016

    Yes, although it can be viewed as a bugfix, it's impossible on Python 2. We _can_ do it on Python 2 too, with

        def __new__(cls, numerator=0, denominator=None, *empty, _normalize=True):
            if empty:
                raise TypeError('too many positional arguments')
            ...

    but I'm not interested in it [and honestly, I'm sick of uglifying my code just to be able to run it on Py2]. Also, Mark Dickinson said (http://bugs.python.org/issue27539#msg273282) it shouldn't even be changed on Py3.5, so surely then it shouldn't be changed on Py2.7. :-)

    I think no deprecation is needed: it isn't (https://docs.python.org/3.5/library/fractions.html?highlight=fraction#fractions.Fraction), and as far as I know has never been, documented.

    @vedgar vedgar mannequin added the stdlib Python modules in the Lib dir label Aug 23, 2016
    @serhiy-storchaka
    Copy link
    Member

    Wouldn't be better to get rid of private parameter (it is visible in the docs) at all?

    Proposed patch removes the _normalize parameter and adds very simple private module level function instead (hidden from the docs).

    @serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Aug 23, 2016
    @mdickinson
    Copy link
    Member

    Sorry, but to be honest, I think this is a non-issue. Writing Fraction(2, 3, 4) doesn't seem like a very likely cause of real bugs. I'd rather see the code left as it is.

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 23, 2016

    Argh. This is not the first time my proposal was blown way out of proportion, and then killed. :-(

    What exactly is wrong with making _normalized keyword-only? All the serious usage (including all the usage already in fractions.py) remain the same, and we avoid an obvious "error passes silently" issue.

    I know Serhiy's proposal is scary, it scares me too. super call inside a non-method is something only wizards use, probably. :-) But that doesn't mean we should give up on a trivial enhancement that correctly counts the given arguments.

    Would you really be fine with, e.g. a builtin chr function, that is documented exactly as it is now documented, but in fact can be called with two arguments, and if the second argument is false, it works exactly the same, but if it is true, it returns an object that looks like a string of length one, but in fact is surrogate-represented (of length two) if the first argument is greater than 65535? I'm sure it would be pronounced a bug almost immediately. And I don't see how it's different from this.

    Python callables _do_ count their arguments. Python is not JavaScript. Calling a function with a different number of arguments than it receives _is_ an error. Errors shouldn't pass silently. _Especially_ if they happen rarely.

    @stevendaprano
    Copy link
    Member

    At the risk of muddying the waters even further, I'd like to make _normalize a public parameter in Python 3.7. There's an interesting operation you can do with fractions, the mediant:

    https://en.wikipedia.org/wiki/Mediant_%28mathematics%29

    http://www.mathteacherctk.com/blog/2011/02/06/mediant-fractions-and-simpsons-paradox/

    It's easy to work with mediants provided you have a way to prevent fractions from being normalised automatically.

    So I'm +1 on making _normalize a keyword-only argument for 3.6 as a first step.

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 23, 2016

    As much as Steven's proposal would give me what I want in Py3.6, I must say I'm -1 on it. The assumption of every Fraction being reduced to lowest terms is really an important invariant that provides numerous opportunities for optimization (look at __eq__ for example, second if). I think it would be very wrong to have to generalize all the algorithms of Fraction to work correctly with non-normalized Fractions.

    Furthermore, current (normalized) fractions are Rationals, thus belonging to the Python numeric tower. non-normalized fractions are not. Even if we _would_ someday have non-normalized fractions in stdlib, they would probably need to be named differently. (Currently Decimals are having an opposite problem: the main reason why adding decimal literals to Python is hard is because they would obviously have to be Reals, thus belonging to numeric tower, while current decimal.Decimals do not belong there.:)

    [And mediants can be usefully researched if you stay within |Q, too; some of my students have done that. Even Wikipedia says (though it doesn't go into details):

    A way around this, where required, is to specify that both rationals are to be represented as fractions in their lowest terms (with c > 0, d > 0). With such a restriction, mediant becomes a well-defined binary operation on rationals.
    
        from fractions import Fraction
        def mediant(p:Fraction, q:Fraction):
            return Fraction(p.numerator + q.numerator,
                                      p.denominator + q.denominator)
    ]

    Now, can we go back to a tiny three-character addition with no disadvantages at all (if I'm wrong, please inform me), making Python stdlib a bit better in catching accidental mistakes?

    @mdickinson mdickinson self-assigned this Aug 23, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 23, 2016

    New changeset ea495a5ded9b by Mark Dickinson in branch 'default':
    Issue bpo-27832: Make _normalize parameter to Fraction.__init__ keyword-only.
    https://hg.python.org/cpython/rev/ea495a5ded9b

    @mdickinson
    Copy link
    Member

    Fixed.

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 23, 2016

    Thank you very much! Not only for this fix, but also for restoring my faith in Python community. :-)

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants