v0.0.136: fix #586 (apply_fn(closure, ()) on Unit-arg closure trips WASM validation)#587
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #587 +/- ##
==========================================
- Coverage 90.98% 90.97% -0.01%
==========================================
Files 59 59
Lines 22927 22929 +2
Branches 259 259
==========================================
+ Hits 20859 20860 +1
- Misses 2061 2062 +1
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:
|
|
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)
📝 WalkthroughWalkthroughFixes WASM codegen so ChangesUnit-Arg Closure Codegen Fix & Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TESTING.md (1)
86-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConformance counts are still stale in two places
After the 85-program update, Line 86 still says “84” and Line 187 still says “410 tests”, which now conflicts with Line 76 (
425) and the updated totals.Suggested doc fix
-The conformance suite is a collection of 84 small, focused programs in `tests/conformance/` that systematically validate every language feature against the spec. +The conformance suite is a collection of 85 small, focused programs in `tests/conformance/` that systematically validate every language feature against the spec. -# Via pytest (parametrized — 410 tests) +# Via pytest (parametrized — 425 tests)Based on learnings: use canonical pytest collection /
scripts/check_doc_counts.pyas the source of truth for documented test counts.Also applies to: 187-188
🤖 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` at line 86, Update the stale counts in TESTING.md to match the canonical numbers from scripts/check_doc_counts.py: run scripts/check_doc_counts.py to obtain the current totals, then replace the “84” in the conformance suite sentence (the phrase describing the number of programs) and the “410 tests” occurrence (lines referencing the total test count) so they match the script output (e.g., change to “85” and the updated total such as “425” if that is what the script reports); ensure both the single-program count (conformance suite description) and the total tests sentence are consistent with scripts/check_doc_counts.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 `@README.md`:
- Line 184: Update the test count in the README status line to match the
canonical collected-test total: run the canonical source (python
scripts/check_doc_counts.py or pytest tests/ --collect-only -q) to regenerate
the accurate test number, then replace the hardcoded "3,729 tests" string in the
README.md status sentence with the new value so the public doc matches the
authoritative script output.
In `@vera/wasm/closures.py`:
- Around line 142-156: The current logic in the call-site that builds
arg_wasm_types uses _infer_expr_wasm_type and treats None as "Unit" implicitly;
make this defensive by asserting or checking that any None result indeed
corresponds to a Unit-typed parameter from the closure signature before skipping
it (or raise a clear error if not). Update the code around the arg handling in
the same function that populates arg_wasm_types (the apply_fn translation path
where wt = self._infer_expr_wasm_type(arg) and the i32_pair handling occurs) to
verify the parameter type (or add an explicit assert/comment referencing that
closure-lift already skips Unit params) so unexpected None values are caught
with a clear message rather than silently ignored.
---
Outside diff comments:
In `@TESTING.md`:
- Line 86: Update the stale counts in TESTING.md to match the canonical numbers
from scripts/check_doc_counts.py: run scripts/check_doc_counts.py to obtain the
current totals, then replace the “84” in the conformance suite sentence (the
phrase describing the number of programs) and the “410 tests” occurrence (lines
referencing the total test count) so they match the script output (e.g., change
to “85” and the updated total such as “425” if that is what the script reports);
ensure both the single-program count (conformance suite description) and the
total tests sentence are consistent with scripts/check_doc_counts.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: 6ca4aae2-161c-4c6d-a57f-546553967012
⛔ 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/ch05_unit_arg_closure.verais excluded by!**/*.verauv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (13)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdHISTORY.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomltests/conformance/manifest.jsonvera/__init__.pyvera/wasm/closures.py
apply_fn(closure, ()) on a (Unit -> X) closure failed WASM validation with "expected i64, found i32" at compile time. Same falsy-pitfall class as #584's _fn_ret_types filter: _translate_apply_fn in vera/wasm/closures.py defaulted the value-arg's WASM type to "i64" when _infer_expr_wasm_type returned None, but None means BOTH "couldn't infer" AND "Unit has no representation" - conflating the two cases. Result: a UnitLit pushed nothing onto the stack but registered a phantom i64 param in the call_indirect signature. The closure-lift side correctly skipped Unit params, so the two sides disagreed and validation rejected the call (the func_table_idx landing where the phantom value-arg was expected). Fix is three lines: change "arg_wasm_types.append(wt or 'i64')" to "elif wt is not None: arg_wasm_types.append(wt)" so Unit args contribute zero entries to the call_indirect sig. Discovered while smoke-testing #514 / #535 closure-capture work on v0.0.135 to confirm those v0.0.119-era bugs were closed. Both #514 and #535 ARE closed; this is a separate adjacent codegen bug. Conformance: tests/conformance/ch05_unit_arg_closure.vera covers three positions: - No-capture Unit-arg closure - Int-capture Unit-arg closure - Array<Bool>-capture Unit-arg closure (exercises #514's heap- capture work + #586's fix together) Conformance count: 84 -> 85. Total tests 3,735 -> 3,740 (+5 from the parametrized conformance runner: parse, check, verify, run, fmt). Rebased on top of PR #590 (which merged #589's UTF-8 hardening into [Unreleased] without a version bump). The rebase rolls #589's [Unreleased] entry into [0.0.136] alongside this PR's #586 entry, so the v0.0.136 release ships both fixes together. HISTORY.md row for v0.0.136 updated to mention both #586 and #589. Doc updates: - CHANGELOG entry under [0.0.136] Fixed for both #586 and #589 - HISTORY.md Stage 11 row for v0.0.136 names both issues - All count surfaces (TESTING.md, ROADMAP.md, etc.) bumped to 3,740 tests / 85 conformance - uv.lock regenerated Co-Authored-By: Claude <noreply@anthropic.invalid>
ff99876 to
9ae5264
Compare
…h + TESTING.md prose
All three findings verified valid; fixed each.
1. README.md line 184: "3,729 tests" was stale (pre-rebase value).
Live count from check_doc_counts.py is 3,740. Updated to match.
2. vera/wasm/closures.py: rabbit suggested making the Unit-skipping
branch more defensive. The original code was
elif wt is not None: arg_wasm_types.append(wt)
which silently treated None as "Unit" (correct per type-checker
invariant, but invisible at the call site).
Restructured to an explicit elif branch:
elif wt is None:
# Unit arg: ... [type-checker invariant explanation]
pass
else:
arg_wasm_types.append(wt)
This makes the Unit-skipping path visually distinct and documents
the type-checker invariant inline (the only well-typed expression
for which _infer_expr_wasm_type returns None is Unit-typed). A
runtime assert was considered but rejected: verifying Unit-
typedness defensively would require re-running type inference,
which defeats the purpose. The structural change makes the
intent explicit without runtime cost.
3. TESTING.md prose:
- Line 86: "84 small, focused programs" → "85 ..." (post-#587)
- Line 187: "parametrized — 410 tests" → "425 tests"
Both were free-form prose mentions that check_doc_counts.py
doesn't lint (it only checks the structured row at line 11 and
the test-files table at line 76). Worth a follow-up to teach
check_doc_counts.py to find these prose mentions, but that's
out of scope for this PR.
Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (1)
184-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRe-sync the published test total.
Line 184 says
3,729 tests, but the PR context and other updated docs in this release disagree on the collected total. Please regenerate this number withpython scripts/check_doc_counts.pybefore merging so the public status line does not drift.Based on learnings: use
python scripts/check_doc_counts.pyorpytest tests/ --collect-only -qin the real development environment as the source of truth for documented test 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 `@README.md` at line 184, The README's public status line currently hardcodes "3,729 tests" and is out of sync; run the authoritative count script (python scripts/check_doc_counts.py) or collect tests via pytest (pytest tests/ --collect-only -q) to regenerate the tests total, then update the README occurrence of "3,729 tests" to the new number returned by those tools so the documented test count matches the real collected total.vera/wasm/closures.py (1)
144-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't equate
NonewithUnitin the signature builder.Line 152 calls
_infer_expr_wasm_type(), whose contract is “unknown orUnit→None”. Silently skipping everyNonetherefore bakes in a stronger assumption than the helper provides. A future expression kind that still pushes values but infers asNonewill desynchronise thecall_indirectsignature again. Please validate against the closure parameter type before skipping, or use a helper whose contract is Unit-only.🤖 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/closures.py` around lines 144 - 156, The code incorrectly treats any None result from _infer_expr_wasm_type as Unit and skips it when building arg_wasm_types; change the logic to explicitly check the closure parameter's declared type (the parameter node/type used when building the signature) and only skip when that declared type is Unit, or alternatively call a helper that returns Unit-only semantics, ensuring you still expand "i32_pair" into ["i32","i32"] and append other concrete wasm types; update the logic around _infer_expr_wasm_type, arg_wasm_types, and the closure signature construction so that only true Unit parameters are omitted from the call_indirect signature.
🤖 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 `@TESTING.md`:
- Around line 9-11: Update the two stale counts in TESTING.md to match the
already-updated values: change the Conformance Suite intro count that currently
reads "84" to "85" (match the table entry "Conformance programs | 85 programs
across 9 spec chapters") and update the pytest example that currently says "410
tests" to "425 tests" so the document is self-consistent with the table and the
expected pytest/ scripts output.
---
Duplicate comments:
In `@README.md`:
- Line 184: The README's public status line currently hardcodes "3,729 tests"
and is out of sync; run the authoritative count script (python
scripts/check_doc_counts.py) or collect tests via pytest (pytest tests/
--collect-only -q) to regenerate the tests total, then update the README
occurrence of "3,729 tests" to the new number returned by those tools so the
documented test count matches the real collected total.
In `@vera/wasm/closures.py`:
- Around line 144-156: The code incorrectly treats any None result from
_infer_expr_wasm_type as Unit and skips it when building arg_wasm_types; change
the logic to explicitly check the closure parameter's declared type (the
parameter node/type used when building the signature) and only skip when that
declared type is Unit, or alternatively call a helper that returns Unit-only
semantics, ensuring you still expand "i32_pair" into ["i32","i32"] and append
other concrete wasm types; update the logic around _infer_expr_wasm_type,
arg_wasm_types, and the closure signature construction so that only true Unit
parameters are omitted from the call_indirect signature.
🪄 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: 2b8ce8f4-2aaf-4cdc-aa97-0e49566a3067
⛔ 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/ch05_unit_arg_closure.verais excluded by!**/*.verauv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (13)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdHISTORY.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomltests/conformance/manifest.jsonvera/__init__.pyvera/wasm/closures.py
|
@coderabbitai re: outside-diff TESTING.md finding (line 86 + line 187): valid, fixed in e67b7b5. Line 86 “84 small, focused programs” → “85” and line 187 “parametrized — 410 tests” → “425 tests”. Both are free-form prose mentions that |
|
Tip For best results, initiate chat on the files or code changes.
The gap in Would you like me to open a GitHub issue to track that follow-up? |
Summary
Closes #586.
apply_fn(closure, ())on a(Unit -> X)closure tripped a WASM type-mismatch at compile time:vera checkpassed (the program is well-typed at the source level); the failure was purely in codegen.Discovered while smoke-testing the v0.0.119 closure-capture bugs (#514 / #535) on v0.0.135 to confirm they were closed. Both ARE closed; this is a separate adjacent codegen bug — same class as #584 (falsy-pitfall:
Noneoverloaded between "Unit" and "unknown").Root cause
_translate_apply_fninvera/wasm/closures.py(~line 148) defaulted the value-arg's WASM type toi64when_infer_expr_wasm_typereturnedNone:For a
UnitLitargument:translate_expr(UnitLit, env)returned[](no WASM value pushed — correct)_infer_expr_wasm_type(UnitLit)returnedNone(correct — Unit has no representation)wt or "i64"evaluated to"i64"— registered a phantom param in the call_indirect sigThe closure-lift side (
vera/codegen/closures.py:85) correctly skips Unit params. The closure was registered with(param i32) (result i64)(env-only); apply_fn emitted acall_indirect (type $closure_sig_1)where$closure_sig_1was(param i32) (param i64) (result i64). The validator saw the closure'sfunc_table_idx(ani32) where the phantom i64 value-arg was expected.Fix
Three-line change in
vera/wasm/closures.py::_translate_apply_fn. Unit args now contribute zero entries to the call_indirect sig, matching the closure-lift side.Test plan
42).tests/conformance/ch05_unit_arg_closure.veracovers three positions:ch05_closures.veraandch05_nested_closures.verastill pass.check+verify.pytest tests/— 3,719 passed, 14 skipped.mypy vera/clean.Doc updates
CHANGELOG.md— new[0.0.136]entry under Fixed.HISTORY.md— Stage 11 row for v0.0.136 + footer release count 135 -> 136.uv.lockregenerated for the version bump.Notes for follow-up
The conformance-test-writer agent flagged a separate pre-existing bug while writing the Array-capture test:
@Array<Bool>.0[index]Bool-element indexing inside a closure body trips a WASM codegen issue unrelated to #586. Worked around in the conformance test by usingarray_length(...)instead. Worth filing as a follow-up once a clean reproducer is isolated; not blocking this PR.Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests