Skip to content

bpo-30140: fix binop dispatch for subclasses #1325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shoyer wants to merge 2 commits into from
Closed

bpo-30140: fix binop dispatch for subclasses #1325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5de219
bpo-30140: Fix binary operator dispatch for subclasses
shoyer Apr 27, 2017
52d67e8
bpo-30140: Add blurb
shoyer Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion Lib/test/test_binop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Tests for binary operators on subtypes of built-in types."""

import unittest
from operator import eq, le, ne
from operator import eq, le, ne, add
from abc import ABCMeta

def gcd(a, b):
Expand Down Expand Up @@ -387,6 +387,38 @@ def test_comparison_orders(self):
self.assertEqual(op_sequence(eq, B, V), ['B.__eq__', 'V.__eq__'])
self.assertEqual(op_sequence(le, B, V), ['B.__le__', 'V.__ge__'])

def test_arithmetic_orders(self):

def logged_op(name):
def op(self, other):
type_name = type(self).__name__
self.log_operation(f'{type_name}.__{name}__')
return NotImplemented
return op

class A(OperationLogger):
__add__ = logged_op('add')
__radd__ = logged_op('radd')

class B(OperationLogger):
__add__ = logged_op('add')
__radd__ = logged_op('radd')

class C(A):
pass

class D(OperationLogger):
pass

self.assertEqual(op_sequence(add, A, A), ['A.__add__'])
self.assertEqual(op_sequence(add, A, D), ['A.__add__'])
self.assertEqual(op_sequence(add, D, A), ['A.__radd__'])
self.assertEqual(op_sequence(add, A, B), ['A.__add__', 'B.__radd__'])
self.assertEqual(op_sequence(add, B, A), ['B.__add__', 'A.__radd__'])
self.assertEqual(op_sequence(add, A, C), ['C.__radd__', 'A.__add__'])
self.assertEqual(op_sequence(add, C, A), ['C.__add__', 'A.__radd__'])


class SupEq(object):
"""Class that can test equality"""
def __eq__(self, other):
Expand Down
12 changes: 0 additions & 12 deletions Lib/test/test_descr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4111,18 +4111,6 @@ def __rfloordiv__(self, other):
self.assertEqual(D() // C(), "D.__floordiv__")
self.assertEqual(C() // D(), "D.__rfloordiv__")

# Case 4: this didn't work right in 2.2.2 and 2.3a1

class E(C):
pass

self.assertEqual(E.__rfloordiv__, C.__rfloordiv__)

self.assertEqual(E() // 1, "C.__floordiv__")
self.assertEqual(1 // E(), "C.__rfloordiv__")
self.assertEqual(E() // C(), "C.__floordiv__")
self.assertEqual(C() // E(), "C.__floordiv__") # This one would fail

@JimJJewett JimJJewett Nov 1, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these being taken out? I don't see div or floordiv in the above, so it doesn't look like it is just being tested differently (unless the tests were really redundant or something).

@shoyer shoyer Nov 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests here seemed redudant with the tests I added for add above, since the logic is not operator specific. I removed these specific testa because they were verifying the old (changed) behavior, but I could also keep them in and change to the new behavior.


@support.impl_detail("testing an internal kind of method object")
def test_meth_class_get(self):
# Testing __get__ method of METH_CLASS C methods...
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2017-09-05-14-54-30.bpo-30140.fnPWpS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix binary operator dispatch to try subclasses implementations first always,
even if the subclass does not directly override the relevant special method.
38 changes: 1 addition & 37 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5841,41 +5841,6 @@ FUNCNAME(PyObject *self, ARG1TYPE arg1) \
return call_method(self, &id, stack, 1); \
}

/* Boolean helper for SLOT1BINFULL().
right.__class__ is a nontrivial subclass of left.__class__. */
static int
method_is_overloaded(PyObject *left, PyObject *right, struct _Py_Identifier *name)
{
PyObject *a, *b;
int ok;

b = _PyObject_GetAttrId((PyObject *)(Py_TYPE(right)), name);
if (b == NULL) {
PyErr_Clear();
/* If right doesn't have it, it's not overloaded */
return 0;
}

a = _PyObject_GetAttrId((PyObject *)(Py_TYPE(left)), name);
if (a == NULL) {
PyErr_Clear();
Py_DECREF(b);
/* If right has it but left doesn't, it's overloaded */
return 1;
}

ok = PyObject_RichCompareBool(a, b, Py_NE);
Py_DECREF(a);
Py_DECREF(b);
if (ok < 0) {
PyErr_Clear();
return 0;
}

return ok;
}


#define SLOT1BINFULL(FUNCNAME, TESTFUNC, SLOTNAME, OPSTR, ROPSTR) \
static PyObject * \
FUNCNAME(PyObject *self, PyObject *other) \
Expand All @@ -5890,8 +5855,7 @@ FUNCNAME(PyObject *self, PyObject *other) \
Py_TYPE(self)->tp_as_number->SLOTNAME == TESTFUNC) { \
PyObject *r; \
if (do_other && \
PyType_IsSubtype(Py_TYPE(other), Py_TYPE(self)) && \
method_is_overloaded(self, other, &rop_id)) { \
PyType_IsSubtype(Py_TYPE(other), Py_TYPE(self))) { \
stack[0] = self; \
r = call_maybe(other, &rop_id, stack, 1); \
if (r != Py_NotImplemented) \
Expand Down