Issue 1530559: struct.pack raises TypeError where it used to convert (original) (raw)
Created on 2006-07-28 18:07 by piman, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (32)
Author: Joe Wreschnig (piman)
Date: 2006-07-28 18:07
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 "", line 1, in
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.
Author: Georg Brandl (georg.brandl) * 
Date: 2006-07-28 18:51
Logged In: YES user_id=849994
Bob?
Author: Bob Ippolito (bob.ippolito) *
Date: 2006-07-28 19:31
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?
Author: Georg Brandl (georg.brandl) *
Date: 2006-07-28 19:44
Logged In: YES user_id=849994
I think that's a question for python-dev.
Author: Neal Norwitz (nnorwitz) *
Date: 2006-07-29 03:21
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.
Author: Bob Ippolito (bob.ippolito) *
Date: 2006-08-03 02:10
Logged In: YES user_id=139309
I've attached a patch which should resolve this issue.
Author: Neal Norwitz (nnorwitz) *
Date: 2006-08-11 04:27
Logged In: YES user_id=33168
This patch (or some variant) was checked in as 51119
Author: Collin Winter (collinwinter) *
Date: 2007-03-29 22:48
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.
Author: Sean Reifschneider (jafo) *
Date: 2007-08-28 08:37
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.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-06-11 01:22
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.
Author: Mark Dickinson (mark.dickinson) *
Date: 2009-01-15 17:23
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'
Author: Mark Dickinson (mark.dickinson) *
Date: 2009-01-15 17:39
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 "", line 1, in TypeError: unsupported operand type(s) for &: 'Decimal' and 'long'
Author: Mark Dickinson (mark.dickinson) *
Date: 2009-04-19 16:17
Stealing this from Bob. Bob, please steal it back it you want it!
Author: Mark Dickinson (mark.dickinson) *
Date: 2009-04-19 20:50
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"?
Author: Bob Ippolito (bob.ippolito) *
Date: 2009-04-19 22:16
I believe that struct.error is just how it worked before 2.5
Author: Mark Dickinson (mark.dickinson) *
Date: 2009-07-05 10:09
This is now fixed in the trunk in r73858. The failing test has been reenabled.
Author: Andreas Kloeckner (inducer)
Date: 2010-03-01 19:08
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?
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-01 19:37
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.
Author: Andreas Kloeckner (inducer)
Date: 2010-03-01 19:55
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?
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-01 20:09
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.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-01 20:13
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.
Author: Andreas Kloeckner (inducer)
Date: 2010-03-01 20:31
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.
Author: Meador Inge (meador.inge) *
Date: 2010-03-07 04:05
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 issue 3132. As that work will be simplified by having the veneer as well.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-07 12:22
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.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-07 14:14
Updated patch, with slightly saner warnings checks.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-07 16:32
Restored use of int for all integer conversion codes in r78762.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-07 16:51
Recent checkins messed up Meador Inge's index patch; here's a regenerated version.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-03-07 17:19
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 issue 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.
Author: Meador Inge (meador.inge) *
Date: 2010-03-08 14:11
(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!
Author: Florent Xicluna (flox) *
Date: 2010-03-08 18:18
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.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-04-03 11:47
Closing this; a separate feature request should be opened for the idea of adding index awareness to struct.pack in py3k.
Author: Mark Dickinson (mark.dickinson) *
Date: 2010-04-03 11:52
I've opened issue 8300 for adding the index handling.
History
Date
User
Action
Args
2022-04-11 14:56:19
admin
set
github: 43741
2010-04-03 11:52:52
mark.dickinson
set
superseder: Allow struct.pack to handle objects with an __index__ method.
messages: +
2010-04-03 11:47:30
mark.dickinson
set
status: open -> closed
resolution: fixed
messages: +
2010-03-08 18🔞33
flox
set
nosy: + flox
messages: +
2010-03-08 14:11:32
meador.inge
set
messages: +
2010-03-07 17:19:58
mark.dickinson
set
messages: +
2010-03-07 16:51:56
mark.dickinson
set
files: + issue-1530559__index__.patch
messages: +
2010-03-07 16:32:24
mark.dickinson
set
messages: +
2010-03-07 14:14:46
mark.dickinson
set
files: + issue1530559__int__2.patch
messages: +
2010-03-07 12:22:54
mark.dickinson
set
files: + issue1530559__int__.patch
messages: +
2010-03-07 04:05:39
meador.inge
set
files: + issue-1530559.patch
nosy: + meador.inge
messages: +
2010-03-01 20:31:51
inducer
set
messages: +
2010-03-01 20:13:41
mark.dickinson
set
priority: normal -> high
resolution: fixed -> (no value)
messages: +
2010-03-01 20:09:15
mark.dickinson
set
messages: +
stage: resolved -> needs patch
2010-03-01 19:55:00
inducer
set
messages: +
2010-03-01 19:37:15
mark.dickinson
set
status: closed -> open
messages: +
2010-03-01 19:08:00
inducer
set
nosy: + inducer
messages: +
2009-07-05 10:09:43
mark.dickinson
set
status: open -> closed
resolution: fixed
messages: +
stage: resolved
2009-05-15 02:34:11
ajaksu2
set
keywords: + patch
dependencies: + struct.pack("I", "foo"); struct.pack("L", "foo") should fail
type: behavior
2009-04-19 22:16:06
bob.ippolito
set
messages: +
2009-04-19 20:50:18
mark.dickinson
set
priority: critical -> normal
messages: +
2009-04-19 16:17:11
mark.dickinson
set
assignee: bob.ippolito -> mark.dickinson
2009-04-19 16:17:02
mark.dickinson
set
messages: +
2009-01-15 17:39:14
mark.dickinson
set
messages: +
2009-01-15 17:23:09
mark.dickinson
set
nosy: + mark.dickinson
messages: +
versions: + Python 2.6, Python 3.0, Python 3.1, Python 2.7, - Python 2.5
2008-06-11 02:47:26
giampaolo.rodola
set
nosy: + giampaolo.rodola
2008-06-11 01:22:45
benjamin.peterson
set
priority: normal -> critical
nosy: + benjamin.peterson
messages: +
2007-09-17 09:57:35
jafo
set
priority: critical -> normal
2007-08-28 08:37:05
jafo
set
nosy: + jafo
messages: +
2006-07-28 18:07:04
piman
create