Binary arithmetic does not always call subclasses first

[shoyer]

BPO	30140
Nosy	@gvanrossum, @tim-one, @rhettinger, @mdickinson, @ncoghlan, @abalkin, @vstinner, @serhiy-storchaka, @eric-wieser, @MojoVampire, @shoyer, @leezu
PRs	bpo-30140: fix binop dispatch for subclasses #1325

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-04-11.03:38:55.960>
created_at = <Date 2017-04-23.04:21:44.659>
labels = ['interpreter-core', 'type-bug', '3.8']
title = 'Binary arithmetic does not always call subclasses first'
updated_at = <Date 2020-04-11.03:38:55.959>
user = 'https://github.com/shoyer'

bugs.python.org fields:

activity = <Date 2020-04-11.03:38:55.959>
actor = 'gvanrossum'
assignee = 'none'
closed = True
closed_date = <Date 2020-04-11.03:38:55.960>
closer = 'gvanrossum'
components = ['Interpreter Core']
creation = <Date 2017-04-23.04:21:44.659>
creator = 'Stephan Hoyer'
dependencies = []
files = []
hgrepos = []
issue_num = 30140
keywords = ['patch', 'needs review']
message_count = 13.0
messages = ['292149', '292157', '292188', '292198', '292278', '292279', '292460', '301382', '304597', '325553', '325876', '347963', '366186']
nosy_count = 12.0
nosy_names = ['gvanrossum', 'tim.peters', 'rhettinger', 'mark.dickinson', 'ncoghlan', 'belopolsky', 'vstinner', 'serhiy.storchaka', 'Eric.Wieser', 'josh.r', 'Stephan Hoyer', 'leezu']
pr_nums = ['1325']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30140'
versions = ['Python 3.8']

[shoyer]

We are writing a system for overloading NumPy operations (see PR [1] and design doc [2]) that is designed to copy and extend Python's system for overloading binary operators.

The reference documentation on binary arithmetic [3] states:

Note: If the right operand's type is a subclass of the left operand’s type and that subclass provides the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

However, this isn't actually done if the right operand merely inherits from the left operand's type. In practice, CPython requires that the right operand defines a different method before it defers to it. Note that the behavior is different for comparisons, which defer to subclasses regardless of whether they implement a new method [4].

I think this behavior is a mistake and should be corrected. It is just as useful to write generic binary arithmetic methods that are well defined on subclasses as generic comparison operations. In fact, this is exactly the design pattern we propose for objects implementing special operators like NumPy arrays (see NDArrayOperatorsMixin in [1] and [2]).

Here is a simple example, of a well-behaved that implements addition by wrapping its value and returns NotImplemented when the other operand has the wrong type:

class A:
   def __init__(self, value):
       self.value = value
   def __add__(self, other):
       if not isinstance(other, A):
           return NotImplemented
       return type(self)(self.value + other.value)
   __radd__ = __add__
   def __repr__(self):
       return f'{type(self).__name__}({self.value!r})'

class B(A):
    pass

class C(A):
   def __add__(self, other):
       if not isinstance(other, A):
           return NotImplemented
       return type(self)(self.value + other.value)
   __radd__ = __add__

A does not defer to B:

>>> A(1) + B(1)
A(2)

But it does defer to C, which defines new methods (literally copied/pasted) for __add__/radd:

>>> A(1) + C(1)
C(2)

With the current behavior, special operator implementations need to explicitly account for the possibility that they are being called from a subclass by returning NotImplemented. My guess is that this is rarely done, which means that most of these methods are broken when used with subclasses, or subclasses needlessly reimplement these methods.

Can we fix this logic for Python 3.7?

[1] numpy/numpy#8247
[2] https://github.com/charris/numpy/blob/406bbc652424fff332f49b0d2f2e5aedd8191d33/doc/neps/ufunc-overrides.rst
[3] https://docs.python.org/3/reference/datamodel.html#object.\_\_ror__
[4] http://bugs.python.org/issue22052

[mdickinson]

This is probably worth bringing up on the python-dev or python-ideas mailing lists for greater visibility. I can't think of any plausible non-historical reason why it makes sense for comparisons to behave one way and arithmetic operators another. Altering this might be a PEP-level change, though.

The "Coercion rules" section[1] of the Python 2.7 docs is a bit more explicit about the intent:

"""
Exception to the previous item: if the left operand is an instance of a built-in type or a new-style class, and the right operand is an instance of a proper subclass of that type or class and overrides the base’s __rop__() method, the right operand’s __rop__() method is tried before the left operand’s __op__() method.
"""

so the check for an override was clearly intentional, rather than an implementation convenience or accident. (It's also clearly intentional in the source and comments.) The 3.x docs don't have the "and overrides" language; I haven't figured out why and when that language changed.

[1] https://docs.python.org/release/2.7.6/reference/datamodel.html#coercion-rules

[shoyer]

Posted to python-ideas:
https://mail.python.org/pipermail/python-ideas/2017-April/045451.html

Mark -- just out of curiosity, could you point me to where this logic is implemented in CPython's source?

This feels like something that could once been called a bug but that by now may have become a feature, by virtue of how long it's lasted out in the world.

[mdickinson]

could you point me to where this logic is implemented in CPython's source?

Most of the relevant code is in Objects/abstract.c and Objects/typeobject.c.

A BINARY_ADD opcode (for example) ends up calling PyNumber_Add:

https://github.com/python/cpython/blob/v3.6.1/Objects/abstract.c#L913

which in turn calls binary_op1, where you see an explicit check for the nb_add slots of the two operands being identical:

https://github.com/python/cpython/blob/v3.6.1/Objects/abstract.c#L769-L770

For a user-defined class, the slots themselves are defined in typeobject.c. Here's where nb_add is defined:

https://github.com/python/cpython/blob/v3.6.1/Objects/typeobject.c#L5952

and here's the explicit check for overloading in the SLOT1BIN macro definition:

https://github.com/python/cpython/blob/v3.6.1/Objects/typeobject.c#L5796

There's also an explicit test for the arithmetic operation behaviour in Lib/test/test_descr.py. In short, I doubt this was ever a bug: everything points to this being a deliberate design decision. I hope someone on python-ideas can elaborate on the rationale behind that design decision (and also on why that rationale doesn't apply to comparisons).

