classification
Title: bool % int has inconsistent return type.
Type: behavior Stage: resolved
Components: Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: mark.dickinson, python-dev, terry.reedy, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-08-18 12:09 by mark.dickinson, last changed 2016-09-10 10:54 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
issue27792.patch xiang.zhang, 2016-08-22 05:48 review
issue27792_v2.patch xiang.zhang, 2016-08-22 10:40 use long_long instead of PyNumber_Long review
Messages (13)
msg273020 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-18 12:09
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/
msg273028 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-18 14:14
Ah, it looks as though this is already fixed in master, and it's probably not worth fixing for 3.5. Closing.
msg273030 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-18 14:27
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'>
msg273042 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-18 16:03
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`).
msg273146 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-08-19 17:32
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.
msg273334 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-22 05:48
issue27792.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.
msg273347 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-22 10:12
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.
msg273348 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-22 10:13
> 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!
msg273352 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-22 10:40
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. :)
msg273356 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-22 11:25
New changeset d998d87f0aa0 by Mark Dickinson in branch 'default':
Issue #27792: force int return type for modulo operations involving bools.
https://hg.python.org/cpython/rev/d998d87f0aa0
msg273357 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-22 11:29
issue27792_v2.patch LGTM. Thanks for the fix!
msg273358 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-22 11:30
Thanks for your work too. ;) You does the most important change.
msg275621 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-09-10 10:54
> 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 issue 28060 for fast path cleanup.
History
Date User Action Args
2016-09-10 10:54:57mark.dickinsonsetmessages: + msg275621
2016-08-22 11:30:48xiang.zhangsetmessages: + msg273358
2016-08-22 11:29:36mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg273357

stage: needs patch -> resolved
2016-08-22 11:25:00python-devsetnosy: + python-dev
messages: + msg273356
2016-08-22 10:40:32xiang.zhangsetfiles: + issue27792_v2.patch

messages: + msg273352
2016-08-22 10:13:39mark.dickinsonsetmessages: + msg273348
2016-08-22 10:12:45mark.dickinsonsetmessages: + msg273347
2016-08-22 07:52:27vstinnersetnosy: + vstinner
2016-08-22 05:48:38xiang.zhangsetfiles: + issue27792.patch
keywords: + patch
messages: + msg273334
2016-08-19 17:32:38terry.reedysetnosy: + terry.reedy
messages: + msg273146
2016-08-19 13:35:20mark.dickinsonsetassignee: mark.dickinson
2016-08-18 16:03:34mark.dickinsonsetmessages: + msg273042
2016-08-18 15:41:28xiang.zhangsetnosy: + xiang.zhang
2016-08-18 14:27:01mark.dickinsonsetstatus: closed -> open
resolution: out of date -> (no value)
messages: + msg273030
2016-08-18 14:14:34mark.dickinsonsetresolution: out of date
2016-08-18 14:14:27mark.dickinsonsetstatus: open -> closed

messages: + msg273028
2016-08-18 12:09:44mark.dickinsoncreate