fix: support Exn<String> via two-parameter WASM exception tag (#416)#452
Conversation
String is a pair type (ptr, len) represented as two consecutive i32 locals in the WASM ABI. Previously, Exn<String> was registered in _exn_types with wasm type "i32_pair", causing the tag declaration to emit `(tag $exn_String (param i32_pair))` — invalid WAT that fails at the assembler stage. Fix: when a type is i32_pair, store "i32 i32" as the WASM tag parameter type in both compilability.py code paths (_check_exn_effect and _scan_body_for_state_handlers). WASM exception tags support multiple parameters natively, so `(tag $exn_String (param i32 i32))` is valid and the catch instruction pushes both values onto the stack. In _translate_handle_exn, detect pair types via _is_pair_type_name: - Allocate two consecutive i32 locals (ptr + len) following the existing convention that _translate_slot_ref uses local_idx and local_idx + 1 for pair types - Emit `(result i32 i32)` for the catch block and try_table - Pop len first, then ptr (WASM stack is LIFO after catch) Also fix result_spec: _infer_block_result_type returns "i32_pair" for String-typed handler bodies, which must expand to "i32 i32" in the WAT block result annotation. Adds conformance test ch07_exn_string.vera (level: run, expected: 10): throws a String exception and catches it, verifying both the throw path (length 5) and the non-throw path (length 5), main returns 10. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds support for pair-encoded exception payloads (e.g., Exn) by normalising WASM exception tags and updating handler translation, adds a new conformance manifest entry, extends end-to-end tests for string exceptions, and updates multiple docs and test counts to reflect 73 conformance programs. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Front-end (parser/typechecker)
participant Compilability as compilability.py
participant WasmGen as wasm/calls.py
participant TestRunner as test runner
rect rgba(200,220,255,0.5)
Frontend->>Compilability: identify handler type Exn<String>
Compilability-->>Frontend: compute wasm_tag_t ("i32 i32"), record (type_name, wasm_tag_t)
end
rect rgba(200,255,200,0.5)
Frontend->>WasmGen: request handler translation for Exn<String>
WasmGen-->>WasmGen: allocate two i32 locals (ptr, len)
WasmGen-->>WasmGen: set try_table result to (result i32 i32)
WasmGen-->>Frontend: emit WAT with local.set order (len then ptr)
end
rect rgba(255,230,200,0.5)
TestRunner->>Frontend: compile ch07_exn_string.vera
Frontend->>WasmGen: generate module
TestRunner->>WasmGen: run module, validate caught payload behaviour
WasmGen-->>TestRunner: return runtime results/assertions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
+ Coverage 90.31% 90.32% +0.01%
==========================================
Files 50 50
Lines 19406 19419 +13
Branches 220 220
==========================================
+ Hits 17527 17541 +14
+ Misses 1875 1874 -1
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 11: Update the stale test-count mention in ROADMAP.md by replacing the
outdated "3,218 tests" reference with the current "3,223 tests" so the in-file
counts match the project summary; search for the literal "3,218 tests" and
change it to "3,223 tests" to keep the roadmap consistent.
In `@tests/conformance/manifest.json`:
- Around line 592-604: The spec_ref for the manifest entry with id
"ch07_exn_string" is incorrect (it currently reads "§7.2 Exn<E> handler — String
payload"); update the spec_ref field for "ch07_exn_string" to match the existing
mapping for the Exn handler (use "§7.4 Exn<E> handler — String payload") so it
is consistent with "ch07_exn_handler" and other exception-handler entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fa54539-985b-461e-921f-5c353872e015
⛔ Files ignored due to path filters (5)
docs/SKILL.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**docs/sitemap.xmlis excluded by!docs/**tests/conformance/ch07_exn_string.verais excluded by!**/*.vera
📒 Files selected for processing (9)
AGENTS.mdCLAUDE.mdFAQ.mdROADMAP.mdSKILL.mdTESTING.mdtests/conformance/manifest.jsonvera/codegen/compilability.pyvera/wasm/calls.py
Coverage: the conformance test (ch07_exn_string.vera) exercises the Exn<String> code path via subprocess, which coverage does not track. Add three in-process tests to TestExnHandlers that cover the new is_pair=True branch in _translate_handle_exn and the new wasm_tag_t lines in compilability.py: - test_exn_string_throw_caught: throw+catch with Int result - test_exn_string_no_throw: non-throwing path through String handler - test_exn_string_handler_returns_string: handler clause returns String (covers result_wt == "i32_pair" → result_spec = "(result i32 i32)") Mark two genuinely defensive guards as pragma: no cover: - handler_instrs is None (requires unsupported handler body syntax) - result_spec = "" (result_wt is None; unreachable from valid Vera code) CodeRabbit fixes: - ROADMAP.md line 182: update stale "3,218 tests" → "3,223 tests" (missed in doc-count sweep; was in mutation-testing description) - manifest.json ch07_exn_string spec_ref: §7.2 → §7.4 to match ch07_exn_handler and reflect the correct spec section Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/conformance/manifest.json (1)
598-598:⚠️ Potential issue | 🟡 MinorCorrect the
spec_refformat to match the existing convention.The
spec_reffield uses"§7.4 Exn<E> handler — String payload", but the existingch07_exn_handlerentry at line 585 uses"Section 7.4". The manifest convention throughout uses"Section X.Y"format rather than"§X.Y".📝 Proposed fix
- "spec_ref": "§7.4 Exn<E> handler — String payload", + "spec_ref": "Section 7.4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conformance/manifest.json` at line 598, Update the spec_ref value that currently reads "§7.4 Exn<E> handler — String payload" in the manifest entry so it matches the project's convention; replace the leading "§7.4" with "Section 7.4" (i.e., make it "Section 7.4 Exn<E> handler — String payload") to be consistent with the existing ch07_exn_handler style.ROADMAP.md (1)
224-224:⚠️ Potential issue | 🟡 MinorSynchronise the final test-count summary with the updated totals.
Line 224 still says 3,223 tests, but Line 11 and Line 182 now state 3,226 tests. Please update the closing summary to avoid in-file metric drift.
As per coding guidelines, “
**/*.md: Review Markdown files for factual accuracy against the codebase, broken links, and outdated information.”Suggested patch
-**630+ commits, 108 tagged releases, 3,223 tests, 96% coverage, 73 conformance programs, 30 examples, 13 spec chapters.** See [HISTORY.md](HISTORY.md) for the full narrative of how the compiler was built. +**630+ commits, 108 tagged releases, 3,226 tests, 96% coverage, 73 conformance programs, 30 examples, 13 spec chapters.** See [HISTORY.md](HISTORY.md) for the full narrative of how the compiler was built.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 224, Update the final summary string in ROADMAP.md that currently reads "**3,223 tests**" (the instance in the closing summary paragraph) to the correct updated total "**3,226 tests**" so it matches the other occurrences on Lines 11 and 182; locate the exact phrase "**630+ commits, 108 tagged releases, 3,223 tests, 96% coverage, 73 conformance programs, 30 examples, 13 spec chapters.**" and replace only the test-count token to preserve the rest of the summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conformance/manifest.json`:
- Line 603: Remove the unsupported "expected" field from the manifest entry
(delete the "expected": 10 line) so the manifest only contains recognized keys;
if you actually intend to enforce return-value checks, implement that in the
test harness (the conformance checker) rather than adding an "expected" metadata
field.
In `@tests/test_codegen.py`:
- Around line 3106-3160: Add a new unit test in tests/test_codegen.py that
exercises the empty-string Exn<String> case by invoking throw("") and asserting
behavior matches the other Exn<String> tests; mirror the patterns of
test_exn_string_throw_caught, test_exn_string_no_throw or
test_exn_string_handler_returns_string but use throw("") to cover the
zero-length payload branch and verify the runtime handles the ptr/len pair (len
== 0) correctly (assert expected return value or printed string as appropriate).
---
Duplicate comments:
In `@ROADMAP.md`:
- Line 224: Update the final summary string in ROADMAP.md that currently reads
"**3,223 tests**" (the instance in the closing summary paragraph) to the correct
updated total "**3,226 tests**" so it matches the other occurrences on Lines 11
and 182; locate the exact phrase "**630+ commits, 108 tagged releases, 3,223
tests, 96% coverage, 73 conformance programs, 30 examples, 13 spec chapters.**"
and replace only the test-count token to preserve the rest of the summary.
In `@tests/conformance/manifest.json`:
- Line 598: Update the spec_ref value that currently reads "§7.4 Exn<E> handler
— String payload" in the manifest entry so it matches the project's convention;
replace the leading "§7.4" with "Section 7.4" (i.e., make it "Section 7.4 Exn<E>
handler — String payload") to be consistent with the existing ch07_exn_handler
style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cb66ba4-b785-4281-8c5c-469b69498d1c
📒 Files selected for processing (5)
ROADMAP.mdTESTING.mdtests/conformance/manifest.jsontests/test_codegen.pyvera/wasm/calls.py
CodeRabbit fixes:
- manifest.json ch07_exn_string: remove unsupported "expected" key
(test harness only checks exit code 0, never reads expected value);
fix spec_ref from "§7.4 ..." to "Section 7.4 ..." to match project
convention used by all other manifest entries
- ROADMAP.md line 224: stale "3,223 tests" → "3,226 tests" (the closing
summary paragraph was missed in the previous count sweep)
New test: test_exn_string_empty_payload — verifies that throw("") passes
a zero-length ptr/len pair correctly through the (param i32 i32) tag and
that string_length of the caught value is 0; confirms the LIFO local.set
sequence works when len == 0.
Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 224: Update the inconsistent test count in the ROADMAP entry that
currently reads "**630+ commits, 108 tagged releases, 3,226 tests, 96% coverage,
73 conformance programs, 30 examples, 13 spec chapters.**" to match the rest of
the file and other occurrences ("3,227 tests"); locate and replace the "3,226
tests" token with "3,227 tests" so all instances are consistent with lines 11
and 182.
In `@tests/test_codegen.py`:
- Around line 3142-3160: Update the test_exn_string_handler_returns_string test
to include an ABI-level WAT assertion that pins the pair-tag encoding: after
compiling the source used in that test (same src and fn="test"), obtain the
emitted WAT (via the existing test helper used in this suite or by adding a
small helper similar to _run_io that returns WAT) and assert the WAT contains
the expected tag/result signature (e.g., the substring "result i32 i32" or the
exact pair-tag encoding your ABI uses). Keep the runtime assertion (assert
_run_io(src, fn="test") == "caught") and add only the additional WAT-level
assert so the test both verifies behavior and pins the codegen shape for
handle[Exn<String>] / result i32 i32.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c73cc79e-4e40-4260-95d8-01af97f22b5e
📒 Files selected for processing (4)
ROADMAP.mdTESTING.mdtests/conformance/manifest.jsontests/test_codegen.py
CodeRabbit fixes:
- ROADMAP.md line 224: "3,226 tests" → "3,227 tests" — the previous sed
pattern matched "3,226 tests, 73 conformance" but line 224 has
"3,226 tests, 96% coverage, 73 conformance" so it was not updated
- test_exn_string_handler_returns_string: add WAT-level assertions that
pin the ABI encoding for Exn<String>:
assert "(tag $exn_String (param i32 i32))" in result.wat
assert "result i32 i32" in result.wat
This guards against future refactors that might silently change the
tag encoding while still producing the correct runtime result; keeps
the existing _run_io behavioral assertion alongside the structural check
Co-Authored-By: Claude <noreply@anthropic.invalid>
Summary
Exn<String>previously produced invalid WAT:(tag $exn_String (param i32_pair))—i32_pairis an internal representation, not a valid WASM value type"i32 i32"instead of"i32_pair"in_exn_types, yielding(tag $exn_String (param i32 i32))— valid WAT_translate_handle_exndetects pair types via_is_pair_type_name, allocates two consecutive locals (following the existing_translate_slot_refconvention of ptr atlocal_idx, len atlocal_idx + 1), emits(result i32 i32)for the catch block, and pops both values in LIFO order after catchresult_specwhen the handler body returns a String (_infer_block_result_typereturns"i32_pair", which must expand to"i32 i32"in WAT block result annotations)Files changed
vera/codegen/compilability.py— two code paths that populate_exn_types: now use"i32 i32"fori32_pairtypesvera/wasm/calls.py—_translate_handle_exn: pair-type detection, two consecutive locals, twolocal.setinstructions, correct(result i32 i32)annotationstests/conformance/ch07_exn_string.vera— new conformance test (level:run, expected:10): catches a thrownStringexception and verifies both the throw and non-throw pathsTest plan
vera run tests/conformance/ch07_exn_string.verareturns10pytest tests/ -v— all 3223 tests passpython scripts/check_conformance.py— all 73 conformance programs passCloses #416
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Documentation