Skip to content
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

Regression in bytes constructor #73345

Closed
abalkin opened this issue Jan 4, 2017 · 13 comments
Closed

Regression in bytes constructor #73345

abalkin opened this issue Jan 4, 2017 · 13 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@abalkin
Copy link
Member

abalkin commented Jan 4, 2017

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
    Copy link
    Member Author

    abalkin commented Jan 4, 2017

    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 abalkin added the type-bug An unexpected behavior, bug, or error label Jan 4, 2017
    @abalkin
    Copy link
    Member Author

    abalkin commented Jan 4, 2017

    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
    Copy link
    Member

    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
    Copy link
    Member

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

    @abalkin
    Copy link
    Member Author

    abalkin commented Jan 4, 2017

    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
    Copy link
    Member Author

    abalkin commented Jan 4, 2017

    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
    Copy link
    Member Author

    abalkin commented Jan 5, 2017

    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
    Copy link
    Member

    methane commented Jan 5, 2017

    @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
    Copy link
    Member Author

    abalkin commented Jan 5, 2017

    @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
    Copy link
    Member Author

    abalkin commented Jan 5, 2017

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

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jan 6, 2017

    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
    Copy link
    Member

    methane commented Jan 6, 2017

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

    fixed.

    @methane methane closed this as completed Jan 6, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants