fix(fields): distinguish a malformed template from a negative length in FieldBase.length

pcapkit/corekit/fields/field.py

@@ -4,6 +4,7 @@
 import abc
 import contextlib
 import contextvars
+import re
 import struct
 from typing import TYPE_CHECKING, Generic, TypeVar, cast
 
@@ -198,6 +199,31 @@ def _zero_pad_budget() -> 'Iterator[list[int]]':
         _zero_pad_ledger.reset(token)
 
 
+#: Pattern[str]: Matches a leading, optionally byte-order-prefixed negative
+#: count in a :attr:`FieldBase.template`, e.g. the ``-1`` in ``'-1s'`` or in
+#: ``'>-1s'``.
+#:
+#: Every template this package builds from a resolved field length is
+#: ``f'{length}s'`` (see :mod:`pcapkit.corekit.fields.strings`,
+#: :mod:`~pcapkit.corekit.fields.misc`, :mod:`~pcapkit.corekit.fields.collections`
+#: and, before it gets byte-order-prefixed, :mod:`~pcapkit.corekit.fields.numbers`
+#: -- :meth:`NumberField.build_template <pcapkit.corekit.fields.numbers.
+#: NumberField.build_template>`'s ``else`` arm), so a negative resolved length
+#: always leaves that literal ``-`` immediately before the digits. What
+#: :class:`~pcapkit.corekit.fields.numbers.NumberField` adds is a byte-order
+#: prefix -- one of ``'@=<>!'`` -- *in front of* that whole template
+#: (``f'{endian}{struct_fmt}'``), which is what a plain ``^-\d+`` misses: the
+#: prefix, not the minus sign, is what starts the string. The optional
+#: character class here accounts for exactly that one prefix position, so a
+#: negative count is recognised whether or not it was byte-order-prefixed, while
+#: still refusing anything else -- a bare byte-order character with no minus
+#: sign after it (``'>Xs'``), or any other malformed template -- which is what
+#: makes checking for it a reliable way to tell those cases apart from each
+#: other *before* :func:`struct.calcsize` is asked to size either one. See
+#: #825, #827.
+_RE_NEGATIVE_LENGTH_TEMPLATE = re.compile(r'^[@=<>!]?-\d+')
+
+
 class FieldMeta(abc.ABCMeta, Generic[_T]):
     """Meta class to add dynamic support to :class:`FieldBase`.
 
@@ -276,16 +302,34 @@ def length(self) -> 'int':
                 (e.g. ``'-5s'``, from a ``length`` callback such as
                 ``lambda pkt: pkt['__length__']`` resolving below zero once
                 the buffer ran short of what the schema declared).
-                :func:`struct.calcsize` cannot size such a template and would
-                otherwise raise a bare :exc:`struct.error`, uncatchable as a
-                pcapkit-specific error. See #805.
+                :func:`struct.calcsize` cannot size such a template and raises
+                a bare :exc:`struct.error`, uncatchable as a pcapkit-specific
+                error; this re-raises it as the negative-length message below.
+                See #805.
+            ProtocolError: If :attr:`template` is otherwise malformed --
+                anything else :func:`struct.calcsize` cannot size, such as a
+                typo'd format character -- rather than the negative-length
+                message above, which would misreport the actual cause.
+                :func:`struct.calcsize` raises the identical bare
+                :exc:`struct.error` for both cases (measured:
+                ``calcsize('-1s')`` and ``calcsize('Xs')`` both raise ``bad
+                char in struct format``), so the two are told apart by
+                :data:`_RE_NEGATIVE_LENGTH_TEMPLATE` against :attr:`template`
+                itself -- which is known already, without needing anything
+                :func:`struct.calcsize`'s own error says -- rather than by the
+                error message. See #825.
 
         """
         try:
             return struct.calcsize(self.template)
         except struct.error as error:
+            if _RE_NEGATIVE_LENGTH_TEMPLATE.match(self.template) is not None:
+                raise ProtocolError(
+                    f'Field {self.name} resolved to a negative length; '
+                    f'template={self.template!r}'
+                ) from error
             raise ProtocolError(
-                f'Field {self.name} resolved to a negative length; '
+                f'Field {self.name} has a malformed template; '
                 f'template={self.template!r}'
             ) from error
 

tests/corekit/test_fields_field.py

@@ -879,10 +879,12 @@ class FieldBaseLengthNegativeResolvedLengthTests(unittest.TestCase):
     def setUp(self) -> None:
         purge_modules(['pcapkit'])
 
