From 9c485f933961ac7d30659035abbe2b5cd497f88d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 20 Oct 2026 19:26:00 -0400 Subject: [PATCH] fix(corekit): record Info's field order instead of reading it from __dict__ (#2003) Info listed its fields in __dict__ iteration order, which on PyPy is the attribute layout of the first instance of the class, so to_dict(), iteration, repr and str could reorder a later instance's fields. - Info.__update__ records each key in a per-instance __order__ dict as it is first set; __iter__, __len__, __str__, __repr__, items(multi=True) and to_dict read the order from there, and a repeated key anchors on the last recorded key. __copy__ copies the record; pickles carry it in the state. - __order__ joins the bookkeeping names, so a field of that name is mangled. - The multi-mapping subclasses keep their own storage and are unaffected. CPython output is unchanged; the three PyPy 8.0.1 failures now pass. --- docs/source/pcapkit/corekit/infoclass.rst | 1 + pcapkit/corekit/infoclass.py | 51 ++++--- .../test_info_field_order_2003_unit.py | 128 ++++++++++++++++++ 3 files changed, 163 insertions(+), 17 deletions(-) create mode 100644 tests/corekit/test_info_field_order_2003_unit.py diff --git a/docs/source/pcapkit/corekit/infoclass.rst b/docs/source/pcapkit/corekit/infoclass.rst index ffe8563b96..5202a5abf9 100644 --- a/docs/source/pcapkit/corekit/infoclass.rst +++ b/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 diff --git a/pcapkit/corekit/infoclass.py b/pcapkit/corekit/infoclass.py index 7964d6a7be..49ea4a9361 100644 --- a/pcapkit/corekit/infoclass.py +++ b/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,14 +559,23 @@ 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) @@ -563,10 +583,7 @@ def __str__(self) -> 'str': 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': diff --git a/tests/corekit/test_info_field_order_2003_unit.py b/tests/corekit/test_info_field_order_2003_unit.py new file mode 100644 index 0000000000..abe9f4bb95 --- /dev/null +++ b/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()