Skip to content

Commit

Permalink
bpo-43977: Properly update the tp_flags of existing subclasses when t…
Browse files Browse the repository at this point in the history
…heir parents are registered (pythonGH-26864)
  • Loading branch information
brandtbucher authored Jun 25, 2021
1 parent 22e7eff commit ca2009d
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 34 deletions.
13 changes: 8 additions & 5 deletions Doc/library/dis.rst
Original file line number Diff line number Diff line change
Expand Up @@ -772,17 +772,20 @@ iterations of the loop.

.. opcode:: MATCH_MAPPING

If TOS is an instance of :class:`collections.abc.Mapping`, push ``True`` onto
the stack. Otherwise, push ``False``.
If TOS is an instance of :class:`collections.abc.Mapping` (or, more technically: if
it has the :const:`Py_TPFLAGS_MAPPING` flag set in its
:c:member:`~PyTypeObject.tp_flags`), push ``True`` onto the stack. Otherwise, push
``False``.

.. versionadded:: 3.10


.. opcode:: MATCH_SEQUENCE

If TOS is an instance of :class:`collections.abc.Sequence` and is *not* an
instance of :class:`str`/:class:`bytes`/:class:`bytearray`, push ``True``
onto the stack. Otherwise, push ``False``.
If TOS is an instance of :class:`collections.abc.Sequence` and is *not* an instance
of :class:`str`/:class:`bytes`/:class:`bytearray` (or, more technically: if it has
the :const:`Py_TPFLAGS_SEQUENCE` flag set in its :c:member:`~PyTypeObject.tp_flags`),
push ``True`` onto the stack. Otherwise, push ``False``.

.. versionadded:: 3.10

Expand Down
110 changes: 87 additions & 23 deletions Lib/test/test_patma.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,43 @@ def test_refleaks(self):

class TestInheritance(unittest.TestCase):

def test_multiple_inheritance(self):
@staticmethod
def check_sequence_then_mapping(x):
match x:
case [*_]:
return "seq"
case {}:
return "map"

@staticmethod
def check_mapping_then_sequence(x):
match x:
case {}:
return "map"
case [*_]:
return "seq"

def test_multiple_inheritance_mapping(self):
class C:
pass
class M1(collections.UserDict, collections.abc.Sequence):
pass
class M2(C, collections.UserDict, collections.abc.Sequence):
pass
class M3(collections.UserDict, C, list):
pass
class M4(dict, collections.abc.Sequence, C):
pass
self.assertEqual(self.check_sequence_then_mapping(M1()), "map")
self.assertEqual(self.check_sequence_then_mapping(M2()), "map")
self.assertEqual(self.check_sequence_then_mapping(M3()), "map")
self.assertEqual(self.check_sequence_then_mapping(M4()), "map")
self.assertEqual(self.check_mapping_then_sequence(M1()), "map")
self.assertEqual(self.check_mapping_then_sequence(M2()), "map")
self.assertEqual(self.check_mapping_then_sequence(M3()), "map")
self.assertEqual(self.check_mapping_then_sequence(M4()), "map")

def test_multiple_inheritance_sequence(self):
class C:
pass
class S1(collections.UserList, collections.abc.Mapping):
Expand All @@ -35,32 +71,60 @@ class S3(list, C, collections.abc.Mapping):
pass
class S4(collections.UserList, dict, C):
pass
class M1(collections.UserDict, collections.abc.Sequence):
self.assertEqual(self.check_sequence_then_mapping(S1()), "seq")
self.assertEqual(self.check_sequence_then_mapping(S2()), "seq")
self.assertEqual(self.check_sequence_then_mapping(S3()), "seq")
self.assertEqual(self.check_sequence_then_mapping(S4()), "seq")
self.assertEqual(self.check_mapping_then_sequence(S1()), "seq")
self.assertEqual(self.check_mapping_then_sequence(S2()), "seq")
self.assertEqual(self.check_mapping_then_sequence(S3()), "seq")
self.assertEqual(self.check_mapping_then_sequence(S4()), "seq")

def test_late_registration_mapping(self):
class Parent:
pass
class M2(C, collections.UserDict, collections.abc.Sequence):
class ChildPre(Parent):
pass
class M3(collections.UserDict, C, list):
class GrandchildPre(ChildPre):
pass
class M4(dict, collections.abc.Sequence, C):
collections.abc.Mapping.register(Parent)
class ChildPost(Parent):
pass
def f(x):
match x:
case []:
return "seq"
case {}:
return "map"
def g(x):
match x:
case {}:
return "map"
case []:
return "seq"
for Seq in (S1, S2, S3, S4):
self.assertEqual(f(Seq()), "seq")
self.assertEqual(g(Seq()), "seq")
for Map in (M1, M2, M3, M4):
self.assertEqual(f(Map()), "map")
self.assertEqual(g(Map()), "map")
class GrandchildPost(ChildPost):
pass
self.assertEqual(self.check_sequence_then_mapping(Parent()), "map")
self.assertEqual(self.check_sequence_then_mapping(ChildPre()), "map")
self.assertEqual(self.check_sequence_then_mapping(GrandchildPre()), "map")
self.assertEqual(self.check_sequence_then_mapping(ChildPost()), "map")
self.assertEqual(self.check_sequence_then_mapping(GrandchildPost()), "map")
self.assertEqual(self.check_mapping_then_sequence(Parent()), "map")
self.assertEqual(self.check_mapping_then_sequence(ChildPre()), "map")
self.assertEqual(self.check_mapping_then_sequence(GrandchildPre()), "map")
self.assertEqual(self.check_mapping_then_sequence(ChildPost()), "map")
self.assertEqual(self.check_mapping_then_sequence(GrandchildPost()), "map")

def test_late_registration_sequence(self):
class Parent:
pass
class ChildPre(Parent):
pass
class GrandchildPre(ChildPre):
pass
collections.abc.Sequence.register(Parent)
class ChildPost(Parent):
pass
class GrandchildPost(ChildPost):
pass
self.assertEqual(self.check_sequence_then_mapping(Parent()), "seq")
self.assertEqual(self.check_sequence_then_mapping(ChildPre()), "seq")
self.assertEqual(self.check_sequence_then_mapping(GrandchildPre()), "seq")
self.assertEqual(self.check_sequence_then_mapping(ChildPost()), "seq")
self.assertEqual(self.check_sequence_then_mapping(GrandchildPost()), "seq")
self.assertEqual(self.check_mapping_then_sequence(Parent()), "seq")
self.assertEqual(self.check_mapping_then_sequence(ChildPre()), "seq")
self.assertEqual(self.check_mapping_then_sequence(GrandchildPre()), "seq")
self.assertEqual(self.check_mapping_then_sequence(ChildPost()), "seq")
self.assertEqual(self.check_mapping_then_sequence(GrandchildPost()), "seq")


class TestPatma(unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Set the proper :const:`Py_TPFLAGS_MAPPING` and :const:`Py_TPFLAGS_SEQUENCE`
flags for subclasses created before a parent has been registered as a
:class:`collections.abc.Mapping` or :class:`collections.abc.Sequence`.
37 changes: 31 additions & 6 deletions Modules/_abc.c
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,32 @@ _abc__abc_init(PyObject *module, PyObject *self)
Py_RETURN_NONE;
}

static void
set_collection_flag_recursive(PyTypeObject *child, unsigned long flag)
{
assert(flag == Py_TPFLAGS_MAPPING || flag == Py_TPFLAGS_SEQUENCE);
if (PyType_HasFeature(child, Py_TPFLAGS_IMMUTABLETYPE) ||
(child->tp_flags & COLLECTION_FLAGS) == flag)
{
return;
}
child->tp_flags &= ~COLLECTION_FLAGS;
child->tp_flags |= flag;
PyObject *grandchildren = child->tp_subclasses;
if (grandchildren == NULL) {
return;
}
assert(PyDict_CheckExact(grandchildren));
Py_ssize_t i = 0;
while (PyDict_Next(grandchildren, &i, NULL, &grandchildren)) {
assert(PyWeakref_CheckRef(grandchildren));
PyObject *grandchild = PyWeakref_GET_OBJECT(grandchildren);
if (PyType_Check(grandchild)) {
set_collection_flag_recursive((PyTypeObject *)grandchild, flag);
}
}
}

/*[clinic input]
_abc._abc_register
Expand Down Expand Up @@ -532,12 +558,11 @@ _abc__abc_register_impl(PyObject *module, PyObject *self, PyObject *subclass)
get_abc_state(module)->abc_invalidation_counter++;

/* Set Py_TPFLAGS_SEQUENCE or Py_TPFLAGS_MAPPING flag */
if (PyType_Check(self) &&
!PyType_HasFeature((PyTypeObject *)subclass, Py_TPFLAGS_IMMUTABLETYPE) &&
((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS)
{
((PyTypeObject *)subclass)->tp_flags &= ~COLLECTION_FLAGS;
((PyTypeObject *)subclass)->tp_flags |= (((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS);
if (PyType_Check(self)) {
unsigned long collection_flag = ((PyTypeObject *)self)->tp_flags & COLLECTION_FLAGS;
if (collection_flag) {
set_collection_flag_recursive((PyTypeObject *)subclass, collection_flag);
}
}
Py_INCREF(subclass);
return subclass;
Expand Down

0 comments on commit ca2009d

Please sign in to comment.