classification
Title: struct.pack raises TypeError where it used to convert
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.0, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: 1741130 Superseder: Allow struct.pack to handle objects with an __index__ method.
View: 8300
Assigned To: mark.dickinson Nosy List: benjamin.peterson, bob.ippolito, collinwinter, flox, georg.brandl, giampaolo.rodola, inducer, jafo, mark.dickinson, meador.inge, nnorwitz, piman
Priority: high Keywords: patch

Created on 2006-07-28 18:07 by piman, last changed 2010-04-03 11:52 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
python2.5-sf1530559-r51066.diff bob.ippolito, 2006-08-03 02:10
issue-1530559.patch meador.inge, 2010-03-07 04:05
issue1530559__int__.patch mark.dickinson, 2010-03-07 12:22
issue1530559__int__2.patch mark.dickinson, 2010-03-07 14:14
issue-1530559__index__.patch mark.dickinson, 2010-03-07 16:51
Messages (32)
msg29366 - (view) 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 "<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.
msg29367 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2006-07-28 18:51
Logged In: YES 
user_id=849994

Bob?
msg29368 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) 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?
msg29369 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2006-07-28 19:44
Logged In: YES 
user_id=849994

I think that's a question for python-dev.
msg29370 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) 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.
msg29371 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) Date: 2006-08-03 02:10
Logged In: YES 
user_id=139309

I've attached a patch which should resolve this issue.
msg29372 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-08-11 04:27
Logged In: YES 
user_id=33168

This patch (or some variant) was checked in as 51119
msg29373 - (view) Author: Collin Winter (collinwinter) * (Python committer) 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.
msg55358 - (view) Author: Sean Reifschneider (jafo) * (Python committer) 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.
msg67933 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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.
msg79903 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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'
msg79906 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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 "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for &: 'Decimal' and 'long'
msg86175 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-04-19 16:17
Stealing this from Bob.  Bob, please steal it back it you want it!
msg86180 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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"?
msg86182 - (view) Author: Bob Ippolito (bob.ippolito) * (Python committer) Date: 2009-04-19 22:16
I believe that struct.error is just how it worked before 2.5
msg90150 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-05 10:09
This is now fixed in the trunk in r73858.  The failing test has been 
reenabled.
msg100260 - (view) 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?
msg100261 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg100264 - (view) 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__?
msg100265 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg100268 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg100270 - (view) 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.
msg100560 - (view) Author: Meador Inge (meador.inge) * (Python committer) 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.
msg100576 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg100581 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-03-07 14:14
Updated patch, with slightly saner warnings checks.
msg100590 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-03-07 16:32
Restored use of __int__ for all integer conversion codes in r78762.
msg100591 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-03-07 16:51
Recent checkins messed up Meador Inge's __index__ patch;  here's a regenerated version.
msg100594 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg100642 - (view) Author: Meador Inge (meador.inge) * (Python committer) 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!
msg100660 - (view) Author: Florent Xicluna (flox) * (Python committer) 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.
msg102244 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg102246 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-04-03 11:52
I've opened issue 8300 for adding the __index__ handling.
History
Date User Action Args
2010-04-03 11:52:52mark.dickinsonsetsuperseder: Allow struct.pack to handle objects with an __index__ method.
messages: + msg102246
2010-04-03 11:47:30mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg102244
2010-03-08 18:18:33floxsetnosy: + flox
messages: + msg100660
2010-03-08 14:11:32meador.ingesetmessages: + msg100642
2010-03-07 17:19:58mark.dickinsonsetmessages: + msg100594
2010-03-07 16:51:56mark.dickinsonsetfiles: + issue-1530559__index__.patch

messages: + msg100591
2010-03-07 16:32:24mark.dickinsonsetmessages: + msg100590
2010-03-07 14:14:46mark.dickinsonsetfiles: + issue1530559__int__2.patch

messages: + msg100581
2010-03-07 12:22:54mark.dickinsonsetfiles: + issue1530559__int__.patch

messages: + msg100576
2010-03-07 04:05:39meador.ingesetfiles: + issue-1530559.patch
nosy: + meador.inge
messages: + msg100560

2010-03-01 20:31:51inducersetmessages: + msg100270
2010-03-01 20:13:41mark.dickinsonsetpriority: normal -> high
resolution: fixed -> (no value)
messages: + msg100268
2010-03-01 20:09:15mark.dickinsonsetmessages: + msg100265
stage: resolved -> needs patch
2010-03-01 19:55:00inducersetmessages: + msg100264
2010-03-01 19:37:15mark.dickinsonsetstatus: closed -> open

messages: + msg100261
2010-03-01 19:08:00inducersetnosy: + inducer
messages: + msg100260
2009-07-05 10:09:43mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg90150

stage: resolved
2009-05-15 02:34:11ajaksu2setkeywords: + patch
dependencies: + struct.pack("I", "foo"); struct.pack("L", "foo") should fail
type: behavior
2009-04-19 22:16:06bob.ippolitosetmessages: + msg86182
2009-04-19 20:50:18mark.dickinsonsetpriority: critical -> normal

messages: + msg86180
2009-04-19 16:17:11mark.dickinsonsetassignee: bob.ippolito -> mark.dickinson
2009-04-19 16:17:02mark.dickinsonsetmessages: + msg86175
2009-01-15 17:39:14mark.dickinsonsetmessages: + msg79906
2009-01-15 17:23:09mark.dickinsonsetnosy: + mark.dickinson
messages: + msg79903
versions: + Python 2.6, Python 3.0, Python 3.1, Python 2.7, - Python 2.5
2008-06-11 02:47:26giampaolo.rodolasetnosy: + giampaolo.rodola
2008-06-11 01:22:45benjamin.petersonsetpriority: normal -> critical
nosy: + benjamin.peterson
messages: + msg67933
2007-09-17 09:57:35jafosetpriority: critical -> normal
2007-08-28 08:37:05jafosetnosy: + jafo
messages: + msg55358
2006-07-28 18:07:04pimancreate