Skip to content

bpo-39274: Ensure Fraction.__bool__ returns a bool #18017

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 6, 2020
Merged

bpo-39274: Ensure Fraction.__bool__ returns a bool #18017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lib/fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,9 @@ def __ge__(a, b):

def __bool__(a):
"""a != 0"""
return a._numerator != 0
# bpo-39274: Use bool() because (a._numerator != 0) can return an
# object which is not a bool.
return bool(a._numerator)

# support for pickling, copy, and deepcopy

Expand Down
37 changes: 37 additions & 0 deletions Lib/test/test_fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import numbers
import operator
import fractions
import functools
import sys
import unittest
import warnings
Expand Down Expand Up @@ -346,6 +347,42 @@ def testConversions(self):

self.assertTypedEquals(0.1+0j, complex(F(1,10)))

def testBoolGuarateesBoolReturn(self):
# Ensure that __bool__ is used on numerator which guarantees a bool
# return. See also bpo-39274.
@functools.total_ordering
class CustomValue:
denominator = 1

def __init__(self, value):
self.value = value

def __bool__(self):
return bool(self.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is that "numerator != 0" returns an object which is not a subtype of bool. Does your test reproduce the bug if you revert the fractions.py change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I am testing that r._numerator.__bool__ is called, which is not quite the same thing of course. Other solutions to achieve the same things (e.g. bool(a._numerator != 0) are possible and would fail the test as well).
I can add __ne__ that does the bad thing, but it would never get called. This forces to adapt the test in case the code is adapted, but without a full fledged numpy integer around, it seems hard to write a generic test that only checks for a bool result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, tried to make it a bit more waterproof (by resetting bool_used), if I can import NumPy – skipping the test when its not available – that may be nicer of course (but I assume that is a no-go). Otherwise it feels like I would have to implement a full fleged rational with added strange logic...

Its over engineered, but right now I do not have a better idea TBH (unless we just skip the test...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot use 3rd party modules like numpy in the Python test suite, sadly. (At least, we are trying hard to avoid that.)

So you should try to mimick numpy's bool behavior. Please just implement a ne() method which raises an AssertionError. It would make me more comfortable. Or better: implement eq and lt, and @ @functools.total_ordering:
https://docs.python.org/dev/library/functools.html

Again, these methods would raise an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next try... Had tried to avoid registering with numbers.Rational but there is probably no reason, so now not inheriting from int anymore, which is much cleaner (and added your error, instead of ensuring __bool__ is called).

Overall, seems much better, since now it really is a crippled object that cannot do anything but what it is supposed to do...


@property
def numerator(self):
# required to preserve `self` during instantiation
return self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove that and use F(numerator, 1) instead.


def __eq__(self, other):
raise AssertionError("Avoid comparisons in Fraction.__bool__")

__lt__ = __eq__

# We did not implement all abstract methods, so register:
numbers.Rational.register(CustomValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may fail if you run the test with reference leak test:

./python -m test -R 3:3 test_fractions

Can you check that it does not leak? I would prefer to avoid .register() if it does leak.

... This test is getting more and more complex... Maybe we should just ignore the test and just merge the fix.

What do you think @mdickinson?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, seems to work:

 % /home/sebastian/python-dev/bin/python3.9d  -m test -R 3:3 test_fractions
0:00:00 load avg: 0.39 Run tests sequentially
0:00:00 load avg: 0.39 [1/1] test_fractions
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 221 ms
Tests result: SUCCESS

I have not used the python refcount checker, so not sure if it clears ABCs are not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself looks good to me; I think we should merge as is.


numerator = CustomValue(1)
r = F(numerator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to explicit the denominator: F(numerator, 1). Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not change much, if I add that, the only difference is that I also have to define __mul__. The only solution is going back to r._numerator = ... (it is used elsewhere in the file)

# ensure the numerator was not lost during instantiation:
self.assertIs(r.numerator, numerator)
self.assertIs(bool(r), True)

numerator = CustomValue(0)
r = F(numerator)
self.assertIs(bool(r), False)

def testRound(self):
self.assertTypedEquals(F(-200), round(F(-150), -2))
self.assertTypedEquals(F(-200), round(F(-250), -2))
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-01-15-23-13-03.bpo-39274.lpc0-n.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
``bool(fraction.Fraction)`` now returns a boolean even if (numerator != 0) does not return a boolean (ex: numpy number).