In contrast, it does seem plausible to me that the *comparison* failure to check for an explicit override may have been accidental.

[MojoVampire]

I'd assume the preference for __rop__ only on subclass overload is because __rop__ method are usually fallback methods, and differ behaviorally from the __op__ methods in type strictness.

In particular, the __rop__ fallbacks are often so non-strict that they return a completely different type; fractions.Fraction.__rop__ is willing to coerce itself and the other operand to float and produce a float result if the other operand is a numbers.Real (and not a Rational). They also tend to be slower (checking against ABCs and doing more type coercion) than the __op__ path.

If you jump straight to __rop__ because the right hand side is a subclass, but the subclass didn't overload it, you end up going through that fallback, assumed extra liberal and slow, code path.

It doesn't work this way with comparison operators because those are 100% reflexive; there is no expectation that comparing in one direction will be more or less type permissive than comparing in the other direction (stuff like __rcmp__ has been gone for ages after all), so unconditionally comparing using the child class comparator first is fine, and more likely to get correct results.

The design pattern that has problems here is a bit unorthodox to start with. It assumes that the child class constructor will work exactly the same as the parent (no additional mandatory arguments for instance), and that it's always correct for parent + child to produce the type of child. Usually, in an OO design, the parent is not supposed to have any specific knowledge of children; it's the job of the children to work with instances of the parent, if necessary. If delegation to the child is desired, implement __op__ with stricter type checking (to preclude subclasses) and __rop__ with relaxed type checking (to allow them); when the __op__ executes, it will return NotImplemented for the child class, then delegate to __rop__, which will use the child's type.

[shoyer]

The design pattern that has problems here is a bit unorthodox to start with.

I agree. This was meant strictly as a simple example for illustrative purposes. Steven D'Aprano's example from python-ideas may be a better one:
https://mail.python.org/pipermail/python-ideas/2017-April/045455.html

class A:
    def __add__(self, other):
        self.log()
        ...
    __radd__ = __add__

class B(A):
    def log(self):
        ...

Our actual use case for NumPy involved writing a mixin that look more like this, that expects a specified method to implement arithmetic, i.e.,

class NDArrayOperatorsMixin:
    def __add__(self, other):
        return self._calculate(np.add, self, other)
    def __radd__(self, other):
        return self._calculate(np.add, other, self)
    ...  # repeat for all special methods

class A(NDArrayOperatorsMixin):
    def _calculate(self, op, *args):
        if not all(isinstance(arg, A) for arg in args):
            return NotImplemented
        ...  # implement calculation

class B(A):
    def _calculate(self, op, *args):
        ...  # something different

In A() + B(), B never gets the chance to override A's implementation of __add__ via _calculate, because it overrode a different method (calculate) which happens to contain the *implementation* for __radd_, but not __radd__ itself.

Anyways, if you have serious concerns about changing this, it is probably best respond to Guido on python-ideas:
https://mail.python.org/pipermail/python-ideas/2017-April/045468.html

[serhiy-storchaka]

binary_op1() is wrong place for fixing this issue. You need to change SLOT1BINFULL in Objects/typeobject.c.

[shoyer]

Serhiy: thanks for the tip. I've updated my PR, which I think is now ready for review.

[shoyer]

Ping -- it would be great if someone could take a look at my PR. (I suspect it needs more documentation, tips on where to put that would be appreciated.)

[gvanrossum]

If anyone cares, here's what I recall of the history of this feature.

Originally my design was just

1. try left arg's __op__
2. if that returns NotImplemented, try right arg's __rop__

That's simple to explain and usually works. But when thinking through edge cases, we found that if the right class is a subclass of the left class, this gives unexpected results, so we added the exception

0. if the right class subclasses the left class *and it overrides __rop__*, try that first

I honestly believe that I didn't realize that it could ever matter if the right class subclasses the left class *without overriding __rop__*. And nobody else realized it either.

I think we decided on the "right class must override __rop__" condition in part to preserve the original, simpler rules for cases where we thought they would suffice, in part because for most users trying __rop__ before __op__ is unexpected, and in part because we didn't realize that it mattered.

And it only matters if the base class uses something like type(self) or self.__class__, which as a design pattern is sometimes dubious and other times useful.

Also note that the language Stephan quoted is somewhat different: it uses "provides" (rop) instead of "overrides". I could imagine that this, again, was a difference too subtle for most folks to notice.

If I had to do it over, I agree that if the right class subclasses the left class, we should always try __rop__ first. But now that this has been an ingrained behavior for so long I worry that it will break existing code, esp. in cases where there is an asymmetry between __op__ and __rop__ and the class author did not expect __rop__ ever to be called first.

Whether that's enough to stop this PR I think this is a question for the next BDFL to decide, alas. (I don't think it requires a PEP, it just requires someone to decide.)