| msg49221 - (view) |
Author: Armin Rigo (arigo) *  |
Date: 2005-12-26 16:09 |
| This is a fix for bug #1355842, with a comprehensive test checking that implementing a binary special method but returning NotImplemented does indeed cause a TypeError to be raised. In addition to the case reported in #1355842 (x+=y) the test found another case where NotImplemented was just returned (A()*5), a case where an OverflowError or ValueError was raised instead of TypeError (A()*large_integer), and a case where an unexpected AttributeError was raised (5*A()). The proposed patch also reverts the changes done in svn revisions 25556-25557 corresponding to bug #516727, because it makes that fix unnecessary by adopting a more radical approach. All the problems are due to hard-to-control interactions between the various PyTypeObject slots for addition and multiplication (nb_add vs sq_concat, nb_multiply vs sq_repeat). The approach of the present patch is to not define sq_concat, sq_repeat, sq_inplace_concat and sq_inplace_repeat slots *at all* for user-defined types, even if they have __add__ and __mul__ methods. This is the only sane solution I found, specially with respect to the OverflowError/ValueError problem (caused by trying to convert the integer to a C int in order to try to call sq_repeat). It is also consistent with the behavior of instances of old-style classes -- the InstanceType did not define sq_concat and sq_repeat, either. I'll propose a redefinition of operator.concat() and operator.repeat() on python-dev. |
|
|
| msg49222 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2005-12-27 16:16 |
| Logged In: YES user_id=4771 Attached a second patch, extending PySequence_Repeat() and PySequence_Concat() to match more precisely their documentation, which says that they should be equivalent to multiplication and addition respectively, at least when the proper arguments are sequences. In 2.4 and HEAD, it works with new-style instances defining __add__ or __mul__, but it does not work with old-style instances. If the concat_repeat_cleanup.diff patch is checked in, then it stops working for new-style instances too. So the sequence_operations.diff patch cleans up the situation by making this work in all cases. Compatibility note: PySequence_Concat/Repeat() are mostly never used in the core; they are essentially only used by operator.concat() and operator.repeat(). Both patches together should thus not break existing code and could be considered as a bug fix. I'd recommend that we check them in both HEAD and the 2.4 branch. The patch also addresses PySequence_InPlaceConcat/Repeat() but without testing them -- they can't be tested from Python, so they are already completely untested. |
|
|
| msg49223 - (view) |
Author: Neil Schemenauer (nascheme) * |
Date: 2005-12-27 21:35 |
| Logged In: YES user_id=35752 Your change to abstract.c that adds "result = binop_type_error(...)" is certainly correct. Py_NotImplemented should not be returned. Small nits: why not remove the SLOT1 lines from typeobject.c instead of commenting them out? If you want to leave them as comments then you should add an explaination as to why they should not be there. Also, I don't personally like having SF issue numbers in comments as I would rather have the comment be self explanatory. Who knows, SF might go away. The changes to PySequence_* look correct to me as well. |
|
|
| msg49224 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2005-12-28 11:09 |
| Logged In: YES user_id=4771 Thanks for the comments about the comments, I will fix them. I don't understand the note about abstract.c, though. I think you are referring to the change in PyNumber_Add(), but this change is basically a no-op. You might be reading the diff in the wrong direction: it appears to *remove* a check for Py_NotImplemented. However, this check was added in r25556 and r25557 to fix another bug, and this is no longer needed if the four SLOT1() lines go away. |
|
|
| msg49225 - (view) |
Author: Neil Schemenauer (nascheme) * |
Date: 2005-12-28 16:10 |
| Logged In: YES user_id=35752 I'm not sure what Py_NotImplemented check you are referring to but I think the patches do the right thing and could be checked in as is. |
|
|
| msg49226 - (view) |
Author: Marc-Andre Lemburg (lemburg) * |
Date: 2005-12-28 21:07 |
| Logged In: YES user_id=38388 Haven't looked at the patches, but your comment "The patch also addresses PySequence_InPlaceConcat/Repeat() but without testing them -- they can't be tested from Python, so they are already completely untested." is not quite correct: We have the _testcapimodule.c for doing C API tests. Do you also address the problem I posted to python-dev ? """...the following code could be the cause for leaking a NotImplemented singleton reference: #define SLOT1BINFULL(FUNCNAME, TESTFUNC, SLOTNAME, OPSTR, ROPSTR) \ static PyObject * \ FUNCNAME(PyObject *self, PyObject *other) \ { \ static PyObject *cache_str, *rcache_str; \ int do_other = self->ob_type != other->ob_type && \ other->ob_type->tp_as_number != NULL && \ other->ob_type->tp_as_number->SLOTNAME == TESTFUNC; \ if (self->ob_type->tp_as_number != NULL && \ self->ob_type->tp_as_number->SLOTNAME == TESTFUNC) { \ PyObject *r; \ if (do_other && \ PyType_IsSubtype(other->ob_type, self->ob_type) && \ method_is_overloaded(self, other, ROPSTR)) { \ r = call_maybe( \ other, ROPSTR, &rcache_str, "(O)", self); \ if (r != Py_NotImplemented) \ return r; \ Py_DECREF(r); \ do_other = 0; \ } \ r = call_maybe( \ self, OPSTR, &cache_str, "(O)", other); \ if (r != Py_NotImplemented | |
\ other->ob_type == self->ob_type) \ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ If both types are of the same type, then a NotImplemented returng value would be returned. return r; \ Py_DECREF(r); \ } \ if (do_other) { \ return call_maybe( \ other, ROPSTR, &rcache_str, "(O)", self); \ } \ Py_INCREF(Py_NotImplemented); \ return Py_NotImplemented; \ } """ |
|
| msg49227 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2005-12-28 22:17 |
| Logged In: YES user_id=4771 Thanks for the reminder! It doesn't change the fact that PySequence_InPlace*() is not tested, but indeed now I know where I should add a test for them. About the python-dev post, see my python-dev answer: it's expected of this function to be able to return Py_NotImplemented, as is obvious from the last return statement :-) (Updated patch to come...) |
|
|
| msg49228 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2005-12-29 15:59 |
| Logged In: YES user_id=4771 Improved the comments and checked in as r41842. I have tested PySequence_InPlace*() "manually". I am thinking about adding the in-place operations to the operator module; this should make all these C functions more easily testable. |
|
|
| msg49229 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2005-12-29 17:09 |
| Logged In: YES user_id=4771 Checked in new built-ins operator.ixxx(), with docs and tests. |
|
|
| msg49230 - (view) |
Author: Armin Rigo (arigo) * |
Date: 2006-02-20 10:21 |
| Logged In: YES user_id=4771 Back-ported to 2.4 as r42511. |
|
|