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

BytesIO methods don't accept integer types, while StringIO counterparts do #73927

Closed
orenmn mannequin opened this issue Mar 6, 2017 · 19 comments
Closed

BytesIO methods don't accept integer types, while StringIO counterparts do #73927

orenmn mannequin opened this issue Mar 6, 2017 · 19 comments
Labels
3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented Mar 6, 2017

BPO 29741
Nosy @amauryfa, @pitrou, @benjaminp, @florentx, @vadmium, @serhiy-storchaka, @zooba, @orenmn
PRs
  • bpo-29741: make some BytesIO methods accept integer types #560
  • bpo-29741: make some BytesIO methods accept integer types (only changes in Modules/_io) #606
  • Files
  • patchDraft1.diff: patch first draft
  • 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-08-26.14:52:50.066>
    created_at = <Date 2017-03-06.21:27:25.320>
    labels = ['type-bug', '3.7', 'expert-IO']
    title = "BytesIO methods don't accept integer types, while StringIO counterparts do"
    updated_at = <Date 2017-08-26.14:52:50.066>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-08-26.14:52:50.066>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-08-26.14:52:50.066>
    closer = 'steve.dower'
    components = ['IO']
    creation = <Date 2017-03-06.21:27:25.320>
    creator = 'Oren Milman'
    dependencies = []
    files = ['46709']
    hgrepos = []
    issue_num = 29741
    keywords = ['patch']
    message_count = 19.0
    messages = ['289136', '289144', '289148', '289151', '289154', '289158', '289160', '289225', '289226', '289227', '289228', '289229', '289230', '289359', '289414', '289416', '289418', '290235', '300796']
    nosy_count = 9.0
    nosy_names = ['amaury.forgeotdarc', 'pitrou', 'benjamin.peterson', 'stutzbach', 'flox', 'martin.panter', 'serhiy.storchaka', 'steve.dower', 'Oren Milman']
    pr_nums = ['560', '606']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29741'
    versions = ['Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 6, 2017

    ------------ current state ------------

    import io
    
    class IntLike():
        def __init__(self, num):
            self._num = num
    
        def __index__(self):
            return self._num
    
        __int__ = __index__

    io.StringIO('blah blah').read(IntLike(2))
    io.StringIO('blah blah').readline(IntLike(2))
    io.StringIO('blah blah').truncate(IntLike(2))

    io.BytesIO(b'blah blah').read(IntLike(2))
    io.BytesIO(b'blah blah').readline(IntLike(2))
    io.BytesIO(b'blah blah').truncate(IntLike(2))

    The three StringIO methods are called without any error, but each of the three
    BytesIO methods raises a "TypeError: integer argument expected, got 'IntLike'".

    This is because the functions which implement the StringIO methods (in
    Modules/_io/stringio.c):
    - _io_StringIO_read_impl
    - _io_StringIO_readline_impl
    - _io_StringIO_truncate_impl
    use PyNumber_AsSsize_t, which might call nb_index.

    However, the functions which implement the BytesIO methods (in
    Modules/_io/bytesio.c):
    - _io_BytesIO_read_impl
    - _io_BytesIO_readline_impl
    - _io_BytesIO_truncate_impl
    use PyLong_AsSsize_t, which accepts only Python ints (or objects whose type is
    a subclass of int).

    ------------ proposed changes ------------

    • change those BytesIO methods so that they would accept integer types (i.e.
      classes that define __index__), mainly by replacing PyLong_AsSsize_t with
      PyNumber_AsSsize_t
    • add tests to Lib/test/test_memoryio.py to verify that all six aforementioned
      methods accept integer types

    @orenmn orenmn mannequin added 3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error labels Mar 6, 2017
    @vadmium
    Copy link
    Member

    vadmium commented Mar 6, 2017

    What is the use case? Unless changing the behaviour would be useful, I think the simplest solution would be to document that the methods should only be given instances of “int”, so that it is clear that other kinds of numbers are unsupported.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 7, 2017

    I don't have a use case for that. (I noticed this behavior by
    chance, while working on some other issue.)

    However, IIUC, commit 4fa88fa,
    by Benjamin Peterson, Antoine Pitrou and Amaury Forgeot d'Arc,
    includes patching the aforementioned StringIO functions, so that
    they would accept integer types.
    So I add them to the nosy list, as I guess that the use cases for
    accepting integer types in StringIO methods are also use cases
    for accepting integer types in BytesIO.

    And now that you mention the docs, according to them, both StringIO
    and BytesIO inherit these methods from BufferedIOBase or IOBase.
    Thus, the methods are already expected to behave the same, aren't
    they?

    @serhiy-storchaka
    Copy link
    Member

    Other objects in the io module use special-purposed converter _PyIO_ConvertSsize_t() which checks PyNumber_Check() and calls PyNumber_AsSsize_t().

    I think StringIO implementation can be changed to reuse _PyIO_ConvertSsize_t() for simplicity.

    After that BytesIO implementation can be changed to use the same converter just for uniformity.

    @serhiy-storchaka
    Copy link
    Member

    Are there tests for accepting non-integers in other classes? If no then don't bother about tests. I suppose all this came from the implementation of the 'n' format unit in PyArg_Parse* functions.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 7, 2017

    using _PyIO_ConvertSsize_t sounds great.

    with regard to tests for accepting integer types in other classes,
    there aren't a lot of them, so I guess it is not always tested.
    still, it is tested in (excluding tests of the actual feature of
    treating integer types as ints): test_bytes, test_cmath, test_int,
    test_math, test_range, test_re, test_slice, test_struct,
    test_unicode and test_weakref.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 7, 2017

    also with regard to adding tests, consider the following scenario:
    in the future, someone writes a patch for these functions, which
    makes them not accept integer types anymore.
    assuming the tests don't exist, the writer of the patch doesn't
    realize the patch broke something, and so the patch is committed.
    years later, when the patch is finally a part of the stable release,
    it breaks a lot of code out there.

    lastly, ISTM adding these tests would be quite simple anyway.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 8, 2017

    I wrote a patch, but I attach it here and not in a PR, as you haven't approved
    adding tests. (I would be happy to open a PR with or without the tests.)
    I ran the test module (on my Windows 10), and seems like the patch doesn't
    break anything.

    also, while running test_memoryio with my added tests, i noticed some places in
    Lib/_pyio.py which seemed like they should be changed.
    in particular, I changed 'var.__index__' to 'var = var.__index__()' in some
    places.
    I feel really uncomfortable about that change, as it undos a change committed
    by Florent Xicluna in b14930c.
    Florent, what was the reason for that change?

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 8, 2017

    should I open a PR (even though we didn't give Florent enough time to
    respond)?

    @serhiy-storchaka
    Copy link
    Member

    Open a PR. It will be not hard to make small changes after opening it.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 8, 2017

    sure.

    In general, should drafts (like this one) be uploaded here?
    or is it always better to open a PR?

    @serhiy-storchaka
    Copy link
    Member

    This for your preference.

    @serhiy-storchaka
    Copy link
    Member

    I would accept changes to Modules/_io/ because I consider them as code cleanup (with little side effect). But I'm not sure about changes to Python implementation and tests. Could you create more narrow PR for Modules/_io/ changes and left other changes for the consideration of the io module maintainers?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 10, 2017

    done

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 10, 2017

    thanks :)
    I would update the original PR soon.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Mar 10, 2017

    or maybe git is smart enough to realize what happened, and I don't have
    to update PR 560?

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7400254 by Serhiy Storchaka (orenmn) in branch 'master':
    bpo-29741: Clean up C implementations of BytesIO and StringIO. (#606)
    7400254

    @zooba
    Copy link
    Member

    zooba commented Aug 24, 2017

    New changeset de50360 by Steve Dower (Oren Milman) in branch 'master':
    bpo-29741: Update some methods in the _pyio module to also accept integer types. Patch by Oren Milman. (#560)
    de50360

    @zooba zooba closed this as completed Aug 26, 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
    3.7 (EOL) end of life topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants