fix(corekit): record Info's field order instead of reading it from __dict__ (#2003)

docs/source/pcapkit/corekit/infoclass.rst

@@ -26,6 +26,7 @@ classes :class:`~pcapkit.corekit.infoclass.MultiInfo` and
 
    .. autoattribute:: __map__
    .. autoattribute:: __map_reverse__
+   .. autoattribute:: __order__
 
 .. autodecorator:: pcapkit.corekit.infoclass.info_final
 

pcapkit/corekit/infoclass.py

@@ -149,7 +149,7 @@ def info_final(cls: 'ST', *, _finalised: 'bool' = True) -> 'ST':
     if cls is Info and cls.__dict__.get('__base_ready__'):
         return cls
 
-    temp = ['__map__', '__map_reverse__', '__multi__', '__builtin__', '__finalised__']
+    temp = ['__map__', '__map_reverse__', '__multi__', '__order__', '__builtin__', '__finalised__']
     temp.extend(cls.__additional__)
     for obj in cls.mro():
         temp.extend(dir(obj))
@@ -341,9 +341,18 @@ class Info(Mapping[str, VT], Generic[VT], metaclass=InfoMeta):
         #: original names).
         __map_reverse__: 'dict[str, str]'
         #: Values of keys held more than once, i.e. every value but the first,
-        #: keyed by the (transformed) key that was last in :attr:`__dict__` when
-        #: each was added, so :meth:`to_dict` writes it right after that key.
+        #: keyed by the (transformed) key that was last in :attr:`__order__`
+        #: when each was added, so :meth:`to_dict` writes it right after that
+        #: key.
         __multi__: 'dict[str, list[tuple[str, Any]]]'
+        #: The (transformed) keys of the fields, in the order each was first
+        #: set, as the keys of a plain :obj:`dict`. Every method that lists the
+        #: fields reads their order from here rather than from
+        #: :attr:`~object.__dict__`, whose iteration order is the interpreter's
+        #: storage layout: PyPy lays an instance's attributes out to match an
+        #: earlier instance of the same class, so the fields came out in that
+        #: instance's order instead (:issue:`2003`).
+        __order__: 'dict[str, None]'
         #: List of builtin methods.
         __builtin__: 'set[str]'
 
@@ -467,6 +476,7 @@ class is finalised on its first construction, whatever its
         super().__setattr__(self, '__map__', {})
         super().__setattr__(self, '__map_reverse__', {})
         super().__setattr__(self, '__multi__', {})
+        super().__setattr__(self, '__order__', {})
 
         return self
 
@@ -478,6 +488,7 @@ def __copy__(self) -> 'Self':
         new.__dict__['__map__'] = dict(self.__map__)
         new.__dict__['__map_reverse__'] = dict(self.__map_reverse__)
         new.__dict__['__multi__'] = {key: list(values) for (key, values) in self.__multi__.items()}
+        new.__dict__['__order__'] = dict(self.__order__)
         return new
 
     def __post_init__(self) -> 'None':
