classification
Title: The result type of bytearray formatting is not stable
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, ethan.furman, python-dev, serhiy.storchaka, vstinner
Priority: high Keywords: patch

Created on 2016-04-15 09:43 by berker.peksag, last changed 2016-04-15 22:20 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
bytearray_mod.diff berker.peksag, 2016-04-15 09:43 review
writer_bytearray_mod.patch vstinner, 2016-04-15 13:37 review
Messages (15)
msg263462 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-15 09:43
I noticed this while looking at issue 26764.

bytearray_mod() and bytearray_format() both have checks for PyByteArray_Check(v). The check in bytearray_format() looks redundant to me.

Here is a patch.
msg263465 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 10:16
Yes, I noticed this, but left for future (post-issue26765) refactoring.

The patch LGTM, but notice Victor's comment to issue26764.
msg263469 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 10:36
And may be bytearray_mod in 3.6 is not correct. In 3.5 it returns bytearray, in 3.6 it returns bytes.
msg263473 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-15 11:19
Do you have an example code? It returns bytearray for me in both 3.5 and 3.6. use_bytearray parameter of _PyBytes_FormatEx() is 1 in bytearray_mod().
msg263477 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 11:35
Python 3.5:

>>> bytearray(b'%d') % 42
bytearray(b'42')

Python 3.6:

>>> bytearray(b'%d') % 42
b'42'
msg263485 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-15 12:12
Thanks! More examples:

Python 3.6:

>>> bytearray(b'hello %b') % b"world"
bytearray(b'hello world')
>>> bytearray(b'hello %b') % b"wor"
b'hello wor'

Python 3.5:

>>> bytearray(b'hello %b') % b"world"
bytearray(b'hello world')
>>> bytearray(b'hello %b') % b"wor"
bytearray(b'hello wor')
msg263486 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 12:28
Hmm, looks the result type is very unstable.

On release build:

>>> bytearray(b'hello %b') % b"world"
b'hello world'
>>> bytearray(b'hello %b') % b"wor"
b'hello wor'

On debug build:

>>> bytearray(b'hello %b') % b"world"
bytearray(b'hello world')
>>> bytearray(b'hello %b') % b"wor"
b'hello wor'

And current test_bytes.py is passed on release build, but is failed on debug build.
msg263492 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-15 13:10
It looks like a bug introduced by me in the issue #25399.

bytearray%args must return a bytearray object.

I understand that test_bytes lacks tests since it missed the bug.
msg263496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-15 13:37
Here is a fix for bytearray%args. As expected, it was a bug in the new _PyBytesWriter API (introduced in Python 3.6 to implementation optimizations).
msg263500 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 15:47
LGTM, please commit. Why the behavior is vary in release and debug builds?
msg263502 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-15 15:52
New changeset 1eb586d5b321 by Victor Stinner in branch 'default':
Issue #26766: Fix _PyBytesWriter_Finish()
https://hg.python.org/cpython/rev/1eb586d5b321
msg263503 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-15 15:52
> LGTM, please commit.

Ok, done.

> Why the behavior is vary in release and debug builds?

See _PyBytesWriter_Alloc(): the code is designed to detect bugs earlier in debug mode, it only uses 10 bytes of the "small buffer" (buffer allocated on the stack) rather than the full size of the smaller buffer (512 bytes).

It looks like this hack helped to find a real bug :-)
msg263504 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-15 15:54
@Berker: bytearray_mod.diff LGTM, you can push it.
msg263529 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-15 22:20
New changeset b3e3efc5afa4 by Berker Peksag in branch 'default':
Issue #26766: Remove redundant bytearray_format() from bytearrayobject.c
https://hg.python.org/cpython/rev/b3e3efc5afa4
msg263530 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-04-15 22:20
Thanks for the reviews!
History
Date User Action Args
2016-04-15 22:20:34berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg263530

stage: resolved
2016-04-15 22:20:00python-devsetmessages: + msg263529
2016-04-15 15:54:44vstinnersetmessages: + msg263504
2016-04-15 15:52:56vstinnersetmessages: + msg263503
2016-04-15 15:52:49python-devsetnosy: + python-dev
messages: + msg263502
2016-04-15 15:47:00serhiy.storchakasetmessages: + msg263500
2016-04-15 13:37:21vstinnersetfiles: + writer_bytearray_mod.patch

messages: + msg263496
2016-04-15 13:10:43vstinnersetmessages: + msg263492
2016-04-15 12:30:01serhiy.storchakalinkissue26764 dependencies
2016-04-15 12:29:15serhiy.storchakasettype: enhancement -> behavior
2016-04-15 12:28:58serhiy.storchakasetpriority: normal -> high

title: Redundant check in bytearray_mod -> The result type of bytearray formatting is not stable
messages: + msg263486
stage: commit review -> (no value)
2016-04-15 12:12:20berker.peksagsetmessages: + msg263485
2016-04-15 11:35:17serhiy.storchakasetmessages: + msg263477
2016-04-15 11:19:01berker.peksagsetmessages: + msg263473
2016-04-15 10:36:14serhiy.storchakasetmessages: + msg263469
2016-04-15 10:16:07serhiy.storchakasetnosy: + vstinner, ethan.furman

messages: + msg263465
stage: patch review -> commit review
2016-04-15 09:43:21berker.peksagcreate