1.0 redesign: unify parser fallibility — sweep 16 Option parsers to Result + one error model #85

New issue

Closed

opened 2026-06-27 11:53:34 +00:00 by p13marc · 0 comments

p13marc commented

2026-06-27 11:53:34 +00:00

(Migrated from github.com)

Strategic / pre-1.0. Breaking — last chance to fix parser fallibility before the 1.0 lock.

Problem — three return conventions + two error philosophies

parse() return types split three ways across the parsers:

Result<T, ParseError> (the #65 sweep — 5 modules): dnp3, ldap, kerberos, quic, smb.
crate::Result<T> (unified Error — 3 modules): icmp (parser.rs:17,28), dns (parser.rs:22,27), layers (mod.rs:278,285).
Option<T> (still — 16 modules): arp, cdp, dhcp, modbus, ssh (kexinit), ntp, tftp, ndp, lldp, wireguard, netbios_ns, stun, ssdp, rdp, snmp, radius.

So #65 fixed the binary wire parsers but left every Tier-2 protocol parser on Option — a caller can't tell "not my protocol" from "malformed/truncated."

Two error philosophies coexist:

A unified crate::Error with Module + ErrorCode (src/error.rs:62,122,184) used by http/tls/dns/icmp/layers/pcap/reassembler/tracker.
5 private enums all named ParseError (dnp3/ldap/kerberos/quic/smb) — and those 5 modules are not even in the Module enum (error.rs:62), so their failures can never be expressed as a unified Error. ErrorCode already has Parse/Truncated/Unsupported variants that duplicate what the per-module enums re-encode.

Proposed direction — pick ONE model for 1.0

Recommended: every fallible parser returns Result<T, ParseError> with a per-module ParseError that exposes the operationally-distinct failure modes (the #65 shape — it's strictly more informative than Option, and "not my protocol" becomes a named variant rather than ambiguous None). Then:

Sweep the 16 Option parsers to Result with a per-module ParseError (mirror the dnp3/smb pattern: Truncated{need,have} / BadMagic / protocol-specific variants).
Resolve the dual philosophy. Either (a) add the missing modules to Module and give every ParseError a From<ParseError> for crate::Error so both the typed and unified views work (recommended — keeps the rich per-module enum and lets ? bubble to crate::Error), or (b) drop the per-module enums in favor of crate::Error everywhere. (a) preserves the strong-typing #65 deliberately chose.
Don't break the trait sink. SessionParser/DatagramParser use the &mut Vec sink (correct, alloc-free) — this issue is only the free parse() functions, not the trait. Keep them distinct.

Why now

Post-1.0, changing a parser's return type is a hard break behind an extension shim. Pre-1.0 it's a mechanical sweep with a CHANGELOG recipe. This is the last cheap moment.

Relationship: completes #65 (which scoped only 5 modules) and unifies the error story #131 started.
Effort: M (mechanical but touches 16 modules + tests).

**Strategic / pre-1.0. Breaking — last chance to fix parser fallibility before the 1.0 lock.** ## Problem — three return conventions + two error philosophies **`parse()` return types split three ways** across the parsers: - **`Result<T, ParseError>`** (the #65 sweep — 5 modules): `dnp3`, `ldap`, `kerberos`, `quic`, `smb`. - **`crate::Result<T>`** (unified `Error` — 3 modules): `icmp` (`parser.rs:17,28`), `dns` (`parser.rs:22,27`), `layers` (`mod.rs:278,285`). - **`Option<T>`** (still — 16 modules): `arp`, `cdp`, `dhcp`, `modbus`, `ssh` (kexinit), `ntp`, `tftp`, `ndp`, `lldp`, `wireguard`, `netbios_ns`, `stun`, `ssdp`, `rdp`, `snmp`, `radius`. So #65 fixed the binary wire parsers but left **every Tier-2 protocol parser on `Option`** — a caller can't tell "not my protocol" from "malformed/truncated." **Two error philosophies coexist:** - A unified `crate::Error` with `Module` + `ErrorCode` (`src/error.rs:62,122,184`) used by http/tls/dns/icmp/layers/pcap/reassembler/tracker. - **5 private enums all named `ParseError`** (`dnp3`/`ldap`/`kerberos`/`quic`/`smb`) — and those 5 modules are **not even in the `Module` enum** (`error.rs:62`), so their failures can never be expressed as a unified `Error`. `ErrorCode` already has `Parse`/`Truncated`/`Unsupported` variants that duplicate what the per-module enums re-encode. ## Proposed direction — pick ONE model for 1.0 Recommended: **every fallible parser returns `Result<T, ParseError>` with a per-module `ParseError` that exposes the operationally-distinct failure modes** (the #65 shape — it's strictly more informative than `Option`, and "not my protocol" becomes a named variant rather than ambiguous `None`). Then: 1. **Sweep the 16 `Option` parsers to `Result`** with a per-module `ParseError` (mirror the dnp3/smb pattern: `Truncated{need,have}` / `BadMagic` / protocol-specific variants). 2. **Resolve the dual philosophy.** Either (a) add the missing modules to `Module` and give every `ParseError` a `From<ParseError> for crate::Error` so both the typed and unified views work (recommended — keeps the rich per-module enum *and* lets `?` bubble to `crate::Error`), or (b) drop the per-module enums in favor of `crate::Error` everywhere. (a) preserves the strong-typing #65 deliberately chose. 3. **Don't break the trait sink.** `SessionParser`/`DatagramParser` use the `&mut Vec` sink (correct, alloc-free) — this issue is only the **free `parse()` functions**, not the trait. Keep them distinct. ## Why now Post-1.0, changing a parser's return type is a hard break behind an extension shim. Pre-1.0 it's a mechanical sweep with a CHANGELOG recipe. This is the last cheap moment. Relationship: completes #65 (which scoped only 5 modules) and unifies the error story #131 started. Effort: M (mechanical but touches 16 modules + tests).