v0.0.141 — RefinementType in i32_pair return inference (#602 family)#629
Merged
v0.0.141 — RefinementType in i32_pair return inference (#602 family)#629
Conversation
Third trigger of the same bug class as #602 (closed in v0.0.140 by PR #627) and the type-alias case fixed in the same PR. This one slipped through because PR #627 was merged before the RefinementType-handling commit landed. Verified reproducer: private fn make(@Unit -> @{ @string | string_length(@String.0) > 0 }) requires(true) ensures(true) effects(pure) { "hello" } public fn main(@Unit -> @Unit) requires(true) ensures(true) effects(<IO>) { IO.print("\(make(()))\n") } Pre-fix: `expected i64, found i32` at WASM validation (offset 911, identical to original #602). Post-fix: prints "hello". Root cause: - `_register_fn` stores the literal AST in `_fn_ret_type_exprs`. An inline-refinement return type produces a `RefinementType` AST node, not a `NamedType`. - v0.0.140's fix was `if isinstance(ret_te, ast.NamedType): ...` which silently fell through to None for `RefinementType`. - `_translate_interpolated_string` then wrapped the FnCall with `to_string(...)`, which reads i64 — but the FnCall pushed i32_pair. Same #602 trap. Same gap also lived in `_infer_index_element_type_expr`'s FnCall branch (the path #614 added) for refinement-`Array<T>` returns indexed via `f()[i]` in interpolation. Fix: - Extracted the i32_pair return-type resolution into a helper `_resolve_i32_pair_ret_te` that handles both `NamedType` (with alias resolution via `_resolve_base_type_name`) and `RefinementType` (unwrap to `base_type`, then resolve). - Applied to both i32_pair branches in `_infer_fncall_vera_type` (non-generic + generic-mono) since they share the shape. - Mirrored the RefinementType-unwrap in `_infer_index_element_type_expr`'s FnCall branch for the parallel inference path. Two new regression tests in TestStringInterpolation: - test_inline_refinement_string_in_interpolation - test_inline_refinement_array_in_indexed_interpolation All four shapes verified post-fix: - Plain `@String` return (#602 baseline) - `type Str = String` alias (v0.0.140) - Inline `@{ @string | predicate }` (this PR) - IndexExpr-of-FnCall returning refinement-Array (this PR) Test count 3,777 → 3,779 (+2). 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:
📝 WalkthroughWalkthroughRelease v0.0.141 fixes i32_pair return-type inference for function-call and indexed-access paths when functions declare inline or nested refinement return types. A new helper _resolve_i32_pair_ret_te canonicalises NamedType and nested RefinementType return-type shapes; index-element inference and FnType→WASM mapping now unwrap refinements; five regression interpolation tests were added and package/docs bumped to v0.0.141. ChangesInline Refinement Return Type Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 #629 +/- ##
=======================================
Coverage 90.93% 90.94%
=======================================
Files 59 59
Lines 23033 23063 +30
Branches 259 259
=======================================
+ Hits 20946 20974 +28
- Misses 2080 2082 +2
Partials 7 7
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 19: Update the verification summary header text so the numeric count
matches the enumerated items: replace "All four shapes verified post-fix" with
"All five shapes verified post-fix" (or remove the explicit count) to match the
listed items `string_concat`, `f()` direct in interpolation, type alias over
String, inline refinement over String, and IndexExpr-of-FnCall returning
refinement-Array in the same line.
🪄 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: 958bd6f2-79eb-48d1-b6bb-fcd290f91be9
⛔ Files ignored due to path filters (5)
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/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (9)
CHANGELOG.mdHISTORY.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_codegen.pyvera/__init__.pyvera/wasm/inference.py
…test Four agents reviewed PR #629; the silent-failure-hunter agent identified a CRITICAL gap that the other three missed: nested RefinementType (`@{ @{ @string | p1 } | p2 }`) is reachable per the grammar AND reproduces the original #602 trap. Empirically verified — `vera check` passes, `vera run` traps at WASM validation with the same `expected i64, found i32` signature. This PR's initial fix used `if isinstance(ret_te, ast.RefinementType): base = ret_te.base_type` — only one level of unwrap. Nested refinements fell through to None, downstream wraps with `to_string(...)`, same #602 trap. Fifth trigger of the same bug class. Fix: replaced the one-level unwrap with a `while isinstance(ret_te, ast.RefinementType): ret_te = ret_te.base_type` loop in both call sites (`_resolve_i32_pair_ret_te` and the parallel IndexExpr-of- FnCall path). Handles arbitrary nesting depth. Also addressed: - comment-analyzer: `core.py:333` ambiguity → `codegen/core.py:333` - comment-analyzer + silent-failure-hunter: added #626 reference to the helper's docstring tying its None-return paths to the broader silent-skip tracking issue - code-reviewer + comment-analyzer: justified the helper-vs-inline choice in the IndexExpr branch (helper returns `str`, the branch needs the `NamedType` AST node for `_alias_array_element`) Two new regression tests in TestStringInterpolation: - test_nested_refinement_string_in_interpolation - test_refinement_over_type_alias_in_interpolation (sibling case that already worked but was previously untested — pins the alias-recursion behaviour against future regressions) Skipped (with reason in the PR thread): - Cross-module gap reproducing the same trap — that's #628, explicitly deferred when filed during PR #627's review - Structural-WAT test for RefinementType — duplicates the existing PR #627 structural test which would catch any inference regression; nice-to-have but redundant - Logging at None-return sites — broader change that lives in #626's Layer 1/2/3 work - Parameterised cross-product test enumerating all shapes × all targets — over-scoped for this PR Test count 3,779 → 3,781 (+2). Co-Authored-By: Claude <noreply@anthropic.invalid>
Owner
Author
Self-review via
|
| Finding | Flagged by | Action |
|---|---|---|
Nested RefinementType is reachable AND reproduces the original #602 trap. Initial fix used a single-level if isinstance(ret_te, ast.RefinementType): base = ret_te.base_type — a @{ @{ @String | p1 } | p2 } return falls through to None, downstream wraps with to_string(...), same expected i64, found i32 at WASM validation. Empirically verified before fix. Fifth trigger of the same bug class. |
silent-failure-hunter (CRITICAL); code-reviewer + pr-test-analyzer rated it as a low-priority edge | Replaced one-level unwrap with while isinstance(ret_te, ast.RefinementType): ret_te = ret_te.base_type in both call sites (_resolve_i32_pair_ret_te and the parallel IndexExpr-of-FnCall path). Handles arbitrary nesting depth. New regression test test_nested_refinement_string_in_interpolation. |
Important issues addressed
| # | Finding | Flagged by | Action |
|---|---|---|---|
| 1 | core.py:333 reference is ambiguous (two files match) |
comment-analyzer | Updated to codegen/core.py:333. |
| 2 | Helper docstring doesn't reference #626 (the broader silent-skip tracking pattern) — every return None here is an instance |
comment-analyzer + silent-failure-hunter | Added explicit #626 callout in the helper's docstring; flagged that None-return paths include the unfixed cross-module case (#628). |
| 3 | IndexExpr branch's inline RefinementType-unwrap doesn't justify why it didn't reuse _resolve_i32_pair_ret_te |
code-reviewer + comment-analyzer | Added one-line explanation: helper returns str, this branch needs the unwrapped NamedType AST node to feed _alias_array_element (which inspects .type_args). |
Suggestions skipped
| # | Suggestion | Skip reason |
|---|---|---|
| 1 | Cross-module gap reproduces the same trap (silent-failure-hunter HIGH) | That's #628, explicitly deferred when filed during PR #627's review. In KNOWN_ISSUES.md. |
| 2 | Structural-WAT test for RefinementType (code-reviewer + pr-test-analyzer) | PR #627's test_no_to_string_wrap_on_string_returning_fncall_602 already catches inference regressions across the helper. Adding a parallel structural test for refinement would be redundant. |
| 3 | Defensive logging at None-return sites (silent-failure-hunter HIGH) | Lives in #626's Layer 1/2/3 work — adding partial logging here without the broader plumbing would create another inconsistent state. |
| 4 | Parameterised cross-product test enumerating all shapes × targets × cross-module (silent-failure-hunter LOW) | Over-scoped for this PR. The structural-fix-shape (cross-product enumeration) is the right way to close the bug class wholesale, but it deserves its own scoped issue rather than being bundled here. |
Strengths confirmed
- pr-test-analyzer: tests now cover all three reachable helper sub-cases (NamedType, NamedType-via-alias, RefinementType-with-NamedType-base) plus nested-refinement and refinement-over-alias.
- comment-analyzer: all issue/PR cross-references valid; helper docstring explains the bug-class history at a single discoverable place.
- code-reviewer: release artefacts complete (CHANGELOG / HISTORY / version sync / docs / counts).
Bonus regression test added
While verifying the silent-failure-hunter finding, also tested the refinement-over-type-alias shape (type Str = String; fn make(-> @{ @Str | p })). That case already worked because _resolve_base_type_name is recursive — but it had no test. Added test_refinement_over_type_alias_in_interpolation to pin the working behaviour against future regressions in the alias-resolution path.
Test count 3,779 → 3,781 (+2). The bug class now has five concrete tests covering: plain (#602), type alias (#627), inline refinement, nested refinement, refinement-over-alias, plus the IndexExpr-of-FnCall variant for the Array case.
…n line Header said 'All five shapes now verified' but enumerated six items (the IndexExpr variant got added during the review-pass commit, pushing the count off). Restructured: '5 return-type shapes ... plus the parallel IndexExpr-of-FnCall path' separates the return-type cases (5) from the qualitatively different element- type-of-IndexExpr inference (1), so the count matches and the two distinct fix paths are visible. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 17: Update the v0.0.141 CHANGELOG note to correctly state four regression
tests were added (inline refinement String, indexed interpolation, nested
refinement, refinement-over-alias) instead of three; edit the sentence that
references the i32_pair helper `_resolve_i32_pair_ret_te`, the i32_pair
branches, and the IndexExpr inference path so it mentions "four new regression
tests in TestStringInterpolation" to match the actual tests added (including the
Array-indexed shape).
In `@TESTING.md`:
- Line 9: Update the stale counts in TESTING.md's Validation Scripts table to
match the authoritative output of python scripts/check_doc_counts.py: change
check_examples.py from 33 to 34 examples and check_spec_examples.py from 148 to
164 spec blocks; run python scripts/check_doc_counts.py to verify the numbers
and commit the updated table entries in the same PR so the overview totals and
per-script counts are consistent.
In `@tests/test_codegen.py`:
- Around line 8319-8346: Update the
test_inline_refinement_array_in_indexed_interpolation case to include a nested
refinement return type so the IndexExpr-of-FnCall path is exercised with nested
unwrapping; specifically, change the `make` function's return refinement from
`@{ `@Array`<Int> | array_length(`@Array`<Int>.0) > 0 }` to a nested form like `@{
@{ `@Array`<Int> | array_length(`@Array`<Int>.0) > 0 } | <outer-predicate> }`
(keeping a predicate that still allows the array length check) and keep the
`main` call and expected assertion `assert _run_io(source, fn="main") == "20\n"`
so the test verifies the nested-refinement case on the FnCall+IndexExpr path.
In `@vera/wasm/inference.py`:
- Around line 820-870: The apply_fn path still treats only bare ast.NamedType
and peels at most one ast.RefinementType, so nested or closure-return
refinements can be misclassified; update apply_fn and the FnType WASM helpers to
canonicalise return TypeExprs by reusing the existing _resolve_i32_pair_ret_te
logic (or extract its loop into a shared helper) so that before checking
vera_type == "String" / "Array" you iteratively unwrap ast.RefinementType layers
and then resolve NamedType via _resolve_base_type_name; ensure any place that
inspects FnType.return_type (the apply_fn flow and the FnType->WASM conversion
helpers) calls that canonicaliser and treats a None result as non-i32_pair.
🪄 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: cc550555-3124-4a96-b71c-5fc710566ecb
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdROADMAP.mdTESTING.mdtests/test_codegen.pyvera/wasm/inference.py
Sixth review pass on PR #629 surfaced a third pair of inference sites walking return-types that only handled NamedType directly. When apply_fn(@FnAlias.0, ()) is used inside an interpolation and FnAlias is a FnType whose return type wraps refinements, three sites in vera/wasm/inference.py returned None for nested refinements and the interpolation fell through to the to_string(...) wrapper, reproducing the original #602 trap with yet another trigger: - _infer_fncall_vera_type apply_fn branch - _resolve_generic_fn_return - _fn_type_return_wasm Fix: same `while isinstance(ret, ast.RefinementType): ret = ret.base_type` loop applied symmetrically at all three sites, matching the FnCall path shape. Also: - Updated test_inline_refinement_array_in_indexed_interpolation to use the nested-refinement form so the array branch exercises the while-loop unwrap (per CodeRabbit finding). - Added test_apply_fn_nested_refinement_in_interpolation pinning the new path. - Fixed CHANGELOG count Three new regression tests - actually five, with accurate enumeration of all return-type shapes now verified. - Refreshed test counts in TESTING.md and ROADMAP.md (3,781 -> 3,782). Skipped: - TESTING.md line 9 stale-counts finding - verified those numbers are already current (33 examples and 148 spec blocks no longer in the file). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
TESTING.md (1)
356-357:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync the validation scripts table with the overview totals (still outstanding).
The overview table at lines 12-13 correctly states 34 examples and 164 spec blocks, but the Validation Scripts table here still shows the stale figures (33 examples at line 356 and 148 spec blocks at line 357). This inconsistency was flagged in a previous review and remains unaddressed.
Based on learnings,
python scripts/check_doc_counts.pyis the authoritative source for verifying these counts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TESTING.md` around lines 356 - 357, Update the Validation Scripts table entries for `check_examples.py` and `check_spec_examples.py` to match the overview totals by changing the counts from 33→34 for the `.vera` examples and from 148→164 for the spec code blocks; verify the updated numbers by running the authoritative script `python scripts/check_doc_counts.py` and commit the corrected table rows referencing `check_examples.py` and `check_spec_examples.py`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vera/wasm/inference.py`:
- Around line 657-673: The branch in _infer_apply_fn_return_type (around the
alias substitution that currently returns self._format_named_type(ta) /
self._format_named_type(base_ret)) returns alias names instead of the canonical
underlying type; change it to fully resolve aliases before formatting: when you
find a NamedType (either base_ret or ta) that appears in alias_map, follow the
mapping, unwrap any ast.RefinementType/base_type layers and recursively follow
any further NamedType aliases until you reach a non-NamedType base, then call
self._format_named_type on that resolved type; apply the same resolution logic
in both the alias-substitution branch and the fallback branch so
_infer_apply_fn_return_type matches the canonicalization behavior of
_infer_fncall_vera_type.
---
Duplicate comments:
In `@TESTING.md`:
- Around line 356-357: Update the Validation Scripts table entries for
`check_examples.py` and `check_spec_examples.py` to match the overview totals by
changing the counts from 33→34 for the `.vera` examples and from 148→164 for the
spec code blocks; verify the updated numbers by running the authoritative script
`python scripts/check_doc_counts.py` and commit the corrected table rows
referencing `check_examples.py` and `check_spec_examples.py`.
🪄 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: 59ae9b6d-7e08-4e05-a598-11b0657c2eee
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdtests/test_codegen.pyvera/wasm/inference.py
Eighth trigger of the #602 bug class — surfaced by CodeRabbit on PR #629's final review. Path: `apply_fn(@Maker.0, ())` inside an interpolation, where `Maker = fn(Unit -> Str) effects(pure)` and `type Str = String;`. `_infer_fncall_vera_type`'s apply_fn branch called `_format_named_type` on `NamedType("Str")` which returns the alias name "Str" — downstream `_translate_interpolated_string` checks `vera_type == "String"`, the alias name missed, and the value fell through to the `to_string(...)` wrapper over an `i32_pair`, reproducing the canonical `expected i64, found i32` WASM-validation surface. Fix: introduced `_format_named_type_canonical` (resolves `te.name` through the alias chain via `_resolve_base_type_name`, then formats with original `type_args`). Replaced both `_format_named_type` calls in the apply_fn branch — substitution and fallback — with the canonical variant. Also unwraps RefinementType layers from the substituted type arg before canonicalising, mirroring the `while`-loop already applied to `alias_te.return_type`. This matches the canonicalisation behaviour of `_resolve_i32_pair_ret_te` for the regular FnCall path — previously the FnCall path resolved alias chains but the apply_fn path did not. Also addresses CodeRabbit duplicate finding on TESTING.md: Validation Scripts table at lines 356-357 had stale counts (33 examples, 148 spec blocks) — refreshed to 34 / 164. The earlier-skipped duplicate of this finding was for line 9 (already current); this hit was at a different line that hadn't been updated. New regression test `test_apply_fn_aliased_string_in_interpolation` pins the path. Test counts refreshed (3,782 → 3,783). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vera/wasm/inference.py (1)
671-725:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle direct anonymous closures in
apply_fnVera-type inference.This branch now canonicalises aliased
SlotRefclosures, butapply_fnwith a directast.AnonFnstill falls through toNonein_infer_fncall_vera_type._infer_apply_fn_return_type()already treats that same call shape asi32_pair, so interpolatingapply_fn(fn (...) -> String { ... }, ...)will still hit the oldto_string(...)fallback and recreate theexpected i64, found i32validation failure.Possible minimal fix
if call.name == "apply_fn" and call.args: closure_arg = call.args[0] + if isinstance(closure_arg, ast.AnonFn): + ret: ast.TypeExpr = closure_arg.return_type + while isinstance(ret, ast.RefinementType): + ret = ret.base_type + if isinstance(ret, ast.NamedType): + return self._format_named_type_canonical(ret) if isinstance(closure_arg, ast.SlotRef): alias_te = self._type_aliases.get(closure_arg.type_name) if isinstance(alias_te, ast.FnType): alias_params = self._type_alias_params.get( closure_arg.type_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vera/wasm/inference.py` around lines 671 - 725, The apply_fn path currently handles SlotRef closures but falls through for direct anonymous closures (ast.AnonFn), causing _infer_fncall_vera_type to return None; add a branch alongside the SlotRef handling in _infer_fncall_vera_type to detect isinstance(closure_arg, ast.AnonFn), extract the anonymous function return type (e.g. closure_arg.fn_type.return_type or the appropriate field on ast.AnonFn), iteratively unwrap ast.RefinementType layers to get the base return type, and if that base is an ast.NamedType return self._format_named_type_canonical(base) so anonymous closures are canonicalised the same way as aliased FnTypes (mirroring the existing use of _format_named_type_canonical and the RefinementType unwrapping).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@vera/wasm/inference.py`:
- Around line 671-725: The apply_fn path currently handles SlotRef closures but
falls through for direct anonymous closures (ast.AnonFn), causing
_infer_fncall_vera_type to return None; add a branch alongside the SlotRef
handling in _infer_fncall_vera_type to detect isinstance(closure_arg,
ast.AnonFn), extract the anonymous function return type (e.g.
closure_arg.fn_type.return_type or the appropriate field on ast.AnonFn),
iteratively unwrap ast.RefinementType layers to get the base return type, and if
that base is an ast.NamedType return self._format_named_type_canonical(base) so
anonymous closures are canonicalised the same way as aliased FnTypes (mirroring
the existing use of _format_named_type_canonical and the RefinementType
unwrapping).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5edfbbe3-4cdc-4589-bdb1-c6c9308dc371
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdTESTING.mdtests/test_codegen.pyvera/wasm/inference.py
PR #629 patched eight distinct triggers of the same canonical-name canonicalisation drift, each one fixed locally. The underlying pattern — five overlapping canonicalisation helpers in `vera/wasm/inference.py` plus a silent fallthrough at `vera/wasm/operators.py:482` that amplifies any inference miss into invalid WASM — remains exposed. The 9th trigger is a matter of when, not if. Filed #630 to close the bug class structurally: - Tier 1: fold the five helpers (`_format_named_type`, `_format_named_type_canonical`, `_resolve_i32_pair_ret_te`, `_resolve_base_type_name`, ad-hoc while-loops) into a single `_canonical_vera_type` covering RefinementType unwrap + alias-chain follow + generic substitution + format-with-args. Apply at every type-walking inference site. - Tier 2: convert the silent fallthrough at operators.py:482 to a hard compile-time error. Dovetails with #626 Layer 1. Together they make the 9th trigger either impossible (T1) or instantly diagnosable (T2). Pure refactor + diagnostic conversion — no behavioural change for any valid program; all 8 existing regression tests in TestStringInterpolation continue to pin the now-fixed triggers. Added to ROADMAP stabilisation tier as item #3 (after #604 and #626; #630 is a sibling of #626 — single canonicalisation site rather than the broader translator-returns-None pattern). Renumbered the rest of the table. Added to KNOWN_ISSUES.md "Bugs" section alongside #628 — both are latent bug-class entries where individual instances are patched but the underlying pattern remains exposed. Co-Authored-By: Claude <noreply@anthropic.invalid>
…er 9) Ninth trigger of the #602 bug class — surfaced by CodeRabbit during PR #629's final review pass, less than an hour after filing #630 (the structural close-out tracking issue). Empirical confirmation that local-fix throughput can't keep up with discovery velocity; this is exactly the argument made in #630 for the Tier 1 + Tier 2 structural fix. Path: `apply_fn(fn(@Unit -> @string) effects(pure) { ... }, ())` inside an interpolation — apply_fn called directly on an inline `AnonFn` literal rather than a `SlotRef` to a let-bound closure. Pre-fix `_infer_fncall_vera_type` only handled the SlotRef arg shape; the AnonFn case fell through to None, downstream interpolation produced `to_string(...)` over an `i32_pair`, and WASM validation rejected the module with the canonical `expected i64, found i32` surface. Fix: added an `elif isinstance(closure_arg, ast.AnonFn)` branch alongside the SlotRef branch. Simpler than the SlotRef path (no alias substitution — AnonFn carries `return_type` directly), but the same RefinementType-unwrap + `_format_named_type_canonical` shape applies. New regression test `test_apply_fn_anon_inline_string_in_interpolation` pins the path. Test counts refreshed (3,783 → 3,784). KNOWN_ISSUES entry for #630 updated to reflect the 9th trigger. Co-Authored-By: Claude <noreply@anthropic.invalid>
Owner
Author
|
@coderabbitai re: outside-diff finding on Empirically reproduced Fixed by adding an Notable: the 9th trigger landed within an hour of filing #630 (the structural close-out tracking issue), which is exactly the empirical argument made there — local fix throughput can't keep up with discovery velocity. Updated #630's body and KNOWN_ISSUES.md to reflect the 9th trigger; #630 is queued as item #3 in the stabilisation tier of ROADMAP.md, ahead of further reactive fixing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3273 🏁 Script executed: Length of output: 4008
One potential gap surfaced in the second if isinstance(ret, ast.RefinementType): # pragma: no cover — closure returns are not refinement types
base = ret.base_type
if isinstance(base, ast.NamedType):
...Two issues with this branch:
if isinstance(ret, ast.RefinementType):
while isinstance(ret, ast.RefinementType):
ret = ret.base_type
if isinstance(ret, ast.NamedType):
resolved_name = self._resolve_type_name_to_wasm_canonical(ret.name)
if resolved_name in ("String", "Array"):
return "i32_pair"
return self._named_type_to_wasm(resolved_name)This would be the 10th trigger if left as-is. Whether to fix it here or file it against |
trigger 10) Tenth trigger of the #602 bug class — surfaced by CodeRabbit on PR #629 immediately after the 9th was fixed. Inverse surface: `expected i32, found i64` rather than the usual `expected i64, found i32`, because this site is on the *WASM-type* inference half of the dispatcher (`_infer_apply_fn_return_type`, which infers the call_indirect sig) rather than the Vera-type-name half (`_infer_fncall_vera_type`, where the 9th trigger hit). Path: `apply_fn(fn(@Unit -> @{ @{ @string | p1 } | p2 }) effects(pure) { ... }, ())` — inline AnonFn declaring a nested-refinement return. Two issues with the pre-fix branch: 1. `# pragma: no cover — closure returns are not refinement types` was empirically disproved. The 9th trigger (PR #629) already established that an inline AnonFn *can* declare a RefinementType return per the grammar (`fn(@Unit -> @{ @string | p })`); the type checker accepts it. The pragma's claim was load-bearing and wrong. 2. Single-level unwrap `if isinstance(ret, ast.RefinementType): base = ret.base_type` — same pattern fixed twice elsewhere in this PR. Nested forms left `base` as another RefinementType, the inner NamedType check missed, and the method fell through to `return "i64"` despite the call site emitting i32_pair. Fix: replaced the single-level if-unwrap with the established `while`-loop shape and removed the disproven pragma comment. This is the second trigger landing within hours of filing #630 (the structural close-out tracking issue). Both surfaces of the bug class are now visible — Vera-type-name half (#629 triggers 1–9) and WASM-type half (this trigger). Both route through #630's centralised canonicaliser plan; verifying #630 closes both surfaces is part of its acceptance criteria. Tension noted with the policy stated when filing #630 ("from here forward, the right move on any new trigger is to fold it into #630's PR rather than landing it as a one-off"). Exception applied here because: (a) the `# pragma: no cover` claim is provably wrong right now and is a misleading code comment that would mislead any reader before #630 lands — removing it is hygiene, not drift back to reactive fixing; (b) the `while`-loop is the same one-line shape applied 4 other times in this PR; cost of local fix is trivial. The structural close-out remains queued as #630. New regression test `test_apply_fn_anon_nested_refinement_in_interpolation` pins the WASM-side path. Test counts refreshed (3,784 → 3,785). KNOWN_ISSUES entry for #630 updated to reflect the 10th trigger. Co-Authored-By: Claude <noreply@anthropic.invalid>
Owner
Author
|
@coderabbitai re: The pragma was disproved. Single-level unwrap was the 10th trigger. Empirically reproduced Tension with the #630 'no more reactive fixing' policy noted. Stated when filing #630 that further triggers should fold into #630's PR, not land as one-offs. Exception applied here because (a) the disproven pragma is a misleading comment that would mislead any reader of this file before #630 lands — removing it is hygiene, not policy drift; (b) cost of the local fix is trivial (one line). The structural close-out remains queued as #630, now with two new audit items folded in: both surfaces of the dispatcher (Vera-type-name + WASM-type) and a pragma-claim audit pass. This is the 10th trigger across 4 PRs over ~3 days, with #9 and #10 both landing within hours of filing #630. The empirical case for the structural fix is now decisive. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5125
The while-loop at Line 1183 is in place: ret: ast.TypeExpr = closure_arg.return_type
while isinstance(ret, ast.RefinementType):
ret = ret.base_type
if isinstance(ret, ast.NamedType):
resolved_name = self._resolve_type_name_to_wasm_canonical(ret.name)
if resolved_name in ("String", "Array"):
return "i32_pair"
return self._named_type_to_wasm(resolved_name)The disproved The file now has 7 consistent while-loop unwrap sites (Lines 691, 742, 952, 1052, 1183, 1220, 1277), which is the right shape for the local stop-gap ahead of |
8 tasks
aallan
added a commit
that referenced
this pull request
May 8, 2026
…ents + CodeRabbit Four review agents (code-reviewer, pr-test-analyzer, comment-analyzer, silent-failure-hunter) plus three CodeRabbit comments produced a batch of cross-cutting findings. This commit addresses them all in one pass to avoid the iterative round-trip cost the bug class itself was filed to eliminate. **Critical fixes:** 1. **Closure body E615 harvest gap (silent-failure-hunter C1).** `_compile_lifted_closure` constructed its own WasmContext but never harvested `_interp_inference_failures` — closure bodies with E615-triggering interpolation segments were silently dropped, producing call_indirect→missing-table-entry WASM validation traps with no source-located diagnostic. Extracted the harvest into `CodeGenerator._harvest_interp_inference_failures` on the codegen base and called it from both functions.py and closures.py. Removed the now-disproved `# pragma: no cover — defensive` claim on closures.py's body-instrs-None path. New regression test `test_e615_in_closure_body_emits_diagnostic`. 2. **`_canonical_named_type` type_args from terminal NamedType (CodeRabbit #3 + code-reviewer I1).** The walker's `outer_type_args` capture rule (always read from first NamedType) lost type_args when alias_map substitution bound a generic param to a parameterised type. Empirically reachable today via `type Mapper<A, B> = fn(A -> B); Mapper<Int, Array<Int>>` etc. Fix: drop the "outermost" rule, return type_args from the terminal NamedType reached. All 21 existing #602-class regression tests still pass. New regression test `test_canonical_named_type_terminal_args_propagation`. 3. **HISTORY.md footer (CodeRabbit #1).** 141 tagged releases → 142 to match the v0.0.142 row added in PR #631. **UX fix:** 4. **Multiple E615 per function (silent-failure-hunter H2).** Pre-this-fix `_translate_interpolated_string` returned None on the first failing segment, requiring N round-trips for N bad segments in one interpolation. Now collects every failing segment via `had_failure` flag, then bails at the end. New regression test `test_multiple_e615_in_one_interpolation`. **Doc-quality fixes (comment-analyzer):** 5. **Trigger-count drift** in `operators.py:491-494` — "every canonicalisation gap (#602 and ten subsequent triggers)" implied 11; rewrote to "the ten triggers of the #602 bug class accumulated across PRs #627 + #629". 6. **`_format_named_type_canonical` docstring drift** — claim "matches the pre-#630 fallback shape" was empirically wrong (pre-#630 resolved through aliases, post-#630 fallback doesn't). Rewrote to acknowledge the deliberate behavioural change. 7. **"Future closure-arg shapes plug in here without further dispatch ladder"** — overstatement. Adding a `FnCall` returning a closure still needs an `elif`; what's saved is canonicalisation logic, not dispatch logic. `IfExpr` between closures has no single `return_type` field — example was misleading. Rewrote in both `_infer_fncall_vera_type` and `_infer_apply_fn_return_type`. 8. **`_canonical_wasm_type` docstring** — added explicit `Unit → None` documentation; clarified that `None` (no WASM type) and `"i64"` (unreachable-NamedType default) are distinct return shapes that callers must not conflate. **Tests added (besides the regressions tied to fixes):** 9. **`test_per_function_isolation_of_failures_list`** — pins that a clean function and a dirty function in the same source produce independent `_interp_inference_failures` lists. 10. **`test_e615_fires_on_result_in_interpolation`** — adjacent ADT shape (Result vs Option) so a future Option-handling broadening doesn't accidentally regress the parallel Result path. 11. **`test_array_map_refinement_returning_closure`** — pins the previously-unaudited `_infer_closure_return_vera_type` path in `calls_arrays.py`, which the #630 migration broadened to handle refinements. **Test enhancements:** 12. **E615 ordering before matching E602** in the existing test (CodeRabbit #2) — pinned via `warnings.index(e615[0])` < `warnings.index(matching_e602)`. Filtered to the E602 that mentions the offending function so prelude-skip E602s don't confound the assertion. 13. **E615 source-location attached** in the existing test — softened to "line > 0" (any location) because SlotRef-inside- InterpolatedString spans are unreliable today (#634, separate follow-up). Tightening to "line points at the segment" is the natural acceptance test for that follow-up. **Skipped with reasons:** - **Cycle-guard regression on `_resolve_base_type_name`** — test-analyzer C1 / code-reviewer S4. Verified pre-existing and dead-code-safe today (cycles crash an upstream resolver first). Filed as #633. - **`_canonical_wasm_type`'s `i64` default for unhandled apply_fn / call_indirect shapes** — silent-failure-hunter H1. Bigger refactor parallel to Tier 2 but for the WASM-side dispatch. Filed as #632. - **SlotRef-inside-InterpolatedString span fidelity** — surfaced when writing the E615 source-location assertion. Diagnostic-quality concern, separate from #630's canonicalisation scope. Filed as #634. - **`_canonical_named_type` worked-example docstring** (comment-analyzer I6) — superseded by fix #2 above (semantics changed; docstring rewritten with concrete examples). - **Per-context isolation prose-vs-test** (comment-analyzer I4) — addressed via fix #9 (test added). - **`_resolve_i32_pair_ret_te` 30-line upper docstring** — comment-analyzer style nit. The prose is load-bearing context (#628 cross-module narrative); trimming would lose history without obvious gain. Defer. **Follow-ups:** #632 (apply_fn / call_indirect E616), #633 (_resolve_base_type_name cycle guard), #634 (interpolation SlotRef source spans). Added to KNOWN_ISSUES.md alongside #628. Counts: 3,792 tests (3,778 passed, 14 skipped) — six new tests under `TestE615LoudInterpolationFallthrough630`. CHANGELOG + KNOWN_ISSUES + ROADMAP + TESTING.md + HISTORY footer all synced. mypy clean. Co-Authored-By: Claude <noreply@anthropic.invalid>
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
Follow-up to PR #627 (v0.0.140, closed #602). Third trigger of the same bug class — surfaced during PR #627's CodeRabbit review (duplicate-comment escalation flagging that the fix needed to handle
RefinementTypein addition toNamedType), but the merge happened before the fix landed in #627 itself.Bumps version
v0.0.140 → v0.0.141.The bug shape
Three triggers in sequence, all producing the same
expected i64, found i32at WASM validation:@Stringreturn (s_arr-shape)type Str = String;alias return@{ @String | predicate }return@{ @Array<T> | ... }_infer_index_element_type_expr)Root cause
_register_fnstores the literal AST in_fn_ret_type_exprs. v0.0.140's fix usedisinstance(ret_te, ast.NamedType)which silently fell through forRefinementType. Same gap in_infer_index_element_type_expr's FnCall branch (the path #614 added).Fix
Extracted a helper
_resolve_i32_pair_ret_te(ret_te)that handles bothNamedType(with alias resolution via the existing_resolve_base_type_name) andRefinementType(unwrap tobase_type, then resolve). Applied to:_infer_fncall_vera_typenon-generic branch_infer_fncall_vera_typegeneric-mono branch_infer_index_element_type_exprFnCall branch (RefinementType-unwrap inlined since it returns a different shape)Verified reproducer
Invalid input WebAssembly code at offset 911: type mismatch: expected i64, found i32helloRegression tests
Two new tests in
TestStringInterpolation:test_inline_refinement_string_in_interpolation— the minimum reproducer above.test_inline_refinement_array_in_indexed_interpolation—@{ @Array<Int> | ... }indexed viaf()[i]inside interpolation; exercises the parallel_infer_index_element_type_exprfix path.Test count 3,777 → 3,779 (+2).
Test plan
hello/20post-fixpytest tests/ -q -n auto— 3,765 passed, 14 skippedmypy vera/— cleanpython scripts/check_doc_counts.py— consistentpython scripts/check_version_sync.py— v0.0.141 across 6 filespython scripts/check_site_assets.py— up to datepython scripts/check_limitations_sync.py— consistentSummary by CodeRabbit
Bug Fixes
Tests
Documentation
Chore