fix: CLI-boundary arity guard at every engine entry#344
Merged
Conversation
vm::setup_call has always pre-allocated register slots with NanVal::nil()
to size the call frame, but it never validated that the caller supplied
exactly `param_count` args. Missing args silently bound parameters to nil
and ran the function body anyway. Extras were silently ignored. The tree
interpreter has rejected this for years (interpreter/mod.rs:4152); the
VM has not.
Add a VmError::Arity variant and a check_entry_arity backstop in vm::run
and vm::run_with_tools, plus a parallel inline check in VmState::call
(the bench-mode reusable handle). All three are the CLI-boundary's
belt-and-braces — the new main.rs guard catches it first in normal use,
but if a future dispatch path bypasses the CLI (embedding, new engine
fallback chain), the VM itself surfaces the same ILO-R004 the tree
interpreter would have.
VmError::Arity maps to ILO-R004 in the diagnostic registry so cross-engine
error codes stay consistent. The message format matches the tree
interpreter exactly: "{name}: expected {n} args, got {m}".
Internal VM::call (HOF callbacks, JIT bridge re-entry, OP_CALL dispatch)
is deliberately untouched — only the public entry points get the check,
so dynamic dispatch inside a running program pays no extra cost.
PR #336's listview reshape made JitCallError::NotEligible silently fall through to vm::run when the JIT bailed on an opcode (e.g. OP_WINDOW). The fallback is legitimate — but call_raw returns NotEligible ALSO when args.len() != param_count, so missing args started silently running on the VM's nil-padded register frame. Worst-case shape from the interactive-cli persona: `ilo main.ilo add` with no task text wrote the literal string `[ ] nil` to tasks.txt and exited 0. Manifesto worst-class footgun: silent on-disk corruption with no signal. Add a CLI-boundary `check_cli_arity` helper that resolves the entry function exactly the way each engine does (caller-supplied name, else first declared fn), looks up its declared parameter count, and emits an ILO-R004 diagnostic identical to the tree interpreter's existing wording if the counts don't match. Wire it into all four engine entries: run_default, run_cranelift_engine, and the Engine::Vm / Engine::Tree match arms in dispatch_run. Fires BEFORE vm::compile, JIT codegen, or any IO — the tracker test verifies tasks.txt is never written on the error path. Tree interpreter already enforced the contract at function dispatch (interpreter/mod.rs:4152). Running the same check at the CLI boundary keeps the diagnostic shape consistent across every engine and means the contract no longer depends on which dispatch path the user happens to land on. Pairs with the VM-side backstop in the previous commit.
17 integration tests that shell out to the binary and pin the contract: sub-arity, super-arity, and exact-arity across all four engines (default, --run-tree, --run-vm, --run-cranelift), inline source + file dispatch, plus the auto-main routing the interactive-cli tracker hit. The tracker test (`file_auto_main_sub_arity_default`) also asserts tasks.txt is NOT created when the arity check fires — the regression marker for the silent on-disk corruption that motivated the fix. examples/cli-arity-strict.ilo documents the contract for agents encountering this surface and doubles as engine-harness happy-path coverage via `-- run:` / `-- out:` lines.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
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
ilo main.ilo add(the interactive-cli tracker called without the task text positional) silently wrote the literal string[ ] nilto tasks.txt and exited 0 on v0.11.6. Same shape inline:ilo 'f x:n>n;+x 1'returnednilexit 0 on default, --run-vm, and --run-cranelift. v0.11.5 errored loudly. Manifesto worst-class footgun: silent on-disk corruption, no signal.Bisected to PR #336 (
e8b66f5 vm: reshape OP_WINDOW / OP_WINDOW_VIEW). #336 changedJitCallError::NotEligiblefrom "print error + exit 1" to "silently fall through tovm::run" — legitimate, because the listview reshape made OP_WINDOW JIT-bail and the VM is the correct fallback. Butjit_cranelift::call_rawreturnsNotEligibleALSO forargs.len() != param_count, andvm::setup_callhad never validated arity — it pre-allocated registers withNanVal::nil(), silently nil-padding any missing arg. So missing args started running on a nil-padded frame and producing garbage. The tree interpreter alone caught it because of its dispatch-time check atinterpreter/mod.rs:4152.Repro
After this PR:
What's in the diff
Three commits:
vm: reject wrong-arity entry calls instead of nil-padding— addsVmError::Arityvariant +check_entry_aritybackstop invm::run/vm::run_with_tools+ parallel inline check inVmState::call. InternalVM::call(HOF callbacks, JIT bridge re-entry,OP_CALLdispatch) is deliberately untouched, so dynamic dispatch inside a running program pays no extra cost. Maps to ILO-R004 in the diagnostic registry. Includes 4 unit tests exercising the backstop directly.cli: arity guard at every engine entry, not just the tree interpreter— addscheck_cli_arityhelper that resolves the entry function the same way each engine does (caller-supplied name, else first declared fn), looks up declared params, and emits an ILO-R004 diagnostic with identical wording to the tree interpreter ({name}: expected {n} args, got {m}). Wired into all four entry points:run_default,run_cranelift_engine, and theEngine::Vm/Engine::Treearms indispatch_run. Fires BEFOREvm::compile, JIT codegen, or any IO.tests: cross-engine + file-dispatch coverage for the arity guard— 17 integration tests covering sub/super/exact arity across all four engines, inline + file dispatch, auto-main routing. The tracker test explicitly assertstasks.txtis NOT created on the error path — the regression marker for the on-disk corruption. Plusexamples/cli-arity-strict.ilofor harness coverage and agent learning.I deliberately did not revert #336's
NotEligible → VMsilent fallback. That fallback is legitimate (OP_WINDOW is supposed to bail on the JIT and run on the VM). The bug was that the VM didn't check arity. Keeping the fallback preserves the listview perf win; the new guards make the engine choice irrelevant to the error contract.Test plan
cargo test --release --features cranelift— 5637 tests, 161 suites, all greenmain.ilo addcorruption — now errors loudly, tasks.txt never writtenilo 'f x:n>n;+x 1'across all 4 engines — all error with ILO-R004 exit 1ilo 'f x:n>n;+x 1' 5) returns 6 exit 0 on all enginesilo tracker.ilowith no positional andmaintaking 2 params) errors withmain: expected 2 args, got 0Follow-ups
--run-vm/--run-cranelift/--run-treefor inline snippets still surfaces asILO-R002 undefined function: <extra>because the explicit engines treat the second positional as a function name. Pre-existing shape, not in scope for this regression; the silent-corruption surface is specifically the auto-resolved default-engine path.--run-llvm) has its own numeric-only arg parser path that wasn't part of the regression and didn't get the new guard. Worth a follow-up if anyone is using it.