fix: type inference for bare None/Err constructors in generic calls (#293)#428
Merged
Conversation
…293) Three-layer fix for option_unwrap_or(None, 99), result_unwrap_or(Err("oops"), 0), and option_map(None, fn(...){...}) failing to type-check or compile. Checker (vera/checker/resolution.py): Fresh-TypeVar placeholder mappings (T$n from _fresh_typevar) are now treated as tentative. A later argument that provides a concrete type for the same forall var overwrites the placeholder, so the default value in option_unwrap_or(None, 99) correctly resolves T=Int. Monomorphizer + WASM inference: Added _ctor_adt_tp_indices and _adt_tp_counts to CodeGenerator (registration.py, core.py) to map each constructor field to its ADT type-param index. This correctly binds Err(e) to Result E (index 1), not T (index 0) as naive positional zip implied. Updated _get_arg_type_info (monomorphize.py) and _get_arg_type_info_wasm (wasm/inference.py) to use these maps; None entries in the sparse tuple are skipped in _unify_param_arg / _unify_param_arg_wasm. Added missing StringLit/InterpolatedString/ArrayLit cases to _infer_vera_type_name (monomorphize.py). Also: - 5 regression tests in test_checker.py (TestGenerics) - New conformance test ch09_none_err_inference.vera (level: run) - Simplified two workarounds in ch09_prelude.vera - Removed limitation from SKILL.md, spec 9.9, vera/README.md, KNOWN_ISSUES.md - ROADMAP: rewrote "Where we are" as current-state summary; deleted Phase 1a - Version bump 0.0.103 to 0.0.104; doc counts updated throughout Co-Authored-By: Claude <noreply@anthropic.invalid>
📝 WalkthroughWalkthroughType inference for bare Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as "User Code"
participant Checker as "Checker\n(resolution.py)"
participant CodeGen as "CodeGen\n(registration.py / core.py)"
participant Context as "WasmContext\n(context.py)"
participant Monomorph as "Monomorph\n(codegen/monomorphize.py)"
participant WasmInf as "WasmInference\n(wasm/inference.py)"
User->>Checker: type-check option_unwrap_or(None, 99)
Checker->>Checker: _unify_for_inference binds/overwrites\nfresh TypeVar when concrete found
Checker-->>User: type-check succeeds
CodeGen->>CodeGen: register ADTs → populate\n_ctor_adt_tp_indices & _adt_tp_counts
CodeGen->>Context: pass ctor_adt_tp_indices, adt_tp_counts
Context-->>Monomorph: provide mapping during compilation
User->>Monomorph: compile call with constructor arg
Monomorph->>Context: query ctor→ADT indices for constructor
Context-->>Monomorph: return index tuple (with None slots)
Monomorph->>Monomorph: infer sparse ADT type-args, leave unknowns None
Monomorph->>WasmInf: emit/validate wasm types using inferred args
WasmInf-->>User: monomorphized WASM/output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 90.30% 90.26% -0.05%
==========================================
Files 49 49
Lines 19163 19218 +55
Branches 220 220
==========================================
+ Hits 17306 17347 +41
- Misses 1853 1867 +14
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:
|
Replace version-specific release note with an overall summary of where the language stands today. The section now describes the compiler's current capabilities and remaining gaps, rather than narrating what changed in the last release (that belongs in CHANGELOG.md). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 ROADMAP.md summary to list all literal cases the
monomorphizer type inferencer now handles: include StringLit,
InterpolatedString, and ArrayLit (in addition to StringLit already mentioned)
and reference that these were added in the monomorphizer type inferencer change;
mention the three-layer fix (checker fresh-TypeVar overwrite rule, monomorphizer
sparse-constructor field-to-type-param mapping, and the added literal cases) so
the roadmap matches the implementation.
In `@TESTING.md`:
- Line 84: Update the sentence in TESTING.md that claims "Each program is
self-contained, imports nothing" to acknowledge the one cross-module exception:
reword it to something like "Each program is self-contained and imports nothing,
with the single exception of ch07_cross_module_contracts.vera which depends on
ch07_cross_module_contracts_lib.vera" so the conformance suite description
remains accurate while explicitly naming the two files
(ch07_cross_module_contracts.vera and ch07_cross_module_contracts_lib.vera) to
clarify the exception.
- Line 76: The document is inconsistent: the table row for `test_conformance.py`
reports 355 parametrized cases while a later inline comment still reads "# Via
pytest (parametrized — 350 tests)"; update the inline comment (the text "# Via
pytest (parametrized — 350 tests)" near Line 180) to match the table by changing
350 to 355 so both references to `test_conformance.py` use the same count.
In `@vera/wasm/context.py`:
- Around line 98-103: When instantiating WasmContext in vera/codegen/closures.py
(the call around lines where WasmContext is created for closure codegen), pass
the ADT type-parameter mappings the same way functions.py does: add
ctor_adt_tp_indices=getattr(self, "_ctor_adt_tp_indices", None) and
adt_tp_counts=getattr(self, "_adt_tp_counts", None) to the WasmContext
constructor arguments so closure bodies that call sparse constructors (e.g.
Err(e), Some(...)) receive the correct field→type-param index info for
inference.
🪄 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: f08fa761-722f-4b79-aef0-9c8ec738414f
⛔ Files ignored due to path filters (7)
docs/SKILL.mdis excluded by!docs/**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/**tests/conformance/ch09_none_err_inference.verais excluded by!**/*.veratests/conformance/ch09_prelude.verais excluded by!**/*.vera
📒 Files selected for processing (25)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdHISTORY.mdKNOWN_ISSUES.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomlscripts/check_skill_examples.pyspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_checker.pyvera/README.mdvera/__init__.pyvera/checker/resolution.pyvera/codegen/core.pyvera/codegen/functions.pyvera/codegen/monomorphize.pyvera/codegen/registration.pyvera/wasm/calls.pyvera/wasm/context.pyvera/wasm/inference.py
💤 Files with no reviewable changes (3)
- KNOWN_ISSUES.md
- spec/09-standard-library.md
- vera/README.md
… lifting Closures.py was missing the two ADT type-param mapping dicts that functions.py already passes. This means closure bodies calling sparse constructors (e.g. Err(e), Some(v)) in generic contexts would not receive correct field→type-param index info for monomorphizer type inference. Also fix two TESTING.md inaccuracies: the inline comment for test_conformance.py now says 355 (not 350) to match the table row, and the conformance suite description notes the one cross-module exception (ch07_cross_module_contracts.vera imports ch07_cross_module_contracts_lib.vera). 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 11: Update the overstated inference claim: replace the absolute phrase
“works correctly across all call sites” with a scoped statement that the recent
fix only addresses type inference for bare constructors 'None', 'Err', and 'Ok';
explicitly note that the related monomorphiser limitation leaving patterns like
option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>> as xfail remains
unresolved. Edit the sentence describing type inference to say the bare
constructor inference fix now works (e.g., “Type inference for bare constructors
('None', 'Err', 'Ok') now works correctly”) and add a short parenthetical or
sentence calling out the outstanding monomorphiser issue with
option_unwrap_or<Map...>/Set... so the claim is no longer absolute.
- Line 201: The document contains inconsistent test counts: the phrase "3,184
tests" (present in the sentence "**630+ commits, 104 tagged releases, 3,184
tests, 96% coverage, 71 conformance programs, 30 examples, 13 spec chapters.**")
does not match the CI tooling table which lists "3,095 tests"; locate both
occurrences by searching for those exact numeric strings and make them
consistent by updating the incorrect value to the authoritative number (verify
against current test suite/CI metrics) so both the narrative sentence and the CI
tooling table use the same test count.
🪄 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: 532c1eb7-83d3-4f5d-835f-d8c3791da663
📒 Files selected for processing (1)
ROADMAP.md
Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ROADMAP.md (1)
11-11:⚠️ Potential issue | 🟡 MinorThe "all call sites" claim remains overstated.
Per retrieved learnings,
option_unwrap_or<Map<...>>andoption_unwrap_or<Set<...>>are still pinned asxfaildue to a separate monomorphiser limitation for opaque i32 handle types. The phrase "works correctly across all call sites" is inaccurate.Suggested fix
-The compiler is complete end-to-end: parse, type-check, verify contracts via Z3, compile to WebAssembly, and run — at the command line and in the browser. The language has 122 built-in functions, algebraic effects (IO, Http, State, Exceptions, Async, Inference), constrained generics, a module system, contract-driven testing, and a canonical formatter. Type inference for bare constructors (`None`, `Err`, `Ok`) now works correctly across all call sites. The compiler has 3,184 tests, 71 conformance programs, 30 examples, and a 13-chapter specification. +The compiler is complete end-to-end: parse, type-check, verify contracts via Z3, compile to WebAssembly, and run — at the command line and in the browser. The language has 122 built-in functions, algebraic effects (IO, Http, State, Exceptions, Async, Inference), constrained generics, a module system, contract-driven testing, and a canonical formatter. Type inference for bare constructors (`None`, `Err`, `Ok`) now works in generic combinator call sites (removing the typed-`let` workaround). The compiler has 3,184 tests, 71 conformance programs, 30 examples, and a 13-chapter specification.Based on learnings,
option_unwrap_or<Map<...>>/option_unwrap_or<Set<...>>is still pinned as xfail due to a separate monomorphiser limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 11, The ROADMAP claim "Type inference for bare constructors (`None`, `Err`, `Ok`) now works correctly across all call sites" is overstated; update the sentence to exclude known failing cases by noting that option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>> remain xfail due to a monomorphiser limitation with opaque i32 handle types; change the wording to something like "works correctly across call sites except for known pinned xfail cases (e.g., option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>>) caused by a monomorphiser limitation for opaque i32 handle types" and ensure the symbols option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>> are referenced exactly so readers can find the related tests/issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ROADMAP.md`:
- Line 11: The ROADMAP claim "Type inference for bare constructors (`None`,
`Err`, `Ok`) now works correctly across all call sites" is overstated; update
the sentence to exclude known failing cases by noting that
option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>> remain xfail due to a
monomorphiser limitation with opaque i32 handle types; change the wording to
something like "works correctly across call sites except for known pinned xfail
cases (e.g., option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>>) caused
by a monomorphiser limitation for opaque i32 handle types" and ensure the
symbols option_unwrap_or<Map<...>> and option_unwrap_or<Set<...>> are referenced
exactly so readers can find the related tests/issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e222214-d05c-417e-8cb9-c93347724362
📒 Files selected for processing (3)
ROADMAP.mdTESTING.mdvera/codegen/closures.py
Owner
Author
|
@coderabbitai The repeated finding about
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
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
letbinding workaroundsStringLitcase in monomorphizer type inferencerWhat changed
Checker (
vera/checker/resolution.py)Fresh-TypeVar placeholder mappings (
T$nfrom_fresh_typevar) are now tentative. WhenNoneis the first arg tooption_unwrap_or(None, 99), the checker producesA → T$1as a placeholder. The second arg (99) providesA = Int, which now overwrites the placeholder. Previously, the first mapping was immutable andAstayed unresolvable.Monomorphizer + WASM inference
Added
_ctor_adt_tp_indicesand_adt_tp_countstoCodeGenerator(initialized inregistration.py). These map each constructor field to its ADT type-param position, soErr(e)'s single field correctly bindsE(index 1 inResult<T,E>), notT(index 0) as naïve positional zip implied.Also added missing
StringLit/InterpolatedString/ArrayLitcases to_infer_vera_type_nameinmonomorphize.py— these causedErr("oops")to returnNonefrom type inference, silently falling through.Documentation / tests
test_checker.pycovering all bare-constructor inference casesch09_none_err_inference.vera(level: run, 71 total)ch09_prelude.veraSKILL.md,spec/09-standard-library.md§9.9,vera/README.md,KNOWN_ISSUES.mdHISTORY.mdStage 9Test plan
pytest tests/ -v— 3175 passed, 9 skippedpython scripts/check_conformance.py— 71 programs passpython scripts/check_examples.py— 30 examples passmypy vera/— cleanpython scripts/check_version_sync.py— 0.0.104 consistentpython scripts/check_doc_counts.py— consistentvera run tests/conformance/ch09_prelude.vera— exits 0Closes #293
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chore