Parse malformed UTF-8 in MatchResult::from_str without panicking (#114) - #122
Conversation
with_reduced_quantity silently no-opped (`_ => self.clone()`) for TrailingStop, PeggedOrder, and MarketToLimit, so a partial fill recorded the executed quantity and decremented the level's visible counter while the resting maker kept its ORIGINAL size. A later taker could execute the same depth again, breaking quantity conservation and underflowing the advisory counter; UpdateQuantity inherited the same silent no-op. The helper is now exhaustive over all seven variants: the three single-quantity types rewrite their quantity (preserving trail and peg parameters -- a fill must never reprice), and ReserveOrder resizes its visible tranche keeping hidden depth and replenish policy, mirroring IcebergOrder (UpdateQuantity path only; its match path was already correct). The match_against and refresh_iceberg fallback arms are also explicit now, so a future variant is a compile error instead of a silent double-execution. Documents the visible-tranche semantics of UpdateQuantity on the public OrderUpdate enum. Regression tests: two consecutive takers against each fixed variant conserve the original quantity across the advisory counter, the live queue, and a snapshot JSON round-trip; UpdateQuantity decrease keeps FIFO position and increase demotes for each variant. Closes #118
…t panicking The parser advanced byte offsets one at a time and sliced with s[pos..] / s[i..]; a multibyte Unicode scalar before the next delimiter put an offset inside the scalar and the slice panicked on a non-char-boundary. FromStr accepts untrusted text and promises Result, so malformed input must never unwind. The scanner now operates on s.as_bytes(): all structural delimiters (=, ;, [, ]) and literal prefixes are ASCII, and every byte of a multibyte scalar is >= 0x80, so a byte comparison never mistakes an interior byte for a delimiter. &str slices are taken only at ASCII-delimiter positions or the string end -- always char boundaries. Every index is bounds-guarded. Malformed input returns the existing InvalidFormat / InvalidFieldValue / MissingField errors deterministically; canonical ASCII Display output round-trips unchanged. No unsafe, no new deps, no lossy normalization. Tests: multibyte scalars (2/3/4-byte) in field names, values, trade bracket data, and filled-order-id lists; unterminated multibyte brackets; Display round-trip; two proptest properties (1024 cases each) asserting from_str never panics on arbitrary and delimiter-dense UTF-8. The old scanner body fails the regression with the char-boundary panic. Closes #114
joaquinbejar
left a comment
There was a problem hiding this comment.
The UTF 8 scanner now handles multibyte input without panicking and the focused parser suite passes. One binding safety rule is still violated by the new direct indexing, so the intended effective verdict is REQUEST_CHANGES. GitHub does not accept that event from the PR author, so this review is submitted as COMMENT.
Every index and range slice in the scanner now goes through .get() with None mapped to InvalidFormat (or a loop restructure over bytes.get(i)), per the global rule banning unchecked [] indexing in production code -- bounds-guarded or not, a later offset change can no longer reintroduce a panic path.
|
Thanks for the review. The indexing rule violation is fixed in 8b48449, the whole scanner is .get() based now. |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (58.64%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
joaquinbejar
left a comment
There was a problem hiding this comment.
The UTF 8 scanner now uses guarded accesses throughout, the prior warning is resolved, focused parser validation and CI pass, and I found no new issues. The intended effective verdict is APPROVE. GitHub does not accept an approval from the PR author, so this review is submitted as COMMENT.
Summary
MatchResult::from_stradvanced byte offsets one at a time and sliced withs[pos..]/s[i..]; a multibyte Unicode scalar before the next delimiterlanded an offset inside the scalar and panicked on a non-char-boundary slice.
FromStraccepts untrusted text and promisesResult— it must never unwind.The scanner now works on
s.as_bytes()and slices only at ASCII-delimiterpositions, which are always char boundaries.
Stack
stack/1-118-reduce-all-variantsmainis cumulative; review against the base branch.Changes
FromStrimpl:find_next_field, thefield-name scan, and both bracket scanners (trades, filled-order ids).
Structural delimiters (
=,;,[,]) and literal prefixes are ASCII,and every byte of a multibyte scalar is ≥ 0x80, so a byte comparison never
mistakes an interior byte for a delimiter.
architectreview);malformed input returns the existing
InvalidFormat/InvalidFieldValue/MissingFielderrors deterministically. Nounsafe, no new deps, no lossynormalization, structural validation unchanged.
Displayoutput round-trips unchanged.Testing
data, filled-order-id lists; unterminated multibyte brackets — all
Err,no panic (old scanner body panics on these)
and delimiter-dense structural fuzz never panic
Display→FromStrround-trip with populated trades + filled ids454 tests passed; 0 failed (435 lib + 9 + 1 + 9 doctests).
Checklist
rules/global_rules.mdandCLAUDE.md.unwrap()/.expect()/ unchecked indexing in production codePriceLevelErrorvariants reused — no new variantsunsafe, no new dependencies (proptestalready a dev-dep)executionuntouched upward)Note: the
filled_order_idsbranch's tolerance of trailing content after]is pre-existing and out of scope here — it gets tightened in #116 (next in the
stack), which routes
FromStrthrough the invariant validator.Closes #114