Skip to content

/ cpython

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

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

Open
wants to merge 1 commit into
base: master
from
Open

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

wants to merge 1 commit into from
+41 −1

Conversation

@seberg
Copy link
Contributor

seberg commented Jan 15, 2020 •
edited by bedevere-bot

Some numerator types used (specifically NumPy) decides to not
return a python boolean for the != operation. Using the equivalent
call to bool() guarantees a bool return also for such types.

Closes bpo-39274

https://bugs.python.org/issue39274

https://bugs.python.org/issue39274
@vstinner
vstinner reviewed Jan 15, 2020
View changes
Lib/test/test_fractions.py Outdated
return bool(self.value)

r = F(1, 1)
r._numerator = custom_value(1)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 15, 2020

Member

I would prefer to avoid accessing private attribute _numerator:

numerator = custom_value(1)
r = F(numerator, 1)
...
self.assertEqual(numerator.bool_used, True)

(same below)

This comment has been minimized.

Sign in to view
Copy link
@seberg

seberg Jan 15, 2020

Author Contributor

I agree fixed, I just got lost a bit because I missed that numerator.numerator is used (which loses the original type), but since its only that it seems nicer. Of course if the instantiation logic changes, there is a chance of the test needing to be revised.
@seberg seberg force-pushed the seberg:fraction-bool-return-bool branch from 56bdc21 to 89fc30c Jan 15, 2020
@vstinner
vstinner reviewed Jan 16, 2020
View changes
Lib/fractions.py Show resolved Hide resolved
Misc/NEWS.d/next/Library/2020-01-15-23-13-03.bpo-39274.lpc0-n.rst Outdated Show resolved Hide resolved
Lib/test/test_fractions.py
return self

numerator = custom_value(1)
r = F(numerator)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 16, 2020

Member

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

This comment has been minimized.

Sign in to view
Copy link
@seberg

seberg Jan 16, 2020

Author Contributor

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)
Lib/test/test_fractions.py

def __bool__(self):
self.bool_used = True
return bool(self.value)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 16, 2020

Member

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?

This comment has been minimized.

Sign in to view
Copy link
@seberg

seberg Jan 16, 2020

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@seberg

seberg Jan 16, 2020

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 17, 2020

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@seberg

seberg Jan 17, 2020

Author Contributor

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...
Lib/test/test_fractions.py
@property
def numerator(self):
# required to preserve `self` during instantiation
return self

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 16, 2020

Member

I would prefer to remove that and use F(numerator, 1) instead.
@seberg seberg force-pushed the seberg:fraction-bool-return-bool branch 2 times, most recently from 08aee01 to 2413700 Jan 16, 2020
@vstinner
vstinner reviewed Jan 17, 2020
View changes
Lib/test/test_fractions.py

def __bool__(self):
self.bool_used = True
return bool(self.value)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 17, 2020

Member

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.
Lib/test/test_fractions.py Outdated
r = F(numerator)
# ensure the numerator was not lost during instantiation:
self.assertTrue(r.numerator is numerator)
numerator.bool_used = False

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 17, 2020

Member

I suggest self.assertFalse(numerator.bool_used) instead.

Suggested change
numerator.bool_used = False
self.assertFalse(numerator.bool_used)
Lib/test/test_fractions.py Outdated
numerator.bool_used = False
self.assertTrue(bool(r) is True)
# If bool(numerator) was called, a bool result should be guaranteed:
self.assertEqual(numerator.bool_used, True)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 17, 2020

Member
Suggested change
self.assertEqual(numerator.bool_used, True)
self.assertTrue(numerator.bool_used)
Lib/test/test_fractions.py
# ensure the numerator was not lost during instantiation:
self.assertTrue(r.numerator is numerator)
numerator.bool_used = False
self.assertTrue(bool(r) is True)

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner Jan 17, 2020

Member

To get the value if the test fails:

Suggested change
self.assertTrue(bool(r) is True)
self.assertIs(bool(r), True)
@seberg
BUG: Ensure `Fraction.__bool__` returns a bool
Loading status checks…
4bf428a 
Some numerator types used (specifically NumPy) decides to not
return a python boolean for the `!=` operation. Using the equivalent
call to `bool()` guarantees a bool return also for such types.

Closes bpo-39274
@seberg seberg force-pushed the seberg:fraction-bool-return-bool branch from 2413700 to 4bf428a Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

Assignees
No one assigned
Labels
CLA signed awaiting review
Projects
None yet
Milestone
No milestone
4 participants
@seberg @vstinner @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.