Fix monomorphization of opaque handle types (#341)#344
Merged
Conversation
Add _BUILTIN_VERA_RETURN_TYPES lookup table to the monomorphizer so generic prelude functions (option_unwrap_or, match) work with Option<Decimal>, Ordering from decimal_compare, and Option<Decimal> from decimal_div/decimal_from_string. Previously all opaque i32 handle types were incorrectly inferred as "Bool". Also fix _infer_expr_wasm_type and _infer_block_result_type in wasm/inference.py to recognize Decimal/Map/Set as i32 handle types for correct if-expr result annotations. Remove workarounds from existing tests, add 10 new monomorphization tests, new ch09_decimal_generics conformance test (60 programs). Remove limitation notes from spec, SKILL.md, README Known Bugs. Bump version to v0.0.98. Closes #341. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds builtin return-type inference for opaque host-managed handles (Decimal, Map, Set) so generics (e.g. Changes
Sequence Diagram(s)sequenceDiagram
participant Src as Vera Source
participant Mono as Monomorphizer
participant Infer as WASM Type Inference
participant CG as Codegen/Backend
Src->>Mono: parse call-sites and builtin calls
Mono->>Mono: consult _BUILTIN_VERA_RETURN_TYPES / _BUILTIN_PARAMETERIZED_RETURNS
Mono->>Infer: supply inferred Vera types (e.g. Option<Decimal>, Ordering)
Infer->>CG: map opaque handles to wasm types (i32, i32_pair, i64)
CG->>Mono: emit specialised functions (e.g. option_unwrap_or$Decimal)
CG->>Src: execute tests / produce artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
+ Coverage 91.96% 92.01% +0.05%
==========================================
Files 47 47
Lines 17849 17879 +30
Branches 211 211
==========================================
+ Hits 16415 16452 +37
+ Misses 1430 1423 -7
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:
|
Owner
Author
|
The fix required changes at two layers: (1) the monomorphizer needed to know that decimal_from_int() returns "Decimal" not "Bool", and (2) the WASM inference layer needed to recognize @decimal slot references as i32 so the if block inside option_unwrap_or$Decimal gets the correct (result i32) annotation. Without both fixes, the generated WASM was invalid — values on the stack with no type annotation on the if block. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TESTING.md`:
- Line 76: The README has inconsistent test counts for test_conformance.py (one
entry shows 300, another 295); open test_conformance.py to determine the actual
number of parametrized tests and then update all occurrences in TESTING.md (the
table row referencing `test_conformance.py` and the later “parametrized — 295
tests” sentence) so they match that verified count and ensure the surrounding
description still reads correctly.
In `@tests/conformance/manifest.json`:
- Around line 815-831: The manifest entry "ch09_decimal_generics" only tests
Decimal generics; add companion conformance programs that exercise
Option<Map<...>> and Option<Set<...>> so the opaque-handle monomorphization for
Map and Set is covered. Create minimal Vera programs (following
tests/conformance style) that use Option<Map<K,V>> and Option<Set<T>> wrappers
and perform simple operations, nesting map_new() and set_new() inside the
map/set operations (e.g., Option::Some(map_new(...)) /
Option::Some(set_new(...))) so the compiler can infer generic types; ensure the
new program IDs are added to manifest features similar to
"ch09_decimal_generics" and reference decimal/generic usage patterns already
present.
In `@tests/test_codegen.py`:
- Around line 8369-8373: The test is shadowing the first Ordering value with a
second `let `@Ordering`` so the `match `@Ordering.0` { ... Equal -> 0 ... }` never
exercises the Equal branch; change the test to preserve both comparison results
(e.g. store the result of `decimal_compare(decimal_from_int(5),
decimal_from_int(5))` in a distinct binding and use that binding in the `match`,
or compute the two comparisons separately so one `@Ordering` binding covers the
Equal case), update references to the original `@Ordering` binding (or rename
the second binding) so the `Equal` arm is actually reached and asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c2a5516-c091-42e4-abea-9e95d805028a
⛔ Files ignored due to path filters (6)
docs/index.htmlis excluded by!docs/**docs/index.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/ch09_decimal_generics.verais excluded by!**/*.vera
📒 Files selected for processing (17)
CHANGELOG.mdCLAUDE.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomlscripts/check_readme_examples.pyscripts/check_spec_examples.pyspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_codegen.pytests/test_readme.pyvera/README.mdvera/__init__.pyvera/codegen/monomorphize.pyvera/wasm/inference.py
💤 Files with no reviewable changes (2)
- README.md
- vera/README.md
- Fix stale parametrized test count (295→300) in TESTING.md - Fix test_decimal_compare_equal_and_greater to assert both Equal and Greater arms independently instead of shadowing - Add Map/Set builtins to _infer_fncall_wasm_type for correct if-expression result type annotations - Add 3 if-expr result type tests (Decimal, Map, Set) exercising the _infer_block_result_type paths 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 `@tests/test_codegen.py`:
- Around line 8452-8493: The tests test_decimal_if_expr_result_type,
test_map_if_expr_result_type, and test_set_if_expr_result_type only exercise the
then-branch; add corresponding assertions that exercise the else-branch at
runtime by creating a second variant of each source (or toggling the
if-condition) so the condition evaluates false and assert the expected else
result (e.g., decimal_from_int(0) for Decimal, empty map size 0 for Map, empty
set size 0 for Set); update the _run(...) assertions to verify both control-flow
paths for the opaque-handle inference paths in those tests.
In `@vera/wasm/inference.py`:
- Around line 277-294: The comment for map_get/map_contains is misleading: both
return "i32" but map_get is an Option heap pointer while map_contains is a
primitive Bool (i32). Update the handling in _infer_fncall_wasm_type (the
expr.name map case) to either split the branch or adjust the comment so it
states: map_get -> "i32" (Option/heap pointer) and map_contains -> "i32" (Bool
primitive), ensuring the comment correctly describes each symbol (map_get,
map_contains).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dc908cd-1df3-4b2e-a21f-466f9a3cc7ac
📒 Files selected for processing (3)
TESTING.mdtests/test_codegen.pyvera/wasm/inference.py
- Split map_get/map_contains in _infer_fncall_wasm_type with accurate comments (Option heap pointer vs Bool) - Add else-branch tests for Decimal/Map/Set if-expressions - Add map_get, map_size, set_contains inference tests - Fix doc counts (2912 tests) Co-Authored-By: Claude <noreply@anthropic.invalid>
Exercise remaining _infer_fncall_wasm_type branches for Map/Set builtins in if-expression contexts. Fix doc counts (2916 tests). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_codegen.py`:
- Around line 8576-8721: The tests only exercise Option<Decimal> and
Ordering—add explicit monomorphization coverage for Map and Set by extending
TestDecimalMonomorphization (or creating a new TestMapSetMonomorphization) with
tests that mirror the Decimal cases: e.g., an option_unwrap_or<Map<...>> and
option_unwrap_or<Set<...>> using Some/None, match on Option<Map<...>> and
Option<Set<...>>, and simple Map/Set operations (insert/get, contains,
empty-case) to exercise codegen/runtime paths for map_insert/map_get and
set_insert/set_contains; ensure you reference the same helper builtins
(option_unwrap_or, match, map/get/insert, set/insert/contains) so the
monomorphizer generates Map/Set instantiations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f00e13a8-15e2-4aa5-8b7f-b70ac8f005ae
📒 Files selected for processing (3)
TESTING.mdtests/test_codegen.pyvera/wasm/inference.py
Tests exercise option_unwrap_or with decimal_div() and decimal_from_string() passed directly (no let binding). Fix doc counts (2918 tests). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_codegen.py (1)
8630-8775:⚠️ Potential issue | 🟡 MinorAdd direct generic coverage for
MapandSetas opaque handles.This class still only proves monomorphisation for
Decimal/Ordering. The production change also extends opaque-handle specialisation toMapandSet, so a regression inoption_unwrap_or<Map<...>>oroption_unwrap_or<Set<...>>would still slip through here.🔧 Minimal coverage to force `Map`/`Set` instantiations
class TestDecimalMonomorphization: @@ def test_decimal_from_string_invalid_match(self) -> None: """decimal_from_string with invalid input returns None.""" source = """ public fn main(-> `@Int`) requires(true) ensures(true) effects(pure) { let `@Option`<Decimal> = decimal_from_string("not_a_number"); match `@Option`<Decimal>.0 { None -> 1, Some(`@Decimal`) -> 0 } } """ assert _run(source) == 1 + + def test_option_unwrap_or_map(self) -> None: + """option_unwrap_or<Map<...>> preserves the Some value.""" + source = """ +public fn main(-> `@Int`) + requires(true) ensures(true) effects(pure) +{ + let `@Map`<String, Int> = map_insert(map_new(), "k", 42); + let `@Option`<Map<String, Int>> = Some(`@Map`<String, Int>.0); + let `@Map`<String, Int> = option_unwrap_or( + `@Option`<Map<String, Int>>.0, + map_insert(map_new(), "fallback", 0)); + option_unwrap_or(map_get(`@Map`<String, Int>.0, "k"), 0) +} +""" + assert _run(source) == 42 + + def test_option_unwrap_or_set(self) -> None: + """option_unwrap_or<Set<...>> preserves the Some value.""" + source = """ +public fn main(-> `@Int`) + requires(true) ensures(true) effects(pure) +{ + let `@Set`<Int> = set_add(set_new(), 7); + let `@Option`<Set<Int>> = Some(`@Set`<Int>.0); + let `@Set`<Int> = option_unwrap_or( + `@Option`<Set<Int>>.0, + set_add(set_new(), 0)); + if set_contains(`@Set`<Int>.0, 7) then { 1 } else { 0 } +} +""" + assert _run(source) == 1As per coding guidelines, "Add codegen/runtime tests in tests/test_codegen.py for built-in functions covering normal cases, edge cases (empty inputs, zero values), and composition with other built-ins."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8630 - 8775, Add tests to instantiate Map and Set generics to force monomorphisation: inside the TestDecimalMonomorphization class add new test methods that create let `@Option`<Map<@Int,`@Int`>> = Some(map_from_entries([(decimal_from_int(1), decimal_from_int(2))])) and let `@Option`<Map<@Int,`@Int`>> = None (and analogous for `@Option`<Set<@Int>> using set_from_list([decimal_from_int(1)])/None), then call option_unwrap_or(`@Option`<Map<...>>.0, map_empty() / set_empty()) and assert the returned map/set contents (and default behavior) via _run; ensure tests reference the symbols option_unwrap_or, Map, Set, map_from_entries, set_from_list, map_empty, set_empty and add assertions for both Some and None cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_codegen.py`:
- Around line 8630-8775: Add tests to instantiate Map and Set generics to force
monomorphisation: inside the TestDecimalMonomorphization class add new test
methods that create let `@Option`<Map<@Int,`@Int`>> =
Some(map_from_entries([(decimal_from_int(1), decimal_from_int(2))])) and let
`@Option`<Map<@Int,`@Int`>> = None (and analogous for `@Option`<Set<@Int>> using
set_from_list([decimal_from_int(1)])/None), then call
option_unwrap_or(`@Option`<Map<...>>.0, map_empty() / set_empty()) and assert the
returned map/set contents (and default behavior) via _run; ensure tests
reference the symbols option_unwrap_or, Map, Set, map_from_entries,
set_from_list, map_empty, set_empty and add assertions for both Some and None
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a2ea940-5857-45aa-860f-dea3b594726e
📒 Files selected for processing (2)
TESTING.mdtests/test_codegen.py
Owner
Author
|
Codecov patch is ~80% — the remaining uncovered lines are structural and cannot be meaningfully exercised:
Project coverage is 95.36% — only -0.01% from baseline. These entries are correct and defensive, but the compiler's dispatch structure means they're bypassed by the normal code paths. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_codegen.py (1)
8630-8805:⚠️ Potential issue | 🟠 MajorCoverage gap: generic monomorphization is still Decimal-only.
Line 8630 onward adds solid
Option<Decimal>/Orderingchecks, but there is still no directoption_unwrap_or<Map<...>>oroption_unwrap_or<Set<...>>runtime coverage. That leaves the Map/Set opaque-handle specialisation path unguarded.Suggested minimal additions
class TestDecimalMonomorphization: @@ def test_decimal_from_string_inline_unwrap(self) -> None: @@ assert _run(source) == 1 + + def test_option_unwrap_or_map(self) -> None: + """option_unwrap_or<Map<...>> keeps the Some value.""" + source = """ +public fn main(-> `@Int`) + requires(true) ensures(true) effects(pure) +{ + let `@Map`<String, Int> = map_insert(map_new(), "k", 42); + let `@Option`<Map<String, Int>> = Some(`@Map`<String, Int>.0); + let `@Map`<String, Int> = option_unwrap_or( + `@Option`<Map<String, Int>>.0, + map_insert(map_new(), "fallback", 0)); + option_unwrap_or(map_get(`@Map`<String, Int>.0, "k"), 0) +} +""" + assert _run(source) == 42 + + def test_option_unwrap_or_set(self) -> None: + """option_unwrap_or<Set<...>> keeps the Some value.""" + source = """ +public fn main(-> `@Int`) + requires(true) ensures(true) effects(pure) +{ + let `@Set`<Int> = set_add(set_new(), 7); + let `@Option`<Set<Int>> = Some(`@Set`<Int>.0); + let `@Set`<Int> = option_unwrap_or( + `@Option`<Set<Int>>.0, + set_add(set_new(), 0)); + if set_contains(`@Set`<Int>.0, 7) then { 1 } else { 0 } +} +""" + assert _run(source) == 1As per coding guidelines, "Add codegen/runtime tests in tests/test_codegen.py for built-in functions covering normal cases, edge cases (empty inputs, zero values), and composition with other built-ins."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 8630 - 8805, Add tests that exercise monomorphization for opaque-handle types by creating Option<Map<...>> and Option<Set<...>> cases and calling option_unwrap_or on them (both Some and None paths) so the Map/Set specialization path is covered; specifically add minimal test methods in tests/test_codegen.py (near class TestDecimalMonomorphization) that construct a Some(Map) and Some(Set), unwrap them with option_unwrap_or, assert behavior via a map operation (e.g., map_get or equivalent) and a set operation (e.g., set_contains or equivalent), and add corresponding None/default tests to hit the default-return branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TESTING.md`:
- Line 9: Update the Tests row in TESTING.md where it currently reads "**Tests**
| 2,918 across 26 files (~33,100 lines of test code)" to reconcile with the
verified run: either change the count to reflect the verified run ("2,903 tests
passed") or explicitly mark the row as an inventory/total test count (e.g.,
"2,918 tests (total in repo) — 2,903 passed in verified run") so the metric is
not stale or ambiguous; ensure the edited text includes both the numeric
reference and clear context so readers understand whether the number represents
inventory or run results.
---
Duplicate comments:
In `@tests/test_codegen.py`:
- Around line 8630-8805: Add tests that exercise monomorphization for
opaque-handle types by creating Option<Map<...>> and Option<Set<...>> cases and
calling option_unwrap_or on them (both Some and None paths) so the Map/Set
specialization path is covered; specifically add minimal test methods in
tests/test_codegen.py (near class TestDecimalMonomorphization) that construct a
Some(Map) and Some(Set), unwrap them with option_unwrap_or, assert behavior via
a map operation (e.g., map_get or equivalent) and a set operation (e.g.,
set_contains or equivalent), and add corresponding None/default tests to hit the
default-return branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69d5086b-699b-4cfa-b3b4-3673a55ae60a
📒 Files selected for processing (2)
TESTING.mdtests/test_codegen.py
- test_option_unwrap_or_map: xfail — confirms #341 gap for Map - test_match_option_set: passes — match is inline, no monomorphization needed for Set handles - Update TESTING.md counts (2920 tests) Co-Authored-By: Claude <noreply@anthropic.invalid>
Map/Set option_unwrap_or monomorphization gap is a known bug. Decimal case is fixed; Map/Set still fail (xfail test confirms). Co-Authored-By: Claude <noreply@anthropic.invalid>
Partial fix only — Decimal works, Map/Set still have the gap. PR no longer closes #341. Co-Authored-By: Claude <noreply@anthropic.invalid>
Co-Authored-By: Claude <noreply@anthropic.invalid>
#293 (combinator bare-constructor inference) is a bug — the type checker should infer T from the second argument. Added bug label to #293 and #341 on GitHub. Updated check_limitations_sync.py to also check the Bugs table, so issues tracked in either table count as documented. Co-Authored-By: Claude <noreply@anthropic.invalid>
This was referenced Mar 25, 2026
7 tasks
9 tasks
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.
Summary
_BUILTIN_VERA_RETURN_TYPESlookup table tomonomorphize.pyso the monomorphizer correctly infers Vera return types for Decimal/Map/Set builtins instead of mapping all i32 returns to "Bool"_infer_expr_wasm_typeand_infer_block_result_typeininference.pyso if-expressions inside monomorphized functions get correct result type annotationsfloor(decimal_to_float(...))workarounds in existing tests with properoption_unwrap_orandmatchusagech09_decimal_generics(60 programs, was 59) — exercisesoption_unwrap_or,match, anddecimal_comparewith DecimalPartial fix for #341 (Decimal fixed; Map/Set still have gap — see xfail tests).
What was wrong
_infer_fncall_vera_type_simple()inmonomorphize.pymapped WASM return types back to Vera types:i64→Int,i32→Bool,f64→Float64. But Decimal, Map, and Set are alli32opaque handles — NOTBool. Sooption_unwrap_or<T>withT=Decimalwas monomorphized asoption_unwrap_or$Bool, which is wrong.Test plan
vera runwithoption_unwrap_or(Some(decimal_from_int(42)), default)→ 1🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation
Chores