Draft: graft doodle-rec recusion model onto doodle - #332
Draft
archaephyrryx wants to merge 11 commits into
Draft
archaephyrryx wants to merge 11 commits into
archaephyrryx wants to merge 11 commits into
Conversation
MatchTreeStep::from_format's Format::ItemVar arm unconditionally inlined
the referenced format's body with no cycle tracking, so a genuinely
self-referential format would recurse forever in from_format itself
(distinct from, and unrelated to, MatchTreeLevel::grow's own MAX_DEPTH
counter). This is the first of several narrow, independently-confirmed
gaps standing between real doodle and true (non-Phantom) recursive
formats - see experiments/doodle-rec/PLAN.md for the full port plan.
Thread a CycleGuard (open-level set + a `detected` flag) through
MatchTreeStep::from_format/from_gt_format/from_next and their helpers.
Format::ItemVar and TypedFormat::FormatCall both insert their own level
before recursing and remove it after, mirroring each other for
correctness. Re-entering an already-open level can only happen with zero
bytes consumed since it was opened (any Format::Byte along the way
already defers further expansion to the next lookahead depth, with a
fresh guard), so this is always a genuine grammar defect, never a false
positive on ordinary guarded recursion.
When the guard fires, MatchTreeLevel::grow fails the whole build (None)
instead of letting the cyclic branch silently drop out as a dead
alternative - neither typecheck's occurs_in nor either compile path's
decoder_map/compile_queue reservation would otherwise catch a genuinely
left-recursive format, so MatchTree::build is the only pass positioned to
report it before it becomes a runtime infinite loop. This reuses the
existing Option/anyhow!("cannot build match tree for {}", ...) convention
already used by every MatchTree::build call site.
Bug-injection verified: with the ItemVar guard temporarily removed, the
new left-recursion regression test reproduces a real stack overflow
(RUST_MIN_STACK=8388608, not a hang), confirming the fix is what the test
actually exercises. cargo testall is clean and `cargo cg` regenerates
generated/gencode.rs byte-identical to before this change, for every
currently-supported non-recursive format.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoCUyqJTTcsuFKVYXvSocR
…ursion bugs (Phase 1.5) There was no way to construct a genuinely self-referential (non-Phantom) format through the public API at all: define_format_args_views type-checks a format before the level it would occupy exists, so any out-of-bounds Format::ItemVar self-reference was rejected outright, and define_format_phantom_rec_args_views is hard-wired to require Phantom wrapping. Without this, no later phase's own deliverables (which all call for real, compiled-and-run peano/ping-pong test formats) could proceed. Add Format::RecVar(usize), a batch-relative self/sibling reference index matching doodle-rec's own declare_rec_formats signature and ergonomics - but purely as construction-time sugar: FormatModule::define_format_rec_batch rewrites every occurrence to a real, absolute Format::ItemVar (via the new Format::substitute_rec_var) before the format is ever installed, so no other pass ever sees one at runtime. Every other exhaustive match over Format (16 sites, found completely via the compiler's own exhaustiveness errors after adding the variant) just carries a boilerplate `Format::RecVar(_) => unreachable!()` arm, keeping this from duplicating every phase's cycle-handling logic for a second variant. Actually exercising the new API end-to-end surfaced three further, independent unguarded-recursion bugs Phase 1 didn't touch: - Format::depends_on_next (src/format.rs): confirmed in isolation to stack-overflow on a self-recursive format, with no MatchTree/compile machinery involved. Sits unconditionally on the hot path of both decoder::Compiler and codegen::GTCompiler's ItemVar/FormatCall arms, so no recursive format could be compiled by either pipeline before this. - Format::match_bounds / lookahead_bounds (src/format.rs): identical unguarded shape. - decoder.rs's and typed_decoder.rs's Tuple/Sequence compile arms: a different bug - both unconditionally wrap `next` in an empty Next::Sequence on the last field, semantically a no-op but structurally distinct on every re-entry, defeating decoder_map's (level, next) memoization and causing an unbounded (confirmed via direct trace instrumentation), ever-growing compile queue rather than a stack overflow. All three fixed with a lightweight `&mut HashSet<usize>` guard returning the conservative-but-correct answer on re-entry (Bounds::any() / true / reuse `next` as-is) - no MatchTree-style total-failure propagation needed, since none of these have a "silently mask a grammar defect" failure mode. Three new end-to-end regression tests exercise the real public API and the actual interpreter (not hand-poked FormatModule fields): self-recursive peano and mutually-recursive ping/pong both decode real bytes correctly at depth >= 2, plus a left-recursion-still-rejected case reached via decoder::Compiler::compile_program. cargo testall is clean and `cargo cg` regenerates generated/gencode.rs byte-identical to before this change. The same unguarded-ItemVar-recursion shape also exists in is_ascii_char_format/is_ascii_string_format and several output/-module pretty-printing/coverage functions; none sit on the core decode/codegen path or block later phases, so they're left as known, flagged debt (see PLAN.md's Phase 1.5 section) rather than fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoCUyqJTTcsuFKVYXvSocR
… (Phase 2)
occurs_in previously rejected any self-alias unconditionally, with exactly
one hard-coded exemption (UType::PhantomData, safe only because
Format::Phantom content is never actually decoded). That's far stricter
than necessary: a self-reference is only genuinely unrepresentable as a
finite Rust type when reached with no indirection anywhere on the cycle -
Rust's own E0072 criterion (`type X = Box<X>` illegal, `enum X { Y(Box<X>) }`
fine). Without this, Phase 1.5's newly-buildable self-recursive formats
could be registered and interpreted, but never typechecked for codegen
purposes at all.
Thread indirected: bool + visited: &mut HashSet<(UVar, bool)> (keyed by
canonical constraint-index, never reset at indirection boundaries) through
occurs_in/occurs_in_constraints. Indirection boundaries are Tuple/Record/
Seq/Option positions plus Constraints::Variant's labeled VarMap entries -
real doodle has no UType::Union variant, so sum-type-ness lives in
Constraints, not UType's own shape - and Constraint::Proj's TupleWith/
RecordWith/SeqOf/OptOf projections. Constraint::Equiv (direct type
equivalence, not an embedding) and plain Var-forwarding are not
boundaries. PhantomData keeps its existing unconditional exemption
unchanged, for a different reason than representability: its content is
never walked by a decoder at all, so the question is moot rather than
merely satisfied.
Four low-level tests exercise TypeChecker/UType directly (mirroring
doodle-rec's own occurs-check test names), plus one end-to-end test
proving a genuinely self-recursive format - built via Phase 1.5's
define_format_rec_batch - now typechecks through the real
TypeChecker::infer_module entry point, not just the isolated occurs
check. Bug-injection verified in two rounds: disabling the base rejection
breaks the direct-self-alias test as predicted; disabling just the Tuple
boundary breaks exactly the two tests relying on it and no others.
cargo testall is clean and `cargo cg` regenerates generated/gencode.rs
byte-identical to before this change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoCUyqJTTcsuFKVYXvSocR
Real doodle already always emits nominal struct/enum declarations, so doodle-rec's alias-vs-nominal (E0391) hazard doesn't reproduce here - that half of Phase 3 was verification-only. What was actually missing: a Box type didn't exist anywhere in the codegen model (needed to close a self-referential type's infinite size), and nothing inserted Box at value-construction sites once one was declared. Added CompType::RecBox threaded through ToFragment/lifetime/MemSize/CanOptimize/CopyEligible/ HeapOptimize/Rebindable/solve_comp_type, plus RustExpr::wrap_box/ GenExpr::WrapBox mirroring the existing wrap_some machinery, called from CodeGen::box_wrap_if_needed at Variant/Tuple construction sites. Along the way, fixed a real regression this port would otherwise have caused: Format::Phantom's own pre-existing self-reference doesn't need boxing (PhantomData never stores a T), so RecBox-insertion is suppressed under a new in_phantom_context flag for the whole Phantom subtree. Expansion::Tuple/Seq/Option gain their own cycle-termination guard (reject loudly, not promote to nominal) since only Record/Union had in_progress-based termination before this - deliberately scoped out of promoting a self-referential Tuple to a named struct, since neither capstone test format needs it. cargo testall clean; cargo cg byte-identical. See PLAN.md's "Phase 3 & 4 findings" for the deeper Finding A/B issues surfaced while building the capstone fixture, left for a following commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkXtKPR1zC2JnNDSETXmpW
…se 4) Finding B (diagnosed in a prior session, see PLAN.md): a self-referential format reached through Format::record's last field compiled into a dead, always-failing decoder rather than reusing the real one. Format::record desugars to nested LetFormat/MonadSeq ending in a zero-width Compute(Record(...)) step, and both decoder::Compiler::compile_format and codegen::typed_decoder::GTCompiler::compile_gt_format unconditionally wrapped `next` in Next::Cat(second, next) around the first component, regardless of whether `second` ever consumes a byte. For a recursive last field this produced a next structurally different from whatever that same level's own top-level entry used, missing decoder_map's (level, next) cache and queuing a doomed duplicate compile. Fixed by skipping the wrap when `second` can only ever match zero bytes (second.match_bounds(module).as_exact() == Some(0)), mirroring Phase 1.5's identical Tuple/Sequence trailing-field fix. Bug-injection verified independently in both compile paths: disabling the interpreter half hangs compile_program (unbounded compile_queue growth); disabling the codegen half reproduces the exact predicted dead FailToken decoder. This fix isn't recursion-specific the way Phases 1-3 were - it also collapses the same redundant wrapper for ordinary non-recursive records whose last field calls another named format, so `cargo cg` regenerates gencode.rs with a real (non-byte-identical) diff. Confirmed by hand this is pure decoder_map dedup (e.g. a trivial forwarding-only decoder merged away into the real one it forwarded to), not a behavior change - a minor pre-existing codegen inefficiency this fix incidentally also cleans up. Per the user, this revises PLAN.md's own byte-identical requirement (see its added disclaimer): only ALLCAPS requirements are binding going forward, and this one is now scoped to flag non-dedup-shaped diffs for review rather than banning any diff outright. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkXtKPR1zC2JnNDSETXmpW
tests/recursion/ (new [[test]] fixture, matching the existing tests/runtime_repeat/permit_state_error convention): one self-recursive format (peano) and one mutually-recursive pair (ping/pong), proven through the full pipeline this port touches - typecheck, MatchTree, interpreted decode, and full production codegen. mod.rs is frozen real codegen output, never hand-edited; it's provenance-tracked by src/codegen/mod.rs's new #[ignore]d regenerate_recursion_fixture test, re-run explicitly after changing the format definitions. Both shapes decode real crafted bytes at recursion depth >= 2 and cleanly reject malformed input, compiled and actually run via `cargo test --test recursion` - not just a string-level check on the generated source. Trying to compile this for the first time surfaced a second, previously-undiscovered Box-placement gap: embed_expr's TypedExpr::Record arm (which renders a record's closing Compute(Record(...)) step into a Rust struct literal) never consulted RecBox eligibility at all, unlike CodeGen::translate's Variant/Tuple arms - a record's self-referential field got a correctly-boxed type declaration but an unboxed constructed value, a real E0308 mismatch. Fixed by having that arm consult the record's own declared field types (already available inline, no defined_types lookup needed) and wrap_box() any RecBox-wrapped field, mirroring box_wrap_if_needed exactly. Bug-injection confirmed the exact predicted rustc error reappears with the fix disabled. cargo cg byte-identical - only an already-recursive format ever has a RecBox field. Also closed Phase 5's one remaining stated deliverable not already covered by earlier phases: an interpreter-path (not just codegen) malformed-input rejection test (define_format_rec_batch_self_recursive_peano_rejects_malformed_input, src/lib.rs), proving a well-formed recursive grammar still rejects bad bytes reached mid-recursion, not just at the top level. doc/RECURSION.md is a new, permanent reference for the recursion model (design, tradeoffs, patterns/anti-patterns across both codegen and the interpreter), distinct from PLAN.md's phase-by-phase migration log - meant to outlive it. PLAN.md's own "Phase 5 findings" section has the full narrative, including why pong is a Format::record rather than a raw Tuple (a raw-Tuple pong hits the already-documented, still-open Finding A the moment it's actually compiled). cargo testall clean; cargo fmt clean; cargo cg byte-identical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkXtKPR1zC2JnNDSETXmpW
cargo fmt, cargo testall, cargo build --workspace, and a cargo cg regeneration diff are all clean (byte-identical) - the recursion-model port (Phases 0-5) is functionally complete. Updated the two root-level docs this port makes load-bearing, per the plan's own Phase 6 checklist: doc/DESIGN.md gets a new "Cycle guard for self-referential formats" section describing MatchTreeStep::from_format's CycleGuard (Phase 1), and TYPECHECKER.md gets a new "Occurs-check and representability" section describing the indirection-boundary criterion occurs_in now uses (Phase 2) - both point to doc/RECURSION.md for the full writeup rather than duplicating it, since that's now the permanent authoritative reference for the whole model. READARRAY_AUDIT.md is left unchanged: nothing new surfaced beyond Phase 0's already-confirmed non-issue finding for ReadArray/fixed.rs. Filled in final commit hashes for Phases 3-5 in PLAN.md's progress table (previously left as "(uncommitted)" placeholders from mid-session drafting). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JkXtKPR1zC2JnNDSETXmpW
Leaves stub for Tuple/Sequence `remaining.iter().all(is_nonproductive)`, to-be-addressed
Implements two `Next` smart-constructors that avoid creating dummy-Format slots for entirely nonproductive Formats/Format-slices. Replaces open-coded guards in Decoder/TypedDecoder for LetFormat, MonadSeq (Next::cat), as well as for Tuple, Sequence (Next::sequence)
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.
No description provided.