Regression in bytes constructor

[abalkin]

BPO	29159
Nosy	@ncoghlan, @abalkin, @methane, @serhiy-storchaka
Files	29159-index-fallback.patch

^{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 = None
closed_at = <Date 2017-01-06.08:46:25.269>
created_at = <Date 2017-01-04.19:49:54.293>
labels = ['type-bug']
title = 'Regression in bytes constructor'
updated_at = <Date 2018-10-14.16:56:09.802>
user = 'https://github.com/abalkin'

bugs.python.org fields:

activity = <Date 2018-10-14.16:56:09.802>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2017-01-06.08:46:25.269>
closer = 'methane'
components = []
creation = <Date 2017-01-04.19:49:54.293>
creator = 'belopolsky'
dependencies = []
files = ['46147']
hgrepos = []
issue_num = 29159
keywords = ['patch', '3.6regression']
message_count = 13.0
messages = ['284663', '284664', '284665', '284666', '284671', '284672', '284689', '284699', '284767', '284768', '284774', '284801', '284802']
nosy_count = 5.0
nosy_names = ['ncoghlan', 'belopolsky', 'methane', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29159'
versions = ['Python 3.6']

[abalkin]

Consider the following code:

$ cat bug.py
from array import array
class C(array):
    def __new__(cls):
        return array.__new__(cls, 'B', b'abc')
    def __index__(self):
        raise TypeError
x = C()
print(bytes(x))

It works under python 3.5:

$ python3.5 bug.py
b'abc'

but raises a TypeError under python 3.6:

$ python3.6 bug.py
Traceback (most recent call last):
  File "bug.py", line 8, in <module>
    print(bytes(x))
  File "bug.py", line 6, in __index__
    raise TypeError
TypeError

It looks like this was introduced in issue bpo-27704.

(Ref: e/pyq#827)

[abalkin]

My test code may seem contrived, but numpy arrays exhibit similar behavior:

>>> a = numpy.array([2, 2])
>>> bytes(a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: only integer arrays with one element can be converted to an index

Under python 3.5, the result was

>>> bytes(a)
b'\x02\x00\x00\x00\x02\x00\x00\x00'

[serhiy-storchaka]

Indeed, there is behavior change. It is easy to revert the old behavior. But
was the old behavior designed? It looks as a side effect of the implementation.

What is a reason of making an array an integer type?

[serhiy-storchaka]

Ah, if numpy arrays are affected, this should be restored. But swallowing an arbitrary exception doesn't look good to me.

[abalkin]

I don't think we should put too much effort into preserving numpy behavior. Consider this python 3.5 session:

>>> import numpy
>>> a = numpy.array([1])
>>> bytes(a)
__main__:1: VisibleDeprecationWarning: converting an array with ndim > 0 to an index will result in an error in the future
b'\x00'
>>> a = numpy.array([2, 2])
>>> bytes(a)
b'\x02\x00\x00\x00\x02\x00\x00\x00'

It looks like this behavior is just an artifact of ndarray providing both __index__ and buffer protocol and not something thought out by numpy developers.

I wonder if we could check for buffer protocol support before detecting an integer argument? I also recall a discussion of deprecating bytes(int) altogether. See <https://mail.python.org/pipermail/python-ideas/2014-March/027295.html\>.

[abalkin]

Also relevant:

• bpo-20895 - Add bytes.empty_buffer and deprecate bytes(17) for the same purpose
• PEP-467 - Minor API improvements for binary sequences

[abalkin]

Inada,

Can you explain what your patch does? I don't understand why you single out the OverflowError. When is __index__ expected to raise it?

>>> (2**300).__index__()
2037035976334486086268445688409378161051468393665936250636140449354381299763336706183397376

[methane]

@belopolsky
It is backported behavior from Python 3.5, to fix regression.

See code review discussion in bpo-27704.
The first patch kept the behavior, but we simplified it in second patch.
That cause this regression.

[abalkin]

@inada.naoki

Sorry, I still don't understand the role of OverflowError.

With respect to my earlier suggestion that buffer protocol should have priority over __index__, note that the documentation implies the same order:

>> help(bytes)

class bytes(object)
 |  bytes(iterable_of_ints) -> bytes
 |  bytes(string, encoding[, errors]) -> bytes
 |  bytes(bytes_or_buffer) -> immutable copy of bytes_or_buffer
 |  bytes(int) -> bytes object of size given by the parameter initialized with null bytes
 |  bytes() -> empty bytes object
 |
 |  Construct an immutable array of bytes from:
 |    - an iterable yielding integers in range(256)
 |    - a text string encoded using the specified encoding
 |    - any object implementing the buffer API.
 |    - an integer
..

[abalkin]

On the other hand, the documentation for the bytearray constructor 1 lists integer argument first.

[serhiy-storchaka]

The code restored by 29159-index-fallback.patch is slightly different from the code before bpo-27704, but the patch LGTM except that arguments of assertEqual are in reversed order.

Can you explain what your patch does? I don't understand why you single out the OverflowError. When is __index__ expected to raise it?

PyNumber_AsSsize_t(arg, PyExc_OverflowError) raises an OverflowError when the result of __index__ don't fit in Py_ssize_t.

I wonder if we could check for buffer protocol support before detecting an integer argument?

This would fix an issue with NumPy arrays, but it is much harder to implement.

[python-dev]

New changeset 505cc50ddc82 by INADA Naoki in branch '3.6':
Issue bpo-29159: Fix regression in bytes(x) when x.__index__() raises Exception.
https://hg.python.org/cpython/rev/505cc50ddc82

[methane]

patch LGTM except that arguments of assertEqual are in reversed order.

fixed.