Skip to content

v0.0.136: fix #586 (apply_fn(closure, ()) on Unit-arg closure trips WASM validation)#587

Merged
aallan merged 2 commits into
mainfrom
claude/fix-apply-fn-unit-arg
May 7, 2026
Merged

v0.0.136: fix #586 (apply_fn(closure, ()) on Unit-arg closure trips WASM validation)#587
aallan merged 2 commits into
mainfrom
claude/fix-apply-fn-unit-arg

Conversation

@aallan

@aallan aallan commented May 6, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #586.

apply_fn(closure, ()) on a (Unit -> X) closure tripped a WASM type-mismatch at compile time:

type mismatch: expected i64, found i32

vera check passed (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: None overloaded between "Unit" and "unknown").

Root cause

_translate_apply_fn in vera/wasm/closures.py (~line 148) defaulted the value-arg's WASM type to i64 when _infer_expr_wasm_type returned None:

wt = self._infer_expr_wasm_type(arg)
if wt == "i32_pair":
    arg_wasm_types.extend(["i32", "i32"])
else:
    arg_wasm_types.append(wt or "i64")  # bug: None means Unit OR unknown

For a UnitLit argument:

  • translate_expr(UnitLit, env) returned [] (no WASM value pushed — correct)
  • _infer_expr_wasm_type(UnitLit) returned None (correct — Unit has no representation)
  • wt or "i64" evaluated to "i64" — registered a phantom param in the call_indirect sig

The 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 a call_indirect (type $closure_sig_1) where $closure_sig_1 was (param i32) (param i64) (result i64). The validator saw the closure's func_table_idx (an i32) 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

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.
  • Version files bumped 0.0.135 -> 0.0.136.
  • Conformance + test count surfaces updated 84 -> 85 / 3,728 -> 3,733.
  • Site assets regenerated.
  • uv.lock regenerated 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 using array_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

    • Added a conformance test covering unit-argument closures, expanding the suite to 85 programs.
  • Bug Fixes

    • Fixed closure WASM code generation to ignore unit-typed arguments, removing phantom parameters.
  • Documentation

    • Updated CHANGELOG, README, ROADMAP, FAQ, HISTORY and other docs to reflect v0.0.136 and the 85-program suite.
  • Chores

    • Bumped package version to v0.0.136.
  • Tests

    • Updated test manifest and testing guidance for the expanded conformance suite.

@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.97%. Comparing base (2c046cb) to head (e67b7b5).

Files with missing lines Patch % Lines
vera/wasm/closures.py 66.66% 1 Missing ⚠️
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              
Flag Coverage Δ
javascript 57.36% <ø> (ø)
python 94.81% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8b266518-552e-4664-b2d9-543f7b984585

📥 Commits

Reviewing files that changed from the base of the PR and between 9ae5264 and e67b7b5.

📒 Files selected for processing (3)
  • README.md
  • TESTING.md
  • vera/wasm/closures.py

📝 Walkthrough

Walkthrough

Fixes WASM codegen so apply_fn does not add phantom i64 params for Unit-arg closures, adds a conformance test ch05_unit_arg_closure, bumps package to v0.0.136, and updates docs/metrics to show 85 conformance programs and updated test totals.

Changes

Unit-Arg Closure Codegen Fix & Release

Layer / File(s) Summary
Core Codegen Logic
vera/wasm/closures.py
_translate_apply_fn now skips unit-typed arguments (wt is not None) when building arg_wasm_types instead of defaulting None to "i64", preventing phantom parameters in call_indirect signatures.
Test Manifest
tests/conformance/manifest.json
Added new manifest entry ch05_unit_arg_closure.vera covering Unit-arg closure apply_fn cases.
Package Versioning
vera/__init__.py, pyproject.toml
Bumped package version from 0.0.135 to 0.0.136.
Release Notes & History
CHANGELOG.md, HISTORY.md
Inserted Unreleased / v0.0.136 entries documenting the WASM codegen tweak and the new conformance test; release counts updated.
Documentation & Project Status
README.md, ROADMAP.md, AGENTS.md, CLAUDE.md, FAQ.md, SKILL.md, TESTING.md
Updated conformance-suite count from 84→85 and adjusted test/metric lines and conformance-check script hints accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • aallan/vera#454: Modifies vera/wasm/closures.py _translate_apply_fn handling (i32_pair/unit-arg related changes).

Suggested labels

