diff --git a/pcapkit/corekit/infoclass.py b/pcapkit/corekit/infoclass.py index 1ad0629e3b..fede71e905 100644 --- a/pcapkit/corekit/infoclass.py +++ b/pcapkit/corekit/infoclass.py @@ -382,19 +382,14 @@ def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self': # pylint: disable=unus :func:`info_final`, i.e. it carries ``__final__`` in its own ``__dict__`` without :attr:`FinalisedState.FINAL`. - Warning: - Out of scope, deliberately: a ``@final`` class descending from a - :attr:`FinalisedState.BASE` ancestor is not caught. ``__finalised__`` - inherits, and a ``BASE`` ancestor means some earlier bare - construction already walked this branch for the chain -- so the - subclass's own ``cls.__finalised__`` reads ``BASE`` too, the branch - below is skipped entirely, and the marker on ``cls`` itself is never - inspected. Closing this means re-finalising every descendant of a - ``BASE`` class on each subclassing, which is the auto-finalisation - behaviour ``test_an_unfinalised_descendant_is_not_mistaken_for_a_ - mismarked_class`` deliberately leaves alone. In the tree today no - ``BASE``-state class is ever marked ``@final`` by hand, so the escape - is theoretical rather than live. + Note: + Whether ``cls`` still needs finalising is read from its *own* + ``__dict__``, never inherited (GitHub issue :issue:`1490`). Each + class is finalised on its first construction, whatever its + ancestors' state, so a subclass of a :attr:`FinalisedState.BASE` or + :attr:`FinalisedState.FINAL` ancestor still gets its own + ``__builtin__``, ``__excluded__`` and generated ``__init__``, and a + ``@final`` class under a ``BASE`` ancestor is refused like any other. """ # NOTE: ``final`` is applied *after* the class object exists, so @@ -407,32 +402,26 @@ def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self': # pylint: disable=unus # ``TypeError: 'int' object is not iterable``, naming neither the class # nor the mistake. # - # It is nested inside the NONE branch rather than run ahead of it, and - # that placement is what makes it free on the hot path: a finalised class - # never enters the branch at all, so it never evaluates the nested - # ``cls.__dict__.get('__final__')`` either -- ``dis.dis(Info.__new__)`` - # shows the ``FinalisedState.NONE`` comparison compiles to a single - # ``POP_JUMP_IF_FALSE`` that, for a ``FINAL`` class, jumps straight past - # this whole block to ``super().__new__``: zero of the added bytecode - # executes, which is a property of the compiled branch rather than of - # any particular timing run, and is the reason to trust here rather than - # a timer. The raise also leaves ``__finalised__`` at NONE, so a second - # attempt re-enters and fails again rather than the error firing once. + # It is nested inside the NONE branch rather than run ahead of it, so a + # finalised class never evaluates it: the outer test is false for it and + # jumps straight to ``super().__new__``. The raise also leaves + # ``__finalised__`` unset, so a second attempt re-enters and fails again + # rather than the error firing once. # - # ``cls.__dict__`` rather than ``getattr``: not because a class that - # inherits ``__final__`` from a *finalised* ancestor would be misread -- - # such a class also inherits ``__finalised__ == FINAL``, so the outer - # ``if`` above already excludes it before this line is ever reached, - # regardless of which lookup is used here. The case ``getattr`` gets - # wrong is an ancestor hand-marked bare ``@final`` and *never finalised*: - # its own ``__finalised__`` stays ``NONE`` and so, inherited, does every - # subclass declared before the marker landed -- so such a subclass still - # reaches this line, and ``getattr`` would read the marker it merely - # inherits and blame it for a mismarking that is its ancestor's, not its - # own. ``OwnDictRuleTests.test_a_subclass_that_predates_an_unfinalised_ - # ancestors_bare_final_is_not_blamed_for_it`` pins exactly this shape -- - # mutating this line to ``getattr`` fails it. - if cls.__finalised__ == FinalisedState.NONE: + # Both lookups read ``cls.__dict__`` rather than ``getattr``, because + # both markers are ordinary, inherited class attributes. The outer one: + # ``info_final`` writes ``BASE`` or ``FINAL`` onto the class it + # finalises, and a subclass reading that state through inheritance was + # never finalised itself -- it would keep the ``__excluded__`` it copied + # at declaration, without the names ``info_final`` adds, and leak + # ``__map__``, ``__map_reverse__`` and ``__multi__`` from ``to_dict()`` + # (GitHub issue #1490). The inner one: an ancestor hand-marked bare + # ``@final`` passes its marker to every subclass declared before it + # landed, and ``getattr`` would blame such a subclass for a mismarking + # that is its ancestor's. ``OwnDictRuleTests.test_a_subclass_that_ + # predates_an_unfinalised_ancestors_bare_final_is_not_blamed_for_it`` + # pins that shape. + if cls.__dict__.get('__finalised__', FinalisedState.NONE) == FinalisedState.NONE: if cls.__dict__.get('__final__'): raise InfoError(f'{cls.__name__}: marked final but never finalised, so it has no generated ' '__init__; apply info_final, which applies final itself, not final alone') diff --git a/tests/corekit/test_info_child_finalisation_1490_unit.py b/tests/corekit/test_info_child_finalisation_1490_unit.py new file mode 100644 index 0000000000..a4d07242d4 --- /dev/null +++ b/tests/corekit/test_info_child_finalisation_1490_unit.py @@ -0,0 +1,120 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #1490: each non-final :class:`~pcapkit.corekit.infoclass.Info` +subclass is finalised on its own state, whatever was built before it. + +:meth:`Info.__new__ ` used to read +``__finalised__`` through inheritance, so once a non-final parent had been built +its non-final children read the parent's ``BASE`` and were never finalised. They +kept the ``__excluded__`` copied at declaration, and ``to_dict()``, ``len()`` and +iteration then included ``__map__``, ``__map_reverse__`` and ``__multi__``. + +The defect depends on process-wide state -- which class was built first -- so +every order runs in a fresh interpreter, started from the repository root so it +imports the tree under test; the child reports ``pcapkit.__file__`` and that is +asserted too. + +""" + +import json +import os +import subprocess # nosec: B404 +import sys +import unittest + +from tests.corekit._roundtrip import skip_without_runtime + +ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +#: Parent and child, by ``module:qualname``; ``synthetic`` is declared in the script. +MODELS = { + 'synthetic': ('', 'Parent', 'Child'), + 'hopopt.QuickStartOption': ('pcapkit.protocols.data.internet.hopopt', 'Option', 'QuickStartOption'), + 'hopopt.SMFDPDOption': ('pcapkit.protocols.data.internet.hopopt', 'Option', 'SMFDPDOption'), + 'ipv6_opts.QuickStartOption': ('pcapkit.protocols.data.internet.ipv6_opts', 'Option', 'QuickStartOption'), + 'ipv4.QSOption': ('pcapkit.protocols.data.internet.ipv4', 'Option', 'QSOption'), + 'tcp.MPTCPJoin': ('pcapkit.protocols.data.transport.tcp', 'MPTCP', 'MPTCPJoin'), +} + +SCRIPT = r''' +import importlib, json, sys +import pcapkit +from pcapkit.corekit.infoclass import Info + +module, parent, child, order = sys.argv[1:] +if module: + ns = vars(importlib.import_module(module)) +else: + class Parent(Info): + a: 'int' + class Child(Parent): + b: 'int' + ns = {'Parent': Parent, 'Child': Child} + +def keys(cls): + out = [] + for base in reversed(cls.__mro__[:cls.__mro__.index(Info)]): + out.extend(key for key in base.__dict__.get('__annotations__', {}) if key not in out) + return out + +result = {'file': pcapkit.__file__} +for name in ((parent, child) if order == 'parent-first' else (child, parent)): + cls = ns[name] + info = cls.from_dict({key: index for index, key in enumerate(keys(cls))}) + result[name] = {'expected': keys(cls), 'to_dict': list(info.to_dict()), + 'len': len(info), 'iter': list(info)} +print(json.dumps(result)) +''' + + +def build(model: 'str', order: 'str') -> 'dict': + """Build ``model``'s parent and child in ``order``, in a fresh interpreter.""" + module, parent, child = MODELS[model] + proc = subprocess.run([sys.executable, '-c', SCRIPT, module, parent, child, order], # nosec: B603 + cwd=ROOT, capture_output=True, text=True, timeout=120, check=False) + if proc.returncode: + raise AssertionError(proc.stderr) + return json.loads(proc.stdout.splitlines()[-1]) + + +class ChildFinalisationTests(unittest.TestCase): + """``to_dict()``, ``len()`` and iteration are right in both construction orders.""" + + def setUp(self) -> None: + skip_without_runtime(self) + + def test_both_orders(self) -> None: + for model, (_, parent, child) in MODELS.items(): + for order in ('parent-first', 'child-first'): + with self.subTest(model=model, order=order): + result = build(model, order) + self.assertEqual(os.path.dirname(os.path.dirname(result['file'])), ROOT) + for name in (parent, child): + got = result[name] + self.assertEqual(got['to_dict'], got['expected'], name) + self.assertEqual(got['iter'], got['expected'], name) + self.assertEqual(got['len'], len(got['expected']), name) + + def test_a_child_declared_after_its_parent_was_built(self) -> None: + """The other side of the inheritance: declared *after* the parent's ``BASE``.""" + from pcapkit.corekit.infoclass import FinalisedState, Info + + class Parent(Info): + a: int + + Parent(a=1) + + class Child(Parent): + b: int + + def method(self) -> 'None': + """A name of its own, which ``__builtin__`` must also cover.""" + + child = Child(1, 2) + self.assertEqual(Child.__dict__.get('__finalised__'), FinalisedState.BASE) + self.assertEqual(list(child.to_dict()), ['a', 'b']) + self.assertEqual((len(child), list(child)), (2, ['a', 'b'])) + self.assertIn('method', Child.__builtin__) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_info_roundtrip_unit.py b/tests/corekit/test_info_roundtrip_unit.py index 2d73b866cc..c8d2a433ff 100644 --- a/tests/corekit/test_info_roundtrip_unit.py +++ b/tests/corekit/test_info_roundtrip_unit.py @@ -38,7 +38,7 @@ from typing import TYPE_CHECKING from tests._support import reimport_once_per_class -from tests.corekit._roundtrip import (OK, Gap, KnownFailureTable, Outcome, describe, run, +from tests.corekit._roundtrip import (OK, KnownFailureTable, Outcome, describe, run, skip_without_runtime) if TYPE_CHECKING: @@ -222,23 +222,7 @@ class InfoRoundTripTests(KnownFailureTable, unittest.TestCase): STATUSES = ('OK', 'BUILD', 'TO_DICT', 'FROM_DICT', 'EQUAL', 'MULTI', 'TYPE', 'TIMEOUT') - KNOWN_FAILURES = ( - Gap(1490, 'A non-final model declared under a non-final parent is never finalised ' - 'once the parent has been: Info.__new__ auto-finalises only while ' - '__finalised__ reads NONE, and it reads the BASE the parent was given ' - 'through inheritance (pcapkit/corekit/infoclass.py:435, :223). The class keeps ' - 'the __excluded__ list it copied at class creation, before info_final added ' - 'the bookkeeping names to the parent\'s (pcapkit/corekit/infoclass.py:272-280, ' - ':146), so to_dict() emits __map__, __map_reverse__ and __multi__ as keys. ' - 'Order-dependent: the census instantiates each parent first', - 'TO_DICT', "'.__map__: builtins.dict'", - ('protocols.data.internet.hopopt:QuickStartOption/*', - 'protocols.data.internet.hopopt:SMFDPDOption/*', - 'protocols.data.internet.ipv6_opts:QuickStartOption/*', - 'protocols.data.internet.ipv6_opts:SMFDPDOption/*', - 'protocols.data.internet.ipv4:QSOption/*', - 'protocols.data.transport.tcp:MPTCPJoin/*')), - ) + KNOWN_FAILURES = () def setUp(self) -> None: skip_without_runtime(self) @@ -337,12 +321,7 @@ def test_repeated_nested_values_stay_nested_and_ordered(self) -> None: self.assertEqual(check(info), OK) def test_an_intermediate_model_after_its_parent(self) -> None: - """Pins the root cause of the census gap above directly (#1490). - - Expected-failure style: it asserts the defect, so a fix turns it red and - has to flip it together with the census entry. - - """ + """#1490: a non-final model built after its non-final parent is finalised too.""" from pcapkit.corekit.infoclass import Info class Parent(Info): @@ -358,7 +337,8 @@ class Child(Parent): Parent.from_dict({'a': 1}) child = Child.from_dict({'a': 1, 'b': 2}) self.assertEqual(list(Parent.from_dict({'a': 1}).to_dict()), ['a']) - self.assertEqual(list(child.to_dict()), ['__map__', '__map_reverse__', '__multi__', 'a', 'b']) + self.assertEqual(list(child.to_dict()), ['a', 'b']) + self.assertEqual((len(child), list(child)), (2, ['a', 'b'])) def test_bookkeeping_keys_survive_a_second_round(self) -> None: """#1465 and #1475: the dunder keys survive ``to_dict`` -> ``from_dict`` twice.""" diff --git a/tests/test_final_enforcement.py b/tests/test_final_enforcement.py index 3809feed7f..61d4affe0e 100644 --- a/tests/test_final_enforcement.py +++ b/tests/test_final_enforcement.py @@ -370,16 +370,9 @@ def test_an_unfinalised_descendant_is_not_mistaken_for_a_mismarked_class(self) - The same shape as above, left *unfinalised*: a class that inherits ``__final__`` from a parent finalised after it was declared has not been - mismarked by anybody, so constructing it must not raise. It inherits its - parent's generated ``__init__`` and behaves accordingly -- which is the - pre-existing "a descendant of a finalised class is not re-finalised" - behaviour, not something this guard is entitled to turn into an error. - - Only the guard's own property is asserted. Such a class is *imperfect* - for the pre-existing reason -- ``__excluded__`` was never populated for - it, so :meth:`to_dict` also returns the inherited ``__map__`` and - ``__map_reverse__`` -- and that is deliberately left unpinned here, so - that fixing the wart does not read as breaking this guard. + mismarked by anybody, so constructing it must not raise. Its own + ``__finalised__`` is unset, so its first construction finalises it to + :attr:`~FinalisedState.BASE` with an ``__init__`` of its own (#1490). """ from pcapkit.corekit.infoclass import Info, info_final @@ -396,7 +389,7 @@ class Child(Parent): self.assertNotIn('__final__', Child.__dict__) # No InfoError: the marker is inherited, not owned by this class. - self.assertEqual(Child(1).x, 1) + self.assertEqual(Child(1, 2).to_dict(), {'x': 1, 'y': 2}) def test_a_subclass_that_predates_an_unfinalised_ancestors_bare_final_is_not_blamed_for_it(self) -> None: """The one shape that actually tells ``__dict__`` and ``getattr`` apart @@ -443,24 +436,17 @@ class Child(Parent): # the marker from ``Parent``, which is not the class this constructs. self.assertEqual(Child(1, 2).to_dict(), {'x': 1, 'y': 2}) - def test_a_final_class_descending_from_a_base_state_ancestor_escapes_the_guard(self) -> None: - """A documented gap, not a guarantee -- see the ``Warning`` on - :meth:`Info.__new__`. - - ``__finalised__`` inherits, and :attr:`~FinalisedState.BASE` is no - exception: a class marked ``@final`` while descending from a ``BASE`` - ancestor reads ``BASE`` off it, never sees its own - :attr:`~FinalisedState.NONE`, and so never reaches the branch that reads - its marker at all. Closing this would mean re-finalising every - descendant of a ``BASE`` class on each subclassing, which is exactly the - auto-finalisation behaviour - :meth:`test_an_unfinalised_descendant_is_not_mistaken_for_a_mismarked_class` - establishes elsewhere in this file. This test pins today's behaviour so - that a change to it is a deliberate decision, not a silent side effect. + def test_a_final_class_descending_from_a_base_state_ancestor_is_refused(self) -> None: + """``__finalised__`` is read from each class's own ``__dict__`` (#1490). + + A class marked ``@final`` under a :attr:`~FinalisedState.BASE` ancestor + no longer reads ``BASE`` off it, so it reaches the branch that reads its + own marker and is refused like any other mismarked class. """ from pcapkit.corekit.infoclass import FinalisedState, Info from pcapkit.utilities.compat import final + from pcapkit.utilities.exceptions import InfoError class Parent(Info): x: int @@ -470,15 +456,15 @@ class Parent(Info): self.assertNotIn('__final__', Parent.__dict__) @final - class Escaped(Parent): + class Mismarked(Parent): pass - self.assertIn('__final__', Escaped.__dict__) - self.assertEqual(Escaped.__finalised__, FinalisedState.BASE) + self.assertIn('__final__', Mismarked.__dict__) + self.assertNotIn('__finalised__', Mismarked.__dict__) - # No InfoError, despite ``Escaped`` carrying its own ``__final__`` and - # never having been run through ``info_final``. - self.assertEqual(Escaped(1).to_dict(), {'x': 1}) + with self.assertRaises(InfoError) as caught: + Mismarked(1) + self.assertIn('Mismarked', str(caught.exception)) @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')