[Python-bugs-list] [ python-Bugs-624807 ] sq_concat prevents __radd__ from working

noreply@sourceforge.net noreply@sourceforge.net
Mon, 30 Dec 2002 08:19:19 -0800


Bugs item #624807, was opened at 2002-10-17 14:18
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=624807&group_id=5470

Category: Python Interpreter Core
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Neil Schemenauer (nascheme)
>Assigned to: Neil Schemenauer (nascheme)
Summary: sq_concat prevents __radd__ from working

Initial Comment:
See attached code 'bug.py'.  What's happening is if the
LHS of an inplace
add has a sq_concat or sq_inplace_concat method but it
cannot handle
the RHS argument then the operation fails rather than
trying __radd__.
I think this is related to my str.__mod__ bug.

Guido, I would appreciate any comments you have on
fiixing this problem.
Fixing this specific problem should be pretty straight
forward but I suspect
more tp_as_sequence vs. tp_as_number confusion exists.
 What's the
the longterm strategy WRT sq_concat and sq_repeat?


----------------------------------------------------------------------

>Comment By: Guido van Rossum (gvanrossum)
Date: 2002-12-30 11:19

Message:
Logged In: YES 
user_id=6380

This looks good. Neil, do you have time to commit the
changes today? If not, reassign to me and I'll do it.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-12-30 10:50

Message:
Logged In: YES 
user_id=6380

I'll look at it right now.


----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-12-30 10:28

Message:
Logged In: YES 
user_id=35752

This should probably get in before the alpha release.  Guido
appears to be busy.  Feel free to reassign if you don't have
time.

----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-12-21 21:58

Message:
Logged In: YES 
user_id=35752

The attached patch fixes the orignal bug reported along with
a number of other related bugs. The patch does two things.
First, it rearranges the code in abstract.c so that
sq_repeat, sq_inplace_repeat, sq_concat and
sq_inplace_concat are always called after trying the nb_*
slots. The code before the patch was inconsistent about this
and that caused the reported bug as well as others.

The patch also consolidates the int and long sequence repeat
code. Before the change, integers checked for overflow but
longs did not. The consolidated code removes duplication and
gets rid of some ugly code in intobject.c and longobject.c.

The bug related to type.__mul__ has not been fixed. I'll
open a new bug for it rather than reusing this one.

Guido, I think this solves all the issues I dicussed with
you a while back on the phone. It's amazing what you can get
done on a long, cross country flight. :-)

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-11-18 15:00

Message:
Logged In: YES 
user_id=6380

Sounds about right -- time to clise this issue?

Note that the sq_repeat slot takes a C int, which somewhat
limits what you can do with it, but that doesn't feel like a
big problem.

Can you figure out why 'a'.__mul__(3.5) does what it does?
That seems like a bug to me.

----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-11-18 14:54

Message:
Logged In: YES 
user_id=35752

Well, perhaps we cannot (eventually) get rid of sq_repeat. 
I actually
quite like the way things work now.  Just to refresh your
memory in
case it's fuzzy, the __mul__ method of integer like objects
check for
sq_repeat on the other object.  If it exists then it calls
it with itself
as the "count" argument.  Sequences don't implement nb_mul or
return NotImplemented.

So, the "protocol" has two parts.  Sequence types expose a
sq_repeat slot
and integer types have a nb_mul that checks for sq_repeat.
It's unclear to me what a __mul__ method on a sequence could do.
What types of integer arguments should it accept?  Allowing
only ints
and longs seems wrong to me.  It should be possible for a
extension
type to work as well.

I just noticed that the type __mul__ is wierd:

>> 'a'.__mul__(3.4)
'aaa'
>>> [1].__mul__(3.4)
[1, 1, 1]
  
To make it cosistent with the current modus operandi it
should call
__mul__ on the argument:

>>> (3).__mul__('a')
'aaa'
>>> (3.3).__mul__('a')
NotImplemented


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-11-18 13:04

Message:
Logged In: YES 
user_id=6380

Haven't looked at the patch, but I find that unacceptable.

"%d" is sort of acceptable (although I wish it would round);
there are a few other places where floats are accidentally
accepted, but all of those are mistakes and we shouldn't add
any more of those.

Now what?


----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-11-18 11:32

Message:
Logged In: YES 
user_id=35752

The attached patch adds nb_add and nb_mul slots to str and
unicode.  All tests pass after this change.  One resulting
behavior
change is that:

  "ab" * 4.5

works like

  "ab" * 4

That's a little unsettling to me but I guess it's
consistient with:

  "%d" % 4.5

Please review.

----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-11-18 11:29

Message:
Logged In: YES 
user_id=35752

The attached patch adds nb_add and nb_mul slots to str and
unicode.  All tests pass after this change.  One resulting
behavior
change is that:

  "ab" * 4.5

works like

  "ab" * 4

That's a little unsettling to me but I guess it's
consistient with:

  "%d" % 4.5

Please review.

----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-10-25 17:27

Message:
Logged In: YES 
user_id=35752

I got a bit worried when test_select failed after adding
tp_as_number  to the str object.  It was triggered by
select.select([], [], [], 'not a string').  The source of
the problem was (hope this comes out okay):

	else if (!PyNumber_Check(tout)) {
		PyErr_SetString(PyExc_TypeError,
				"timeout must be a float or None");
		return NULL;
	}
	else {
		tout = PyNumber_Float(tout);
		if (!tout)
                    return NULL;

The test was expecting TypeError but getting ValueError.  I
guess PyNumber_Check is pretty useless.  Luckily it is only
used in the select and operator modules.


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-10-17 14:53

Message:
Logged In: YES 
user_id=6380

Boy what a mess indeed!

Most embarrassingly, this code worked in Python 2.0! :-(

I'm not sure how to fix this (and have no time to really
look into it), but I have a long-term comment.

Long-term, at the C API level, the separate notions of
"sequence repeat" and "sequence concat" should disappear,
and instead "PyNumber_Multiply" and "PyNumber_Add" should be
used. (PySequence_Repeat can stay as a shorthand that takes
a C int rather than an object).

In general, whenever two operations look the same at the
python level, there should only be one C API to invoke that,
not two. Other examples are PySequence_GetItem vs.
PyObject_GetItem.

As a transition, the built-in types should probably start
supporting the more generic ops (so strings would support
nb_add and nb_mul), and the generic API functions should
look for the numeric ops before trying the sequence or
mapping ops. (And also try the mapping ops before the --
more restrictive -- sequence ops.) Some of this is already
going on, but it would be a good policy to try and fix all
of this in 2.3. I expect it wouldn't break much -- you'd
have to have a type that interprets e.g. sq_repeat different
than nb_mul to get code breakage.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=624807&group_id=5470