@@ -516,7 +527,7 @@ def __update__(self, dict_: 'Optional[Mapping[str, VT] | Iterable[tuple[str, VT]
         for (key, value) in multi_iter:
             new_key = f'_{__name__}{key}' if key in self.__builtin__ else key
             if new_key in self.__dict__ and new_key not in self.__excluded__:
-                anchor = next(reversed(self.__dict__))
+                anchor = next(reversed(self.__order__))
                 self.__multi__.setdefault(anchor, []).append((new_key, _freeze(value)))
             else:
                 self.__update__({key: value})
@@ -548,25 +559,31 @@ def __update__(self, dict_: 'Optional[Mapping[str, VT] | Iterable[tuple[str, VT]
             #     key = re.sub(r'\W', '_', key)
             self.__dict__[key] = _freeze(value)
 
+            # NOTE: The order is recorded here rather than read back from
+            # ``__dict__``, whose iteration order is the interpreter's storage
+            # layout and not the order the fields were set in (#2003). A key
+            # set again keeps its place, as it would in a :obj:`dict`.
+            self.__order__.setdefault(key)
+
     __init__ = __update__
 
+    def __fields(self) -> 'Iterator[tuple[str, VT]]':
+        """Every field, as ``(key, value)`` with the transformed key, in :attr:`__order__`."""
+        for key in self.__order__:
+            if key not in self.__excluded__:
+                yield key, self.__dict__[key]
+
     def __str__(self) -> 'str':
         temp = []  # type: list[str]
-        for (key, value) in self.__dict__.items():
-            if key in self.__excluded__:
-                continue
-
+        for (key, value) in self.__fields():
             out_key = self.__map_reverse__.get(key, key)
             temp.append(f'{out_key}={value}')
         args = ', '.join(temp)
         return f'{type(self).__name__}({args})'
 
     def __repr__(self) -> 'str':
         temp = []  # type: list[str]
-        for (key, value) in self.__dict__.items():
-            if key in self.__excluded__:
-                continue
-
+        for (key, value) in self.__fields():
             out_key = self.__map_reverse__.get(key, key)
             # NOTE: a multi-mapping :class:`Info`, i.e. an option list, is
             # written out in full, as the multi-mapping it is.
@@ -580,12 +597,12 @@ def __repr__(self) -> 'str':
     def __len__(self) -> 'int':
         # NOTE: count exactly the keys :meth:`__iter__` yields, so the
         # bookkeeping attributes in ``__excluded__`` are not counted.
-        return sum(1 for key in self.__dict__ if key not in self.__excluded__)
+        return sum(1 for key in self.__order__ if key not in self.__excluded__)
 
     def __iter__(self) -> 'Iterator[str]':
-        for key in self.__dict__:
+        for key in self.__order__:
             if key in self.__excluded__:
-               continue
+                continue
             yield self.__map_reverse__.get(key, key)
 
     def __getitem__(self, name: 'str') -> 'VT':
@@ -608,9 +625,9 @@ def items(self, multi: 'bool' = False) -> 'ItemsView[str, VT] | Iterator[tuple[s
 
     def __items_multi(self) -> 'Iterator[tuple[str, VT]]':
         """Every ``(key, value)`` pair, with the transformed keys, in order."""
-        for (key, value) in self.__dict__.items():
+        for key in self.__order__:
             if key not in self.__excluded__:
-                yield key, value
+                yield key, self.__dict__[key]
             yield from self.__multi__.get(key, ())
 
     def __setattr__(self, name: 'str', value: 'VT') -> 'NoReturn':

tests/corekit/test_info_field_order_2003_unit.py

@@ -0,0 +1,128 @@
+# -*- coding: utf-8 -*-
+"""GitHub issue #2003: an :class:`~pcapkit.corekit.infoclass.Info` lists its
+fields in the order they were set, whatever the interpreter's attribute layout.
+
+:class:`~pcapkit.corekit.infoclass.Info` used to read its field order from the
+instance's ``__dict__``. On PyPy that is the storage layout, which follows the
+first instance of the class the interpreter saw: a later instance that set its
+fields in another order listed them in the earlier one's, so ``to_dict()`` and
+anything rebuilt from it came out reordered. The order is now recorded as each
+field is set.
+
+The defect needs PyPy to happen on its own, so most tests here reorder an
+instance's ``__dict__`` by hand, which is what PyPy's layout does to it, and
+assert that nothing reads the order from there. The last runs the reordering
+PyPy performs itself, and so fails without the fix only on PyPy.
+
+"""
+
+import copy
+import pickle  # nosec: B403 -- round-trips objects built here
+import unittest
+
+from pcapkit.corekit.infoclass import Info
+from pcapkit.corekit.multidict import OrderedMultiDict
+
+#: Test tier (#1958): attribute storage order differs between interpreters.
+__tier__ = 'ALL'
+
+
+class Declared(Info):
+    """A field model, whose generated ``__init__`` sets the fields in declared order."""
+
+    a: 'int'
+    b: 'int'
+    c: 'int'
+
+
+def relayout(info: 'Info', key: 'str') -> 'None':
+    """Move ``key`` to the end of ``info.__dict__``, as PyPy's layout may place it."""
+    value = info.__dict__.pop(key)
+    info.__dict__[key] = value
+
+
+class StorageOrderTests(unittest.TestCase):
+    """Every reader lists the fields in the order they were set."""
+
+    def assert_order(self, info: 'Info', keys: 'list[str]') -> 'None':
+        self.assertEqual(list(info), keys)
+        self.assertEqual(list(info.keys()), keys)
+        self.assertEqual([key for key, _ in info.items()], keys)
+        self.assertEqual([key for key, _ in info.items(multi=True)], keys)
+        self.assertEqual(list(info.to_dict().keys()), keys)
+        self.assertEqual(len(info), len(keys))
+        self.assertEqual([part.split('=')[0] for part in str(info)[len(type(info).__name__) + 1:-1].split(', ')],
+                         keys)
+        self.assertEqual([part.split('=')[0] for part in repr(info)[len(type(info).__name__) + 2:-1].split(', ')],
+                         keys)
+
+    def test_a_bare_info_keeps_the_order_its_fields_were_set_in(self) -> None:
+        info = Info(a=1, b=2, c=3)
+        relayout(info, 'a')
+        self.assert_order(info, ['a', 'b', 'c'])
+
+    def test_a_field_model_keeps_its_declared_order(self) -> None:
+        info = Declared(1, 2, 3)
+        relayout(info, 'a')
+        self.assert_order(info, ['a', 'b', 'c'])
+
+    def test_a_renamed_key_keeps_its_place(self) -> None:
+        # ``items`` names a method, so it is stored under a mangled key.
+        info = Info(items=1, b=2)
+        relayout(info, info.__map__['items'])
+        self.assert_order(info, ['items', 'b'])
+
+    def test_a_key_set_again_keeps_its_place(self) -> None:
+        info = Info(a=1, b=2)
+        info.__update__(a=3)
+        relayout(info, 'b')
+        relayout(info, 'a')
+        self.assert_order(info, ['a', 'b'])
+        self.assertEqual(info.a, 3)
+
+    def test_a_repeated_key_is_written_after_the_last_field_set(self) -> None:
+        info = Info(a=1, b=2)
+        relayout(info, 'a')
+        info.__update__(OrderedMultiDict([('a', 3)]))
+        self.assertEqual(list(info.items(multi=True)), [('a', 1), ('b', 2), ('a', 3)])
+        self.assertEqual(list(info.to_dict().items(multi=True)), [('a', 1), ('b', 2), ('a', 3)])
+
+    def test_a_copy_keeps_the_order_and_records_its_own(self) -> None:
+        info = Info(a=1, b=2)
+        relayout(info, 'a')
+        clone = copy.copy(info)
+        self.assertEqual(list(clone), ['a', 'b'])
+        clone.__update__(c=3)
+        self.assertEqual(list(clone), ['a', 'b', 'c'])
+        self.assertEqual(list(info), ['a', 'b'])
+
+    def test_a_pickle_keeps_the_order(self) -> None:
+        for info in (Info(a=1, b=2, c=3), Declared(1, 2, 3)):
+            with self.subTest(type=type(info).__name__):
+                relayout(info, 'a')
+                self.assertEqual(list(pickle.loads(pickle.dumps(info))), ['a', 'b', 'c'])  # nosec: B301
+
+    def test_a_field_named_like_the_record_is_a_field(self) -> None:
+        info = Info(**{'__order__': 1, 'b': 2})
+        self.assertEqual(list(info), ['__order__', 'b'])
+        self.assertEqual(info['__order__'], 1)
+
+
+class EarlierInstanceTests(unittest.TestCase):
+    """An earlier instance of the class does not reorder a later one's fields."""
+
+    def test_a_later_instance_keeps_its_own_order(self) -> None:
+        # NOTE: a class of its own, so no instance built elsewhere has laid it
+        # out first. PyPy lays the second instance out as the first, placing
+        # ``hopopt`` last, which is the shape #2003 was found in.
+        class Fresh(Info):
+            pass
+
+        Fresh(x=1, opts=2, tail=3)
+        later = Fresh(x=1, hopopt=0, opts=2, tail=3)
+        self.assertEqual(list(later), ['x', 'hopopt', 'opts', 'tail'])
+        self.assertEqual(list(later.to_dict().keys()), ['x', 'hopopt', 'opts', 'tail'])
+
+
+if __name__ == '__main__':
+    unittest.main()