Fix panic parsing legacy try/catch with a multi-value handler signature

[avrabe]

Summary

A legacy-exceptions catch / catch_all handler block has a signature implicit in the encoding — a bare catch <tag> carries no blocktype — equal to [tag params] -> [try results]. That Type is never present in ModuleTypes, so the IR builder's InstrSeqType::existing lookup returns None and the .unwrap() in local_function/mod.rs panics:

attempted to push a control frame for an instruction sequence with a type that does not exist

Reported in #315.

Reproducer (panics before this change, no GC needed)

(module
  (tag $e (param i32))
  (func $f (result i32)
    try (result i32) i32.const 0 catch $e end))

Fix — why a pre-pass rather than InstrSeqType::new at the catch arm

#315's own "Suggested fix" was to swap InstrSeqType::existing → InstrSeqType::new in the Catch / CatchAll arms. That's structurally blocked here:

• ValidationContext { module: &'a Module, .. } — the IR builder holds &Module, by-design.
• LocalFunction::parse is invoked from a maybe_parallel! block, so &Module is shared across threads.
• InstrSeqType::new requires &mut ModuleTypes.

Swapping existing → new at the catch arms would force either giving up parallel parsing or restructuring the borrow shape across the IR builder — a much larger change than the bug warrants.

The pre-pass keeps all type-arena mutation in the existing serial setup loop in parse_local_functions, alongside add_entry_ty — which already solves the same shape of problem for the function entry block (its type is also implicit in the encoding). New helper register_legacy_catch_block_types walks the function body once, tracks the try frame stack, and pre-registers each non-Simple handler signature. Simple (0 params, ≤1 result) handlers are skipped; types.add dedups, so re-registering a declared type is a no-op.

Tests

crates/tests/tests/legacy-exceptions-multivalue-catch.rs — 4 parse + round-trip cases (i32, GC ref, multi-catch, catch_all with try-with-params). All four panic on main; all four pass with this change. Full cargo test --all is green.

Fixes #315.

---

Drafted with AI assistance (Claude Code). The fix was developed against the walrus source; the regression tests were confirmed to fail without it and pass with it, and cargo test --all was run green, during that AI-assisted preparation.

[guybedford]

Reviewed the patch end-to-end against the repo. Confirmed all 4 new tests panic on main with the exact error from #315 and pass with the patch; cargo test --all is green locally.

Diagnosis

Accurate. Legacy Catch/CatchAll are the only arms in the IR builder that fabricate a multi-value handler signature with no guaranteed declared type — Else reuses the parent frame and TryTable carries a real blocktype, so they're incidentally safe.

Strengths

• Test coverage is well-chosen: the i32 case is the minimal repro, catch_all exercises the [] -> results arm where it's distinct from both the try blocktype and the function type, the GC case is the real-world report shape, and multi-catch covers frame stacking.
• Mirroring add_entry_ty is precedent-consistent.
• Skipping Simple (0,0)/(0,1) handlers correctly avoids bloating the type section.
• Delegate correctly pops the try frame.
• types.add deduplicates via the id == next_id check, so re-registering an already-declared type is a no-op.

Questions before proceeding

1. Why the pre-pass over the alternative from #315?

#315's own "Suggested fix" is to switch the Catch/CatchAll arms in local_function/mod.rs:1727 / :1744 to InstrSeqType::new instead of existing — a ~6-line localized change instead of a ~62-line pre-pass that re-implements operator scanning. The PR's stated justification ("IR builder holds &Module and runs in parallel") is true, but the lookup could equivalently happen in the existing serial pre-loop in parse_local_functions per-handler without duplicating control-flow tracking. What made the heavier approach preferable? If it's a deliberate separation-of-concerns call (keeping all type-arena mutation outside the parallel IR build), that's defensible — worth stating that rationale in the PR description so reviewers can weigh it.

2. Can the extra walk be skipped when there's no legacy try?

Every function body now gets a second full operator iteration before the real IR build, even for modules with zero legacy exception use (which is the vast majority). For large modules this is non-trivial overhead. A cheap early-out — return immediately once the loop has seen no Try after N operators, or skip the call until a Try is observed — would address this. Could you add one?

Minor

• Doc nit: "every multi-value legacy catch / catch_all handler" — the code correctly registers any non-Simple shape including (N, 0) cases, which aren't strictly "multi-value" in the colloquial sense. Phrasing is slightly imprecise.

Happy to approve once Q1 and Q2 are addressed (either with an explanatory comment for Q1 and an early-out for Q2, or by switching to the InstrSeqType::new approach which obviates both).

[avrabe]

Thanks for the careful review. Pushed an amended commit (ae623af) that addresses Q1 and Q3; Q2 needs a conversation before I commit either way — details below.

Q1 — rationale now in the PR description

Updated the description to spell out why the existing → new swap at the catch arm isn't structurally available (ValidationContext.module: &Module held across the parallel maybe_parallel! parse; InstrSeqType::new needs &mut ModuleTypes). The pre-pass keeps type-arena mutation in the same serial path where add_entry_ty already mutates — that's the separation-of-concerns call you flagged as defensible.

Q3 — doc tightened

every multi-value legacy catch / catch_all handler → every non-Simple legacy catch / catch_all handler. Trimmed the surrounding prose to match the walrus style elsewhere (e.g. add_entry_ty's 4-line doc).

Q2 — I tried tags.is_empty(), it's unsound

I implemented and tested it. A module containing try (param X) (result Y) ... catch_all ... end with no tags declared is legal wasm (wasm-tools validate --features=all accepts it), and the catch_all handler signature [] -> Y is multi-value and not necessarily declared elsewhere. With tags.is_empty() short-circuiting, the try_catch_all_multivalue_handler regression test panics — exactly the kind of edge case the test suite is there to catch.

No real-world toolchain emits that shape (a catch_all with no tags can never be hit at runtime — nothing in-module can throw), so the practical impact would be near-zero. But it's a real soundness hole and I'd rather not introduce one for a heuristic gain.

I couldn't find a sound zero-cost gate. The options I considered:

• tags.is_empty() — unsound (above).
• Look at the type section for a [] -> tuple shape — needs scanning, defeats the purpose.
• Hook into the validator's frame tracking — bigger structural change.
• Bail within the loop after N operators — heuristic, same soundness issue.

For now the pre-pass runs unconditionally: one extra operator iteration per function body, which is dominated by the validator + IR-build passes that already iterate.

Two paths forward, your call:

1. Land as-is, accept the iteration cost (it's small relative to existing per-function work).
2. Add the tags.is_empty() gate with a code comment honestly documenting the trade-off and the test case that exercises the skipped shape (which we'd then have to mark #[ignore]d or remove). I can prepare either.

Happy to do (2) if you'd prefer the cheaper common-case path even at the cost of the documented hole.

[guybedford]

Thanks for clarifying. LGTM.