Folks, There's a bug about number coercion about Decimal (http://www.python.org/sf/1355842). The bug appeared after some changes Raymond and I did a few months ago, solving something else (started to return NotImplemented instead of raising TypeError, to better work with custom objects that implements type coercion from Decimal). The point is that I'm really astonished about the following behaviour, and don't know where to start searching:
d Decimal("1") # using decimal.py rev. 39328 from svn d + 1.2 Traceback (most recent call last): File "<stdin>", line 1, in ? TypeError: unsupported operand type(s) for +: 'Decimal' and 'float' d += 1.2 d NotImplemented
Thanks for any tip. . Facundo Blog: http://www.taniquetil.com.ar/plog/ PyAr: http://www.python.org/ar/
Hi Facundo, On Sat, Dec 24, 2005 at 02:31:19PM -0300, Facundo Batista wrote:
d += 1.2 d NotImplemented
The situation appears to be a mess. Some combinations of specific operators fail to convert NotImplemented to a TypeError, depending on old- or new-style-class-ness, although this is clearly a bug (e.g. in an example like yours but using -= instead of +=, we get the correct TypeError.) Obviously, we need to write some comprehensive tests about this. But now I just found out that the old, still-pending SF bug #847024 about A()*5 in new-style classes hasn't been given any attention; my theory is that nobody fully understands the convoluted code paths of abstract.c any more :-( A bientot, Armin
Armin Rigo wrote:
Hi Facundo,
On Sat, Dec 24, 2005 at 02:31:19PM -0300, Facundo Batista wrote:
d += 1.2 d NotImplemented
The situation appears to be a mess. Some combinations of specific operators fail to convert NotImplemented to a TypeError, depending on old- or new-style-class-ness, although this is clearly a bug (e.g. in an example like yours but using -= instead of +=, we get the correct TypeError.)
Obviously, we need to write some comprehensive tests about this. But now I just found out that the old, still-pending SF bug #847024 about A()*5 in new-style classes hasn't been given any attention; my theory is that nobody fully understands the convoluted code paths of abstract.c any more :-(
Time for a rewrite? Reinhold -- Mail address is perfectly valid!
Hi Reinhold, On Sun, Dec 25, 2005 at 12:37:53PM +0100, Reinhold Birkenfeld wrote:
that nobody fully understands the convoluted code paths of abstract.c any more :-(
Time for a rewrite?
Of course, speaking of a rewrite, PyPy does the "right thing" in these two areas. Won't happen to CPython, though. There are too much backward-compatibility issues with the PyTypeObject structure; I think we're doomed with patching the bugs as they show up. Looking up in the language reference, I see no mention of NotImplemented in the page about __add__, __radd__, etc. I guess it's a documentation bug as well, isn't it? The current code base tries to implement the following behavior: Returning NotImplemented from any of the binary special methods (__xxx__, __rxxx__, __ixxx__) makes Python proceed as if the method was not defined in the first place. If we agree on this, I could propose a doc fix, a test, and appropriate bug fixes. A bientot, Armin
Armin Rigo wrote:
Hi Reinhold,
On Sun, Dec 25, 2005 at 12:37:53PM +0100, Reinhold Birkenfeld wrote:
that nobody fully understands the convoluted code paths of abstract.c any more :-( Time for a rewrite?
Of course, speaking of a rewrite, PyPy does the "right thing" in these two areas. Won't happen to CPython, though. There are too much backward-compatibility issues with the PyTypeObject structure; I think we're doomed with patching the bugs as they show up.
Looking up in the language reference, I see no mention of NotImplemented in the page about __add__, __radd__, etc. I guess it's a documentation bug as well, isn't it? The current code base tries to implement the following behavior: Returning NotImplemented from any of the binary special methods (__xxx__, __rxxx__, __ixxx__) makes Python proceed as if the method was not defined in the first place.
If we agree on this, I could propose a doc fix, a test, and appropriate bug fixes.
That sounds like the right definition to me (I believe this behaviour is what Raymond and Facundo were aiming for with the last round of updates to Decimal). Regards, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://www.boredomandlaziness.org
Hi, On Mon, Dec 26, 2005 at 02:40:38AM +1000, Nick Coghlan wrote:
That sounds like the right definition to me (I believe this behaviour is what Raymond and Facundo were aiming for with the last round of updates to Decimal).
Done in patch #1390657. Although this patch passes all existing tests plus the ones it adds, there is a corner and untested case where it could potentially break code. Indeed, the only sane patch I could come up with makes user-defined types fail to work with PySequence_Concat() and PySequence_Repeat() -- details in the patch. So I propose that we clarify what these two functions really mean in term of the Python language spec, instead of just in term of the CPython-specific sq_concat and sq_repeat slots. (BTW that's needed for PyPy/Jython/etc., too, to give a reasonable meaning to the operator.concat() and operator.repeat() built-ins.) I propose the following definitions (which are mostly what the docstrings already explain anyway): * PySequence_Concat(a, b) and operator.concat(a, b) mean "a + b", with the difference that we check that both arguments appear to be sequences (as checked with operator.isSequenceType()). * PySequence_Repeat(a, b) and operator.repeat(a, b) mean "a * b", where "a" is checked to be a sequence and "b" is an integer. Some bounds can be enforced on "b" -- for CPython, it means that it must fit in a C int. The idea is to extend PySequence_Concat() and PySequence_Repeat() to match the above definitions precisely, which means that for objects not defining sq_repeat or sq_concat but still appearing to be sequences, nb_add and nb_multiply should be tried. I don't think that this would break existing C or Python code, but it should probably only go in 2.5, together with the patch #1390657 that relies on it. A bientot, Armin
2005/12/26, Armin Rigo
Done in patch #1390657.
Fantastic, Armin, thank you!
nb_add and nb_multiply should be tried. I don't think that this would break existing C or Python code, but it should probably only go in 2.5, together with the patch #1390657 that relies on it.
It'd be good to know if this will be applied for the next version 2.4.x or will wait until 2.4.5, for me to search a workaround in Decimal or not (really don't know if I can find a solution here). Thank you! . Facundo Blog: http://www.taniquetil.com.ar/plog/ PyAr: http://www.python.org/ar/
Hi Facundo, On Mon, Dec 26, 2005 at 02:31:10PM -0300, Facundo Batista wrote:
nb_add and nb_multiply should be tried. I don't think that this would break existing C or Python code, but it should probably only go in 2.5, together with the patch #1390657 that relies on it.
It'd be good to know if this will be applied for the next version 2.4.x or will wait until 2.4.5, for me to search a workaround in Decimal or not (really don't know if I can find a solution here).
I completed the patch on the SF tracker, and now I believe that it could safely be checked in the HEAD and in the 2.4 branch (after the appropriate review). A bientot, Armin
On 12/25/05, Armin Rigo
Hi Reinhold,
On Sun, Dec 25, 2005 at 12:37:53PM +0100, Reinhold Birkenfeld wrote:
that nobody fully understands the convoluted code paths of abstract.c any more :-(
Time for a rewrite?
Maybe. Also realize we will have a chance to clean it up when Python 3 comes around since the classic class stuff will be ripped out. That way we might have a chance to streamline the code.
Of course, speaking of a rewrite, PyPy does the "right thing" in these two areas. Won't happen to CPython, though. There are too much backward-compatibility issues with the PyTypeObject structure; I think we're doomed with patching the bugs as they show up.
Looking up in the language reference, I see no mention of NotImplemented in the page about __add__, __radd__, etc. I guess it's a documentation bug as well, isn't it? The current code base tries to implement the following behavior: Returning NotImplemented from any of the binary special methods (__xxx__, __rxxx__, __ixxx__) makes Python proceed as if the method was not defined in the first place.
This is what I always assumed the behaviour was supposed to be, so I am quite happy to go with that. -Brett
Hi Brett, On Sun, Dec 25, 2005 at 11:55:11AM -0800, Brett Cannon wrote:
Maybe. Also realize we will have a chance to clean it up when Python 3 comes around since the classic class stuff will be ripped out. That way we might have a chance to streamline the code.
For once, old-style classes are not to blame here: it's only about the oldest aspects of the PyTypeObject structure and substructures. I-said-that-no-one-knows-this-code-any-more'ly yours, Armin
Armin Rigo
Of course, speaking of a rewrite, PyPy does the "right thing" in these two areas. Won't happen to CPython, though. There are too much backward-compatibility issues with the PyTypeObject structure; I think we're doomed with patching the bugs as they show up.
This is definitely something that should be cleaned up for Python 3k.
Looking up in the language reference, I see no mention of NotImplemented in the page about __add__, __radd__, etc. I guess it's a documentation bug as well, isn't it? The current code base tries to implement the following behavior: Returning NotImplemented from any of the binary special methods (__xxx__, __rxxx__, __ixxx__) makes Python proceed as if the method was not defined in the first place.
If we agree on this, I could propose a doc fix, a test, and appropriate bug fixes.
I believe that's correct. Neil
Armin Rigo wrote:
Hi Facundo,
On Sat, Dec 24, 2005 at 02:31:19PM -0300, Facundo Batista wrote:
d += 1.2 d NotImplemented
The situation appears to be a mess. Some combinations of specific operators fail to convert NotImplemented to a TypeError, depending on old- or new-style-class-ness, although this is clearly a bug (e.g. in an example like yours but using -= instead of +=, we get the correct TypeError.)
Obviously, we need to write some comprehensive tests about this. But now I just found out that the old, still-pending SF bug #847024 about A()*5 in new-style classes hasn't been given any attention; my theory is that nobody fully understands the convoluted code paths of abstract.c any more :-(
The PEP documenting the coercion logic has complete tables for what should happen: http://www.python.org/peps/pep-0208.html Looking at the code in abstract.c the above problem appears to be related to the special cases applied to += and *= in case both operands cannot deal with the type combination. In such a case, a check is done whether the operation could be interpreted as sequence operation (concat or repeat) and then delegated to the appropriate handlers. But then again, looking in typeobject.c, 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; \ } Strange enough, the SLOT1BINFULL macro is not used by the inplace concat or repeat slots... -- Marc-Andre Lemburg eGenix.com Professional Python Services directly from the Source (#1, Dec 28 2005)
Python/Zope Consulting and Support ... http://www.egenix.com/ mxODBC.Zope.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
::: Try mxODBC.Zope.DA for Windows,Linux,Solaris,FreeBSD for free ! ::::
Hi Marc, On Wed, Dec 28, 2005 at 09:56:43PM +0100, M.-A. Lemburg wrote:
d += 1.2 d NotImplemented
The PEP documenting the coercion logic has complete tables for what should happen:
Well, '+=' does not invoke coercion at all, with new-style classes like Decimal.
Looking at the code in abstract.c the above problem appears to be related to the special cases applied to += and *= in case both operands cannot deal with the type combination.
In such a case, a check is done whether the operation could be interpreted as sequence operation (concat or repeat) and then delegated to the appropriate handlers.
Indeed. The bug was caused by this delegation, which (prior to my patch) would also return a Py_NotImplemented that would leak through abstract.c. My patch is to remove this unnecessary delegation by not defining sq_concat/sq_repeat for user-defined classes, and restoring the original expectation that the sq_concat/sq_repeat slots should not return Py_NotImplemented. How does this relate to coercion?
But then again, looking in typeobject.c, 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.
Indeed, however:
return r; \ Py_DECREF(r); \ } \ if (do_other) { \ return call_maybe( \ other, ROPSTR, &rcache_str, "(O)", self); \ } \ Py_INCREF(Py_NotImplemented); \ return Py_NotImplemented; \ }
This last statement also returns Py_NotImplemented. So it's expected of this function to be able to return Py_NotImplemented, isn't it? The type slots like nb_add can return Py_NotImplemented; the code that converts it to a TypeError is in the caller, which is abstract.c. A bientot, Armin
Hi Armin,
On Wed, Dec 28, 2005 at 09:56:43PM +0100, M.-A. Lemburg wrote:
> d += 1.2 > d NotImplemented The PEP documenting the coercion logic has complete tables for what should happen:
Well, '+=' does not invoke coercion at all, with new-style classes like Decimal.
True, it doesn't invoke coercion in the sense that a coercion method is called, but the mechanism described in the PEP is still used via PyNumber_InPlaceAdd().
Looking at the code in abstract.c the above problem appears to be related to the special cases applied to += and *= in case both operands cannot deal with the type combination.
In such a case, a check is done whether the operation could be interpreted as sequence operation (concat or repeat) and then delegated to the appropriate handlers.
Indeed. The bug was caused by this delegation, which (prior to my patch) would also return a Py_NotImplemented that would leak through abstract.c. My patch is to remove this unnecessary delegation by not defining sq_concat/sq_repeat for user-defined classes, and restoring the original expectation that the sq_concat/sq_repeat slots should not return Py_NotImplemented. How does this relate to coercion?
The Py_NotImplemented singleton was introduced in the coercion proposal to mean "there is no implementation to execute the requested operation on the given combination of types". At the time we also considered using an exception for this, but it turned out that this caused too much of a slow-down. Hence the use of a special singleton which could be tested for by a simple pointer comparison. Originally, the singleton was only needed for mixed-type operations. It seems that its use has spread to other areas as well and can now also refer to missing same-type operator implementations.
But then again, looking in typeobject.c, 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.
Indeed, however:
return r; \ Py_DECREF(r); \ } \ if (do_other) { \ return call_maybe( \ other, ROPSTR, &rcache_str, "(O)", self); \ } \ Py_INCREF(Py_NotImplemented); \ return Py_NotImplemented; \ }
This last statement also returns Py_NotImplemented. So it's expected of this function to be able to return Py_NotImplemented, isn't it? The type slots like nb_add can return Py_NotImplemented; the code that converts it to a TypeError is in the caller, which is abstract.c.
You're right - silly me. Regards, -- Marc-Andre Lemburg eGenix.com Professional Python Services directly from the Source (#1, Dec 29 2005)
Python/Zope Consulting and Support ... http://www.egenix.com/ mxODBC.Zope.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
::: Try mxODBC.Zope.DA for Windows,Linux,Solaris,FreeBSD for free ! ::::
participants (7)
-
Armin Rigo -
Brett Cannon -
Facundo Batista -
M.-A. Lemburg -
Neil Schemenauer -
Nick Coghlan -
Reinhold Birkenfeld