+        from pcapkit.corekit.fields.numbers import NumberField
         from pcapkit.corekit.fields.strings import BytesField
         from pcapkit.utilities.exceptions import ProtocolError
 
         self.BytesField = BytesField
+        self.NumberField = NumberField
         self.ProtocolError = ProtocolError
 
     def test_a_negative_resolved_length_raises_protocolerror_not_structerror(self) -> None:
@@ -919,6 +921,91 @@ def test_a_zero_or_positive_resolved_length_is_unaffected(self) -> None:
                     {'__length__': resolved})
                 self.assertEqual(field.length, resolved)
 
+    def test_a_malformed_template_raises_protocolerror_naming_the_template(self) -> None:
+        """A typo'd template is a category error, not a negative length (#825).
+
+        :func:`struct.calcsize` raises the identical bare :exc:`struct.error`
+        -- ``bad char in struct format`` -- for a malformed template as it
+        does for a negative one (measured on 3.14.7: ``calcsize('-1s')`` and
+        ``calcsize('Xs')`` both raise it). Pre-fix, :attr:`FieldBase.length
+        <pcapkit.corekit.fields.field.FieldBase.length>` caught that blanket
+        and always reported "resolved to a negative length", which would have
+        misdiagnosed this case. The template here (``'Xs'``) never matches
+        the leading-minus-sign shape a resolved negative count takes, so it
+        must fall to the distinct, template-naming message instead.
+        """
+        field = self.BytesField(length=4)
+        field.name = 'weird'
+        field._template = 'Xs'  # a typo, not a resolved negative count
+
+        with self.assertRaises(self.ProtocolError) as ctx:
+            field.length  # noqa: B018 -- property access is the point
+
+        message = str(ctx.exception)
+        self.assertIn('Xs', message)
+        self.assertNotIn('negative length', message)
+
+        # must not be a bare struct.error, and must still chain to the real
+        # one struct.calcsize actually raised.
+        self.assertNotIsInstance(ctx.exception, struct.error)
+        self.assertIsInstance(ctx.exception.__cause__, struct.error)
+
+    def test_a_byte_order_prefixed_negative_length_is_still_reported_as_negative(self) -> None:
+        """A prefixed negative template must not be misdiagnosed as malformed (#827).
+
+        :class:`~pcapkit.corekit.fields.numbers.NumberField` builds its
+        template as ``f'{endian}{struct_fmt}'``
+        (:meth:`NumberField.__init__ <pcapkit.corekit.fields.numbers.
+        NumberField.__init__>`) over the same ``f'{length}s'`` fall-through
+        every other field uses (:meth:`NumberField.build_template
+        <pcapkit.corekit.fields.numbers.NumberField.build_template>`'s
+        ``else`` arm) -- so a negative resolved length comes out prefixed,
+        e.g. ``'>-1s'``, not bare ``'-1s'``. Uses the real ``NumberField``
+        construction path rather than hand-setting ``_template``, so this
+        would catch a future change to how templates are assembled, not
+        just to the regex.
+        """
+        field = self.NumberField(length=-1)
+        field.name = 'number'
+
+        self.assertEqual(field.template, '>-1s')
+        with self.assertRaises(self.ProtocolError) as ctx:
+            field.length  # noqa: B018 -- property access is the point
+
+        message = str(ctx.exception)
+        self.assertIn('negative length', message)
+        self.assertNotIn('malformed', message)
+        self.assertNotIsInstance(ctx.exception, struct.error)
+        self.assertIsInstance(ctx.exception.__cause__, struct.error)
+
+    def test_negative_and_malformed_templates_are_reported_distinctly(self) -> None:
+        """The two categories never trade messages, and neither leaks a bare
+        :exc:`struct.error` -- across both, not just one.
+        """
+        cases = {
+            'negative': (self.BytesField(length=lambda pkt: pkt['__length__'])(
+                {'__length__': -1}), None),
+            'malformed': (self.BytesField(length=4), 'Xs'),
+        }
+        for label, (field, forced_template) in cases.items():
+            with self.subTest(case=label):
+                field.name = 'field'
+                if forced_template is not None:
+                    field._template = forced_template
+
+                with self.assertRaises(self.ProtocolError) as ctx:
+                    field.length  # noqa: B018 -- property access is the point
+
+                message = str(ctx.exception)
+                if label == 'negative':
+                    self.assertIn('negative length', message)
+                else:
+                    self.assertNotIn('negative length', message)
+                    self.assertIn(field.template, message)
+
+                self.assertNotIsInstance(ctx.exception, struct.error)
+                self.assertIsInstance(ctx.exception.__cause__, struct.error)
+
 
 if __name__ == '__main__':
     unittest.main()