PyNumber_*() binary operations & coercion
While re-writing the PyNumber_InPlace*() functions in augmented assignment to something Guido and I agree on should be the Right Way, I found something that *might* be a bug. But I'm not sure. The PyNumber_*() methods for binary operations (found in abstract.c) have the following construct: if (v->ob_type->tp_as_number != NULL) { PyObject *x = NULL; PyObject * (*f)(PyObject *, PyObject *); if (PyNumber_Coerce(&v, &w) != 0) return NULL; if ((f = v->ob_type->tp_as_number->nb_xor) != NULL) x = (*f)(v, w); Py_DECREF(v); Py_DECREF(w); if (f != NULL) return x; } (This is after a check if either argument is an instance object, so both are C objects here.) Now, I'm not sure how coercion is supposed to work, but I see one problem here: 'v' can be changed by PyNumber_Coerce(), and the new object's tp_as_number pointer could be NULL. I bet it's pretty unlikely that (numeric) coercion of a numeric object and an unspecified object turns up a non-numeric object, but I don't see anything guaranteeing it won't, either. Is this a non-issue, or should I bother with adding the extra check in the current binary operations (and the new inplace ones) ? -- Thomas Wouters <thomas@xs4all.net> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
While re-writing the PyNumber_InPlace*() functions in augmented assignment to something Guido and I agree on should be the Right Way, I found something that *might* be a bug. But I'm not sure.
The PyNumber_*() methods for binary operations (found in abstract.c) have the following construct:
if (v->ob_type->tp_as_number != NULL) { PyObject *x = NULL; PyObject * (*f)(PyObject *, PyObject *); if (PyNumber_Coerce(&v, &w) != 0) return NULL; if ((f = v->ob_type->tp_as_number->nb_xor) != NULL) x = (*f)(v, w); Py_DECREF(v); Py_DECREF(w); if (f != NULL) return x; }
(This is after a check if either argument is an instance object, so both are C objects here.) Now, I'm not sure how coercion is supposed to work, but I see one problem here: 'v' can be changed by PyNumber_Coerce(), and the new object's tp_as_number pointer could be NULL. I bet it's pretty unlikely that (numeric) coercion of a numeric object and an unspecified object turns up a non-numeric object, but I don't see anything guaranteeing it won't, either.
Is this a non-issue, or should I bother with adding the extra check in the current binary operations (and the new inplace ones) ?
I think this currently can't happen because coercions never return non-numeric objects, but it sounds like a good sanity check to add. Please check this in as a separate patch (not as part of the huge augmented assignment patch). --Guido van Rossum (home page: http://www.pythonlabs.com/~guido/)
On Wed, Aug 23, 2000 at 06:28:03PM -0500, Guido van Rossum wrote:
Now, I'm not sure how coercion is supposed to work, but I see one problem here: 'v' can be changed by PyNumber_Coerce(), and the new object's tp_as_number pointer could be NULL. I bet it's pretty unlikely that (numeric) coercion of a numeric object and an unspecified object turns up a non-numeric object, but I don't see anything guaranteeing it won't, either.
I think this currently can't happen because coercions never return non-numeric objects, but it sounds like a good sanity check to add.
Please check this in as a separate patch (not as part of the huge augmented assignment patch).
Alright, checking it in after 'make test' finishes. I'm also removing some redundant PyInstance_Check() calls in PyNumber_Multiply: the first thing in that function is a BINOP call, which expands to if (PyInstance_Check(v) || PyInstance_Check(w)) \ return PyInstance_DoBinOp(v, w, opname, ropname, thisfunc) So after the BINOP call, neither argument can be an instance, anyway. Also, I'll take this opportunity to explain what I'm doing with the PyNumber_InPlace* functions, for those that are interested. The comment I'm placing in the code should be enough information: /* The in-place operators are defined to fall back to the 'normal', non in-place operations, if the in-place methods are not in place, and to take class instances into account. This is how it is supposed to work: - If the left-hand-side object (the first argument) is an instance object, let PyInstance_DoInPlaceOp() handle it. Pass the non in-place variant of the function as callback, because it will only be used if any kind of coercion has been done, and if an object has been coerced, it's a new object and shouldn't be modified in-place. - Otherwise, if the object has the appropriate struct members, and they are filled, call that function and return the result. No coercion is done on the arguments; the left-hand object is the one the operation is performed on, and it's up to the function to deal with the right-hand object. - Otherwise, if the second argument is an Instance, let PyInstance_DoBinOp() handle it, but not in-place. Again, pass the non in-place function as callback. - Otherwise, both arguments are C objects. Try to coerce them and call the ordinary (not in-place) function-pointer from the type struct. - Otherwise, we are out of options: raise a type error. */ If anyone sees room for unexpected behaviour under these rules, let me know and you'll get an XS4ALL shirt! (Sorry, only ones I can offer ;) -- Thomas Wouters <thomas@xs4all.net> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
Thomas Wouters wrote:
On Wed, Aug 23, 2000 at 06:28:03PM -0500, Guido van Rossum wrote:
Now, I'm not sure how coercion is supposed to work, but I see one problem here: 'v' can be changed by PyNumber_Coerce(), and the new object's tp_as_number pointer could be NULL. I bet it's pretty unlikely that (numeric) coercion of a numeric object and an unspecified object turns up a non-numeric object, but I don't see anything guaranteeing it won't, either.
I think this currently can't happen because coercions never return non-numeric objects, but it sounds like a good sanity check to add.
Please check this in as a separate patch (not as part of the huge augmented assignment patch).
Alright, checking it in after 'make test' finishes. I'm also removing some redundant PyInstance_Check() calls in PyNumber_Multiply: the first thing in that function is a BINOP call, which expands to
if (PyInstance_Check(v) || PyInstance_Check(w)) \ return PyInstance_DoBinOp(v, w, opname, ropname, thisfunc)
So after the BINOP call, neither argument can be an instance, anyway.
Also, I'll take this opportunity to explain what I'm doing with the PyNumber_InPlace* functions, for those that are interested. The comment I'm placing in the code should be enough information:
/* The in-place operators are defined to fall back to the 'normal', non in-place operations, if the in-place methods are not in place, and to take class instances into account. This is how it is supposed to work:
- If the left-hand-side object (the first argument) is an instance object, let PyInstance_DoInPlaceOp() handle it. Pass the non in-place variant of the function as callback, because it will only be used if any kind of coercion has been done, and if an object has been coerced, it's a new object and shouldn't be modified in-place.
- Otherwise, if the object has the appropriate struct members, and they are filled, call that function and return the result. No coercion is done on the arguments; the left-hand object is the one the operation is performed on, and it's up to the function to deal with the right-hand object.
- Otherwise, if the second argument is an Instance, let PyInstance_DoBinOp() handle it, but not in-place. Again, pass the non in-place function as callback.
- Otherwise, both arguments are C objects. Try to coerce them and call the ordinary (not in-place) function-pointer from the type struct.
- Otherwise, we are out of options: raise a type error.
*/
If anyone sees room for unexpected behaviour under these rules, let me know and you'll get an XS4ALL shirt! (Sorry, only ones I can offer ;)
I just hope that with all these new operators you haven't closed the door for switching to argument based handling of coercion. One of these days (probably for 2.1), I would like to write up the proposal I made on my Python Pages about a new coercion mechanism as PEP. The idea behind it is to only use centralized coercion as fall-back solution in case the arguments can't handle the operation with the given type combination. To implement this, all builtin types will have to be changed to support mixed type argument slot functions (this ability will be signalled to the interpreter using a type flag). More infos on the proposal page at: http://starship.python.net/crew/lemburg/CoercionProposal.html Is this still possible under the new code you've added ? -- Marc-Andre Lemburg ______________________________________________________________________ Business: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
I just hope that with all these new operators you haven't closed the door for switching to argument based handling of coercion.
Far from it! Actually, the inplace operators won't do any coercions when the left argument supports the inplace version, and otherwise exactly the same rules apply as for the non-inplace version. (I believe this isn't in the patch yet, but it will be when Thomas checks it in.) --Guido van Rossum (home page: http://www.pythonlabs.com/~guido/)
On Thu, Aug 24, 2000 at 09:00:56AM -0500, Guido van Rossum wrote:
I just hope that with all these new operators you haven't closed the door for switching to argument based handling of coercion.
Far from it! Actually, the inplace operators won't do any coercions when the left argument supports the inplace version, and otherwise exactly the same rules apply as for the non-inplace version. (I believe this isn't in the patch yet, but it will be when Thomas checks it in.)
Exactly. (Actually, I'm again re-working the patch: If I do it the way I intended to, you'd sometimes get the 'non in-place' error messages, instead of the in-place ones. But the result will be the same.) -- Thomas Wouters <thomas@xs4all.net> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
participants (3)
-
Guido van Rossum -
M.-A. Lemburg -
Thomas Wouters