fractions.Fraction with 3 arguments: error passes silently

[vedgar]

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]

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]

+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]

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.

[serhiy-storchaka]

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).

[mdickinson]

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]

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]

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]

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?

[python-dev]

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]

Fixed.

[vedgar]

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