diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f9089c0..5005f564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ For the release process and tag conventions, see [RELEASING.md](RELEASING.md). ### Fixed +- **CLI arguments are type-checked against the entry function's parameters (ILO-517).** A shell string that didn't match its declared parameter type used to be bound as-is, because `parse_cli_arg`'s ladder falls through to `Text` for anything non-numeric. So `tri n:n>n` invoked as `ilo tri.@ main` bound `Value::Text("main")` to a `n` parameter: the tree-walker and VM printed `NaN`, the Cranelift JIT echoed `main`, and every engine exited **0** — a silent wrong answer with a success exit code, which is worse than a crash for any caller that checks `$?`. The CLI was therefore less safe than the language it fronts, where `num "main"` returns `R n t` and the type checker forces the failure to be handled. A new CLI-boundary guard (`check_cli_arg_types`, sibling of the existing `check_cli_arity`) now rejects unambiguously mismatched values with `ILO-R600`, naming the parameter and showing the offending literal, and exits 1. Wired into all four dispatch sites (VM, interpreter, JIT, default) so the error contract can't drift per engine the way ILO-177 did. The guard is deliberately permissive — `_` accepts anything, `O T` still accepts `nil`, and structural / user-defined types are waved through — so the ILO-182 single-fn pass-through keeps working (`ilo greet.@ world` against `s:t` is legitimate usage, not a typo'd function name). Also closes the same hole for `b` parameters. Found while running the ILO-364 closed-loop benchmark, where it cost a model retries on an otherwise-correct program. + - **`get-stream` yields lines on newline, not buffer-fill (ILO-489).** The client-side streaming builtins (`get-stream`, `get-stream-h`, `pst-stream`, `pst-stream-h`, ILO-448) wrapped minreq's `ResponseLazy` in `BufReader::lines()`, which blocks on a full ~8 KiB read-buffer fill before surfacing any line. So a slow SSE upstream that flushes one short event then idles had its lines batched until the buffer filled or the connection closed - functionally correct but latency was buffer-bound, not event-bound. The line splitter now consumes `ResponseLazy`'s byte iterator incrementally and emits each line the instant its `\n` arrives (trailing `\r` stripped for CRLF / chunked encoding), so each event surfaces promptly. EOF still yields a trailing newline-less partial line; mid-stream errors still surface as `ILO-R009 http-stream read error: ...`. Unblocks ILO-482's previously flaky end-to-end streaming test. ### Added diff --git a/examples/cli-arg-types.ilo b/examples/cli-arg-types.ilo new file mode 100644 index 00000000..df5c4c08 --- /dev/null +++ b/examples/cli-arg-types.ilo @@ -0,0 +1,32 @@ +-- CLI arguments are checked against the entry function's declared parameter +-- types before any engine runs (ILO-517). A mismatch is `ILO-R600` with a +-- non-zero exit, not a silently-bound wrong value. +-- +-- Previously `ilo file.ilo main` against `tri n:n>n` bound the text "main" +-- to a `n` parameter: tree/VM printed `NaN`, the JIT echoed `main`, and all +-- of them exited 0. +-- +-- The guard is deliberately permissive, and this file pins the shapes that +-- must keep working: +-- * `n` accepts a numeric literal +-- * `t` accepts a bare ident (the ILO-182 single-fn pass-through) +-- * `_` accepts anything +-- * `O n` accepts `nil` +-- +-- The rejection cases can't live here — the examples harness asserts on +-- successful output — so they're covered by +-- tests/regression_cli_arg_type_mismatch.rs, which runs each one across +-- the default, --vm and --jit engines. + +tri n:n>n;s=+n 1;p=*n s;/p 2 + +greet s:t>t;+"hello " s + +anything x:_>t;"got" + +maybe x:O n>t;??x 0;"ok" + +main>t;fmt "{} {} {} {}" (tri 10) (greet "world") (anything "main") (maybe nil) + +-- run: main +-- out: 55 hello world got ok diff --git a/src/diagnostic/registry.rs b/src/diagnostic/registry.rs index cd84d1c6..41a7eccf 100644 --- a/src/diagnostic/registry.rs +++ b/src/diagnostic/registry.rs @@ -1710,6 +1710,34 @@ A division operation (`/`) was performed with a zero divisor. An operation was applied to a value of the wrong type at runtime. This may indicate a verifier gap for a dynamic code path. +"#, + }, + ErrorEntry { + code: "ILO-R600", + phase: Phase::Runtime, + short: "CLI argument does not match the declared parameter type", + long: r#"## ILO-R600: CLI argument does not match the declared parameter type + +A value passed on the command line could not be interpreted as the type +the entry function declares for that parameter. + +``` +tri n:n>n;s=+n 1;p=*n s;/p 2 + +$ ilo tri.@ main +ILO-R600: argument 1 (`n`) expects n, got text `main` +``` + +Before this check existed the mismatched value was bound as-is, so a `n` +parameter could receive text. Arithmetic on it produced `NaN` and the +process still exited 0 — a silent wrong answer rather than an error. + +This mirrors the guarantee the language already makes in-band: `num "main"` +returns `R n t`, so the type checker forces the failure to be handled. +The CLI boundary now behaves the same way. + +**Fix:** pass a value of the declared type, or widen the parameter type +(`_` accepts anything, `O n` accepts `nil`). "#, }, ErrorEntry { diff --git a/src/main.rs b/src/main.rs index 9d0921d9..629f9625 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4771,6 +4771,11 @@ fn dispatch_run( { return code; } + if let Err(code) = + check_cli_arg_types(&program, func_name, &run_args, &source, mode) + { + return code; + } let compiled = match vm::compile(&program) { Ok(c) => c, Err(e) => { @@ -4810,6 +4815,11 @@ fn dispatch_run( { return code; } + if let Err(code) = + check_cli_arg_types(&program, func_name, &run_args, &source, mode) + { + return code; + } run_interp_with_provider( &program, func_name, @@ -5005,6 +5015,9 @@ fn run_cranelift_engine( if let Err(code) = check_cli_arity(program, func_name, run_args.len(), source, mode) { return code; } + if let Err(code) = check_cli_arg_types(program, func_name, &run_args, source, mode) { + return code; + } let suppress = program_result_should_suppress(program, func_name); #[cfg(feature = "cranelift")] @@ -5466,6 +5479,9 @@ fn run_default( if let Err(code) = check_cli_arity(program, func_name, args.len(), source, mode) { return code; } + if let Err(code) = check_cli_arg_types(program, func_name, &args, source, mode) { + return code; + } let suppress = program_result_should_suppress(program, func_name); // Default engine is the bytecode register VM: it supports every opcode // (closures, listview, len-has-k-count, every modern shape), and avoids @@ -6532,6 +6548,110 @@ fn check_cli_arity( Err(1) } +/// Describe a parsed CLI value for a diagnostic message: the ilo type sigil +/// plus the offending literal, so the reader can see both what arrived and +/// what was expected. +fn describe_cli_value(v: &interpreter::Value) -> String { + match v { + interpreter::Value::Number(n) => format!("number `{n}`"), + interpreter::Value::Text(s) => format!("text `{s}`"), + interpreter::Value::Bool(b) => format!("bool `{b}`"), + interpreter::Value::Nil => "nil".to_string(), + interpreter::Value::List(_) => "a list".to_string(), + _ => "a value of a different type".to_string(), + } +} + +/// Whether a parsed CLI value is compatible with a declared parameter type. +/// +/// Deliberately permissive: this guard exists to catch values that are +/// *unambiguously* wrong (text bound to `n`), not to re-implement the type +/// checker at the CLI boundary. Anything structural or user-defined is +/// waved through and left to the engines. +fn cli_value_matches_type(v: &interpreter::Value, ty: &ast::Type) -> bool { + match ty { + // The numeric family. This is the case ILO-R600 exists for: binding + // text here yielded NaN with a zero exit code. + ast::Type::Number | ast::Type::U32 | ast::Type::U64 | ast::Type::I64 => { + matches!(v, interpreter::Value::Number(_)) + } + ast::Type::Bool => matches!(v, interpreter::Value::Bool(_)), + // `parse_cli_arg_for_param` guarantees Text for `t` params, so there + // is nothing left to check. + ast::Type::Text => true, + // `nil` is a legitimate value for an optional; anything else must + // satisfy the inner type. + ast::Type::Optional(inner) => { + matches!(v, interpreter::Value::Nil) || cli_value_matches_type(v, inner) + } + // `parse_cli_args_typed` already wraps non-lists for `L _` params. + // Everything below is either "don't care" or too structural to judge + // from a shell string without guessing. + ast::Type::Any + | ast::Type::List(_) + | ast::Type::Map(_, _) + | ast::Type::Result(_, _) + | ast::Type::Sum(_) + | ast::Type::Fn(_, _) + | ast::Type::Named(_) => true, + } +} + +/// CLI-boundary type guard. Sibling of `check_cli_arity`: that one checks how +/// many args arrived, this one checks that each is the declared type. +/// +/// Without it, a `n:n` parameter could receive `Value::Text` (a shell string +/// that isn't numeric falls through `parse_cli_arg`'s ladder to `Text`). +/// Downstream arithmetic then produced `NaN` and the process exited 0, so a +/// caller passing an unvalidated string got a silent wrong answer instead of +/// an error — the CLI was less safe than the language it fronts, where +/// `num "main"` returns `R n t` and must be handled. +/// +/// Returns `Err(1)` on the first mismatch so callers can short-circuit before +/// handing off to any engine, keeping the diagnostic identical across +/// tree / VM / JIT dispatch. +fn check_cli_arg_types( + program: &ast::Program, + func_name: Option<&str>, + args: &[interpreter::Value], + source: &str, + mode: OutputMode, +) -> Result<(), i32> { + let target = match resolve_entry_func_name(program, func_name) { + Some(t) => t, + None => return Ok(()), + }; + let params = match lookup_param_types(program, Some(target)) { + Some(p) => p, + None => return Ok(()), + }; + for (i, param) in params.iter().enumerate() { + let Some(v) = args.get(i) else { break }; + if cli_value_matches_type(v, ¶m.ty) { + continue; + } + let err = interpreter::RuntimeError { + code: "ILO-R600", + message: format!( + "argument {} (`{}`) expects {}, got {}", + i + 1, + param.name, + ilo::codegen::fmt::type_str(¶m.ty), + describe_cli_value(v) + ), + span: None, + call_stack: Vec::new(), + propagate_value: None, + }; + report_diagnostic( + &Diagnostic::from(&err).with_source(source.to_string()), + mode, + ); + return Err(1); + } + Ok(()) +} + /// Parse and coerce CLI args against a function's declared parameter types. /// /// - For `Type::Text` params, the raw CLI string is preserved as `Text` diff --git a/tests/regression_bench_silent.rs b/tests/regression_bench_silent.rs index 5b91ea33..abf3cddb 100644 --- a/tests/regression_bench_silent.rs +++ b/tests/regression_bench_silent.rs @@ -55,7 +55,7 @@ fn bench_silent_suppresses_program_stdout_under_json() { non_json_lines.is_empty(), "expected only JSON envelopes on stdout under --silent; saw {} non-JSON lines (first few: {:?})", non_json_lines.len(), - &non_json_lines.iter().take(3).collect::>() + non_json_lines.iter().take(3).collect::>() ); // ...and we still got bench numbers — at least one envelope per diff --git a/tests/regression_cli_arg_type_mismatch.rs b/tests/regression_cli_arg_type_mismatch.rs new file mode 100644 index 00000000..ec6d66b0 --- /dev/null +++ b/tests/regression_cli_arg_type_mismatch.rs @@ -0,0 +1,159 @@ +// Regression: ILO-517 — a CLI argument that doesn't match its declared +// parameter type used to be bound as-is, so a `n:n` parameter could receive +// `Value::Text`. Arithmetic on it produced `NaN` (or, on the JIT, echoed the +// raw string) and the process still exited 0 — a silent wrong answer. +// +// Before: +// ilo prog.ilo main (single fn `tri n:n>n`) -> "NaN", rc=0 default/VM +// -> "main", rc=0 JIT +// After: +// -> ILO-R600 "argument 1 (`n`) expects n, got text `main`", rc=1 +// on every engine. +// +// The CLI was less safe than the language it fronts: in-band, `num "main"` +// returns `R n t`, so the type checker forces the failure to be handled. +// +// Cross-engine on purpose. The guard is wired into all four dispatch sites +// (VM, interpreter, JIT, default) so the error contract can't drift per +// engine the way ILO-177 did. + +use std::process::Command; + +fn ilo() -> Command { + Command::new(env!("CARGO_BIN_EXE_ilo")) +} + +fn run(args: &[&str]) -> (bool, String, String) { + let out = ilo() + .args(args) + .output() + .unwrap_or_else(|e| panic!("failed to spawn ilo: {e}")); + ( + out.status.success(), + String::from_utf8_lossy(&out.stdout).to_string(), + String::from_utf8_lossy(&out.stderr).to_string(), + ) +} + +fn write_temp(content: &str) -> (tempfile::TempDir, std::path::PathBuf) { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("prog.ilo"); + std::fs::write(&path, content).expect("write temp ilo"); + (dir, path) +} + +/// Engine dispatch variants. Empty slice = default engine. +fn engines() -> Vec<&'static [&'static str]> { + vec![&[], &["--vm"], &["--jit"]] +} + +const TRI: &str = "tri n:n>n;s=+n 1;p=*n s;/p 2\nmain>n;tri 3\n"; + +#[test] +fn text_bound_to_number_param_is_rejected_all_engines() { + let (_d, path) = write_temp(TRI); + let p = path.to_str().unwrap(); + for eng in engines() { + let mut args: Vec<&str> = eng.to_vec(); + args.extend_from_slice(&[p, "tri", "main"]); + let (ok, out, err) = run(&args); + assert!( + !ok, + "engine {eng:?}: expected failure, got success. out={out}" + ); + assert!( + err.contains("ILO-R600"), + "engine {eng:?}: expected ILO-R600, stderr={err}" + ); + // The silent-corruption symptoms must be gone. + assert!( + !out.contains("NaN"), + "engine {eng:?}: NaN leaked to stdout: {out}" + ); + assert!( + !out.trim().eq("main"), + "engine {eng:?}: raw arg echoed as result: {out}" + ); + } +} + +#[test] +fn valid_number_arg_still_runs_all_engines() { + let (_d, path) = write_temp(TRI); + let p = path.to_str().unwrap(); + for eng in engines() { + let mut args: Vec<&str> = eng.to_vec(); + args.extend_from_slice(&[p, "tri", "10"]); + let (ok, out, err) = run(&args); + assert!(ok, "engine {eng:?}: expected success, stderr={err}"); + assert_eq!(out.trim(), "55", "engine {eng:?}"); + } +} + +// The ILO-182 single-fn pass-through must survive: passing a bare ident to a +// sole `s:t` parameter is legitimate usage, not a typo'd function name. A +// dispatch-level fix would have broken this; the coercion-level fix must not. +#[test] +fn bare_ident_to_text_param_still_works() { + let (_d, path) = write_temp("greet s:t>t;+\"hello \" s\n"); + let (ok, out, err) = run(&[path.to_str().unwrap(), "world"]); + assert!(ok, "expected success, stderr={err}"); + assert_eq!(out.trim(), "hello world"); +} + +#[test] +fn non_bool_bound_to_bool_param_is_rejected() { + let (_d, path) = write_temp("f b:b>t;\"ok\"\n"); + let (ok, _out, err) = run(&[path.to_str().unwrap(), "notabool"]); + assert!(!ok, "expected failure"); + assert!(err.contains("ILO-R600"), "stderr={err}"); +} + +#[test] +fn bool_param_accepts_true_and_false() { + let (_d, path) = write_temp("f b:b>t;\"ok\"\n"); + for v in ["true", "false"] { + let (ok, out, err) = run(&[path.to_str().unwrap(), v]); + assert!(ok, "value {v}: expected success, stderr={err}"); + assert_eq!(out.trim(), "ok", "value {v}"); + } +} + +// `_` means "don't care" — the guard must wave anything through rather than +// re-implementing the type checker at the CLI boundary. +#[test] +fn any_param_accepts_anything() { + let (_d, path) = write_temp("f x:_>t;\"got\"\n"); + for v in ["main", "42", "true", "nil"] { + let (ok, out, err) = run(&[path.to_str().unwrap(), v]); + assert!(ok, "value {v}: expected success, stderr={err}"); + assert_eq!(out.trim(), "got", "value {v}"); + } +} + +// `O n` accepts nil (that's the point of an optional) but must still reject a +// non-nil value that can't be the inner type. +#[test] +fn optional_number_accepts_nil_rejects_text() { + let (_d, path) = write_temp("f x:O n>t;\"ok\"\n"); + let p = path.to_str().unwrap(); + + let (ok, out, err) = run(&[p, "nil"]); + assert!(ok, "nil should be accepted, stderr={err}"); + assert_eq!(out.trim(), "ok"); + + let (ok, _out, err) = run(&[p, "main"]); + assert!(!ok, "text should be rejected for O n"); + assert!(err.contains("ILO-R600"), "stderr={err}"); +} + +// The diagnostic has to name the parameter and show the offending value, +// otherwise a repair loop gets no more signal than the old bare "NaN" did. +#[test] +fn diagnostic_names_parameter_and_value() { + let (_d, path) = write_temp(TRI); + let (_ok, _out, err) = run(&[path.to_str().unwrap(), "tri", "main"]); + assert!(err.contains("argument 1"), "stderr={err}"); + assert!(err.contains('n'), "should name the param, stderr={err}"); + assert!(err.contains("main"), "should show the value, stderr={err}"); +}