fix(tcp): seed TCP.read's flag accumulator with Enum_Flags(0), not a no-op cast (#616)

pcapkit/protocols/transport/tcp.py

@@ -478,7 +478,19 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_TCP':
         )
 
         # connection control flags
-        _flag = cast('Enum_Flags', 0)
+        #
+        # NOTE: ``Enum_Flags(0)``, not ``cast('Enum_Flags', 0)``. :func:`typing.cast` is a
+        # runtime no-op -- it returns its second argument unchanged -- so the accumulator
+        # used to stay the plain :class:`int` ``0`` for a segment whose flags octet is all
+        # zero. Nothing then promoted it, because the ``|=`` below is the only promotion and
+        # it never runs; ``Enum_Flags.SYN in self._flags`` raised ``TypeError: argument of
+        # type 'int' is not a container or iterable`` instead of answering. A segment with
+        # any flag set masked the defect entirely. :class:`Enum_Flags` is an
+        # :class:`aenum.IntFlag` and declares no ``_missing_`` of its own, so
+        # ``Enum_Flags(0)`` is a valid flagless member that still compares equal to ``0``
+        # and still ORs as before; only the type, and hence the ``repr``, differs. That is
+        # what :attr:`connection` already advertises it returns. C.f. #616.
+        _flag = Enum_Flags(0)
         for key, val in schema.flags.items():
             if val == 1:
                 _flag |= Enum_Flags.get(key.upper())
