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

Allow struct.pack to handle objects with an __index__ method. #52547

Closed
mdickinson opened this issue Apr 3, 2010 · 14 comments
Closed

Allow struct.pack to handle objects with an __index__ method. #52547

mdickinson opened this issue Apr 3, 2010 · 14 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@mdickinson
Copy link
Member

BPO 8300
Nosy @mdickinson, @meadori, @anacrolix
Files
  • struct_index_trunk.patch
  • struct_index_trunk2.patch
  • struct_index_trunk3.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 = 'https://github.com/mdickinson'
    closed_at = <Date 2010-04-05.07:47:44.833>
    created_at = <Date 2010-04-03.11:51:46.746>
    labels = ['extension-modules', 'type-feature']
    title = 'Allow struct.pack to handle objects with an __index__ method.'
    updated_at = <Date 2011-02-20.04:14:38.102>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2011-02-20.04:14:38.102>
    actor = 'anacrolix'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2010-04-05.07:47:44.833>
    closer = 'mark.dickinson'
    components = ['Extension Modules']
    creation = <Date 2010-04-03.11:51:46.746>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['16746', '16748', '16751']
    hgrepos = []
    issue_num = 8300
    keywords = ['patch']
    message_count = 14.0
    messages = ['102245', '102257', '102258', '102262', '102263', '102311', '102325', '102326', '102327', '102361', '102364', '128849', '128877', '128891']
    nosy_count = 3.0
    nosy_names = ['mark.dickinson', 'meador.inge', 'anacrolix']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8300'
    versions = ['Python 2.7', 'Python 3.2']

    @mdickinson
    Copy link
    Member Author

    In Python 2.7, struct.pack with an integer format can handle non-integers that provide an __int__ method (although this *does* raise a DeprecationWarning).

    Python 2.7a4+ (trunk:79659:79661, Apr  3 2010, 11:28:19) 
    [GCC 4.2.1 (Apple Inc. build 5646) (dot 1)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from struct import pack
    [35194 refs]
    >>> pack('L', 3.1415)
    '\x03\x00\x00\x00\x00\x00\x00\x00'
    [35210 refs]

    This behaviour isn't particularly desirable for floats or Decimal instances, but it's useful for integer-like objects.

    In Python 3.x, there's no provision for handling integer-like objects than aren't actually integers.

    I propose that in 3.x, struct.pack should try to convert any non-integer to an integer by using its __index__ method, before packing.

    @mdickinson mdickinson self-assigned this Apr 3, 2010
    @mdickinson mdickinson added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Apr 3, 2010
    @mdickinson
    Copy link
    Member Author

    Here's a patch for trunk. It combines the docs and tests from Meador Inge's patch in bpo-1530559 with a C-level change to get_pylong in Modules/struct.c.

    @mdickinson
    Copy link
    Member Author

    Adding Meador Inge to nosy.

    @mdickinson
    Copy link
    Member Author

    That patch was a bit hasty in many respects; here's a better one.

    For 2.7, the scheme is as follows: when packing a non-integer with an integer format:

    (1) First __index__ is tried
    (2) If the __index__ method doesn't exist, or the call to __index__ raises TypeError, then the __int__ method is tried.
    (3) If the __index__ method raises something other than TypeError, or returns a non-integer, then struct.pack fails.

    @mdickinson
    Copy link
    Member Author

    Committed this patch to trunk in r79674. Will forward port to py3k.

    @meadori
    Copy link
    Member

    meadori commented Apr 4, 2010

    I may be missing something subtle, but how can 'PyNumber_Index(v) != NULL' *and* '!PyInt_Check(v) && !PyLong_Check(v)' both be satisfied in the 'get_pylong' mods? It seems to me that 'PyNumber_Index' only returns non-NULL when the object being returned is an 'int' or 'long'.

    Attached a patch with the extra check removed and a few more test cases.

    @mdickinson
    Copy link
    Member Author

    Probably both those conditions can't be satisfied; I'm wasn't sure what happened if something's __index__ method returned something other than an int or long.

    But now I bother to look at the source (in Objects/abstract.c) I see that there *is* already an explicit check for the result of nb_index being int or long (with TypeError being raised if the result isn't one of those). Mea culpa. I'll remove those lines (though I may leave an assert, just to be on the safe side).

    The 2.x behaviour isn't ideal: I'd prefer to just stop if the __index__ method is present and raises TypeError, rather than going on to check __int__ in that case. But that presents problems with old-style classes, where PyIndex_Check is true even when no __index__ method is explicitly defined.

    Thanks for the extra tests!

    @mdickinson
    Copy link
    Member Author

    Committed (with some tabs in test_struct.py changed to spaces) to trunk in r79745.

    @mdickinson
    Copy link
    Member Author

    Merged to py3k in r79746.

    Meador, does this all look okay, now?

    @meadori
    Copy link
    Member

    meadori commented Apr 5, 2010

    Looks good to me.

    @mdickinson
    Copy link
    Member Author

    Thanks.

    @anacrolix
    Copy link
    Mannequin

    anacrolix mannequin commented Feb 19, 2011

    Why isn't this implemented to work with __int__ as well?

    @mdickinson
    Copy link
    Member Author

    Because (arguably) we don't want to be able to pack non-integral floats (or Decimal instances, or ...) using integer formats:

    >>> import struct
    [56090 refs]
    >>> struct.pack('L', 2.3)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    struct.error: required argument is not an integer
    [56125 refs]

    @anacrolix
    Copy link
    Mannequin

    anacrolix mannequin commented Feb 20, 2011

    Thanks Mark for clearing that up. I found this link to be useful in explaining the purpose of __index__: http://docs.python.org/release/2.5.1/whatsnew/pep-357.html

    I think the choice of allowing only __index__ was the right choice.

    @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
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants