diff --git a/docs/source/pcapkit/protocols/protocol.rst b/docs/source/pcapkit/protocols/protocol.rst index 8a578c63c1..cacbe1cd29 100644 --- a/docs/source/pcapkit/protocols/protocol.rst +++ b/docs/source/pcapkit/protocols/protocol.rst @@ -67,6 +67,8 @@ Internal Definitions below. :no-value: .. autoattribute:: __keywords__ :no-value: + .. autoattribute:: __own_make_keywords__ + :no-value: .. autoattribute:: __schema__ :no-value: @@ -234,6 +236,7 @@ Internal Definitions .. py:attribute:: __layer__ __proto__ __keywords__ + __own_make_keywords__ __schema__ __header__ _data @@ -252,7 +255,8 @@ Internal Definitions :no-contents-entry: :attr:`~Protocol.__layer__`, :attr:`~Protocol.__proto__`, - :attr:`~Protocol.__keywords__`, :attr:`~Protocol.__schema__`, + :attr:`~Protocol.__keywords__`, :attr:`~Protocol.__own_make_keywords__`, + :attr:`~Protocol.__schema__`, :attr:`~Protocol.__header__`, :attr:`~Protocol._data`, :attr:`~Protocol._file`, :attr:`~Protocol._info`, :attr:`~Protocol._next`, :attr:`~Protocol._protos`, diff --git a/pcapkit/protocols/internet/ipv6_ext.py b/pcapkit/protocols/internet/ipv6_ext.py index a89f4dafe2..5cb5145a0f 100644 --- a/pcapkit/protocols/internet/ipv6_ext.py +++ b/pcapkit/protocols/internet/ipv6_ext.py @@ -236,6 +236,12 @@ class keywords implement it. """ + #: Every subclass overrides :meth:`make` with its own fields and forwards + #: nothing to this one, so this class's ``len`` and ``data`` are accepted + #: only by this class itself. Without the marker, a subclass whose ``make`` + #: does not declare them would accept and then drop them (:issue:`2286`). + __own_make_keywords__ = True + ########################################################################## # Properties. ########################################################################## @@ -500,10 +506,14 @@ def make(self, *, construction**: a plain ``IPv6_Ext(next=..., len=..., data=..., payload=...)`` raises ``UnsupportedCall: IPv6_Ext: unexpected keyword(s)``. The - ``__keywords__`` escape hatch would cover that too, with no - difference for subclasses: signatures and ``__keywords__`` are - both unioned down the MRO, so every subclass's allowlist takes - these names either way. + ``__keywords__`` escape hatch would cover that too, but a set + there is unioned down the MRO, so every subclass would accept + these names and drop them. A ``make`` signature can be kept from + subclasses instead: this class sets + :attr:`ProtocolBase.__own_make_keywords__ + `, + so a subclass that overrides ``make`` accepts ``len`` or ``data`` + only where its own ``make`` declares it (:issue:`2286`). They are keyword-*only* because this class is also a base and each subclass's ``make`` puts different names in the same positions. As diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index e25506d6b7..6393e892a0 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -281,7 +281,11 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': The walk covers the whole MRO rather than the most derived override of each method, so that a subclass declaring its own keyword and forwarding the rest - to its parent does not make the parent's keywords unreachable. + to its parent does not make the parent's keywords unreachable. The one + exception is a class that sets :attr:`ProtocolBase.__own_make_keywords__ + `: its + ``make`` counts only while it is the ``make`` of ``cls``, not once a + subclass overrides it (:issue:`2286`). """ try: @@ -291,6 +295,9 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': unchecked = False names = set(OUT_OF_BAND_KEYWORDS) + # NOTE: The class whose ``make`` is the one ``cls`` resolves to; any other + # ``make`` in the MRO has been overridden. + make_owner = next((klass for klass in cls.__mro__ if 'make' in klass.__dict__), None) for klass in cls.__mro__: # NOTE: A keyword read out of ``**kwargs`` by name is invisible to # :func:`inspect.signature`, so the class lists it in ``__keywords__``. @@ -323,6 +330,13 @@ def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': if func is None: continue + # NOTE: A ``make`` whose keywords its class keeps to itself is read + # only while it is the one ``cls`` uses. Read from ``__dict__`` so + # the marker, like the :obj:`None` opt-out above, is not inherited. + if (method == 'make' and klass is not make_owner + and klass.__dict__.get('__own_make_keywords__', False)): + continue + try: signature = inspect.signature(func) except (TypeError, ValueError): # pragma: no cover @@ -556,6 +570,18 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta): #: classes that can have it. __keywords__: 'Optional[frozenset[str]]' = frozenset() + #: Whether the parameters of this class's own :meth:`make` are accepted only + #: while that ``make`` is the one in use. By default + #: :func:`_declared_keywords` takes every ``make`` in the MRO, so a subclass + #: that overrides ``make`` and forwards the rest to its parent keeps the + #: parent's keywords. A base whose subclasses override ``make`` *without* + #: forwarding sets this, so that its keywords are refused for them rather than + #: accepted and dropped -- :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext` + #: does, for ``len`` and ``data`` (:issue:`2286`). Read from the defining + #: class alone, so it is not inherited, and it leaves :attr:`__keywords__` + #: and the other construction methods unaffected. + __own_make_keywords__: 'bool' = False + #: Whether this instance is being rebuilt by :meth:`from_data` from a parsed #: data model, as against constructed from keywords somebody wrote. It governs #: only whether the construction keyword check of :meth:`__init__` raises or diff --git a/tests/protocols/internet/test_ipv6_ext_unit.py b/tests/protocols/internet/test_ipv6_ext_unit.py index 94757b6eb6..88fa52398d 100644 --- a/tests/protocols/internet/test_ipv6_ext_unit.py +++ b/tests/protocols/internet/test_ipv6_ext_unit.py @@ -757,6 +757,36 @@ def test_an_absent_or_unregistered_alias_uses_the_generic_rule(self) -> None: # ``read`` with no explicit length falls back to ``len(self)`` self.assertEqual(inst.read().length, 16) + def test_base_make_keywords_are_refused_where_a_subclass_ignores_them(self) -> None: + """#2286: without ``__own_make_keywords__``, the base ``make``'s ``len`` + and ``data`` stay in every subclass's construction allowlist, so + ``IPv6_Route(len=99)`` builds the same octets as ``IPv6_Route()``. With + the marker, a subclass accepts each only if its own ``make`` declares + it, and accepts the ``next`` they all declare; the base keeps all three. + """ + import inspect + + from pcapkit.const.reg.transtype import TransType + from pcapkit.protocols.internet.ipv6_ext import IPv6_Ext + from pcapkit.protocols.protocol import _declared_keywords + from pcapkit.utilities.exceptions import UnsupportedCall + + self.assertTrue({'next', 'len', 'data'} <= _declared_keywords(IPv6_Ext)) + self.assertEqual(bytes(IPv6_Ext(next=TransType.TCP, len=0, data=b'\x00' * 6, + version=6))[:2], bytes([int(TransType.TCP), 0])) + + for klass in self._family(): + own = inspect.signature(klass.make).parameters + for name, value in (('len', 99), ('data', b'zzzz')): + with self.subTest(klass=klass.__name__, keyword=name): + if name in own: + self.assertIn(name, _declared_keywords(klass)) + continue + with self.assertRaisesRegex(UnsupportedCall, f"unexpected keyword\\(s\\): '{name}'"): + klass(**{name: value}) + with self.subTest(klass=klass.__name__, keyword='next'): + self.assertIsInstance(bytes(klass(next=TransType.TCP)), bytes) + def test_length_hint_is_the_eight_octet_rfc_6564_minimum(self) -> None: """Eight: :rfc:`6564#section-4`'s Hdr Ext Len excludes the first 8 octets, so no conforming header is shorter (:rfc:`8200#section-4`). diff --git a/tests/protocols/test_construction_keyword_check_unit.py b/tests/protocols/test_construction_keyword_check_unit.py index d18ecdd363..2c445738ac 100644 --- a/tests/protocols/test_construction_keyword_check_unit.py +++ b/tests/protocols/test_construction_keyword_check_unit.py @@ -426,6 +426,51 @@ def make(self, ham: 'bytes' = b'ab', **kwargs: 'Any') -> 'Any': with self.subTest(keyword=name): self.assertIn(name, accepted) + def test_a_base_may_keep_its_make_keywords_to_itself(self) -> None: + """``__own_make_keywords__`` drops a base's ``make`` once it is overridden. + + For a base whose subclasses replace ``make`` without forwarding to it, as + :class:`~pcapkit.protocols.internet.ipv6_ext.IPv6_Ext`'s do (#2286). The + base's ``make`` counts for the base itself and for a subclass that does + not override it, and the marker is not inherited: an overriding + subclass's own ``make`` is handed down as usual. ``__keywords__`` is + unaffected. + + """ + from pcapkit.protocols.protocol import _declared_keywords + from pcapkit.utilities.exceptions import UnsupportedCall + + base = _protocol_class(__own_make_keywords__=True, + __keywords__=frozenset({'eggs'})) + + class Inheriting(base): # type: ignore[misc,valid-type] + pass + + class Overriding(base): # type: ignore[misc,valid-type] + def make(self, ham: 'bytes' = b'ab', **kwargs: 'Any') -> 'Any': + return super().make(spam=ham) # the base's name, never the caller's + + class Grandchild(Overriding): # type: ignore[misc,valid-type] + def make(self, beans: 'bytes' = b'ab', **kwargs: 'Any') -> 'Any': + return super().make(ham=beans, **kwargs) + + for klass in (base, Inheriting): + with self.subTest(klass=klass.__name__): + self.assertIn('spam', _declared_keywords(klass)) + self.assertEqual(bytes(Inheriting(spam=b'xy')), b'xy') + + for klass, own in ((Overriding, {'ham'}), (Grandchild, {'ham', 'beans'})): + with self.subTest(klass=klass.__name__): + accepted = _declared_keywords(klass) + self.assertNotIn('spam', accepted) + self.assertTrue(own | {'eggs'} <= accepted) + + with self.assertRaisesRegex(UnsupportedCall, r"unexpected keyword\(s\): 'spam'"): + Overriding(spam=b'xy') + # Its own keyword, and the base's ``__keywords__``. + self.assertEqual(bytes(Overriding(ham=b'xy', eggs=1)), b'xy') + self.assertEqual(bytes(Grandchild(beans=b'xy')), b'xy') + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class ScopeTests(unittest.TestCase):