bool % int has inconsistent return type.

[mdickinson]

BPO	27792
Nosy	@terryjreedy, @mdickinson, @vstinner, @zhangyangyu
Files	issue27792.patch issue27792_v2.patch: use long_long instead of PyNumber_Long

^{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-22.11:29:36.406>
created_at = <Date 2016-08-18.12:09:44.531>
labels = ['type-bug']
title = 'bool % int has inconsistent return type.'
updated_at = <Date 2016-09-10.10:54:57.498>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2016-09-10.10:54:57.498>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2016-08-22.11:29:36.406>
closer = 'mark.dickinson'
components = []
creation = <Date 2016-08-18.12:09:44.531>
creator = 'mark.dickinson'
dependencies = []
files = ['44185', '44186']
hgrepos = []
issue_num = 27792
keywords = ['patch']
message_count = 13.0
messages = ['273020', '273028', '273030', '273042', '273146', '273334', '273347', '273348', '273352', '273356', '273357', '273358', '275621']
nosy_count = 5.0
nosy_names = ['terry.reedy', 'mark.dickinson', 'vstinner', 'python-dev', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27792'
versions = ['Python 3.5', 'Python 3.6']

[mdickinson]

Seen on reddit [1]:

>>> True % 1
0
>>> True % 2
True

I believe that we should be returning an int in both these cases; this looks like a longobject.c fast path gone wrong.

[1] https://www.reddit.com/r/learnpython/comments/4y5bh1/can_someone_explain_why_true_1_0_but_true_2_true/

[mdickinson]

Ah, it looks as though this is already fixed in master, and it's probably not worth fixing for 3.5. Closing.

[mdickinson]

Whoops; no, it's not fixed. 3.6 introduced a fast path that has the side-effect of fixing this issue for the True % 2 case, but not for all cases:

>>> False % 2
False
>>> True % 2
1
>>> class MyInt(int): pass
... 
>>> type(MyInt(0) % 6)
<class '__main__.MyInt'>
>>> type(MyInt(1) % 6)
<class 'int'>

[mdickinson]

FWIW, I'd suggest not changing this in 3.5; only in 3.6. It's a fairly harmless bug that's been with us throughout the 3.x series so far (and even back into 2.x, if you start looking at behaviour with subclasses of long).

[terryjreedy]

This can only happen because of a hole in the tests. test_bool.BoolTest.test_math appears to test every binary int op, including bitwise, *except* %.

After
self.assertIsNot(False/1, False)
add
self.assertEqual(False%1, 0)
self.assertIsNot(False%1, False) # currently fails
self.assertEqual(True%1, 1)
self.assertIsNot(True%1, True)

test_int tests int() calls, not int math, so I don't know where the equivalent tests on int math with subclasses are or would go.

[zhangyangyu]

bpo-27792.patch tries to fix this.

BTW, Mark, do you think the fast path in long_mod is really needed? Actually the same fast path has already existed in l_divmod.

[mdickinson]

Thanks for the patch. I think what we really want here is a call to long_long rather than PyNumber_Long; PyNumber_Long includes all the conversions using __trunc__, etc., which we don't need here.

[mdickinson]

BTW, Mark, do you think the fast path in long_mod is really needed?

I agree that the fast paths need looking at; it seems there's a fair bit of redundancy there. But not in this issue, please!

[zhangyangyu]

Oh, sorry. I forgot about long_long. Actually I did think for a while to search for a good function here. :( v2 uses long_long now.

I agree that the fast paths need looking at; it seems there's a fair bit of redundancy there. But not in this issue, please!

Don't worry. It's only a passing mention. Not in this thread. :)

[python-dev]

New changeset d998d87f0aa0 by Mark Dickinson in branch 'default':
Issue bpo-27792: force int return type for modulo operations involving bools.
https://hg.python.org/cpython/rev/d998d87f0aa0

[mdickinson]

issue27792_v2.patch LGTM. Thanks for the fix!

[zhangyangyu]

Thanks for your work too. ;) You does the most important change.

[mdickinson]

BTW, Mark, do you think the fast path in long_mod is really needed? Actually the same fast path has already existed in l_divmod.

See bpo-28060 for fast path cleanup.