compiler, tests, docs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately identifies the primary fix—Unit-argument closure handling in apply_fn WASM codegen—and includes version bump and issue reference.
Linked Issues check ✅ Passed All acceptance criteria from #586 are met: reproducer fixed; new conformance test added; existing closure tests pass; full test suite green.
Out of Scope Changes check ✅ Passed Changes are appropriately scoped to the fix, docs, and version bump. Documentation updates and test counts reflect the single new conformance test; no unrelated modifications present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-apply-fn-unit-arg

Comment @coderabbitai help to get the list of available commands and usage tips.

@aallan

aallan commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@aallan

aallan commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@aallan

aallan commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Conformance 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.py as 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

📥 Commits

Reviewing files that changed from the base of the PR and between c5c0263 and ff99876.

⛔ Files ignored due to path filters (7)
  • docs/SKILL.md is excluded by !docs/**
  • docs/index.html is excluded by !docs/**
  • docs/index.md is excluded by !docs/**
  • docs/llms-full.txt is excluded by !docs/**
  • docs/llms.txt is excluded by !docs/**
  • tests/conformance/ch05_unit_arg_closure.vera is excluded by !**/*.vera
  • uv.lock is excluded by !**/*.lock, !uv.lock
📒 Files selected for processing (13)
  • AGENTS.md
  • CHANGELOG.md
  • CLAUDE.md
  • FAQ.md
  • HISTORY.md
  • README.md
  • ROADMAP.md
  • SKILL.md
  • TESTING.md
  • pyproject.toml
  • tests/conformance/manifest.json
  • vera/__init__.py
  • vera/wasm/closures.py

Comment thread README.md Outdated
Comment thread vera/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>
@aallan aallan force-pushed the claude/fix-apply-fn-unit-arg branch from ff99876 to 9ae5264 Compare May 6, 2026 21:13
…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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
README.md (1)

184-184: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Re-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 with python scripts/check_doc_counts.py before merging so the public status line does not drift.

Based on learnings: use python scripts/check_doc_counts.py or pytest tests/ --collect-only -q in 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 win

Don't equate None with Unit in the signature builder.

Line 152 calls _infer_expr_wasm_type(), whose contract is “unknown or UnitNone”. Silently skipping every None therefore bakes in a stronger assumption than the helper provides. A future expression kind that still pushes values but infers as None will desynchronise the call_indirect signature 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

📥 Commits

Reviewing files that changed from the base of the PR and between ff99876 and 9ae5264.

⛔ Files ignored due to path filters (7)
  • docs/SKILL.md is excluded by !docs/**
  • docs/index.html is excluded by !docs/**
  • docs/index.md is excluded by !docs/**
  • docs/llms-full.txt is excluded by !docs/**
  • docs/llms.txt is excluded by !docs/**
  • tests/conformance/ch05_unit_arg_closure.vera is excluded by !**/*.vera
  • uv.lock is excluded by !**/*.lock, !uv.lock
📒 Files selected for processing (13)
  • AGENTS.md
  • CHANGELOG.md
  • CLAUDE.md
  • FAQ.md
  • HISTORY.md
  • README.md
  • ROADMAP.md
  • SKILL.md
  • TESTING.md
  • pyproject.toml
  • tests/conformance/manifest.json
  • vera/__init__.py
  • vera/wasm/closures.py

Comment thread TESTING.md
@aallan

aallan commented May 6, 2026

Copy link
Copy Markdown
Owner Author

@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 check_doc_counts.py doesn’t lint today (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 out of scope for this PR — the immediate fix matches the canonical numbers.

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@aallan — confirmed, both corrections are correct:

  • Line 86: 84 small, focused programs85
  • Line 187: parametrized — 410 tests425 tests

The gap in check_doc_counts.py coverage is a real correctness risk — prose counts drifting silently while the structured table stays green is exactly the class of stale-doc bug that's hard to catch in review. Extending the linter to grep for numeric prose patterns (e.g. \d+ small, focused programs and parametrized — \d+ tests) against the canonical values would close it.

Would you like me to open a GitHub issue to track that follow-up?

@aallan aallan merged commit ca16925 into main May 7, 2026
20 checks passed
@aallan aallan deleted the claude/fix-apply-fn-unit-arg branch May 7, 2026 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

apply_fn(closure, ()) on (Unit -> X) closure trips WASM type mismatch (phantom i64 param in call_indirect sig)

1 participant