Refactor Expr::eval callstack to use Result instead of panic - #336
Draft
archaephyrryx wants to merge 11 commits into
Draft
archaephyrryx wants to merge 11 commits into
archaephyrryx wants to merge 11 commits into
Conversation
Adds a test-case that clarifies the scope of a bug noticed while implemention BSON, where unguarded subtraction that could underflow on nonconformant input bytes led to a panic rather than a recoverable decoder-error while running the doodle interpreter. Error exists for both `doodle::Expr` and `doodle::numeric::core::Expr` versions of subtraction.
Adds `doodle::error::EvalError` to bridge the gap between previous panicking behavior in `Expr::eval` and the sub-functions it calls, and a revised approach that turns each non-structural (i.e. data-based) panic in to a return-err case instead. The negative_size integration test now passes as a result of `value::__arith` returning err rather than panicking.
The two evaluators are hand-maintained mirrors. This harness runs ~75 Expr cases through both and compares the resulting Value, error message, or panic. Known drift is recorded as `Expect::Diverges` and fails once the evaluators start to agree, so it cannot be fixed silently: - AsChar on Usize (loc lacks the arm) - FlatMap with an EnumFromTo-returning lambda (loc lacks the arm) - FlatMapAccum/LeftFold over EnumFromTo (main requires Value::Seq) - Match on Permit(Err(Some(_))) (main coerces nominally, loc does not) Safety net for unifying the two evaluators. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AsU8/16/32/64, AsChar, the U16Be/Le..U64Be/Le byte-tuple decoders, and SubSeqInflate's back-reference expansion were duplicated near-verbatim between the two evaluators. Pull the common logic onto shared Value::cast_to_u8/16/32/64, Value::cast_to_char, Value::unwrap_byte_array::<N>, and a free decoder::sub_seq_inflate<V> generic over both value types, so both `eval` bodies now just call through in one or two lines. This is Stage 1 of unifying Expr::eval/eval_with_loc (Stage 0: the eval_parity_tests differential harness, 8f99b0a). It incidentally fixes a real divergence: loc's AsChar had no Usize arm, so `cast_to_char` now covers it and the as_char_usize parity case moves from `diverges` to `parity`. Byte-tuple panic messages for U64Be/Le are also corrected (they previously said "U32Be"/"U32Le", a copy-paste typo). No other behavior change; full workspace test suite and the parity test both pass.
Scope<'a> (decoder.rs) and LocScope<'a> (loc_decoder.rs), along with their Multi/Single/Decoder/View companions, were two hand-maintained mirrors differing only in their leaf value type (Value vs. ParsedValue). Replace both with a single generic family in scope.rs: GScope<'a, V>, GMultiScope<'a, V>, GSingleScope<'a, V>, GDecoderScope<'a, V>, GViewScope<'a, V>, and a generic ScopeEntry<V>, with Scope<'a>/MultiScope<'a>/... and LocScope<'a>/ LocMultiScope<'a>/... now type aliases instantiating V = Value / ParsedValue respectively. decoder.rs and loc_decoder.rs re-export the aliases so no external call site changes. This is Stage 2 of unifying Expr::eval/eval_with_loc (Stage 0: parity tests, 8f99b0a; Stage 1: shared leaf logic, 5413d39). The one representational change: LocMultiScope's entries used to store ParsedValue directly, whereas MultiScope stored Cow<Value>. The generic type uses Cow<V> uniformly, so LocMultiScope::push (which only ever took an owned ParsedValue) is now push_owned, matching MultiScope's existing push (borrowed) / push_owned (owned) split; the three call sites are updated accordingly. No behavior change: full workspace test suite (`cargo testall`), decode snapshot tests, and the eval_parity_tests differential harness all pass unchanged. Stage 3 (a single generic Expr::eval<V: EvalValue>) is next.
Expr::eval (decoder.rs, on Value) and Expr::eval_with_loc (loc_decoder.rs, on ParsedValue) were the last hand-maintained mirrors of one another. Both are now thin wrappers around a single new Expr::eval_generic<V: EvalValue> (new module decoder/eval.rs), parameterized over the leaf value type via a small EvalValue trait (from_evaluated, coerce_mapped_value, tuple_proj_raw/record_proj_raw, get_sequence, collect_fields, matches). impl EvalValue for Value is a thin identity wrapper around existing methods (decoder/value.rs); impl EvalValue for ParsedValue threads parse-location info through the same operations (loc_decoder.rs). eval_value/eval_value_with_loc and eval_lambda/ eval_lambda_with_loc are unified the same way. This is Stage 3 (final stage) of unifying Expr::eval/eval_with_loc (Stage 0: parity tests, 8f99b0a; Stage 1: shared leaf logic, 5413d39; Stage 2: generic Scope, 1fc7aca). Net effect: -743/+138 lines, with decoder.rs's and loc_decoder.rs's hand-written eval bodies (both ~300-450 lines) replaced by 3-line delegations. Per user-confirmed resolutions, this intentionally fixes 4 real behavioral divergences between the two evaluators (previously tracked as `diverges` cases in eval_parity_tests.rs, now `parity` cases since both now share one code path): - FlatMap: a lambda returning EnumFromTo is now supported by both (previously loc-only panicked). - FlatMapAccum/LeftFold: an EnumFromTo (not just Value::Seq) input is now accepted by both, via get_sequence() (previously main-only panicked). - Match/Destructure: Permit(Err(Some(_))) is now transparently unwrapped for pattern-matching by both, via a new ParsedValue::coerce_nominal_value mirroring Value's (previously loc-only failed to match, falling through to a wildcard/binding branch). This also fixes the same divergence in Decoder::Match at the format level, since ParsedValue::matches is shared by both call sites. - TupleProj: ParsedValue gains the Permit(Err(None)) => self pass-through that Value::_tuple_proj already had (found while writing the shared tuple_proj_raw), instead of panicking. No other behavior change: full workspace test suite (`cargo testall`, including all 23 decode snapshot tests) and the eval_parity_tests differential harness (now with zero known divergences) both pass unchanged.
SeqIx, SubSeq, and SubSeqInflate could all panic on data-reachable out-of-range
indices/offsets (a raw slice index, a `.nth().unwrap()`, or an internal
back-reference index into a not-yet-populated accumulator). Since Stage 3
unified Expr::eval/eval_with_loc into one Expr::eval_generic, this only needs
fixing in one place (src/decoder/eval.rs) instead of two.
New EvalError::SeqBounds(SeqBoundsError { op, index, len }) (SeqBoundsOp:
SeqIx/SubSeq/SubSeqInflate), mirroring ArithError's existing op+context shape.
check_index/check_sub_range in eval.rs validate bounds before calling into the
(still-panicking, now provably-safe-to-call) lower-level helpers.
SubSeqInflate's `length == 0` case is exempted from the check, since it never
dereferences `start` and previously succeeded silently for an out-of-range
`start` in that case too - preserved, not just converted to Err.
Also fixes a real off-by-one in seq_kind::sub_range's bounds assert (`start +
len < range.len()` rejected the valid full-range case `start + len ==
range.len()`; should be `<=`), found while designing the SubSeq bounds check.
Folded in per the original plan: Decoder::parse's and
Decoder::parse_with_loc's `WithRelativeOffset` arms (decoder.rs,
loc_decoder.rs - not covered by the Expr::eval unification, since these are
Decoder-level, not Expr-level) computed `base + offset` with an unchecked add,
which panics on overflow in debug builds and silently wraps in release
builds. Switched to `checked_add(...).unwrap_or(usize::MAX)`, routing an
overflow through the same already-handled `seek_to`/`bad_seek` error path as
any other out-of-range seek target, rather than either failure mode.
New tests: 5 direct (non-differential) `eval::tests` cases asserting the
actual Ok/Err outcome (not just that the two evaluators agree), plus 5 new
eval_parity_tests.rs regression cases covering the same scenarios. Full
workspace test suite (158 lib tests, all 23 decode snapshots) passes
unchanged.
Adopts the alternatives selected after reviewing Stage 3's and Group 5's design decisions: - EvalValue::coerce_mapped_value returns Coerced<&Self> again (not bare &Self), and gains a new extract_mapped_value(self) -> Value consuming method, restoring main's original owned-vs-borrowed clone-avoidance optimization in eval_value_generic (Cow::Owned skips a clone via the new method; Cow::Borrowed still has to clone at the leaf either way). ParsedValue's coerce_mapped_value is always `Coerced::pure` (never fallback), since - unlike Value's - it never actually unwraps Permit(Err(Some(_))); that asymmetry is pre-existing, not introduced here. TupleProj/RecordProj now thread the Coerced wrapper through their projection via `.map(...)` before discarding it, matching the original pre-unification structure. - Tried making the numeric StrictValue: TryFrom<&Self> bound a supertrait on EvalValue instead of repeating it on 4 functions; reverted after it broke inference at call sites (a real rustc limitation: the HRTB for<'x> combined with an associated-type-equality bound doesn't elaborate cleanly through a supertrait). Left as repeated where-clauses, documented why. - SeqBoundsOp/SeqBoundsError moved from decoder/value.rs into decoder/seq_kind.rs, and check_index/check_sub_range moved off of eval.rs as free functions onto ValueSeq itself (ValueSeq::check_index/ check_sub_range), for cohesion with the sequence type they check. - WithRelativeOffset's overflow case (decoder.rs, loc_decoder.rs) now constructs a dedicated BufferLimitError::OffsetOverflow (with a BufferKind::offset_overflow constructor) instead of reusing bad_seek with a usize::MAX sentinel, giving a clearer error message. Full workspace test suite (158 lib tests, all 23 decode snapshots) passes unchanged. Remaining triaged items (FindByKey/FlatMapList's non-generic handling and the extra clone it costs, SubSeqInflate's length==0 bounds exemption, and self-checking sub_range/sub_seq_inflate) are left for a follow-up decision - see conversation for the detailed write-up on each.
Item 14 (self-checking sequence ops): SeqKind::sub_seq, seq_kind::sub_range, and decoder::sub_seq_inflate now validate their own bounds and return Result<_, SeqBoundsError> instead of panicking, with eval.rs's SubSeq/ SubSeqInflate arms using `?` directly. This removes the now-redundant external ValueSeq::check_sub_range (deleted) - the check and the operation it guards were either duplicating each other or the panic was unreachable; folding them into one is strictly simpler either way. ValueSeq::check_index is kept, since SeqIx still needs an external check (cow_map/cow_remap's closures can't return Result). Item 11 (SubSeqInflate's length==0 edge case): rather than picking a side, sub_seq_inflate now permits an out-of-bounds `start` when `length == 0` (start is never dereferenced) exactly as before, but logs an error in debug builds only, so the case stays visible without becoming a hard behavior change. Items 5+9 (FindByKey/FlatMapList V-genericity): investigated concretely rather than doing a blanket "make everything generic" pass: - FindByKey gets the real fix: it now stays in `V`-space (via get_sequence()) instead of downgrading to plain Value up front, so (a) the lambda-eval closure no longer clones+re-wraps each element through eval_lambda_value_generic (removed, now dead) - it calls eval_lambda_generic directly - and (b) the matched result is built via a new EvalValue::lift_option, which preserves the matched element's real ParsedValue location instead of discarding it through Value::Option. New regression test asserts the preserved location directly, not just that nothing broke. search.rs needed no changes: find_index_by_key_sorted/ unsorted were already fully generic over the element type. - FlatMapList: investigation found a more serious issue than the "extra clone" originally flagged. Its arg.clone() (via eval_lambda_value_generic) clones the *entire* accumulator embedded in the tuple argument, once per iteration - an O(n) clone repeated n times, so Stage 3 had silently turned main's FlatMapList from O(n) into O(n^2) (loc already paid this cost before Stage 3, so it's not a new regression there). Making the input V-generic wouldn't have fixed this (the lambda's return type is always plain Value regardless, and was never location-preserving pre-Stage-3 either) - the actual fix is avoiding the clone: build `arg` by moving vs/v into it (via V::from_evaluated) and reclaim vs afterward via extract_mapped_value (also a move), instead of cloning arg up front. This restores O(n) for the main evaluator and removes the redundant clone for loc (loc's own from_evaluated reconstruction cost is unrelated and unchanged). Full workspace test suite (159 lib tests, all 23 decode snapshots) passes unchanged.
Expr::Match ("non-exhaustive patterns") and Expr::Destructure ("refuted
pattern") could both panic in eval_generic when a scrutinee matched none of
the given pattern(s). The type-checker (pattern.rs's build_scope/
infer_expr_branch_type) only checks structural compatibility - tuple arity,
variant labels existing in the union, etc. - never exhaustiveness of
concrete-literal patterns (U8/ZRange/etc.) against the full value domain, so
both panics are genuinely data-reachable, not typechecker invariants. This
matches existing precedent: Decoder::Match, the Format-level sibling of
Expr::Match, already treats the identical scenario (no branch's pattern
matched) as a proper Result error (DecodeErrorKind::RefutedPatternMatch)
rather than a panic.
New EvalError::RefutedPattern { cases: Vec<Pattern>, value: Box<Value> },
mirroring RefutedPatternMatch's shape/wording (source() -> None, same as
RefutedPatternMatch itself - no wrapped std error). Since Stage 3 already
unified Expr::eval/eval_with_loc into one Expr::eval_generic, this only
needed fixing once, in decoder/eval.rs. `value` is always a plain `Value`
(via the existing EvalValue::clone_into_value), regardless of which
evaluator (Value or ParsedValue) produced it.
New tests: 2 direct (non-differential) eval::tests cases asserting the
actual Err outcome. No changes needed in eval_parity_tests.rs - its existing
match_non_exhaustive/destructure_refuted cases already exercised this path
and transitioned cleanly from Outcome::Panic/Panic parity to matching
Outcome::Err/Err parity, since both evaluators share eval_generic and now
produce identical error text. Full workspace test suite passes unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AsU8/AsU16/AsU32/AsU64/AsChar all panicked when applied to a Value::Numeric (TypedConst), since cast_to_u8/u16/u32/u64/char had no arm for it. Added one to each, converting purely by value and ignoring the numeric's declared NumRep - e.g. a NumRep::Concrete(MachineRep::U32)-tagged constant with value 0 now casts to U8 successfully, mirroring how AsU8(Expr::U32(0)) already succeeds for native-grammar values regardless of source width. This is deliberately more permissive than IntRel's existing Numeric-vs-native arms, which require the declared NumRep to match (or be Auto). New TypedConst::as_native<U>() in numeric/core.rs supplies the conversion: a generalization of the existing as_usize to an arbitrary target width, likewise ignoring NumRep entirely and succeeding whenever the raw value fits U's range. New tests: 6 direct unit tests in decoder/value.rs (rep-irrelevance, range errors, Auto-rep widening, valid/invalid-surrogate/out-of-range AsChar), plus 5 differential regression cases in eval_parity_tests.rs. Full workspace test suite and cargo fmt --check pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the newly surfaced bug demonsrated by the integration test
doodle-formats/tests/negative_size.rs,where underflowing subtraction for a slice-length would panic (instead of error-fallback) on underflow.
Also cleans up some of the under-engineered panic-logic within
Expr::eval.