Bug in PyNumber_InPlacePower implementation?
I was looking at the implementation of PyNumber_InPlacePower last night, and something about it struck me as odd. It starts off: if (HASINPLACE(v) && v->ob_type->tp_as_number && v->ob_type->tp_as_number->nb_inplace_power != NULL) { return ternary_op(v, w, z, NB_SLOT(nb_inplace_power), "**="); Now, looking at ternary_op, it appears that under some circumstances this could call the nb_inplace_power slot of the second or third argument before trying the first one: if (slotv) { if (slotw && PyType_IsSubtype(w->ob_type, v->ob_type)) { x = slotw(v, w, z); i.e. if the 2nd argument is a subtype of the 1st, and it has an nb_inplace_power method, it will be called first. This looks wrong to me. Surely only the *first* argument should be checked for an inplace method when doing an inplace operation? That's the way it seems to be for all the other inplace operations. Is this a bug? Greg Ewing, Computer Science Dept, +--------------------------------------+ University of Canterbury, | A citizen of NewZealandCorp, a | Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. | greg@cosc.canterbury.ac.nz +--------------------------------------+
I was looking at the implementation of PyNumber_InPlacePower last night, and something about it struck me as odd. It starts off:
if (HASINPLACE(v) && v->ob_type->tp_as_number && v->ob_type->tp_as_number->nb_inplace_power != NULL) { return ternary_op(v, w, z, NB_SLOT(nb_inplace_power), "**=");
Now, looking at ternary_op, it appears that under some circumstances this could call the nb_inplace_power slot of the second or third argument before trying the first one:
if (slotv) { if (slotw && PyType_IsSubtype(w->ob_type, v->ob_type)) { x = slotw(v, w, z);
i.e. if the 2nd argument is a subtype of the 1st, and it has an nb_inplace_power method, it will be called first.
This looks wrong to me. Surely only the *first* argument should be checked for an inplace method when doing an inplace operation? That's the way it seems to be for all the other inplace operations. Is this a bug?
This is exactly the same as what binary_op1() does. I added this twist intentionally because of a use case where a subclass of a numeric type wants to override a binary (or ternary) operator defined by the base class. If the subclass wasn't tried first, it would never be able to override the case where a base class instance is the left operand, because the base class implementation is generally happy to accept a subclass instance as the right operand. (I admit that a comment explaining this would have been handy. :-) Do you have a use case where this does the wrong thing, or is this just a theoretical musing? --Guido van Rossum (home page: http://www.python.org/~guido/)
This is exactly the same as what binary_op1() does. I added this twist intentionally because of a use case where a subclass of a numeric type wants to override a binary (or ternary) operator defined by the base class.
I don't think you understand what I mean. I'm talking about *in-place* operations. For in-place binary operations we have, e.g. INPLACE_BINOP(PyNumber_InPlaceSubtract, nb_inplace_subtract, nb_subtract, "-=") ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^ Both the in-place and non-in-place slots are passed to binary_iop, which uses the in-place slot for the first operand and the non-in-place slot for the second. But ternary_op only gets *one* slot passed to it, which it uses for everything. When doing an in-place ternary op, this is the in-place slot. What this seems to mean is that, if we have isinstance(a, A) and isinstance(b, B) and issubclass(B, A), then a **= b has the potential to in-place-modify b instead of a! It seems to me that there ought to be a ternary_iop routine that does for ternary ops what binary_iop does for binary ones.
Do you have a use case where this does the wrong thing, or is this just a theoretical musing?
It's a theoretical musing that came out of my attempts to figure out whether, when one is implementing an nb_inplace_power slot for a new type, the first argument is guaranteed to "self". I need to know this so that Pyrex can do the right thing. I've done some experiments with Python code, and it seems to do the right thing in terms of calling the correct __pow__, __rpow__ and __ipow__ methods. But I can't tell from that what's really going on at the typeslot level. I've looked at the code for the nb_inplace_power of instance objects (the only example I can find of such a method!), and it seems to assume that the first argument is always "self". But I can't see how this is guaranteed. In short, I'm confused! Greg Ewing, Computer Science Dept, +--------------------------------------+ University of Canterbury, | A citizen of NewZealandCorp, a | Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. | greg@cosc.canterbury.ac.nz +--------------------------------------+
This is exactly the same as what binary_op1() does. I added this twist intentionally because of a use case where a subclass of a numeric type wants to override a binary (or ternary) operator defined by the base class.
I don't think you understand what I mean. I'm talking about *in-place* operations.
You're right, I hadn't realized that this was about inplace.
For in-place binary operations we have, e.g.
INPLACE_BINOP(PyNumber_InPlaceSubtract, nb_inplace_subtract, nb_subtract, "-=") ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^
Both the in-place and non-in-place slots are passed to binary_iop, which uses the in-place slot for the first operand and the non-in-place slot for the second.
But ternary_op only gets *one* slot passed to it, which it uses for everything. When doing an in-place ternary op, this is the in-place slot. What this seems to mean is that, if we have isinstance(a, A) and isinstance(b, B) and issubclass(B, A), then
a **= b
has the potential to in-place-modify b instead of a!
Not quite -- the arguments won't be reversed, but it may call B's inplace power function with an A instance as the first argument. This can return NotImplemented if it doesn't know what to do in that case.
It seems to me that there ought to be a ternary_iop routine that does for ternary ops what binary_iop does for binary ones.
Or we could just document the status quo. It's a pretty esoteric case -- numeric types are traditionally immutable so don't have to implement inplace power at all, and non-numeric types aren't very likely to implement ** at all, let alone **=. The correct code would be pretty hairy I think (the non-inplace ternary is hairy enough).
Do you have a use case where this does the wrong thing, or is this just a theoretical musing?
It's a theoretical musing that came out of my attempts to figure out whether, when one is implementing an nb_inplace_power slot for a new type, the first argument is guaranteed to "self". I need to know this so that Pyrex can do the right thing.
I've done some experiments with Python code, and it seems to do the right thing in terms of calling the correct __pow__, __rpow__ and __ipow__ methods. But I can't tell from that what's really going on at the typeslot level. I've looked at the code for the nb_inplace_power of instance objects (the only example I can find of such a method!), and it seems to assume that the first argument is always "self". But I can't see how this is guaranteed.
In short, I'm confused!
Are you still confused after my assertion above? --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido:
Me:
a **= b
has the potential to in-place-modify b instead of a!
Not quite -- the arguments won't be reversed, but it may call B's inplace power function with an A instance as the first argument.
Hmmm, I see. I guess it's just a matter of being aware that this can happen and doing the necessary type tests.
This can return NotImplemented if it doesn't know what to do in that case.
Although if you're implementing inplace-power you're probably also implementing non-inplace-power, in which case the right thing is probably to call that instead. I don't think that will happen automatically if you return NotImplemented -- or will it?
It's a pretty esoteric case ... The correct code would be pretty hairy I think (the non-inplace ternary is hairy enough).
I think the existing ternary_op routine could be converted fairly easily into one that could be used for both. Just pass in two slot arguments, and use one for the first arg and the other one for the second and third args. For non-inplace ops, call it with the non-inplace slot for both. It's not a big deal, I suppose, but it would be nice if it could be made consistent with the other in-place ops. Greg Ewing, Computer Science Dept, +--------------------------------------+ University of Canterbury, | A citizen of NewZealandCorp, a | Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. | greg@cosc.canterbury.ac.nz +--------------------------------------+
Guido:
GregE:
a **= b
has the potential to in-place-modify b instead of a!
Not quite -- the arguments won't be reversed, but it may call B's inplace power function with an A instance as the first argument.
Hmmm, I see. I guess it's just a matter of being aware that this can happen and doing the necessary type tests.
This can return NotImplemented if it doesn't know what to do in that case.
Although if you're implementing inplace-power you're probably also implementing non-inplace-power, in which case the right thing is probably to call that instead. I don't think that will happen automatically if you return NotImplemented -- or will it?
For binary ops it will (see binary_iop()). But because the ternary code doesn't have the inplace support, it won't.
It's a pretty esoteric case ... The correct code would be pretty hairy I think (the non-inplace ternary is hairy enough).
I think the existing ternary_op routine could be converted fairly easily into one that could be used for both. Just pass in two slot arguments, and use one for the first arg and the other one for the second and third args. For non-inplace ops, call it with the non-inplace slot for both.
It's not a big deal, I suppose, but it would be nice if it could be made consistent with the other in-place ops.
Do you care enough about this to supply a patch? I would apply it, but I don't care enough to write it. :-) --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido:
Do you care enough about this to supply a patch? I would apply it, but I don't care enough to write it. :-)
I'll see what I can do. Thanks for the help, Greg Ewing, Computer Science Dept, +--------------------------------------+ University of Canterbury, | A citizen of NewZealandCorp, a | Christchurch, New Zealand | wholly-owned subsidiary of USA Inc. | greg@cosc.canterbury.ac.nz +--------------------------------------+
participants (2)
-
Greg Ewing -
Guido van Rossum