classification
Title: fractions.Fraction with 3 arguments: error passes silently
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: josh.r, mark.dickinson, python-dev, rhettinger, serhiy.storchaka, steven.daprano, veky
Priority: normal Keywords: patch

Created on 2016-08-23 08:39 by veky, last changed 2016-08-23 15:58 by veky. This issue is now closed.

Files
File name Uploaded Description Edit
fraction_from_normalized.patch serhiy.storchaka, 2016-08-23 10:43 review
Messages (11)
msg273422 - (view) Author: Vedran Čačić (veky) * Date: 2016-08-23 08:39
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.
msg273423 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2016-08-23 08:44
+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?
msg273427 - (view) Author: Vedran Čačić (veky) * Date: 2016-08-23 09:07
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.
msg273432 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-23 10:43
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).
msg273439 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-23 12:47
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.
msg273440 - (view) Author: Vedran Čačić (veky) * Date: 2016-08-23 13:20
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.
msg273454 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2016-08-23 14:30
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.
msg273463 - (view) Author: Vedran Čačić (veky) * Date: 2016-08-23 14:59
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?
msg273465 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-23 15:17
New changeset ea495a5ded9b by Mark Dickinson in branch 'default':
Issue #27832: Make _normalize parameter to Fraction.__init__ keyword-only.
https://hg.python.org/cpython/rev/ea495a5ded9b
msg273466 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-23 15:18
Fixed.
msg273471 - (view) Author: Vedran Čačić (veky) * Date: 2016-08-23 15:58
Thank you very much! Not only for this fix, but also for restoring my faith in Python community. :-)
History
Date User Action Args
2016-08-23 15:58:52vekysetmessages: + msg273471
2016-08-23 15:18:31mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg273466

stage: patch review -> resolved
2016-08-23 15:17:12python-devsetnosy: + python-dev
messages: + msg273465
2016-08-23 15:06:23mark.dickinsonsetassignee: mark.dickinson
2016-08-23 14:59:15vekysetmessages: + msg273463
2016-08-23 14:30:54steven.dapranosetnosy: + steven.daprano
messages: + msg273454
2016-08-23 13:20:49vekysetmessages: + msg273440
2016-08-23 12:47:44mark.dickinsonsetmessages: + msg273439
2016-08-23 10:43:22serhiy.storchakasetfiles: + fraction_from_normalized.patch

type: enhancement

keywords: + patch
nosy: + rhettinger, serhiy.storchaka, mark.dickinson
messages: + msg273432
stage: patch review
2016-08-23 09:07:53vekysetmessages: + msg273427
components: + Library (Lib)
versions: + Python 3.6
2016-08-23 08:44:41josh.rsetnosy: + josh.r
messages: + msg273423
2016-08-23 08:39:41vekycreate