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

struct.pack raises TypeError where it used to convert #43741

Closed
piman mannequin opened this issue Jul 28, 2006 · 32 comments
Closed

struct.pack raises TypeError where it used to convert #43741

piman mannequin opened this issue Jul 28, 2006 · 32 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@piman
Copy link
Mannequin

piman mannequin commented Jul 28, 2006

BPO 1530559
Nosy @birkenfeld, @etrepum, @mdickinson, @giampaolo, @benjaminp, @florentx, @meadori
Dependencies
  • bpo-1741130: struct.pack("I", "foo"); struct.pack("L", "foo") should fail
  • Superseder
  • bpo-8300: Allow struct.pack to handle objects with an index method.
  • Files
  • python2.5-sf1530559-r51066.diff
  • issue-1530559.patch
  • issue1530559__int__.patch
  • issue1530559__int__2.patch
  • issue-1530559__index__.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-03.11:47:30.737>
    created_at = <Date 2006-07-28.18:07:04.000>
    labels = ['type-bug', 'library']
    title = 'struct.pack raises TypeError where it used to convert'
    updated_at = <Date 2010-04-03.11:52:52.024>
    user = 'https://bugs.python.org/piman'

    bugs.python.org fields:

    activity = <Date 2010-04-03.11:52:52.024>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2010-04-03.11:47:30.737>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2006-07-28.18:07:04.000>
    creator = 'piman'
    dependencies = ['1741130']
    files = ['2085', '16476', '16482', '16484', '16485']
    hgrepos = []
    issue_num = 1530559
    keywords = ['patch']
    message_count = 32.0
    messages = ['29366', '29367', '29368', '29369', '29370', '29371', '29372', '29373', '55358', '67933', '79903', '79906', '86175', '86180', '86182', '90150', '100260', '100261', '100264', '100265', '100268', '100270', '100560', '100576', '100581', '100590', '100591', '100594', '100642', '100660', '102244', '102246']
    nosy_count = 12.0
    nosy_names = ['nnorwitz', 'georg.brandl', 'collinwinter', 'jafo', 'bob.ippolito', 'mark.dickinson', 'piman', 'inducer', 'giampaolo.rodola', 'benjamin.peterson', 'flox', 'meador.inge']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'needs patch'
    status = 'closed'
    superseder = '8300'
    type = 'behavior'
    url = 'https://bugs.python.org/issue1530559'
    versions = ['Python 2.6', 'Python 3.0', 'Python 3.1', 'Python 2.7']

    @piman
    Copy link
    Mannequin Author

    piman mannequin commented Jul 28, 2006

    piman@toybox:~$ python2.4 -c "import struct;
    struct.pack('>H', 1.0)"
    piman@toybox:~$ python2.5 -c "import struct;
    struct.pack('>H', 1.0)"
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/usr/lib/python2.5/struct.py", line 63, in pack
        return o.pack(*args)
    TypeError: unsupported operand type(s) for &: 'float'
    and 'long'

    This might have appeared as part of the struct
    optimizations; if struct isn't going to convert anymore
    for performance reasons, I think this should be
    mentioned in the release notes. Though personally I
    would prefer struct go back to converting its arguments.

    @piman piman mannequin assigned etrepum Jul 28, 2006
    @piman piman mannequin added the stdlib Python modules in the Lib dir label Jul 28, 2006
    @piman piman mannequin assigned etrepum Jul 28, 2006
    @piman piman mannequin added the stdlib Python modules in the Lib dir label Jul 28, 2006
    @birkenfeld
    Copy link
    Member

    birkenfeld commented Jul 28, 2006

    Logged In: YES
    user_id=849994

    Bob?

    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Jul 28, 2006

    Logged In: YES
    user_id=139309

    That wasn't really intentional, but the old behavior looks a bit suspect:

    $ python2.4 -c "import struct; print repr(struct.pack('>H', 1.6))"
    '\x00\x01'

    We could change it to check for floats and do a DeprecationWarning?

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Jul 28, 2006

    Logged In: YES
    user_id=849994

    I think that's a question for python-dev.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Jul 29, 2006

    Logged In: YES
    user_id=33168

    I'd like to see a deprecation warning so old code continues
    to work. struct is way to loose and needs to be tightened
    up, but that might not fully happen until py3k.

    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Aug 3, 2006

    Logged In: YES
    user_id=139309

    I've attached a patch which should resolve this issue.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Aug 11, 2006

    Logged In: YES
    user_id=33168

    This patch (or some variant) was checked in as 51119

    @collinwinter
    Copy link
    Mannequin

    collinwinter mannequin commented Mar 29, 2007

    Reopening. The test case added in r51119 (specifically, test_1530559() and its infrastructure) doesn't work. The check_float_coerce() function is overwritten by a later assignment, meaning that test_1530559() calls the wrong function. Actually making it call the right check_float_coerce() function results in a NameError ("global name 'func' is not defined").

    The reason the test currently passes is because check_float_coerce() is effectively an alias for deprecated_err(). test_1530559() passes deprecated_err() a string as its first argument; when deprecated_err() tries to call the string, a TypeError results. deprecated_err() traps the TypeError and so everything appears from the outside to have gone smoothly.

    If the test is tweaked so that struct.pack() is actually invoked with the proper arguments, it fails to raise the expected error on float coercion.

    @jafo
    Copy link
    Mannequin

    jafo mannequin commented Aug 28, 2007

    etrepum: Can you fix the test which caused this to be re-opened?

    I'm tempted to push this down in priority from urgent to high, since the
    code is (apparently) fixed, just the test is failing, but ideally this
    could just get fixed.

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Jun 11, 2008

    I ran into this issue converting test_struct to unittest. I would fix
    it, but I'm not sure exactly what is intended. I'm raising priority.

    @mdickinson
    Copy link
    Member

    mdickinson commented Jan 15, 2009

    Retargeting, now that 2.5 is in security-fix-only mode.

    Bob, can you clarify what *should* be happening in 2.6, 2.7, 3.0 and 3.1
    for things like:

    struct.pack('L', 2009.1)
    struct.pack('L', Decimal('3.14')) 

    ?

    It also seems that 'L' and 'Q behave differently. For example, with 2.7:

    >>> struct.pack('L', 3.1)
    sys:1: DeprecationWarning: integer argument expected, got float
    '\x03\x00\x00\x00'
    >>> struct.pack('Q', 3.1)
    '\x03\x00\x00\x00\x00\x00\x00\x00'

    @mdickinson
    Copy link
    Member

    mdickinson commented Jan 15, 2009

    Some more strange results:

    Python 2.6.1+ (release26-maint:68613, Jan 15 2009, 15:19:54) 
    [GCC 4.0.1 (Apple Inc. build 5490)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from struct import pack
    >>> from decimal import Decimal
    >>> pack('l', Decimal('3.14'))
    '\x03\x00\x00\x00'
    >>> pack('L', Decimal('3.14'))
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: unsupported operand type(s) for &: 'Decimal' and 'long'

    @mdickinson
    Copy link
    Member

    mdickinson commented Apr 19, 2009

    Stealing this from Bob. Bob, please steal it back it you want it!

    @mdickinson mdickinson assigned mdickinson and unassigned etrepum Apr 19, 2009
    @mdickinson
    Copy link
    Member

    mdickinson commented Apr 19, 2009

    The deprecated struct features (float coercion, overflow wrapping) have
    been removed for py3k in r70497, r70688, r71754. I don't plan to backport
    this to 2.7; I'll just try to fix the behaviour in a minimal way there.

    One thing that's not clear to me: what's the rationale for raising
    struct.error everywhere instead of more specific Python errors; e.g.,
    TypeError for struct.pack('L', 'not an integer') and OverflowError for
    struct.pack('L', 10**100)? Is there a particular use-case for "except
    struct.error"?

    @etrepum
    Copy link
    Mannequin

    etrepum mannequin commented Apr 19, 2009

    I believe that struct.error is just how it worked before 2.5

    @devdanzin devdanzin mannequin added type-bug An unexpected behavior, bug, or error labels May 15, 2009
    @mdickinson
    Copy link
    Member

    mdickinson commented Jul 5, 2009

    This is now fixed in the trunk in r73858. The failing test has been
    reenabled.

    @inducer
    Copy link
    Mannequin

    inducer mannequin commented Mar 1, 2010

    The fix breaks my package PyCUDA, which relies on handing struct something that can be cast to long. (i.e. not a float, but something representing a memory address on a GPU)

    Am I out of luck?

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 1, 2010

    I'd be open to re-allowing use of __int__ (and __long__) consistently for all integer packing codes in 2.7, as a temporary measure; I'd really prefer not to allow this for 3.x.

    What would make more sense, IMO, would be to allow use of the __index__ method (in both 2.7 and 3.x) to convert custom non-integer classes to integers before packing; this is supposed to be the modern approach to creating integer-like classes that can be used as integers (e.g., in list indices). Andreas, would this work for you, or do you need to be able to use __int__ and/or __long__?

    Re-opening while we're discussing this.

    @mdickinson mdickinson reopened this Mar 1, 2010
    @mdickinson mdickinson reopened this Mar 1, 2010
    @inducer
    Copy link
    Mannequin

    inducer mannequin commented Mar 1, 2010

    AFAICS, __index__ would be fine.

    To make sure I understand your proposed solution correctly: You'd go through the argument list beforehand and cast everything that's not a number type or str to int by means of __index__?

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 1, 2010

    More or less, yes: when trying to pack a non-integer x (i.e. something that's not an instance of int or long) with an integer format code (one of 'bBhHiIlLqQ', or 'P'), x.__index__() would be called to convert x to an integer, and that integer would be packed as usual (possibly raising OverflowError). Other format codes wouldn't be affected.

    I'm not quite sure what you mean by 'beforehand': the conversion would happen at the same time as it currently does.

    It might be a struggle for me to get to this before the 2.7 betas. If anyone's interested in submitting a patch, it would be welcome.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 1, 2010

    Also, it may be that some of r73858 needs to be reverted; going from accepting non-ints and longs in 2.6 to a TypeError in 2.7 is a bit much; there should have at least been a DeprecationWarning. I need to find some time to look at this properly, and work out what on earth I was thinking at the time.

    @inducer
    Copy link
    Mannequin

    inducer mannequin commented Mar 1, 2010

    Thanks for the clarification of 'beforehand'--I had understood from your description that this would be some sort of preprocessing step.

    I agree that the TypeError is a bit much, though I'm biased of course. If you'd like my input on things you come up with, please don't hesitate to ping me.

    @meadori
    Copy link
    Member

    meadori commented Mar 7, 2010

    If anyone's interested in submitting a patch, it would be welcome.

    Sure. I saw where this was partly addressed in r78690. The attached patch adds support for the '__index__' conversion that Mark
    suggested.

    At first glance, the patch may seem more than what is needed. However, most of the diffs are due to pulling parts of the C
    implementation into a Python veneer. This will make preprocessing work (like what was needs for this fix) easier now and in the future. In addition, it will lay a small amount of groundwork for the changes that are needed for resolving bpo-3132. As that work will be simplified by having the veneer as well.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 7, 2010

    Here's a patch to restore the old usage of __int__ to convert non-integer arguments; it also produces a DeprecationWarning whenever __int__ is used in this way. For consistency and simplicity, __int__ will be tried for *any* non-integer argument when packing with an integer format; this goes beyond the conversions that 2.6 allows. (In 2.6, the behaviour is somewhat random: it works only for 'bBhHil' in native mode and 'bhil' in non-native mode.)

    It doesn't seem worth deliberately trying __long__ as well, so I've left that out. So there's still some possibility for breakage relative to 2.6, when (1) packing using 'Q' or 'q', *and* (2) the object to be packed defines __long__ but not __int__, or defines both __long__ and __int__ in inconsistent ways. The likelihood of (2) seems small enough that this isn't worth worrying about in practice (and the workaround is easy, too).

    Andreas, are you in a position to test this patch?

    Supporting conversions to integer via __index__ is orthogonal to this; I'll take a look at Meador's patch shortly.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 7, 2010

    Updated patch, with slightly saner warnings checks.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 7, 2010

    Restored use of __int__ for all integer conversion codes in r78762.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 7, 2010

    Recent checkins messed up Meador Inge's __index__ patch; here's a regenerated version.

    @mdickinson
    Copy link
    Member

    mdickinson commented Mar 7, 2010

    Comments and thoughts on the __index__ patch:

    (1) Thank you for a remarkably complete patch!

    (2) For 2.x, I'm a bit uncomfortable with introducing the extra Python layer on top of the C layer. Partly I'm worried about accidentally breaking something (it's not 100% clear to me whether there might be hidden side-effects to such a change), but I also notice that this seems to have a significant impact on performance. In fact, I seem to recall that the previously existing Python component of the struct module was absorbed into Modules/_struct.c precisely for performance reasons.

    A quick, unscientific benchmark: the time taken to run test_struct with this patch (excluding the changes to test_struct itself) on my machine (OS X 10.6, 64-bit non-framework non-debug build of Python) is around 1.52--1.53 seconds; without the patch it's around 1.02--1.03 seconds.

    (3) For 3.x, and for the bpo-3132 work, I agree it might make sense to have a fatter Python layer; this would also help other Python implementations that are trying to keep up with CPython. I'm still a bit worried about performance, though.

    (4) For 2.x, perhaps we don't need the extra __index__ functionality anyway, now that the previous use of __int__ has been restored. That would give us a bit more time to think about this for 3.x.

    @meadori
    Copy link
    Member

    meadori commented Mar 8, 2010

    (2) For 2.x, I'm a bit uncomfortable with introducing the extra Python layer
    on top of the C layer. Partly I'm worried about accidentally breaking
    something (it's not 100% clear to me whether there might be hidden
    side-effects to such a change),

    I understand. I am worried about that as well. The main area that
    concerns me is the interface for the 'Struct' class. I did my best to
    match the methods and functions from the C implementation. I need to
    look into this in more detail, though. One quirk I currently know
    about is that the following methods:

    '__delattr__', '__getattribute__', '__setattr__', '__new__'

    show up in 'help' for the C implementation, but not the Python one.
    Although, the implementations do exist in both places.

    but I also notice that this seems to have a significant impact on performance.
    In fact, I seem to recall that the previously existing Python component of
    the struct module was absorbed into Modules/_struct.c precisely for
    performance reasons.

    A quick, unscientific benchmark: the time taken to run test_struct with this
    patch (excluding the changes to test_struct itself) on my machine (OS X 10.6,
    64-bit non-framework non-debug build of Python) is around 1.52--1.53 seconds; > without the patch it's around 1.02--1.03 seconds.

    Agreed that this is not a really scientific benchmark. I did
    experiment with this idea a little further though on my machine (OS X
    10.5 32-bit):

    ==============================================
    | Configuration | Seconds |
    ==============================================
    | Original | 1.26 |
    ----------------------------------------------
    | __index__ patch v1 | 1.88 |
    ----------------------------------------------
    | Hoisted imports out of functions | 1.53 |
    ----------------------------------------------
    | Hoisted imports and no __index__ | 1.34 |
    ----------------------------------------------

    So with this simple experiment pulling the 'imports' out of the function
    wrappers made quite a difference. And, of course, removing the
    '__index__' transform brought the times down even more. So, the
    wrapper overhead does not look to be terrible for this simple test.

    However, as you alluded to, we should find a better benchmark. Any
    ideas on a good one?

    (4) For 2.x, perhaps we don't need the extra __index__ functionality anyway,
    now that the previous use of __int__ has been restored. That would give us
    a bit more time to think about this for 3.x.

    Agreed. Thanks for taking the time to look at the patch anyway!

    @florentx
    Copy link
    Mannequin

    florentx mannequin commented Mar 8, 2010

    I would suggest to raise a py3k warning instead of a plain warning.
    AFAIU the implicit conversion is OK in 2.7, and it is removed in 3.x.

    @mdickinson
    Copy link
    Member

    mdickinson commented Apr 3, 2010

    Closing this; a separate feature request should be opened for the idea of adding __index__ awareness to struct.pack in py3k.

    @mdickinson
    Copy link
    Member

    mdickinson commented Apr 3, 2010

    I've opened bpo-8300 for adding the __index__ handling.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants