From da396cc55067418a1bd4d14fef4b6589a142616f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 3 Jul 2024 14:19:03 +0200
Subject: [PATCH 01/17] fix a crash in `OrderedDict`

---
 Lib/collections/__init__.py | 34 +++++++++++++++++++++++++++++++++-
 Objects/odictobject.c       | 27 +++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/Lib/collections/__init__.py b/Lib/collections/__init__.py
index b47e728484c8ac..3e2bc1e6d56b8a 100644
--- a/Lib/collections/__init__.py
+++ b/Lib/collections/__init__.py
@@ -103,6 +103,7 @@ def __new__(cls, /, *args, **kwds):
         self.__root = root = _proxy(self.__hardroot)
         root.prev = root.next = root
         self.__map = {}
+        self.__state = 0
         return self
 
     def __init__(self, other=(), /, **kwds):
@@ -123,6 +124,7 @@ def __setitem__(self, key, value,
             link.prev, link.next, link.key = last, root, key
             last.next = link
             root.prev = proxy(link)
+            self.__state += 1
         dict_setitem(self, key, value)
 
     def __delitem__(self, key, dict_delitem=dict.__delitem__):
@@ -137,6 +139,7 @@ def __delitem__(self, key, dict_delitem=dict.__delitem__):
         link_next.prev = link_prev
         link.prev = None
         link.next = None
+        self.__state += 1
 
     def __iter__(self):
         'od.__iter__() <==> iter(od)'
@@ -160,6 +163,7 @@ def clear(self):
         'od.clear() -> None.  Remove all items from od.'
         root = self.__root
         root.prev = root.next = root
+        self.__state += 1
         self.__map.clear()
         dict.clear(self)
 
@@ -184,6 +188,7 @@ def popitem(self, last=True):
         key = link.key
         del self.__map[key]
         value = dict.pop(self, key)
+        self.__state += 1
         return key, value
 
     def move_to_end(self, key, last=True):
@@ -210,6 +215,7 @@ def move_to_end(self, key, last=True):
             link.next = first
             first.prev = soft_link
             root.next = link
+        self.__state += 1
 
     def __sizeof__(self):
         sizeof = _sys.getsizeof
@@ -218,6 +224,7 @@ def __sizeof__(self):
         size += sizeof(self.__map) * 2          # internal dict and inherited dict
         size += sizeof(self.__hardroot) * n     # link objects
         size += sizeof(self.__root) * n         # proxy objects
+        size += sizeof(self.__state)            # linked list state
         return size
 
     update = __update = _collections_abc.MutableMapping.update
@@ -255,6 +262,7 @@ def pop(self, key, default=__marker):
             link_next.prev = link_prev
             link.prev = None
             link.next = None
+            self.__state += 1
             return result
         if default is marker:
             raise KeyError(key)
@@ -314,8 +322,32 @@ def __eq__(self, other):
         while comparison to a regular mapping is order-insensitive.
 
         '''
+        # The Python implementation is not optimal but matches
+        # the C implementation and the exceptions it may raise.
         if isinstance(other, OrderedDict):
-            return dict.__eq__(self, other) and all(map(_eq, self, other))
+            if not dict.__eq__(self, other):
+                return False
+            state_a, count_a = self.__state, len(self)
+            root_a, curr_a = self.__root, self.__root.next
+            state_b, count_b = other.__state, len(other)
+            root_b, curr_b = other.__root, other.__root.next
+            while True:
+                if curr_a is root_a and curr_b is root_b:
+                    return True
+                if curr_a is root_a or curr_b is root_b:
+                    return False
+                # With the C implementation, calling '==' might have side
+                # effects that would end in segmentation faults, thus the
+                # state and size of the operands need to be checked.
+                res = curr_a.key == curr_b.key
+                if self.__state != state_a or other.__state != state_b:
+                    # changing the size always changes the state
+                    if len(self) != count_a or len(other) != count_b:
+                        raise RuntimeError("OrderedDict changed size during iteration")
+                    raise RuntimeError("OrderedDict mutated during iteration")
+                elif not res:
+                    return False
+                curr_a, curr_b = curr_a.next, curr_b.next
         return dict.__eq__(self, other)
 
     def __ior__(self, other):
diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index 30277aa0c23883..dcf52d76d0287e 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -699,7 +699,7 @@ _odict_add_new_node(PyODictObject *od, PyObject *key, Py_hash_t hash)
 
     _odictnode_KEY(node) = key;
     _odictnode_HASH(node) = hash;
-    _odict_add_tail(od, node);
+    _odict_add_tail(od, node); // this updates 'od_state'
     od->od_fast_nodes[i] = node;
     return 0;
 }
@@ -773,7 +773,7 @@ _odict_clear_node(PyODictObject *od, _ODictNode *node, PyObject *key,
 
     // Now clear the node.
     od->od_fast_nodes[i] = NULL;
-    _odict_remove_node(od, node);
+    _odict_remove_node(od, node); // this updates 'od_state'
     _odictnode_DEALLOC(node);
     return 0;
 }
@@ -796,6 +796,7 @@ _odict_clear_nodes(PyODictObject *od)
         _odictnode_DEALLOC(node);
         node = next;
     }
+    od->od_state++;
 }
 
 /* There isn't any memory management of nodes past this point. */
@@ -806,6 +807,12 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
 {
     _ODictNode *node_a, *node_b;
 
+    // keep operands' state and size to detect undesired mutations
+    const size_t state_a = a->od_state;
+    const Py_ssize_t size_a = PyODict_SIZE(a);
+    const size_t state_b = b->od_state;
+    const Py_ssize_t size_b = PyODict_SIZE(b);
+
     node_a = _odict_FIRST(a);
     node_b = _odict_FIRST(b);
     while (1) {
@@ -820,10 +827,22 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
                                             (PyObject *)_odictnode_KEY(node_a),
                                             (PyObject *)_odictnode_KEY(node_b),
                                             Py_EQ);
-            if (res < 0)
+            if (res < 0) {
                 return res;
-            else if (res == 0)
+            }
+            else if (a->od_state != state_a || b->od_state != state_b) {
+                // If the size changed, then the state must also have changed
+                // since the former changes only if keys are added or removed,
+                // which in turn updates the state.
+                PyErr_SetString(PyExc_RuntimeError,
+                                (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
+                                    ? "OrderedDict changed size during iteration"
+                                    : "OrderedDict mutated during iteration");
+                return -1;
+            }
+            else if (res == 0) {
                 return 0;
+            }
 
             /* otherwise it must match, so move on to the next one */
             node_a = _odictnode_NEXT(node_a);

From 6cdabc6296e76cd81b51549449da8cb546093928 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 3 Jul 2024 14:19:05 +0200
Subject: [PATCH 02/17] add tests

---
 Lib/test/test_ordered_dict.py | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index 06a0e81227188c..c807eebcf85f6f 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -2,7 +2,9 @@
 import contextlib
 import copy
 import gc
+import operator
 import pickle
+import re
 from random import randrange, shuffle
 import struct
 import sys
@@ -612,6 +614,44 @@ def test_issue24667(self):
                 key = c0 + c1
                 od[key] = key
 
+    def test_issue119004_change_size(self):
+        count = 0
+        class ChangeExternSize:
+            def __eq__(self, other):
+                nonlocal count
+                if count == 1:
+                    l.clear()
+                count += 1
+                return True
+            def __hash__(self):
+                return 3
+
+        l0, r0 = ChangeExternSize(), ChangeExternSize()
+        l = self.OrderedDict({l0: 1, 2: 3})
+        r = self.OrderedDict({r0: 1, 2: 3})
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, l, r)
+
+    def test_issue119004_change_item(self):
+        count = 0
+        class ChangeExternItem:
+            def __eq__(self, other):
+                nonlocal count
+                if count == 1:
+                    # change the layout of the underlying linked list
+                    del lhs[0]
+                    lhs[object()] = object()
+                count += 1
+                return True
+            def __hash__(self):
+                return 3
+
+        l1, r1 = ChangeExternItem(), ChangeExternItem()
+        lhs = self.OrderedDict(dict.fromkeys((0, l1)))
+        rhs = self.OrderedDict(dict.fromkeys((0, r1)))
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+
     # Direct use of dict methods
 
     def test_dict_setitem(self):

From 3a374dc69a8579f30de977cd87501a9345aabd11 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 3 Jul 2024 14:23:37 +0200
Subject: [PATCH 03/17] update docs

---
 Doc/library/collections.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Doc/library/collections.rst b/Doc/library/collections.rst
index ce89101d6b667c..787e19e8980850 100644
--- a/Doc/library/collections.rst
+++ b/Doc/library/collections.rst
@@ -1169,8 +1169,10 @@ Some differences from :class:`dict` still remain:
 In addition to the usual mapping methods, ordered dictionaries also support
 reverse iteration using :func:`reversed`.
 
+.. _collections_OrderedDict__eq__:
+
 Equality tests between :class:`OrderedDict` objects are order-sensitive
-and are implemented as ``list(od1.items())==list(od2.items())``.
+and are roughly equivalent to ``list(od1.items())==list(od2.items())``.
 Equality tests between :class:`OrderedDict` objects and other
 :class:`~collections.abc.Mapping` objects are order-insensitive like regular
 dictionaries.  This allows :class:`OrderedDict` objects to be substituted

From 5c7bccc28232d39f7c27b053b884e2e7e98a83cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 3 Jul 2024 14:23:39 +0200
Subject: [PATCH 04/17] blurb

---
 .../next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst

diff --git a/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst b/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst
new file mode 100644
index 00000000000000..03e01dfcae43f8
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst
@@ -0,0 +1,2 @@
+Fix a crash in :ref:`OrderedDict.__eq__ <collections_OrderedDict__eq__>`
+when operands are mutated during the check.  Patch by Bénédikt Tran.

From 24f0807a4a0dfd5d1b4d215ca5fd3eac03d575c8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 4 Jul 2024 15:08:02 +0200
Subject: [PATCH 05/17] update docs

---
 Doc/library/collections.rst | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Doc/library/collections.rst b/Doc/library/collections.rst
index 787e19e8980850..1a5467a4074b22 100644
--- a/Doc/library/collections.rst
+++ b/Doc/library/collections.rst
@@ -1173,6 +1173,7 @@ reverse iteration using :func:`reversed`.
 
 Equality tests between :class:`OrderedDict` objects are order-sensitive
 and are roughly equivalent to ``list(od1.items())==list(od2.items())``.
+
 Equality tests between :class:`OrderedDict` objects and other
 :class:`~collections.abc.Mapping` objects are order-insensitive like regular
 dictionaries.  This allows :class:`OrderedDict` objects to be substituted
@@ -1188,7 +1189,11 @@ anywhere a regular dictionary is used.
    method.
 
 .. versionchanged:: 3.9
-    Added merge (``|``) and update (``|=``) operators, specified in :pep:`584`.
+   Added merge (``|``) and update (``|=``) operators, specified in :pep:`584`.
+
+.. versionchanged:: 3.14
+   Mutating :class:`OrderedDict` objects during an equality comparison raises
+   a :exc:`RuntimeError`.
 
 
 :class:`OrderedDict` Examples and Recipes

From 2f29ca6cca1665eed6778a69d00fd72eae00e53b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 4 Jul 2024 15:08:13 +0200
Subject: [PATCH 06/17] improve C implementation

---
 Objects/odictobject.c | 68 ++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index dcf52d76d0287e..eba576db580189 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -808,47 +808,49 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
     _ODictNode *node_a, *node_b;
 
     // keep operands' state and size to detect undesired mutations
-    const size_t state_a = a->od_state;
     const Py_ssize_t size_a = PyODict_SIZE(a);
-    const size_t state_b = b->od_state;
     const Py_ssize_t size_b = PyODict_SIZE(b);
+    if (size_a == -1 || size_b == -1) {
+        return -1;
+    }
+    if (size_a != size_b) {
+        // dictionaries with different number of keys can never be equal
+        return 0;
+    }
+
+    const size_t state_a = a->od_state;
+    const size_t state_b = b->od_state;
 
     node_a = _odict_FIRST(a);
     node_b = _odict_FIRST(b);
-    while (1) {
-        if (node_a == NULL && node_b == NULL)
-            /* success: hit the end of each at the same time */
-            return 1;
-        else if (node_a == NULL || node_b == NULL)
-            /* unequal length */
+    while (node_a != NULL && node_b != NULL) {
+        int res = PyObject_RichCompareBool((PyObject *)_odictnode_KEY(node_a),
+                                           (PyObject *)_odictnode_KEY(node_b),
+                                           Py_EQ);
+        if (res < 0) {
+            return res;
+        }
+        else if (a->od_state != state_a || b->od_state != state_b) {
+            // If the size changed, then the state must also have changed
+            // since the former changes only if keys are added or removed,
+            // which in turn updates the state.
+            PyErr_SetString(PyExc_RuntimeError,
+                            (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
+                                ? "OrderedDict changed size during iteration"
+                                : "OrderedDict mutated during iteration");
+            return -1;
+        }
+        else if (res == 0) {
             return 0;
-        else {
-            int res = PyObject_RichCompareBool(
-                                            (PyObject *)_odictnode_KEY(node_a),
-                                            (PyObject *)_odictnode_KEY(node_b),
-                                            Py_EQ);
-            if (res < 0) {
-                return res;
-            }
-            else if (a->od_state != state_a || b->od_state != state_b) {
-                // If the size changed, then the state must also have changed
-                // since the former changes only if keys are added or removed,
-                // which in turn updates the state.
-                PyErr_SetString(PyExc_RuntimeError,
-                                (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
-                                    ? "OrderedDict changed size during iteration"
-                                    : "OrderedDict mutated during iteration");
-                return -1;
-            }
-            else if (res == 0) {
-                return 0;
-            }
-
-            /* otherwise it must match, so move on to the next one */
-            node_a = _odictnode_NEXT(node_a);
-            node_b = _odictnode_NEXT(node_b);
         }
+
+        /* otherwise it must match, so move on to the next one */
+        node_a = _odictnode_NEXT(node_a);
+        node_b = _odictnode_NEXT(node_b);
     }
+    assert(node_a == NULL && node_b == NULL);
+    /* success: hit the end of each at the same time */
+    return 1;
 }
 
 

From dad3ef1917e31823907b6606b61deaf2a3f07a2a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 4 Jul 2024 15:08:20 +0200
Subject: [PATCH 07/17] improve Python implementation

---
 Lib/collections/__init__.py | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/Lib/collections/__init__.py b/Lib/collections/__init__.py
index 3e2bc1e6d56b8a..75addf0ee82eab 100644
--- a/Lib/collections/__init__.py
+++ b/Lib/collections/__init__.py
@@ -327,15 +327,16 @@ def __eq__(self, other):
         if isinstance(other, OrderedDict):
             if not dict.__eq__(self, other):
                 return False
-            state_a, count_a = self.__state, len(self)
-            root_a, curr_a = self.__root, self.__root.next
-            state_b, count_b = other.__state, len(other)
-            root_b, curr_b = other.__root, other.__root.next
-            while True:
-                if curr_a is root_a and curr_b is root_b:
-                    return True
-                if curr_a is root_a or curr_b is root_b:
-                    return False
+
+            count_a, count_b = len(self), len(other)
+            if count_a != count_b:
+                return False
+
+            state_a, state_b = self.__state, other.__state
+            root_a, root_b = self.__root, other.__root
+            curr_a, curr_b = root_a.next, root_b.next
+
+            while (curr_a is not root_a and curr_b is not root_b):
                 # With the C implementation, calling '==' might have side
                 # effects that would end in segmentation faults, thus the
                 # state and size of the operands need to be checked.
@@ -348,6 +349,9 @@ def __eq__(self, other):
                 elif not res:
                     return False
                 curr_a, curr_b = curr_a.next, curr_b.next
+            # we stopped simultaneously (size was unchanged)
+            assert curr_a is root_a and curr_b is root_b
+            return True
         return dict.__eq__(self, other)
 
     def __ior__(self, other):

From 5b8e4a5f2009e4030ca0b5d72b9859074afdaa16 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 4 Jul 2024 15:08:22 +0200
Subject: [PATCH 08/17] update tests

---
 Lib/test/test_ordered_dict.py | 46 +++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index c807eebcf85f6f..0f6167f739fe6a 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -5,10 +5,12 @@
 import operator
 import pickle
 import re
+import types
 from random import randrange, shuffle
 import struct
 import sys
 import unittest
+import unittest.mock
 import weakref
 from collections.abc import MutableMapping
 from test import mapping_tests, support
@@ -620,17 +622,26 @@ class ChangeExternSize:
             def __eq__(self, other):
                 nonlocal count
                 if count == 1:
-                    l.clear()
+                    lhs.clear()
+                    rhs.clear()
                 count += 1
                 return True
             def __hash__(self):
-                return 3
+                return -1
 
-        l0, r0 = ChangeExternSize(), ChangeExternSize()
-        l = self.OrderedDict({l0: 1, 2: 3})
-        r = self.OrderedDict({r0: 1, 2: 3})
+        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
+        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
         msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, l, r)
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        # There are two calls to ChangeExternSize.__eq__: one when doing
+        # value comparisons (without order) by dict.__eq__ and one when
+        # doing key comparisons (for the order).
+        self.assertEqual(count, 2)
+        with unittest.mock.patch.object(ChangeExternSize, '__eq__') as fn:
+            self.assertDictEqual(dict(lhs), {})
+            fn.assert_not_called()
+            self.assertDictEqual(dict(rhs), {})
+            fn.assert_not_called()
 
     def test_issue119004_change_item(self):
         count = 0
@@ -638,19 +649,28 @@ class ChangeExternItem:
             def __eq__(self, other):
                 nonlocal count
                 if count == 1:
-                    # change the layout of the underlying linked list
-                    del lhs[0]
-                    lhs[object()] = object()
+                    # change the layout of the underlying linked list,
+                    # but only in the second call (not in the first call)
+                    del lhs[self], rhs[other]
+                    lhs['a'] = rhs['b'] = 'c'
                 count += 1
                 return True
             def __hash__(self):
-                return 3
+                return -1
 
-        l1, r1 = ChangeExternItem(), ChangeExternItem()
-        lhs = self.OrderedDict(dict.fromkeys((0, l1)))
-        rhs = self.OrderedDict(dict.fromkeys((0, r1)))
+        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
+        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
         msg = re.escape("OrderedDict mutated during iteration")
         self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        # There are two calls to ChangeExternItem.__eq__: one when doing
+        # value comparisons (without order) by dict.__eq__ and one when
+        # doing key comparisons (for the order).
+        self.assertEqual(count, 2)
+        with unittest.mock.patch.object(ChangeExternItem, '__eq__') as fn:
+            self.assertDictEqual(dict(lhs), {0: None, 'a': 'c'})
+            fn.assert_not_called()
+            self.assertDictEqual(dict(rhs), {0: None, 'b': 'c'})
+            fn.assert_not_called()
 
     # Direct use of dict methods
 

From a2eef07a5b70b56766acf964d13fae247a891f01 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 4 Jul 2024 15:09:43 +0200
Subject: [PATCH 09/17] remove unused imports

---
 Lib/test/test_ordered_dict.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index 0f6167f739fe6a..a534aee8395bac 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -5,7 +5,6 @@
 import operator
 import pickle
 import re
-import types
 from random import randrange, shuffle
 import struct
 import sys

From 9c16c24db7b980492176bb8b19712fed4fa18d7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 18 Jul 2024 15:11:23 +0200
Subject: [PATCH 10/17] address review

---
 Lib/collections/__init__.py   |  45 +----
 Lib/test/test_ordered_dict.py | 303 +++++++++++++++++++++++++++-------
 Objects/odictobject.c         |  64 +++----
 3 files changed, 287 insertions(+), 125 deletions(-)

diff --git a/Lib/collections/__init__.py b/Lib/collections/__init__.py
index 75addf0ee82eab..b68692d6e8253d 100644
--- a/Lib/collections/__init__.py
+++ b/Lib/collections/__init__.py
@@ -103,7 +103,6 @@ def __new__(cls, /, *args, **kwds):
         self.__root = root = _proxy(self.__hardroot)
         root.prev = root.next = root
         self.__map = {}
-        self.__state = 0
         return self
 
     def __init__(self, other=(), /, **kwds):
@@ -124,7 +123,6 @@ def __setitem__(self, key, value,
             link.prev, link.next, link.key = last, root, key
             last.next = link
             root.prev = proxy(link)
-            self.__state += 1
         dict_setitem(self, key, value)
 
     def __delitem__(self, key, dict_delitem=dict.__delitem__):
@@ -139,7 +137,6 @@ def __delitem__(self, key, dict_delitem=dict.__delitem__):
         link_next.prev = link_prev
         link.prev = None
         link.next = None
-        self.__state += 1
 
     def __iter__(self):
         'od.__iter__() <==> iter(od)'
@@ -163,7 +160,6 @@ def clear(self):
         'od.clear() -> None.  Remove all items from od.'
         root = self.__root
         root.prev = root.next = root
-        self.__state += 1
         self.__map.clear()
         dict.clear(self)
 
@@ -188,7 +184,6 @@ def popitem(self, last=True):
         key = link.key
         del self.__map[key]
         value = dict.pop(self, key)
-        self.__state += 1
         return key, value
 
     def move_to_end(self, key, last=True):
@@ -215,7 +210,6 @@ def move_to_end(self, key, last=True):
             link.next = first
             first.prev = soft_link
             root.next = link
-        self.__state += 1
 
     def __sizeof__(self):
         sizeof = _sys.getsizeof
@@ -224,7 +218,6 @@ def __sizeof__(self):
         size += sizeof(self.__map) * 2          # internal dict and inherited dict
         size += sizeof(self.__hardroot) * n     # link objects
         size += sizeof(self.__root) * n         # proxy objects
-        size += sizeof(self.__state)            # linked list state
         return size
 
     update = __update = _collections_abc.MutableMapping.update
@@ -262,7 +255,6 @@ def pop(self, key, default=__marker):
             link_next.prev = link_prev
             link.prev = None
             link.next = None
-            self.__state += 1
             return result
         if default is marker:
             raise KeyError(key)
@@ -322,36 +314,15 @@ def __eq__(self, other):
         while comparison to a regular mapping is order-insensitive.
 
         '''
-        # The Python implementation is not optimal but matches
-        # the C implementation and the exceptions it may raise.
+        # The Python implementation differs from the C implementation in the
+        # sense that it does not track mutations occurring in __eq__() of keys
+        # or values.
+        #
+        # Since it was decided not to change the Python implementation,
+        # calling ``del self[key]`` in the ``key.__class__.__eq__`` may
+        # raise an AttributeError during iteration.
         if isinstance(other, OrderedDict):
-            if not dict.__eq__(self, other):
-                return False
-
-            count_a, count_b = len(self), len(other)
-            if count_a != count_b:
-                return False
-
-            state_a, state_b = self.__state, other.__state
-            root_a, root_b = self.__root, other.__root
-            curr_a, curr_b = root_a.next, root_b.next
-
-            while (curr_a is not root_a and curr_b is not root_b):
-                # With the C implementation, calling '==' might have side
-                # effects that would end in segmentation faults, thus the
-                # state and size of the operands need to be checked.
-                res = curr_a.key == curr_b.key
-                if self.__state != state_a or other.__state != state_b:
-                    # changing the size always changes the state
-                    if len(self) != count_a or len(other) != count_b:
-                        raise RuntimeError("OrderedDict changed size during iteration")
-                    raise RuntimeError("OrderedDict mutated during iteration")
-                elif not res:
-                    return False
-                curr_a, curr_b = curr_a.next, curr_b.next
-            # we stopped simultaneously (size was unchanged)
-            assert curr_a is root_a and curr_b is root_b
-            return True
+            return dict.__eq__(self, other) and all(map(_eq, self, other))
         return dict.__eq__(self, other)
 
     def __ior__(self, other):
diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index a534aee8395bac..31ad4cdc0e4ce2 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -615,62 +615,6 @@ def test_issue24667(self):
                 key = c0 + c1
                 od[key] = key
 
-    def test_issue119004_change_size(self):
-        count = 0
-        class ChangeExternSize:
-            def __eq__(self, other):
-                nonlocal count
-                if count == 1:
-                    lhs.clear()
-                    rhs.clear()
-                count += 1
-                return True
-            def __hash__(self):
-                return -1
-
-        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
-        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
-        msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        # There are two calls to ChangeExternSize.__eq__: one when doing
-        # value comparisons (without order) by dict.__eq__ and one when
-        # doing key comparisons (for the order).
-        self.assertEqual(count, 2)
-        with unittest.mock.patch.object(ChangeExternSize, '__eq__') as fn:
-            self.assertDictEqual(dict(lhs), {})
-            fn.assert_not_called()
-            self.assertDictEqual(dict(rhs), {})
-            fn.assert_not_called()
-
-    def test_issue119004_change_item(self):
-        count = 0
-        class ChangeExternItem:
-            def __eq__(self, other):
-                nonlocal count
-                if count == 1:
-                    # change the layout of the underlying linked list,
-                    # but only in the second call (not in the first call)
-                    del lhs[self], rhs[other]
-                    lhs['a'] = rhs['b'] = 'c'
-                count += 1
-                return True
-            def __hash__(self):
-                return -1
-
-        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
-        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
-        msg = re.escape("OrderedDict mutated during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        # There are two calls to ChangeExternItem.__eq__: one when doing
-        # value comparisons (without order) by dict.__eq__ and one when
-        # doing key comparisons (for the order).
-        self.assertEqual(count, 2)
-        with unittest.mock.patch.object(ChangeExternItem, '__eq__') as fn:
-            self.assertDictEqual(dict(lhs), {0: None, 'a': 'c'})
-            fn.assert_not_called()
-            self.assertDictEqual(dict(rhs), {0: None, 'b': 'c'})
-            fn.assert_not_called()
-
     # Direct use of dict methods
 
     def test_dict_setitem(self):
@@ -799,11 +743,78 @@ def test_ordered_dict_items_result_gc(self):
         # when it's mutated and returned from __next__:
         self.assertTrue(gc.is_tracked(next(it)))
 
+
+class _TriggerSideEffectOnEqual:
+    count = 0   # number of calls to __eq__
+    trigger = 1 # count value when to trigger side effect
+    # reference to non-local objects (in the test body)
+    key_lhs = key_rhs = None
+    OrderedDict = None
+
+    def __init__(self, label=None):
+        self.label = label
+
+    def __repr__(self):
+        return f'Key({self.label})'
+
+    def __eq__(self, other):
+        if self.__class__.count == self.__class__.trigger:
+            self.__class__.side_effect()
+        self.__class__.count += 1
+        return True
+
+    def __hash__(self):
+        return -1
+
+    @classmethod
+    def side_effect(cls):
+        raise NotImplementedError
+
+    @classmethod
+    def setup(cls):
+        assert cls.OrderedDict is not None, "missing OrderedDict class"
+        assert cls.key_lhs is None, "cannot call setup twice on the same class"
+        cls.key_lhs = cls(label='a')
+        cls.key_rhs = cls(label='b')
+        lhs = cls.OrderedDict(dict.fromkeys((0, cls.key_lhs)))
+        rhs = cls.OrderedDict(dict.fromkeys((0, cls.key_rhs)))
+        return lhs, rhs
+
+    @classmethod
+    def teardown(cls):
+        cls.count = 0
+        cls.key_lhs = cls.key_rhs = None
+
+
 class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase):
 
     module = py_coll
     OrderedDict = py_coll.OrderedDict
 
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        class KeyClass(_TriggerSideEffectOnEqual):
+            OrderedDict = cls.OrderedDict
+        cls.KeyClass = KeyClass
+
+    def test_issue119004_attribute_error(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                del lhs[cls.key_lhs]
+
+        lhs, rhs = Key.setup()
+        # This causes an AttributeError due to the linked list being changed
+        msg = re.escape("'NoneType' object has no attribute 'key'")
+        self.assertRaisesRegex(AttributeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        with unittest.mock.patch.object(Key, 'side_effect') as fn:
+            self.assertDictEqual(lhs, {0: None})
+            fn.assert_not_called()
+            self.assertDictEqual(rhs, {0: None, Key(): None})
+            fn.assert_not_called()
+
 
 class CPythonBuiltinDictTests(unittest.TestCase):
     """Builtin dict preserves insertion order.
@@ -824,8 +835,186 @@ class CPythonBuiltinDictTests(unittest.TestCase):
 del method
 
 
+class CPythonOrderedDictSideEffects:
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        class KeyClass(_TriggerSideEffectOnEqual):
+            OrderedDict = cls.OrderedDict
+        cls.KeyClass = KeyClass
+
+    def check_side_effect_after_eq(self, key_class, actual, expect):
+        with unittest.mock.patch.object(key_class, 'side_effect') as fn:
+            self.assertDictEqual(actual, expect)
+            fn.assert_not_called()
+
+    def test_issue119004_change_size_by_clear(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                lhs.clear()
+                rhs.clear()
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        # There are two calls to ChangeExternSize.__eq__: one when doing
+        # value comparisons (without order) by dict.__eq__ and one when
+        # doing key comparisons (for the order).
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {})
+        self.check_side_effect_after_eq(Key, rhs, {})
+
+    def test_issue119004_change_size_by_clear_asymmetric(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                lhs.clear()
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {})
+        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+
+    def test_issue119004_change_size_by_delete_key(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                del lhs[cls.key_lhs]
+                del rhs[cls.key_rhs]
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {0: None})
+        self.check_side_effect_after_eq(Key, rhs, {0: None})
+
+    def test_issue119004_change_size_by_delete_key_asymmetric(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                del lhs[cls.key_lhs]
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {0: None})
+        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+
+    def test_issue119004_change_linked_list_by_clear(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                # change the layout of the underlying linked list
+                lhs.clear()
+                rhs.clear()
+                lhs['a'] = lhs['b'] = 'c'
+                rhs['a'] = rhs['b'] = 'c'
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        expect = {'a': 'c', 'b': 'c'}
+        self.check_side_effect_after_eq(Key, lhs, expect)
+        self.check_side_effect_after_eq(Key, rhs, expect)
+
+    def test_issue119004_change_linked_list_by_clear_asymmetric(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                # change the layout of the underlying linked list
+                lhs.clear()
+                lhs['a'] = lhs['b'] = 'c'
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        with unittest.mock.patch.object(Key, 'side_effect') as fn:
+            self.assertDictEqual(lhs, {'a': 'c', 'b': 'c'})
+            self.assertEqual(Key.count, 2)
+            fn.assert_not_called()
+            self.assertDictEqual(rhs, {0: None, Key(): None})
+            self.assertEqual(Key.count, 3)
+            fn.assert_not_called()
+
+    def test_issue119004_change_linked_list_by_delete_key(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                # change the layout of the underlying linked list
+                del lhs[cls.key_lhs]
+                del rhs[cls.key_rhs]
+                lhs['a'] = rhs['b'] = 'c'
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {0: None, 'a': 'c'})
+        self.check_side_effect_after_eq(Key, rhs, {0: None, 'b': 'c'})
+
+    def test_issue119004_change_linked_list_by_delete_key_asymmetric(self):
+        class Key(self.KeyClass):
+            @classmethod
+            def side_effect(cls):
+                # change the layout of the underlying linked list
+                del lhs[cls.key_lhs]
+                lhs['a'] = 'c'
+
+        lhs, rhs = Key.setup()
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        self.assertEqual(Key.count, 2)
+        self.check_side_effect_after_eq(Key, lhs, {0: None, 'a': 'c'})
+        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+
+    def test_issue119004_change_size_by_delete_key_in_dict_eq(self):
+        class Key(self.KeyClass):
+            trigger = 0
+            @classmethod
+            def side_effect(cls):
+                del lhs[cls.key_lhs]
+                del rhs[cls.key_rhs]
+
+        lhs, rhs = Key.setup()
+        self.assertEqual(Key.count, 0)
+        # the side effect is triggered in dict.__eq__ and modifies the length
+        self.assertNotEqual(lhs, rhs)
+        self.assertEqual(len(lhs), 1)
+        self.assertEqual(len(rhs), 1)
+        self.assertEqual(Key.count, 1)
+        self.check_side_effect_after_eq(Key, lhs, {0: None})
+        self.check_side_effect_after_eq(Key, rhs, {0: None})
+
+    def test_issue119004_change_size_by_delete_key_in_dict_eq_asymmetric(self):
+        class Key(self.KeyClass):
+            trigger = 0
+            @classmethod
+            def side_effect(cls):
+                del lhs[cls.key_lhs]
+
+        lhs, rhs = Key.setup()
+        self.assertEqual(Key.count, 0)
+        # the side effect is triggered in dict.__eq__ and modifies the length
+        self.assertNotEqual(lhs, rhs)
+        self.assertEqual(len(lhs), 1)
+        self.assertEqual(len(rhs), 2)
+        self.assertEqual(Key.count, 1)
+        self.check_side_effect_after_eq(Key, lhs, {0: None})
+        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+
+
 @unittest.skipUnless(c_coll, 'requires the C version of the collections module')
-class CPythonOrderedDictTests(OrderedDictTests, unittest.TestCase):
+class CPythonOrderedDictTests(OrderedDictTests,
+                              CPythonOrderedDictSideEffects,
+                              unittest.TestCase):
 
     module = c_coll
     OrderedDict = c_coll.OrderedDict
diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index eba576db580189..2feec606c9551b 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -810,47 +810,49 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
     // keep operands' state and size to detect undesired mutations
     const Py_ssize_t size_a = PyODict_SIZE(a);
     const Py_ssize_t size_b = PyODict_SIZE(b);
-    if (size_a == -1 || size_b == -1) {
-        return -1;
-    }
-    if (size_a != size_b) {
-        // dictionaries with different number of keys can never be equal
-        return 0;
-    }
-
     const size_t state_a = a->od_state;
     const size_t state_b = b->od_state;
 
     node_a = _odict_FIRST(a);
     node_b = _odict_FIRST(b);
-    while (node_a != NULL && node_b != NULL) {
-        int res = PyObject_RichCompareBool((PyObject *)_odictnode_KEY(node_a),
-                                           (PyObject *)_odictnode_KEY(node_b),
-                                           Py_EQ);
-        if (res < 0) {
-            return res;
+    while (1) {
+        if (node_a == NULL && node_b == NULL) {
+            /* success: hit the end of each at the same time */
+            return 1;
         }
-        else if (a->od_state != state_a || b->od_state != state_b) {
-            // If the size changed, then the state must also have changed
-            // since the former changes only if keys are added or removed,
-            // which in turn updates the state.
-            PyErr_SetString(PyExc_RuntimeError,
-                            (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
-                                ? "OrderedDict changed size during iteration"
-                                : "OrderedDict mutated during iteration");
-            return -1;
-        }
-        else if (res == 0) {
+        else if (node_a == NULL || node_b == NULL) {
+            /* unequal length */
             return 0;
         }
+        else {
+            PyObject *key_a = Py_NewRef(_odictnode_KEY(node_a));
+            PyObject *key_b = Py_NewRef(_odictnode_KEY(node_b));
+            int res = PyObject_RichCompareBool(key_a, key_b, Py_EQ);
+            Py_DECREF(key_a);
+            Py_DECREF(key_b);
+            if (res < 0) {
+                return res;
+            }
+            else if (a->od_state != state_a || b->od_state != state_b) {
+                // If the size changed, then the state must also have changed
+                // since the former changes only if keys are added or removed,
+                // which in turn updates the state.
+                PyErr_SetString(PyExc_RuntimeError,
+                                (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
+                                    ? "OrderedDict changed size during iteration"
+                                    : "OrderedDict mutated during iteration");
+                return -1;
+            }
+            else if (res == 0) {
+                // this check must come after the check on the state
+                return 0;
+            }
 
-        /* otherwise it must match, so move on to the next one */
-        node_a = _odictnode_NEXT(node_a);
-        node_b = _odictnode_NEXT(node_b);
+            /* otherwise it must match, so move on to the next one */
+            node_a = _odictnode_NEXT(node_a);
+            node_b = _odictnode_NEXT(node_b);
+        }
     }
-    assert(node_a == NULL && node_b == NULL);
-    /* success: hit the end of each at the same time */
-    return 1;
 }
 
 

From 8815a17e7d8b0db2fd3b41bde22965e0ea99a02d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 10:38:19 +0200
Subject: [PATCH 11/17] remove un-necessary comments

---
 Doc/library/collections.rst | 4 ----
 Lib/collections/__init__.py | 7 -------
 Objects/odictobject.c       | 4 ++--
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/Doc/library/collections.rst b/Doc/library/collections.rst
index 1a5467a4074b22..cee4e350c498fe 100644
--- a/Doc/library/collections.rst
+++ b/Doc/library/collections.rst
@@ -1191,10 +1191,6 @@ anywhere a regular dictionary is used.
 .. versionchanged:: 3.9
    Added merge (``|``) and update (``|=``) operators, specified in :pep:`584`.
 
-.. versionchanged:: 3.14
-   Mutating :class:`OrderedDict` objects during an equality comparison raises
-   a :exc:`RuntimeError`.
-
 
 :class:`OrderedDict` Examples and Recipes
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/Lib/collections/__init__.py b/Lib/collections/__init__.py
index b68692d6e8253d..b47e728484c8ac 100644
--- a/Lib/collections/__init__.py
+++ b/Lib/collections/__init__.py
@@ -314,13 +314,6 @@ def __eq__(self, other):
         while comparison to a regular mapping is order-insensitive.
 
         '''
-        # The Python implementation differs from the C implementation in the
-        # sense that it does not track mutations occurring in __eq__() of keys
-        # or values.
-        #
-        # Since it was decided not to change the Python implementation,
-        # calling ``del self[key]`` in the ``key.__class__.__eq__`` may
-        # raise an AttributeError during iteration.
         if isinstance(other, OrderedDict):
             return dict.__eq__(self, other) and all(map(_eq, self, other))
         return dict.__eq__(self, other)
diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index 2feec606c9551b..72c4e2d10cd2a3 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -699,7 +699,7 @@ _odict_add_new_node(PyODictObject *od, PyObject *key, Py_hash_t hash)
 
     _odictnode_KEY(node) = key;
     _odictnode_HASH(node) = hash;
-    _odict_add_tail(od, node); // this updates 'od_state'
+    _odict_add_tail(od, node);
     od->od_fast_nodes[i] = node;
     return 0;
 }
@@ -773,7 +773,7 @@ _odict_clear_node(PyODictObject *od, _ODictNode *node, PyObject *key,
 
     // Now clear the node.
     od->od_fast_nodes[i] = NULL;
-    _odict_remove_node(od, node); // this updates 'od_state'
+    _odict_remove_node(od, node);
     _odictnode_DEALLOC(node);
     return 0;
 }

From a7143418e01a1d447bd21338b4ae2a7928ec35bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 10:40:54 +0200
Subject: [PATCH 12/17] update exception message

---
 Objects/odictobject.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index 72c4e2d10cd2a3..233cdcdc03969f 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -807,9 +807,7 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
 {
     _ODictNode *node_a, *node_b;
 
-    // keep operands' state and size to detect undesired mutations
-    const Py_ssize_t size_a = PyODict_SIZE(a);
-    const Py_ssize_t size_b = PyODict_SIZE(b);
+    // keep operands' state to detect undesired mutations
     const size_t state_a = a->od_state;
     const size_t state_b = b->od_state;
 
@@ -834,17 +832,13 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
                 return res;
             }
             else if (a->od_state != state_a || b->od_state != state_b) {
-                // If the size changed, then the state must also have changed
-                // since the former changes only if keys are added or removed,
-                // which in turn updates the state.
                 PyErr_SetString(PyExc_RuntimeError,
-                                (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
-                                    ? "OrderedDict changed size during iteration"
-                                    : "OrderedDict mutated during iteration");
+                                "OrderedDict mutated during iteration");
                 return -1;
             }
             else if (res == 0) {
-                // this check must come after the check on the state
+                // This check comes after the check on the state
+                // in order for the exception to be set correctly.
                 return 0;
             }
 

From 65e6c7efa62bf0d1b62a4844764e3d444686f82c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 11:44:18 +0200
Subject: [PATCH 13/17] simplify tests

---
 Lib/test/test_ordered_dict.py | 313 +++++++++++++---------------------
 1 file changed, 123 insertions(+), 190 deletions(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index 31ad4cdc0e4ce2..51f68620dfaf00 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -747,73 +747,56 @@ def test_ordered_dict_items_result_gc(self):
 class _TriggerSideEffectOnEqual:
     count = 0   # number of calls to __eq__
     trigger = 1 # count value when to trigger side effect
-    # reference to non-local objects (in the test body)
-    key_lhs = key_rhs = None
     OrderedDict = None
-
-    def __init__(self, label=None):
-        self.label = label
-
-    def __repr__(self):
-        return f'Key({self.label})'
+    # reference to non-local objects (in the test body)
+    d1 = d2 = None  # d1, d2 are ordered dictionaries
+    k1 = k2 = None  # k1, k2 are instances of _TriggerSideEffectOnEqual()
 
     def __eq__(self, other):
         if self.__class__.count == self.__class__.trigger:
-            self.__class__.side_effect()
+            self.side_effect()
         self.__class__.count += 1
         return True
 
     def __hash__(self):
+        # all instances represent the same key
         return -1
 
-    @classmethod
-    def side_effect(cls):
+    def side_effect(self):
         raise NotImplementedError
 
     @classmethod
-    def setup(cls):
+    @contextlib.contextmanager
+    def test_context(cls):
         assert cls.OrderedDict is not None, "missing OrderedDict class"
-        assert cls.key_lhs is None, "cannot call setup twice on the same class"
-        cls.key_lhs = cls(label='a')
-        cls.key_rhs = cls(label='b')
-        lhs = cls.OrderedDict(dict.fromkeys((0, cls.key_lhs)))
-        rhs = cls.OrderedDict(dict.fromkeys((0, cls.key_rhs)))
-        return lhs, rhs
-
-    @classmethod
-    def teardown(cls):
-        cls.count = 0
-        cls.key_lhs = cls.key_rhs = None
 
+        try:
+            cls.k1, cls.k2 = k1, k2 = cls(), cls()
+            cls.d1 = d1 = cls.OrderedDict(dict.fromkeys((0, k1, 4.2)))
+            cls.d2 = d2 = cls.OrderedDict(dict.fromkeys((0, k2, 4.2)))
+            yield d1, d2
+        finally:
+            cls.count = 0
 
 class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase):
 
     module = py_coll
     OrderedDict = py_coll.OrderedDict
 
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        class KeyClass(_TriggerSideEffectOnEqual):
-            OrderedDict = cls.OrderedDict
-        cls.KeyClass = KeyClass
-
     def test_issue119004_attribute_error(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                del lhs[cls.key_lhs]
-
-        lhs, rhs = Key.setup()
-        # This causes an AttributeError due to the linked list being changed
-        msg = re.escape("'NoneType' object has no attribute 'key'")
-        self.assertRaisesRegex(AttributeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        with unittest.mock.patch.object(Key, 'side_effect') as fn:
-            self.assertDictEqual(lhs, {0: None})
-            fn.assert_not_called()
-            self.assertDictEqual(rhs, {0: None, Key(): None})
-            fn.assert_not_called()
+        class Key(_TriggerSideEffectOnEqual):
+            OrderedDict = self.OrderedDict
+
+            def side_effect(self):
+                del self.d1[self.k1]
+
+        with Key.test_context() as (d1, d2):
+            # This causes an AttributeError due to the linked list being changed
+            msg = re.escape("'NoneType' object has no attribute 'key'")
+            self.assertRaisesRegex(AttributeError, msg, operator.eq, d1, d2)
+            self.assertEqual(Key.count, 2)
+            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
+            self.assertDictEqual(d2, dict.fromkeys((0, Key(), 4.2)))
 
 
 class CPythonBuiltinDictTests(unittest.TestCase):
@@ -840,175 +823,125 @@ class CPythonOrderedDictSideEffects:
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
-        class KeyClass(_TriggerSideEffectOnEqual):
-            OrderedDict = cls.OrderedDict
-        cls.KeyClass = KeyClass
 
-    def check_side_effect_after_eq(self, key_class, actual, expect):
-        with unittest.mock.patch.object(key_class, 'side_effect') as fn:
-            self.assertDictEqual(actual, expect)
-            fn.assert_not_called()
+        class Key(_TriggerSideEffectOnEqual):
+            OrderedDict = cls.OrderedDict
+        cls.Key = Key
+
+    def _test_issue119004(self, keyclass, expect1, expect2):
+        assert issubclass(keyclass, _TriggerSideEffectOnEqual)
+        with keyclass.test_context() as (d1, d2):
+            msg = re.escape("OrderedDict mutated during iteration")
+            self.assertRaisesRegex(RuntimeError, msg, operator.eq, d1, d2)
+            # There are two calls to KeyClass.__eq__: one when doing
+            # value comparisons (without order) by dict.__eq__ and
+            # one when doing key comparisons (for the order).
+            self.assertEqual(keyclass.count, 2)
+            self.assertDictEqual(d1, expect1)
+            self.assertDictEqual(d2, expect2)
 
     def test_issue119004_change_size_by_clear(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                lhs.clear()
-                rhs.clear()
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        # There are two calls to ChangeExternSize.__eq__: one when doing
-        # value comparisons (without order) by dict.__eq__ and one when
-        # doing key comparisons (for the order).
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {})
-        self.check_side_effect_after_eq(Key, rhs, {})
+        class Key(self.Key):
+            def side_effect(self):
+                self.d1.clear()
+                self.d2.clear()
+
+        self._test_issue119004(Key, {}, {})
 
     def test_issue119004_change_size_by_clear_asymmetric(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                lhs.clear()
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {})
-        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+        class Key(self.Key):
+            def side_effect(self):
+                self.d1.clear()
+
+        expect2 = dict.fromkeys((0, Key(), 4.2))
+        self._test_issue119004(Key, {}, expect2)
 
     def test_issue119004_change_size_by_delete_key(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                del lhs[cls.key_lhs]
-                del rhs[cls.key_rhs]
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {0: None})
-        self.check_side_effect_after_eq(Key, rhs, {0: None})
+        class Key(self.Key):
+            def side_effect(self):
+                del self.d1[self.k1], self.d2[self.k2]
+
+        expect = dict.fromkeys((0, 4.2))
+        self._test_issue119004(Key, expect, expect)
 
     def test_issue119004_change_size_by_delete_key_asymmetric(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                del lhs[cls.key_lhs]
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict changed size during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {0: None})
-        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+        class Key(self.Key):
+            def side_effect(self):
+                del self.d1[self.k1]
+
+        expect1 = dict.fromkeys((0, 4.2))
+        expect2 = dict.fromkeys((0, Key(), 4.2))
+        self._test_issue119004(Key, expect1, expect2)
 
     def test_issue119004_change_linked_list_by_clear(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                # change the layout of the underlying linked list
-                lhs.clear()
-                rhs.clear()
-                lhs['a'] = lhs['b'] = 'c'
-                rhs['a'] = rhs['b'] = 'c'
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict mutated during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
+        class Key(self.Key):
+            def side_effect(self):
+                self.d1.clear()
+                self.d1['a'] = self.d1['b'] = 'c'
+                self.d2.clear()
+                self.d2['a'] = self.d2['b'] = 'c'
+
         expect = {'a': 'c', 'b': 'c'}
-        self.check_side_effect_after_eq(Key, lhs, expect)
-        self.check_side_effect_after_eq(Key, rhs, expect)
+        self._test_issue119004(Key, expect, expect)
 
     def test_issue119004_change_linked_list_by_clear_asymmetric(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                # change the layout of the underlying linked list
-                lhs.clear()
-                lhs['a'] = lhs['b'] = 'c'
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict mutated during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        with unittest.mock.patch.object(Key, 'side_effect') as fn:
-            self.assertDictEqual(lhs, {'a': 'c', 'b': 'c'})
-            self.assertEqual(Key.count, 2)
-            fn.assert_not_called()
-            self.assertDictEqual(rhs, {0: None, Key(): None})
-            self.assertEqual(Key.count, 3)
-            fn.assert_not_called()
+        class Key(self.Key):
+            def side_effect(self):
+                self.d1.clear()
+                self.d1['a'] = self.d1['b'] = 'c'
+
+        expect1 = dict.fromkeys(('a', 'b'), 'c')
+        expect2 = dict.fromkeys((0, Key(), 4.2))
+        self._test_issue119004(Key, expect1, expect2)
 
     def test_issue119004_change_linked_list_by_delete_key(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                # change the layout of the underlying linked list
-                del lhs[cls.key_lhs]
-                del rhs[cls.key_rhs]
-                lhs['a'] = rhs['b'] = 'c'
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict mutated during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {0: None, 'a': 'c'})
-        self.check_side_effect_after_eq(Key, rhs, {0: None, 'b': 'c'})
+        class Key(self.Key):
+            def side_effect(self):
+                del self.d1[self.k1], self.d2[self.k2]
+                self.d1['a'] = self.d2['b'] = 'c'
+
+        expect1 = {0: None, 'a': 'c', 4.2: None}
+        expect2 = {0: None, 'b': 'c', 4.2: None}
+        self._test_issue119004(Key, expect1, expect2)
 
     def test_issue119004_change_linked_list_by_delete_key_asymmetric(self):
-        class Key(self.KeyClass):
-            @classmethod
-            def side_effect(cls):
-                # change the layout of the underlying linked list
-                del lhs[cls.key_lhs]
-                lhs['a'] = 'c'
-
-        lhs, rhs = Key.setup()
-        msg = re.escape("OrderedDict mutated during iteration")
-        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
-        self.assertEqual(Key.count, 2)
-        self.check_side_effect_after_eq(Key, lhs, {0: None, 'a': 'c'})
-        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+        class Key(self.Key):
+            def side_effect(self):
+                del self.d1[self.k1]
+                self.d1['a'] = 'c'
+
+        expect1 = {0: None, 'a': 'c', 4.2: None}
+        expect2 = dict.fromkeys((0, Key(), 4.2))
+        self._test_issue119004(Key, expect1, expect2)
 
     def test_issue119004_change_size_by_delete_key_in_dict_eq(self):
-        class Key(self.KeyClass):
+        class Key(self.Key):
             trigger = 0
-            @classmethod
-            def side_effect(cls):
-                del lhs[cls.key_lhs]
-                del rhs[cls.key_rhs]
-
-        lhs, rhs = Key.setup()
-        self.assertEqual(Key.count, 0)
-        # the side effect is triggered in dict.__eq__ and modifies the length
-        self.assertNotEqual(lhs, rhs)
-        self.assertEqual(len(lhs), 1)
-        self.assertEqual(len(rhs), 1)
-        self.assertEqual(Key.count, 1)
-        self.check_side_effect_after_eq(Key, lhs, {0: None})
-        self.check_side_effect_after_eq(Key, rhs, {0: None})
+            def side_effect(self):
+                del self.d1[self.k1]
+                del self.d2[self.k2]
+
+        with Key.test_context() as (d1, d2):
+            self.assertEqual(Key.count, 0)
+            # the side effect is in dict.__eq__ and modifies the length
+            self.assertNotEqual(d1, d2)
+            self.assertEqual(Key.count, 1)
+            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
+            self.assertDictEqual(d2, dict.fromkeys((0, 4.2)))
 
     def test_issue119004_change_size_by_delete_key_in_dict_eq_asymmetric(self):
-        class Key(self.KeyClass):
+        class Key(self.Key):
             trigger = 0
-            @classmethod
-            def side_effect(cls):
-                del lhs[cls.key_lhs]
-
-        lhs, rhs = Key.setup()
-        self.assertEqual(Key.count, 0)
-        # the side effect is triggered in dict.__eq__ and modifies the length
-        self.assertNotEqual(lhs, rhs)
-        self.assertEqual(len(lhs), 1)
-        self.assertEqual(len(rhs), 2)
-        self.assertEqual(Key.count, 1)
-        self.check_side_effect_after_eq(Key, lhs, {0: None})
-        self.check_side_effect_after_eq(Key, rhs, {0: None, Key(): None})
+            def side_effect(self):
+                del self.d1[self.k1]
+
+        with Key.test_context() as (d1, d2):
+            self.assertEqual(Key.count, 0)
+            # the side effect is in dict.__eq__ and modifies the length
+            self.assertNotEqual(d1, d2)
+            self.assertEqual(Key.count, 2)
+            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
+            self.assertDictEqual(d2, dict.fromkeys((0, Key(), 4.2)))
 
 
 @unittest.skipUnless(c_coll, 'requires the C version of the collections module')

From d59f2507bc61940908b67a58a0eb39cae4cc5125 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 12:25:50 +0200
Subject: [PATCH 14/17] remove unused import

---
 Lib/test/test_ordered_dict.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index 51f68620dfaf00..ababe00310d29f 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -9,7 +9,6 @@
 import struct
 import sys
 import unittest
-import unittest.mock
 import weakref
 from collections.abc import MutableMapping
 from test import mapping_tests, support

From b37a0488fc12071c4274ac98958899c3a88073fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 12:47:56 +0200
Subject: [PATCH 15/17] simplify tests again

---
 Lib/test/test_ordered_dict.py | 190 +++++++++++-----------------------
 1 file changed, 61 insertions(+), 129 deletions(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index ababe00310d29f..d0feeb02aaaae0 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -746,10 +746,6 @@ def test_ordered_dict_items_result_gc(self):
 class _TriggerSideEffectOnEqual:
     count = 0   # number of calls to __eq__
     trigger = 1 # count value when to trigger side effect
-    OrderedDict = None
-    # reference to non-local objects (in the test body)
-    d1 = d2 = None  # d1, d2 are ordered dictionaries
-    k1 = k2 = None  # k1, k2 are instances of _TriggerSideEffectOnEqual()
 
     def __eq__(self, other):
         if self.__class__.count == self.__class__.trigger:
@@ -764,19 +760,6 @@ def __hash__(self):
     def side_effect(self):
         raise NotImplementedError
 
-    @classmethod
-    @contextlib.contextmanager
-    def test_context(cls):
-        assert cls.OrderedDict is not None, "missing OrderedDict class"
-
-        try:
-            cls.k1, cls.k2 = k1, k2 = cls(), cls()
-            cls.d1 = d1 = cls.OrderedDict(dict.fromkeys((0, k1, 4.2)))
-            cls.d2 = d2 = cls.OrderedDict(dict.fromkeys((0, k2, 4.2)))
-            yield d1, d2
-        finally:
-            cls.count = 0
-
 class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase):
 
     module = py_coll
@@ -784,18 +767,18 @@ class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase):
 
     def test_issue119004_attribute_error(self):
         class Key(_TriggerSideEffectOnEqual):
-            OrderedDict = self.OrderedDict
-
             def side_effect(self):
-                del self.d1[self.k1]
+                del dict1[TODEL]
 
-        with Key.test_context() as (d1, d2):
-            # This causes an AttributeError due to the linked list being changed
-            msg = re.escape("'NoneType' object has no attribute 'key'")
-            self.assertRaisesRegex(AttributeError, msg, operator.eq, d1, d2)
-            self.assertEqual(Key.count, 2)
-            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
-            self.assertDictEqual(d2, dict.fromkeys((0, Key(), 4.2)))
+        TODEL = Key()
+        dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        # This causes an AttributeError due to the linked list being changed
+        msg = re.escape("'NoneType' object has no attribute 'key'")
+        self.assertRaisesRegex(AttributeError, msg, operator.eq, dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, dict.fromkeys((0, 4.2)))
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
 
 class CPythonBuiltinDictTests(unittest.TestCase):
@@ -819,128 +802,77 @@ class CPythonBuiltinDictTests(unittest.TestCase):
 
 class CPythonOrderedDictSideEffects:
 
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-
-        class Key(_TriggerSideEffectOnEqual):
-            OrderedDict = cls.OrderedDict
-        cls.Key = Key
-
-    def _test_issue119004(self, keyclass, expect1, expect2):
-        assert issubclass(keyclass, _TriggerSideEffectOnEqual)
-        with keyclass.test_context() as (d1, d2):
-            msg = re.escape("OrderedDict mutated during iteration")
-            self.assertRaisesRegex(RuntimeError, msg, operator.eq, d1, d2)
-            # There are two calls to KeyClass.__eq__: one when doing
-            # value comparisons (without order) by dict.__eq__ and
-            # one when doing key comparisons (for the order).
-            self.assertEqual(keyclass.count, 2)
-            self.assertDictEqual(d1, expect1)
-            self.assertDictEqual(d2, expect2)
+    def check_runtime_error_issue119004(self, dict1, dict2):
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, dict1, dict2)
 
     def test_issue119004_change_size_by_clear(self):
-        class Key(self.Key):
-            def side_effect(self):
-                self.d1.clear()
-                self.d2.clear()
-
-        self._test_issue119004(Key, {}, {})
-
-    def test_issue119004_change_size_by_clear_asymmetric(self):
-        class Key(self.Key):
+        class Key(_TriggerSideEffectOnEqual):
             def side_effect(self):
-                self.d1.clear()
+                dict1.clear()
 
-        expect2 = dict.fromkeys((0, Key(), 4.2))
-        self._test_issue119004(Key, {}, expect2)
+        dict1 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        self.check_runtime_error_issue119004(dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, {})
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
     def test_issue119004_change_size_by_delete_key(self):
-        class Key(self.Key):
-            def side_effect(self):
-                del self.d1[self.k1], self.d2[self.k2]
-
-        expect = dict.fromkeys((0, 4.2))
-        self._test_issue119004(Key, expect, expect)
-
-    def test_issue119004_change_size_by_delete_key_asymmetric(self):
-        class Key(self.Key):
+        class Key(_TriggerSideEffectOnEqual):
             def side_effect(self):
-                del self.d1[self.k1]
+                del dict1[key1]
 
-        expect1 = dict.fromkeys((0, 4.2))
-        expect2 = dict.fromkeys((0, Key(), 4.2))
-        self._test_issue119004(Key, expect1, expect2)
+        key1, key2 = Key(), Key()
+        dict1 = self.OrderedDict(dict.fromkeys((0, key1, 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, key2, 4.2)))
+        self.check_runtime_error_issue119004(dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, dict.fromkeys((0, 4.2)))
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
     def test_issue119004_change_linked_list_by_clear(self):
-        class Key(self.Key):
-            def side_effect(self):
-                self.d1.clear()
-                self.d1['a'] = self.d1['b'] = 'c'
-                self.d2.clear()
-                self.d2['a'] = self.d2['b'] = 'c'
-
-        expect = {'a': 'c', 'b': 'c'}
-        self._test_issue119004(Key, expect, expect)
-
-    def test_issue119004_change_linked_list_by_clear_asymmetric(self):
-        class Key(self.Key):
+        class Key(_TriggerSideEffectOnEqual):
             def side_effect(self):
-                self.d1.clear()
-                self.d1['a'] = self.d1['b'] = 'c'
+                dict1.clear()
+                dict1['a'] = dict1['b'] = 'c'
 
-        expect1 = dict.fromkeys(('a', 'b'), 'c')
-        expect2 = dict.fromkeys((0, Key(), 4.2))
-        self._test_issue119004(Key, expect1, expect2)
+        dict1 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        self.check_runtime_error_issue119004(dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, dict.fromkeys(('a', 'b'), 'c'))
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
     def test_issue119004_change_linked_list_by_delete_key(self):
-        class Key(self.Key):
-            def side_effect(self):
-                del self.d1[self.k1], self.d2[self.k2]
-                self.d1['a'] = self.d2['b'] = 'c'
-
-        expect1 = {0: None, 'a': 'c', 4.2: None}
-        expect2 = {0: None, 'b': 'c', 4.2: None}
-        self._test_issue119004(Key, expect1, expect2)
-
-    def test_issue119004_change_linked_list_by_delete_key_asymmetric(self):
-        class Key(self.Key):
+        class Key(_TriggerSideEffectOnEqual):
             def side_effect(self):
-                del self.d1[self.k1]
-                self.d1['a'] = 'c'
+                del dict1[TODEL]
+                dict1['a'] = 'c'
 
-        expect1 = {0: None, 'a': 'c', 4.2: None}
-        expect2 = dict.fromkeys((0, Key(), 4.2))
-        self._test_issue119004(Key, expect1, expect2)
+        TODEL = Key()
+        dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        self.check_runtime_error_issue119004(dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, {0: None, 'a': 'c', 4.2: None})
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
     def test_issue119004_change_size_by_delete_key_in_dict_eq(self):
-        class Key(self.Key):
-            trigger = 0
-            def side_effect(self):
-                del self.d1[self.k1]
-                del self.d2[self.k2]
-
-        with Key.test_context() as (d1, d2):
-            self.assertEqual(Key.count, 0)
-            # the side effect is in dict.__eq__ and modifies the length
-            self.assertNotEqual(d1, d2)
-            self.assertEqual(Key.count, 1)
-            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
-            self.assertDictEqual(d2, dict.fromkeys((0, 4.2)))
-
-    def test_issue119004_change_size_by_delete_key_in_dict_eq_asymmetric(self):
-        class Key(self.Key):
+        class Key(_TriggerSideEffectOnEqual):
             trigger = 0
             def side_effect(self):
-                del self.d1[self.k1]
-
-        with Key.test_context() as (d1, d2):
-            self.assertEqual(Key.count, 0)
-            # the side effect is in dict.__eq__ and modifies the length
-            self.assertNotEqual(d1, d2)
-            self.assertEqual(Key.count, 2)
-            self.assertDictEqual(d1, dict.fromkeys((0, 4.2)))
-            self.assertDictEqual(d2, dict.fromkeys((0, Key(), 4.2)))
+                del dict1[TODEL]
+
+        TODEL = Key()
+        dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
+        self.assertEqual(Key.count, 0)
+        # the side effect is in dict.__eq__ and modifies the length
+        self.assertNotEqual(dict1, dict2)
+        self.assertEqual(Key.count, 2)
+        self.assertDictEqual(dict1, dict.fromkeys((0, 4.2)))
+        self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2)))
 
 
 @unittest.skipUnless(c_coll, 'requires the C version of the collections module')

From 28c89b2ed6e6a04e473b273056039b4e1dc06157 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Mon, 22 Jul 2024 12:49:14 +0200
Subject: [PATCH 16/17] simplify tests again

---
 Lib/test/test_ordered_dict.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py
index d0feeb02aaaae0..a9b6a84996e659 100644
--- a/Lib/test/test_ordered_dict.py
+++ b/Lib/test/test_ordered_dict.py
@@ -821,11 +821,11 @@ def side_effect(self):
     def test_issue119004_change_size_by_delete_key(self):
         class Key(_TriggerSideEffectOnEqual):
             def side_effect(self):
-                del dict1[key1]
+                del dict1[TODEL]
 
-        key1, key2 = Key(), Key()
-        dict1 = self.OrderedDict(dict.fromkeys((0, key1, 4.2)))
-        dict2 = self.OrderedDict(dict.fromkeys((0, key2, 4.2)))
+        TODEL = Key()
+        dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2)))
+        dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2)))
         self.check_runtime_error_issue119004(dict1, dict2)
         self.assertEqual(Key.count, 2)
         self.assertDictEqual(dict1, dict.fromkeys((0, 4.2)))

From 6c2a684ce2a8803d94a1404788814060e9466aaf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 4 Sep 2024 12:46:35 +0200
Subject: [PATCH 17/17] Update
 Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
---
 .../next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst b/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst
index 03e01dfcae43f8..899bd163d36644 100644
--- a/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst
+++ b/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst
@@ -1,2 +1,2 @@
 Fix a crash in :ref:`OrderedDict.__eq__ <collections_OrderedDict__eq__>`
-when operands are mutated during the check.  Patch by Bénédikt Tran.
+when operands are mutated during the check. Patch by Bénédikt Tran.