fix(corekit): finalise each non-final Info subclass on its own state (#1490)

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')

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__ <pcapkit.corekit.infoclass.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()

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."""

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')