@@ -580,11 +592,12 @@ def make(self,
         # so this keeps the newly reachable no-SYN-no-ACK case raising the library's
         # documented error rather than a bare Python one. :class:`Enum_Flags` is an
         # :class:`aenum.IntFlag`, so ``Enum_Flags(0)`` is a valid flagless member that
-        # still compares equal to ``0`` and still ORs as before. The read path keeps its
-        # ``cast`` at the top of ``read``: it cannot reach these branches, because
+        # still compares equal to ``0`` and still ORs as before. :meth:`read` seeds itself
+        # the same way; it kept the ``cast`` until #616, on the grounds that
         # ``mptcp_data_selector`` rejects a flagless MP_JOIN before ``_read_mptcp_join``
-        # runs, and changing it would alter the ``connection`` value reported for every
-        # flagless parsed segment. C.f. #587.
+        # runs, so no caller could reach the ``TypeError``. That made the read path latent
+        # rather than sound -- latent by virtue of a guard in another file -- which is a
+        # fragile reason for a ``TypeError`` not to happen. C.f. #587, #616.
         _flag = Enum_Flags(0)
         for key, val in flags.items():
             if val == 1:

tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py

@@ -615,13 +615,13 @@ def test_the_flags_accumulator_is_a_real_enum_member(self) -> None:
             self._info = self.unpack(...)   # -> read(), which sets _flags again
 
         so a fully constructed instance's ``_flags`` is whatever the **read** path left,
-        and the read path still seeds with ``cast('Enum_Flags', 0)`` -- measured: a
-        flagless ``TCP(...)`` reports ``_flags`` as the plain ``int`` ``0``. That read
-        path is deliberately unchanged (see
-        :meth:`TCPMPTCPJoinReadPathUnitTests.test_a_flagless_segment_is_rejected_by_the_schema_selector`),
-        and it cannot affect option construction, which has already finished by then.
-        Observing ``make`` alone is the only way to assert on the value the option makers
-        actually see.
+        not what ``make`` computed. When this test was written the read path also seeded
+        with ``cast('Enum_Flags', 0)``, so a flagless ``TCP(...)`` reported ``_flags`` as
+        the plain ``int`` ``0``; #616 changed that seed to ``Enum_Flags(0)``, so both paths
+        now leave an ``Enum_Flags`` member. The re-parse still overwrites what ``make``
+        assigned either way, and it cannot affect option construction, which has already
+        finished by then. Observing ``make`` alone is the only way to assert on the value
+        the option makers actually see.
 
         """
         from pcapkit.const.tcp.flags import Flags as Enum_Flags
@@ -675,9 +675,15 @@ def test_a_flagless_segment_is_rejected_by_the_schema_selector(self) -> None:
 
         ``mptcp_data_selector`` cannot choose an MP_JOIN schema with neither flag set, so
         it raises :exc:`~pcapkit.utilities.exceptions.FieldError` there. That is why
-        ``_read_mptcp_join``'s closing ``ProtocolError`` remains unreachable and why its
-        ``cast('Enum_Flags', 0)`` was deliberately left as it was -- the make path needed
-        the ``Enum_Flags(0)`` seed; this one does not.
+        ``_read_mptcp_join``'s closing ``ProtocolError`` is unreachable from a caller, and
+        it is still unreachable: this guard is what #616 measured as identical either side
+        of changing the read path's seed. That unreachability was the original reason for
+        leaving the seed as ``cast('Enum_Flags', 0)``, and #616's reason for changing it
+        anyway -- a ``TypeError`` averted only by a guard in a different file is averted
+        fragilely. ``read`` seeds ``Enum_Flags(0)`` now, as ``make`` has since #597, so
+        calling the dispatcher directly on a flagless parsed segment reaches its
+        ``ProtocolError`` rather than a bare ``TypeError``; see
+        :meth:`tests.protocols.transport.test_tcp_udp_unit.TCPUDPUnitTests.test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`.
 
         """
         from pcapkit.protocols.transport.tcp import TCP

tests/protocols/transport/test_tcp_udp_unit.py

@@ -992,12 +992,16 @@ def mark(schema, length: int, subtype: MPTCPOption):
         # dispatchers read the same attribute with the same ``in`` tests, and ``make`` is
         # the public entry point that assigns it, so this installs the ``Enum_Flags``
         # member production assigns instead of a ``set`` that merely answers ``in``. C.f.
-        # #603. It has to be ``make`` and not a parsed segment for the flagless case
-        # further down: ``read`` still accumulates into ``cast('Enum_Flags', 0)``, a runtime
-        # no-op, so a flagless *parsed* instance carries a plain ``int`` and
-        # ``_read_mptcp_join`` raises ``TypeError`` rather than its documented
-        # ``ProtocolError``. That is deliberate and unreachable from a caller --
-        # ``mptcp_data_selector`` rejects a flagless MP_JOIN in the schema layer first, and
+        # #603. Until #616 it *had* to be ``make`` rather than a parsed segment for the
+        # flagless case further down: ``read`` seeded its accumulator with
+        # ``cast('Enum_Flags', 0)``, a runtime no-op, so a flagless *parsed* instance
+        # carried a plain ``int`` and ``_read_mptcp_join`` raised ``TypeError`` instead of
+        # its documented ``ProtocolError``. #616 made ``read`` seed ``Enum_Flags(0)`` too,
+        # so either entry point serves now; ``make`` stays because these rows are about the
+        # dispatchers rather than about parsing, and
+        # :meth:`test_a_flagless_segment_seeds_its_connection_flags_as_an_enum` below
+        # covers the parsed side. Either way ``mptcp_data_selector`` rejects a flagless
+        # MP_JOIN in the schema layer first, and
         # ``tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit`` pins both
         # halves against real segments.
         proto.make(syn=True)
@@ -1523,6 +1527,122 @@ def test_unregistered_mptcp_subtype_does_not_mutate_the_class_registry(self) ->
         finally:
             registry.pop(MPTCPOption(0xF), None)
 
+    def test_a_flagless_segment_seeds_its_connection_flags_as_an_enum(self) -> None:
+        """A segment whose flags octet is all zero still parses to an ``Enum_Flags``. C.f. #616.
+
+        :meth:`TCP.read <pcapkit.protocols.transport.tcp.TCP.read>` seeded its flag
+        accumulator with ``cast('Enum_Flags', 0)``. :func:`typing.cast` is a runtime
+        no-op -- it returns its second argument unchanged -- so the accumulator began
+        life as the plain :class:`int` ``0``. The ``|=`` in the loop below it is the
+        only thing that promotes it to an
+        :class:`~pcapkit.const.tcp.flags.Flags` member, and that never runs when no
+        flag bit is set, so a flagless segment left ``self._flags`` an :obj:`int`.
+        ``Enum_Flags.SYN in self._flags`` then raised ``TypeError: argument of type
+        'int' is not a container or iterable`` rather than answering. Any flag at all
+        masked it, which is why it survived a file at 100% coverage.
+
+        The type assertions are the load-bearing ones. ``0 == Enum_Flags(0)`` is
+        :data:`True`, so ``assertEqual(proto.connection, 0)`` passes on both sides of
+        the fix and cannot discriminate -- and neither can a membership test alone, on
+        the flagful rows. Hence ``assertIs(type(...), Enum_Flags)``, which is what the
+        :attr:`~pcapkit.protocols.transport.tcp.TCP.connection` property has always
+        advertised it returns.
+
+        The last block pins the consequence rather than the type. ``_read_mptcp_join``
+        chooses between :rfc:`8684` section 3.2's three MP_JOIN layouts by testing
+        ``self._flags`` for SYN and ACK, and falls through to the library's own
+        ``ProtocolError`` when neither is set. On an :obj:`int` it died on a bare
+        :exc:`TypeError` two lines earlier instead. That branch is not reachable from
+        a caller -- :func:`~pcapkit.protocols.schema.transport.tcp.mptcp_data_selector`
+        rejects a flagless MP_JOIN with a ``FieldError`` before the dispatcher runs --
+        so this was latent, but latent only by virtue of a guard in a different file.
+        The instance is parsed through ``TCP(...)`` rather than having ``_flags``
+        written onto a bare ``object.__new__(TCP)``, for the reason #603 and #612 give
+        in :func:`mptcp_option` above: a hand-written attribute proves nothing about
+        what production puts there.
+
+        The closing block pins the one difference a consumer can see, so that it is a
+        recorded decision rather than a silent change: a flagless segment's
+        ``connection`` now dumps as the string ``'Flags::None [0]'`` where it dumped as
+        the number ``0``. That is not a regression so much as the removal of an
+        inconsistency -- the field was a number for a flagless segment and a string for
+        every other one -- but the literal ``None`` in it is a rendering defect of
+        :func:`~pcapkit.dumpkit.common.make_dumper`'s hook, which interpolates ``o.name``
+        without accounting for a nameless composite member. It would do the same to any
+        zero-valued flag enumeration in the library, so it is left to its own change.
+
+        """
+        import struct
+
+        import dictdumper
+
+        from pcapkit.const.tcp.flags import Flags as Enum_Flags
+        from pcapkit.const.tcp.mp_tcp_option import MPTCPOption
+        from pcapkit.const.tcp.option import Option
+        from pcapkit.dumpkit.common import make_dumper
+        from pcapkit.protocols.transport.tcp import TCP
+        from pcapkit.utilities.exceptions import ProtocolError
+
+        def segment(flags_octet: 'int') -> 'bytes':
+            """A bare 20-octet header -- data offset 5, no options -- and a flags octet."""
+            return struct.pack('!HHIIBBHHH', 1, 2, 0, 0, 5 << 4, flags_octet, 0, 0, 0)
+
+        # (flags octet, the Enum_Flags the octet's low bits mean)
+        cases = [
+            (0x00, Enum_Flags(0)),
+            (0x02, Enum_Flags.SYN),
+            (0x10, Enum_Flags.ACK),
+            (0x12, Enum_Flags.SYN | Enum_Flags.ACK),
+        ]
+        for octet, expected in cases:
+            with self.subTest(flags_octet=octet):
+                raw = segment(octet)
+                proto = TCP(raw, len(raw))
+
+                # The accumulator, the property, and the data field are one value.
+                self.assertIs(type(proto._flags), Enum_Flags)
+                self.assertIs(type(proto.connection), Enum_Flags)
+                self.assertIs(type(proto.info.connection), Enum_Flags)
+                self.assertEqual(proto._flags, expected)
+                self.assertIs(proto.connection, proto._flags)
+                self.assertIs(proto.info.connection, proto._flags)
+
+                # Membership answers instead of raising -- the whole point of #616.
+                self.assertEqual(Enum_Flags.SYN in proto.connection, bool(octet & 0x02))
+                self.assertEqual(Enum_Flags.ACK in proto.connection, bool(octet & 0x10))
+
+                # The numeric value is unchanged by the fix, which is exactly why the
+                # type is what has to be asserted.
+                self.assertEqual(int(proto.connection), int(expected))
+                self.assertEqual(proto.connection, int(expected))
+
+        # The flagless segment reaches the dispatcher's own fall-through error rather
+        # than a bare TypeError from the SYN membership test above it.
+        flagless = segment(0x00)
+        proto = TCP(flagless, len(flagless))
+        self.assertIs(type(proto._flags), Enum_Flags)
+        schema = DummyData(kind=Option.Multipath_TCP, subtype=MPTCPOption.MP_JOIN)
+        with self.assertRaisesRegex(ProtocolError, 'invalid flags combination'):
+            proto._read_mptcp_join(schema, options=DummyData())  # type: ignore[arg-type]
+
+        # The one place the change is observable to a consumer, pinned here so it cannot
+        # drift silently. ``make_dumper``'s hook renders any enum member as
+        # ``Type::name [value]``, and an ``aenum.IntFlag`` pseudo-member carrying no bits
+        # has ``name is None`` -- so a flagless segment dumps as the string
+        # ``'Flags::None [0]'`` where it used to dump as the number ``0``. Note what that
+        # replaced: ``connection`` was a JSON *number* for a flagless segment and a
+        # *string* for every other one, so the field is consistently typed now rather than
+        # switching type with the flags. The literal ``None`` is a rendering defect in
+        # ``pcapkit.dumpkit.common`` -- it reads ``o.name`` without accounting for a
+        # nameless composite member, and would do the same to any zero-valued flag enum in
+        # the library -- so it is left to its own change rather than fixed from here.
+        # ``self`` is reached only by the fallback ``super()`` call at the end of the hook,
+        # which an enum never gets to, so the unbound form needs no dumper instance.
+        self.assertIsNone(Enum_Flags(0).name)
+        hook = make_dumper(dictdumper.JSON).object_hook
+        self.assertEqual(hook(None, proto.info.connection), 'Flags::None [0]')
+        self.assertEqual(hook(None, Enum_Flags.ACK), 'Flags::ACK [2048]')
+
 
 if __name__ == '__main__':
     unittest.main()