diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 629e33ec..e000fc94 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -2,9 +2,9 @@ name: Rust on: push: - branches: [ "main", "next" ] + branches: [ "main", "next", "release/**" ] pull_request: - branches: [ "main", "next" ] + branches: [ "main", "next", "release/**" ] env: CARGO_TERM_COLOR: always diff --git a/src/constrain.rs b/src/constrain.rs index 8cb291b4..fd916afe 100644 --- a/src/constrain.rs +++ b/src/constrain.rs @@ -891,8 +891,7 @@ pub fn state_machine_json() -> Value { ) }) .collect(); - let states_map: serde_json::Map = - states.into_iter().map(|(k, v)| (k, v)).collect(); + let states_map: serde_json::Map = states.into_iter().collect(); json!({ "schemaVersion": 1, @@ -1118,7 +1117,6 @@ mod tests { #[test] fn logit_masks_toplevel_has_valid_tokens() { let lm = logit_masks_json(); - let vocab = lm["vocabulary"].as_array().unwrap(); let mask = lm["masks"]["TopLevel"].as_array().unwrap(); let count: usize = mask.iter().map(|v| v.as_u64().unwrap() as usize).sum(); assert!(count > 0, "TopLevel must have at least one valid token"); @@ -1244,7 +1242,6 @@ mod tests { #[test] fn completions_after_fn_name() { - let result = completions_at_cursor("fn add ", 1, 8); // `fn` is a reserved keyword that the parser rejects, but the state // machine should still track it — `fn` doesn't have a TokenCat, so it's // skipped and we stay at TopLevel. Use `add` (an ident) instead. diff --git a/src/main.rs b/src/main.rs index c50d87de..e7f4b4d0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5529,7 +5529,7 @@ fn check_cmd( } } else { let exp_val = literal_to_value(expected_lit); - if &result != &exp_val { + if result != exp_val { report_diagnostic( &enrich( Diagnostic::error(format!( diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 74b9b774..ad656c1c 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -5063,11 +5063,59 @@ or write `({fmt_name} \"...\" ...)` so its args are grouped." ident_span.end > 0 && bang_span.start == ident_span.end }; if !(is_record || is_field || is_zero_arg_call || is_unwrap) { + // Paren-form call as a call ARGUMENT: `prnt fmt2(3.14, 2)`, + // `prnt at([5 6 7], 1)`. The statement-head and operand + // positions already handle adjacent-paren calls (ILO-544); + // without the same branch here the arity-driven positional + // loop below parses `(3.14, 2)` as a grouped expression and + // dies on the comma with ILO-P003. + let inner_name = name.clone(); // break borrow before pos manipulation + if next == Some(&Token::LParen) { + let paren_name = inner_name.clone(); + let saved = self.pos; + self.advance(); // consume the inner function ident + if self.is_adjacent_lparen() && !self.looks_like_inline_lambda() { + let args = self.parse_paren_call_args_for(Some(&paren_name))?; + let call = Expr::Call { + function: paren_name, + args, + unwrap: UnwrapMode::None, + }; + // ILO-544: the paren group need not be the whole + // argument list — `at([5 6 7])1` glues the remaining + // operand on. Extend inline up to the known arity + // rather than via `paren_call_atom`, which is only + // consumed by `parse_call_or_atom` and would leak + // into an unrelated atom from here. + let Expr::Call { + function, + mut args, + unwrap, + } = call + else { + unreachable!("just constructed a Call") + }; + while args.len() < arity && self.can_start_operand() { + let arg_idx = args.len(); + let in_fn_pos = self.is_fn_ref_position(&function, arg_idx); + args.push(self.parse_call_arg( + in_fn_pos, + Some((&function.clone(), arity, arg_idx)), + )?); + } + let call = Expr::Call { + function, + args, + unwrap, + }; + return self.parse_field_chain(call, None); + } + self.pos = saved; + } // ILO-540b: if NO operands follow this known-arity name, // create a Ref (not a 0-arg Call). This lets local bindings // that shadow builtins (rev=5; prnt rev) resolve to the // local variable instead of a mis-dispatched builtin call. - let inner_name = name.clone(); // break borrow before pos manipulation let next_starts_operand = { let saved = self.pos; self.pos = saved + 1; diff --git a/src/verify.rs b/src/verify.rs index 1d5c5060..231b27ec 100644 --- a/src/verify.rs +++ b/src/verify.rs @@ -255,6 +255,16 @@ struct VerifyContext { /// binding is at the top (last element). Used by ILO-T048 to detect /// `x=+x 1`-style rebinds of the loop iterator variable inside `@x` loops. loop_bindings: Vec, + /// Names bound by top-level `Let` statements in `main`'s body. Used by + /// the ILO-T004 hint (ILO-546): when an undefined variable inside + /// another function is actually a main/script-level binding, the model + /// almost always glued a top-level statement onto that function's line + /// (script mode collects own-line statements into main; same-line + /// statements join the preceding body). Naming the real fix beats the + /// generic enclosing-fn lambda advisory, which sends the model the + /// wrong way for this shape (grade-calculator failed 3/3 on it in the + /// ILO-364 N=5 benchmark). + main_bindings: std::collections::HashSet, /// Function names whose declaration failed to parse. Populated from /// `Program.parse_failed_fns` at the start of `verify`. Two effects: /// 1. We skip type-checking the body of any function in this set (its @@ -1496,8 +1506,10 @@ fn builtin_check_args( } (Ty::Result(Box::new(Ty::Number), Box::new(Ty::Text)), errors) } - "abs" | "flr" | "cel" | "rou" | "sqrt" | "log" | "exp" | "sin" | "cos" | "tan" - | "log10" | "log2" | "asin" | "acos" | "atan" => { + "abs" | "flr" | "cel" | "sqrt" | "log" | "exp" | "sin" | "cos" | "tan" | "log10" + | "log2" | "asin" | "acos" | "atan" | "rou" + if !(name == "rou" && arg_types.len() == 2) => + { if let Some(arg) = arg_types.first() && !compatible(arg, &Ty::Number) { @@ -4800,6 +4812,7 @@ impl VerifyContext { errors: Vec::new(), in_loop: false, loop_bindings: Vec::new(), + main_bindings: std::collections::HashSet::new(), parse_failed_fns: HashMap::new(), glued_eq_binding_sites: std::collections::HashSet::new(), suppressed_undef_reported: std::collections::HashSet::new(), @@ -4844,6 +4857,19 @@ impl VerifyContext { /// Phase 1: collect all declarations, check for duplicates and undefined Named types. fn collect_declarations(&mut self, program: &Program) { + // ILO-546: record main's top-level Let names for the T004 glue hint. + for decl in &program.declarations { + if let Decl::Function { name, body, .. } = decl + && name == "main" + { + for s in body { + if let crate::ast::Stmt::Let { name: n, .. } = &s.node { + self.main_bindings.insert(n.clone()); + } + } + } + } + // Pass 0: collect type aliases (before types so aliases can be used in type fields) let builtin_type_names = ["n", "t", "b", "L", "R"]; let mut raw_aliases: HashMap = HashMap::new(); @@ -5616,9 +5642,10 @@ impl VerifyContext { format!( "call to '{function}' may violate precondition {precond_str} — add a guard" ), - Some(format!( + Some( "guard before the call: negate the condition (e.g. for req b!=0, add `=b 0 ^\"...\"` before the call) or wrap in a match on R" - )), + .to_string(), + ), Some(span), ); } @@ -6451,14 +6478,27 @@ impl VerifyContext { closest_match(name, candidates.iter()) .map(|s| format!("did you mean '{s}'?")) }); - // ILO-504: append hoisting advisory for all undefined - // variables. Nested fn declarations are silently - // hoisted to siblings in single-line form; if the - // model intended a capture, the undefined-variable - // error is the only signal. - base_hint.map(|h| format!( - "{h} (or if '{name}' is from an enclosing fn, use an inline lambda: `(x:n>n;...{name}...)`)" - )) + // ILO-546: if the name IS a main/script-level binding, + // the statement referencing it was almost certainly + // glued onto this function's line — script mode only + // collects statements that start their own top-level + // line. Name the one-edit fix instead of the generic + // lambda advisory, which sends the model the wrong way + // for this shape. + if func != "main" && self.main_bindings.contains(name) { + Some(format!( + "'{name}' is bound at top level, but this statement is glued into '{func}''s body — put it on its own line (unindented) so script mode runs it in main" + )) + } else { + // ILO-504: append hoisting advisory for all undefined + // variables. Nested fn declarations are silently + // hoisted to siblings in single-line form; if the + // model intended a capture, the undefined-variable + // error is the only signal. + base_hint.map(|h| format!( + "{h} (or if '{name}' is from an enclosing fn, use an inline lambda: `(x:n>n;...{name}...)`)" + )) + } }; self.err( "ILO-T004", @@ -6554,9 +6594,7 @@ impl VerifyContext { "pst" | "put" | "pat" | "pstx" | "wr" | "padl" | "padr" ) { "2 or 3".to_string() - } else if callee == "min" || callee == "max" { - "1 or 2".to_string() - } else if callee == "rou" { + } else if matches!(callee.as_str(), "min" | "max" | "rou") { "1 or 2".to_string() } else if callee == "run" || callee == "run2" { "2 or 3".to_string() diff --git a/tests/coverage_parser.rs b/tests/coverage_parser.rs index 0f31fdc5..aaeaf489 100644 --- a/tests/coverage_parser.rs +++ b/tests/coverage_parser.rs @@ -169,7 +169,10 @@ fn fld_as_decl_name() { #[test] fn builtin_as_decl_name() { - fail_code("map=5", "ILO-P011"); + // 69565d44 made builtin names legal as BINDINGS: the local shadows the + // builtin in value position, matching Python. Only builtin FN names + // (`builtin_as_fn_name` below) and reserved words still fire ILO-P011. + ok("map=5"); } #[test] @@ -457,8 +460,10 @@ fn stmt_cnt_in_loop() { } #[test] -fn stmt_builtin_let_rejected() { - fail_code("f>n;flat=5;flat", "ILO-P011"); +fn stmt_builtin_let_accepted() { + // Same shadowing rule inside a fn body (69565d44). Reserved words are + // still rejected there — see `stmt_var_in_body_rejected` below. + ok("f>n;flat=5;flat"); } #[test] diff --git a/tests/regression_builtin_binding_name.rs b/tests/regression_builtin_binding_name.rs index a6496860..ca1d0981 100644 --- a/tests/regression_builtin_binding_name.rs +++ b/tests/regression_builtin_binding_name.rs @@ -1,20 +1,23 @@ -// Regression: binding a local variable whose name collides with a builtin -// must surface the friendly ILO-P011 reserved-name error at parse time, -// not silently accept the binding and later mis-dispatch the use site to -// the builtin (surfacing a misleading ILO-T006 arity error). +// Regression: a local binding whose name collides with a builtin shadows +// that builtin in VALUE position, while CALL position still dispatches the +// builtin. Python's rule, and the one 69565d44 introduced. // -// Repro before the fix: `flat=cat ls " "` followed by `spl flat ". "` -// silently bound `flat` locally, but the call `spl flat ". "` parsed `flat` -// as a 0-arg call to the `flat` builtin (the verifier checks `is_builtin` -// before locals in operand position), producing -// `ILO-T006 arity mismatch: 'flat' expects 1 args, got 0`. The agent has no -// signal that the local binding is being shadowed. +// History. The original persona report (2026-05-16 pdf-analyst, friction #6) +// was that `flat=cat ls " "` then `spl flat ". "` silently bound `flat` and +// then parsed the use site as a 0-arg call to the `flat` BUILTIN, surfacing a +// misleading `ILO-T006 arity mismatch: 'flat' expects 1 args, got 0`. The +// first fix rejected such bindings outright at parse time (ILO-P011), and +// this suite pinned that rejection. // -// Mirrors the `parse_fn_decl` precedent from PR #245 (regression_builtin_fn_name.rs): -// reject builtin-named binding LHS at parse time on every engine. +// 69565d44 replaced rejection with shadowing, because reserved-name collisions +// were the #1 benchmark failure category and the repair loop could not fix +// them: renaming is not something the model reliably infers. The bug the +// original report describes is still fixed, just differently - the use site +// now resolves to the local instead of mis-dispatching. These tests assert +// that resolution directly, so the T006 mis-dispatch cannot come back. // -// Originating persona report: 2026-05-16 pdf-analyst re-run against v0.11.2, -// friction #6 "`flat` is a builtin name and shadows your local". +// `fld` keeps its hard rejection (see the tail of this file): it is reserved +// for the fold builtin with a dedicated message, not a general shadowable name. use std::process::Command; @@ -22,89 +25,66 @@ fn ilo() -> Command { Command::new(env!("CARGO_BIN_EXE_ilo")) } -fn run(engine: &str, src: &str, entry: &str) -> (bool, String) { +fn run(engine: &str, src: &str, entry: &str) -> (bool, String, String) { let out = ilo() .args([src, engine, entry]) .output() .expect("failed to run ilo"); ( out.status.success(), + String::from_utf8_lossy(&out.stdout).into_owned(), String::from_utf8_lossy(&out.stderr).into_owned(), ) } -// Top-level (`parse_decl`) binding form: `name=expr` outside any function. -fn check_top_level_binding(engine: &str, name: &str) { - let src = format!("{name}=5\nmain>n;42"); - let (ok, stderr) = run(engine, &src, "main"); - assert!( - !ok, - "engine={engine} name={name}: expected parse failure for top-level `{name}=...`" - ); +// In-function (`parse_stmt`) binding form: `name=expr` inside a function body. +fn check_in_fn_binding(engine: &str, name: &str) { + let src = format!("main>n;{name}=5;{name}"); + let (ok, stdout, stderr) = run(engine, &src, "main"); assert!( - stderr.contains("ILO-P011"), - "engine={engine} name={name}: missing ILO-P011, stderr={stderr}" + ok, + "engine={engine} name={name}: builtin-named binding should be accepted, stderr={stderr}" ); - assert!( - stderr.contains(&format!( - "`{name}` is a builtin and cannot be used as a binding name" - )), - "engine={engine} name={name}: missing friendly message, stderr={stderr}" + assert_eq!( + stdout.trim(), + "5", + "engine={engine} name={name}: use site should resolve to the local, stderr={stderr}" ); + // The bug from the original report: the use site must not dispatch to the + // shadowed builtin and report an arity mismatch against it. assert!( - stderr.contains("rename to something like"), - "engine={engine} name={name}: missing rename hint, stderr={stderr}" + !stderr.contains("ILO-T006"), + "engine={engine} name={name}: builtin mis-dispatch returned, stderr={stderr}" ); } -// In-function (`parse_stmt`) binding form: `name=expr` inside a function body. -fn check_in_fn_binding(engine: &str, name: &str) { - let src = format!("main>n;{name}=5;42"); - let (ok, stderr) = run(engine, &src, "main"); +// Top-level (`parse_decl`) binding form: `name=expr` outside any function. +// Script mode wraps the bare statements into main, so no explicit `main` here +// (that shape is ILO-P104 and is covered by the script-mode suite). +fn check_top_level_binding(engine: &str, name: &str) { + let src = format!("{name}=5\nprnt {name}"); + let (ok, stdout, stderr) = run(engine, &src, "main"); assert!( - !ok, - "engine={engine} name={name}: expected parse failure for in-fn `{name}=...`" + ok, + "engine={engine} name={name}: top-level builtin-named binding should be accepted, stderr={stderr}" ); - assert!( - stderr.contains("ILO-P011"), - "engine={engine} name={name}: missing ILO-P011, stderr={stderr}" + assert_eq!( + stdout.trim(), + "5", + "engine={engine} name={name}: top-level use site should resolve to the local, stderr={stderr}" ); - assert!( - stderr.contains(&format!( - "`{name}` is a builtin and cannot be used as a binding name" - )), - "engine={engine} name={name}: missing friendly message, stderr={stderr}" - ); - // The bug we're fixing: the misleading arity error from the shadowed - // builtin must not be the first thing the agent sees. - let p011_pos = stderr.find("ILO-P011").unwrap(); - if let Some(t006_pos) = stderr.find("ILO-T006") { - assert!( - p011_pos < t006_pos, - "engine={engine} name={name}: ILO-P011 must come before any ILO-T006 cascade, stderr={stderr}" - ); - } } // Builtin names a persona is likely to reach for as a local-binding name. -// Mix of the rerun3-cited `flat`, the historical `fld` (covered by an -// earlier specific message but should still surface ILO-P011), list/map -// builtins (`map`, `flt`, `frq`, `cat`, `len`), and short-name builtins -// (`hd`, `tl`, `at`, `ord`) that collide with natural single-letter -// abbreviations. +// Mix of the rerun3-cited `flat`, list/map builtins (`map`, `flt`, `frq`, +// `cat`, `len`), and short-name builtins (`hd`, `tl`, `at`, `ord`) that +// collide with natural single-letter abbreviations. const BINDING_NAMES: &[&str] = &[ "flat", "frq", "map", "flt", "cat", "len", "hd", "tl", "at", "ord", "srt", "sum", ]; #[test] -fn builtin_binding_rejected_in_fn_tree() { - for name in BINDING_NAMES { - check_in_fn_binding("--vm", name); - } -} - -#[test] -fn builtin_binding_rejected_in_fn_vm() { +fn builtin_binding_accepted_in_fn_vm() { for name in BINDING_NAMES { check_in_fn_binding("--vm", name); } @@ -112,21 +92,14 @@ fn builtin_binding_rejected_in_fn_vm() { #[test] #[cfg(feature = "cranelift")] -fn builtin_binding_rejected_in_fn_cranelift() { +fn builtin_binding_accepted_in_fn_cranelift() { for name in BINDING_NAMES { check_in_fn_binding("--jit", name); } } #[test] -fn builtin_binding_rejected_top_level_tree() { - for name in BINDING_NAMES { - check_top_level_binding("--vm", name); - } -} - -#[test] -fn builtin_binding_rejected_top_level_vm() { +fn builtin_binding_accepted_top_level_vm() { for name in BINDING_NAMES { check_top_level_binding("--vm", name); } @@ -134,42 +107,24 @@ fn builtin_binding_rejected_top_level_vm() { #[test] #[cfg(feature = "cranelift")] -fn builtin_binding_rejected_top_level_cranelift() { +fn builtin_binding_accepted_top_level_cranelift() { for name in BINDING_NAMES { check_top_level_binding("--jit", name); } } -// The exact pdf-analyst rerun3 repro: `flat=cat ls " "` then a use site. -// Before the fix this surfaced `ILO-T006 arity mismatch: 'flat' expects 1 -// args, got 0` from the shadowed builtin, with no signal that the local -// `flat` binding was being silently ignored at the call site. After the fix -// the agent sees ILO-P011 immediately and renames. +// The exact pdf-analyst rerun3 repro, now asserting the shadow resolves +// rather than that the binding is rejected. const FLAT_REPRO: &str = "main>n;flat=5;flat"; fn check_flat_repro(engine: &str) { - let (ok, stderr) = run(engine, FLAT_REPRO, "main"); - assert!(!ok, "engine={engine}: expected parse failure"); - assert!( - stderr.contains("ILO-P011"), - "engine={engine}: missing ILO-P011, stderr={stderr}" - ); + let (ok, stdout, stderr) = run(engine, FLAT_REPRO, "main"); + assert!(ok, "engine={engine}: expected success, stderr={stderr}"); + assert_eq!(stdout.trim(), "5", "engine={engine}: stderr={stderr}"); assert!( - stderr.contains("`flat` is a builtin"), - "engine={engine}: missing friendly message, stderr={stderr}" + !stderr.contains("ILO-T006"), + "engine={engine}: builtin mis-dispatch returned, stderr={stderr}" ); - let p011_pos = stderr.find("ILO-P011").unwrap(); - if let Some(t006_pos) = stderr.find("ILO-T006") { - assert!( - p011_pos < t006_pos, - "engine={engine}: ILO-P011 must come before ILO-T006, stderr={stderr}" - ); - } -} - -#[test] -fn flat_repro_tree() { - check_flat_repro("--vm"); } #[test] @@ -183,70 +138,49 @@ fn flat_repro_cranelift() { check_flat_repro("--jit"); } -// Sanity: the more-specific `fld` message from the earlier fix still fires -// (and still mentions the fold builtin specifically). The generic builtin -// check runs after the per-name checks, so the friendlier message wins. -fn check_fld_keeps_specific_message(engine: &str) { - let (ok, stderr) = run(engine, "main>n;fld=5;fld", "main"); - assert!(!ok, "engine={engine}: expected parse failure"); - assert!( - stderr.contains("ILO-P011"), - "engine={engine}: missing ILO-P011, stderr={stderr}" - ); - assert!( - stderr.contains("`fld` is reserved for the fold builtin"), - "engine={engine}: expected fld-specific message, stderr={stderr}" - ); +// Shadowing is value-position only: with a local `len` in scope, `len xs` in +// call position still dispatches the builtin. This is the half of the rule an +// over-eager future change is most likely to break. +fn check_call_position_still_dispatches_builtin(engine: &str) { + let src = "main>n;len=5;xs=[1,2,3];r=len xs;+r len"; + let (ok, stdout, stderr) = run(engine, src, "main"); + assert!(ok, "engine={engine}: expected success, stderr={stderr}"); + // len xs = 3 (builtin), + local len (5) = 8 + assert_eq!(stdout.trim(), "8", "engine={engine}: stderr={stderr}"); } #[test] -fn fld_specific_message_preserved_tree() { - check_fld_keeps_specific_message("--vm"); -} - -#[test] -fn fld_specific_message_preserved_vm() { - check_fld_keeps_specific_message("--vm"); +fn call_position_dispatches_builtin_vm() { + check_call_position_still_dispatches_builtin("--vm"); } #[test] #[cfg(feature = "cranelift")] -fn fld_specific_message_preserved_cranelift() { - check_fld_keeps_specific_message("--jit"); +fn call_position_dispatches_builtin_cranelift() { + check_call_position_still_dispatches_builtin("--jit"); } -// Sanity: renaming to a non-builtin name works on every engine. The hint -// the new ILO-P011 produces points to `myflat` / `flatv` style names, so -// that path must actually compile and run. -fn check_renamed_binding_works(engine: &str) { - let out = ilo() - .args(["main>n;myflat=5;myflat", engine, "main"]) - .output() - .expect("failed to run ilo"); +// `fld` is exempt from shadowing and keeps its dedicated ILO-P011 message. +fn check_fld_keeps_specific_message(engine: &str) { + let (ok, _stdout, stderr) = run(engine, "main>n;fld=5;fld", "main"); + assert!(!ok, "engine={engine}: expected parse failure"); assert!( - out.status.success(), - "engine={engine}: rename should compile, stderr={}", - String::from_utf8_lossy(&out.stderr) + stderr.contains("ILO-P011"), + "engine={engine}: missing ILO-P011, stderr={stderr}" ); - let stdout = String::from_utf8_lossy(&out.stdout); assert!( - stdout.contains("5"), - "engine={engine}: expected 5, got: {stdout}" + stderr.contains("`fld` is reserved for the fold builtin"), + "engine={engine}: expected fld-specific message, stderr={stderr}" ); } #[test] -fn rename_workaround_binding_tree() { - check_renamed_binding_works("--vm"); -} - -#[test] -fn rename_workaround_binding_vm() { - check_renamed_binding_works("--vm"); +fn fld_keeps_specific_message_vm() { + check_fld_keeps_specific_message("--vm"); } #[test] #[cfg(feature = "cranelift")] -fn rename_workaround_binding_cranelift() { - check_renamed_binding_works("--jit"); +fn fld_keeps_specific_message_cranelift() { + check_fld_keeps_specific_message("--jit"); } diff --git a/tests/regression_listlit_builtin_call_hint.rs b/tests/regression_listlit_builtin_call_hint.rs index 3a30d5b9..3f92fe38 100644 --- a/tests/regression_listlit_builtin_call_hint.rs +++ b/tests/regression_listlit_builtin_call_hint.rs @@ -57,30 +57,22 @@ fn run_ok(engine: &str, src: &str, entry: &str, args: &[&str]) -> String { stdout.trim().to_string() } -// --- Repro: variadic builtin (fmt2) inside list literal ----------------- +// --- fmt2 now parses as a call: known arity, capped --------------------- // -// The exact shape from data-wrangler rerun10: a CSV row built inline, -// mixing locals (`k`, `c`) with a formatted-number column. +// The original data-wrangler rerun10 shape. `fmt2` has since gained a known +// arity (2), so the parser caps the element at `fmt2 rv 2` and the list gets +// three elements - the outcome the agent meant. ILO-P101 no longer fires +// here, and should not: the diagnostic exists for shapes that cannot be +// parsed correctly, and this one now can. `fmt` (genuinely variadic) still +// triggers it - see below. const FMT2_IN_LIST: &str = "f rv:n>L t;k=\"foo\";c=\"bar\";[k c fmt2 rv 2]"; -fn check_fmt2_hint(engine: &str) { - let (ok, stdout, stderr) = run_capture(engine, FMT2_IN_LIST, "f", &["3.14"]); - assert!(!ok, "fmt2 inside list literal must reject at parse time"); - let combined = format!("{stdout}{stderr}"); - assert!( - combined.contains("ILO-P101"), - "expected ILO-P101 in output, got: {combined}" - ); - assert!( - combined.contains("fmt2"), - "diagnostic should name the offending builtin, got: {combined}" - ); - // The hint should mention parens or bind-first - both shapes are - // documented in the registry entry. - assert!( - combined.contains("paren") || combined.contains("(") || combined.contains("bind"), - "diagnostic should suggest parens or bind-first, got: {combined}" +fn check_fmt2_parses_as_call(engine: &str) { + let out = run_ok(engine, FMT2_IN_LIST, "f", &["3.14"]); + assert_eq!( + out, "[foo, bar, 3.14]", + "fmt2 with known arity should parse as one capped call element {engine}" ); } @@ -153,7 +145,7 @@ fn check_bare_locals_unchanged(engine: &str) { } fn check_all(engine: &str) { - check_fmt2_hint(engine); + check_fmt2_parses_as_call(engine); check_fmt_hint(engine); check_parens_workaround(engine); check_bind_workaround(engine); diff --git a/tests/regression_multi_line_body_span_drift.rs b/tests/regression_multi_line_body_span_drift.rs index d1376f05..5789e68a 100644 --- a/tests/regression_multi_line_body_span_drift.rs +++ b/tests/regression_multi_line_body_span_drift.rs @@ -14,6 +14,11 @@ //! every token span (and lex-error position) back to original-source //! coordinates before returning, so `SourceMap::lookup` and downstream //! span-consumers see offsets that match what the user typed. +//! +//! The fixtures use `fld` as the faulting binding because it is still a +//! hard-reserved name (ILO-P011 with the fold-specific message). They used +//! `rev` until 69565d44 made builtin names legal as bindings, which removed +//! the error these span assertions ride on; `fld` keeps the same shape. use std::process::Command; @@ -81,24 +86,24 @@ fn first_error_line_and_start(stderr: &str) -> (usize, usize) { } #[test] -fn rev_binding_in_indented_main_body_lands_on_actual_line() { - // Canonical persona repro: `rev = ...` inside a multi-line main body. +fn fld_binding_in_indented_main_body_lands_on_actual_line() { + // Canonical persona repro: `fld = ...` inside a multi-line main body. // Before the fix the ILO-P011 span anchored to line 5 (the first body - // statement after the header) regardless of how far down `rev =` lives. - let src = "helper p:_>R n t;\n ~mget!! m p\n\nmain>R t t\n s = \"x\"\n a = 1\n b = 2\n c = 3\n rev = mget!! m p\n ~s\n"; - let path = write_tmp("rev-in-body", src); + // statement after the header) regardless of how far down `fld =` lives. + let src = "helper p:_>R n t;\n ~mget!! m p\n\nmain>R t t\n s = \"x\"\n a = 1\n b = 2\n c = 3\n fld = mget!! m p\n ~s\n"; + let path = write_tmp("fld-in-body", src); let err = run_err_json_file(&path); let (line, start) = first_error_line_and_start(&err); assert_eq!( line, 9, - "ILO-P011 must point at the `rev =` line (9), got stderr:\n{err}" + "ILO-P011 must point at the `fld =` line (9), got stderr:\n{err}" ); - // The span's start byte must sit inside the `rev` token in the + // The span's start byte must sit inside the `fld` token in the // original source — not on a `;` upstream. - let rev_off = src.find("rev =").expect("repro contains `rev =`"); + let fld_off = src.find("fld =").expect("repro contains `fld =`"); assert_eq!( - start, rev_off, - "ILO-P011 start byte must be the `r` of `rev` ({rev_off}), got {start}, stderr:\n{err}" + start, fld_off, + "ILO-P011 start byte must be the `r` of `fld` ({fld_off}), got {start}, stderr:\n{err}" ); } @@ -139,13 +144,13 @@ fn match_arm_body_parse_error_lands_on_offending_token() { // normalization as the rest of multi-line syntax, so the offset map // has to thread through here too. Without it, the ILO-P011 span // drifted forward to a downstream arm separator. - let src = "main>n\n r = num \"1\"\n y = ?r{\n ~v:{\n a = 2\n rev = +a v\n *a 3\n }\n ^er:0\n }\n y\n"; + let src = "main>n\n r = num \"1\"\n y = ?r{\n ~v:{\n a = 2\n fld = +a v\n *a 3\n }\n ^er:0\n }\n y\n"; let path = write_tmp("match-arm", src); let err = run_err_json_file(&path); let line = first_error_line(&err); assert_eq!( line, 6, - "ILO-P011 must land on `rev = +a v` (6), got stderr:\n{err}" + "ILO-P011 must land on `fld = +a v` (6), got stderr:\n{err}" ); } @@ -154,13 +159,13 @@ fn deeply_nested_body_span_does_not_drift() { // Two levels of nesting (foreach inside guard inside main) puts many // `;` rewrites between the start of main and the faulting binding. // Before the fix the span drifted by 4+ lines. - let src = "main>n\n n = 3\n acc = 0\n >n 0{\n @i 0..n{\n t = +acc i\n rev = t\n acc = +acc 1\n }\n }\n ~acc\n"; + let src = "main>n\n n = 3\n acc = 0\n >n 0{\n @i 0..n{\n t = +acc i\n fld = t\n acc = +acc 1\n }\n }\n ~acc\n"; let path = write_tmp("nested", src); let err = run_err_json_file(&path); let line = first_error_line(&err); assert_eq!( line, 7, - "ILO-P011 must land on `rev = t` (7), got stderr:\n{err}" + "ILO-P011 must land on `fld = t` (7), got stderr:\n{err}" ); } @@ -169,18 +174,18 @@ fn function_last_statement_parse_error_lands_on_last_line() { // Fault on the final statement of a long multi-line body. Drift // historically pushed the span back to an earlier statement because // each preceding line shed indent and gained a `;`. - let src = "main>n\n a = 1\n b = 2\n c = 3\n d = 4\n f = 5\n rev = 6\n"; + let src = "main>n\n a = 1\n b = 2\n c = 3\n d = 4\n f = 5\n fld = 6\n"; let path = write_tmp("last-stmt", src); let err = run_err_json_file(&path); let (line, start) = first_error_line_and_start(&err); assert_eq!( line, 7, - "ILO-P011 must land on the last `rev = 6` line (7), got stderr:\n{err}" + "ILO-P011 must land on the last `fld = 6` line (7), got stderr:\n{err}" ); - let rev_off = src.find("rev = 6").expect("repro contains `rev = 6`"); + let fld_off = src.find("fld = 6").expect("repro contains `fld = 6`"); assert_eq!( - start, rev_off, - "span start ({start}) must equal byte offset of `rev` ({rev_off}), stderr:\n{err}" + start, fld_off, + "span start ({start}) must equal byte offset of `fld` ({fld_off}), stderr:\n{err}" ); } @@ -190,17 +195,17 @@ fn comment_stripping_does_not_shift_following_line_span() { // emitting `;`/`\n`. Without the offset map the bytes after the // comment line shift backward by `comment.len()`, so a fault on the // very next line landed at column 0 of a phantom earlier offset. - let src = "main>n\n a = 1\n -- explanatory comment text that is long\n rev = 2\n ~a\n"; + let src = "main>n\n a = 1\n -- explanatory comment text that is long\n fld = 2\n ~a\n"; let path = write_tmp("comment", src); let err = run_err_json_file(&path); let (line, start) = first_error_line_and_start(&err); assert_eq!( line, 4, - "ILO-P011 must land on `rev = 2` (4), got stderr:\n{err}" + "ILO-P011 must land on `fld = 2` (4), got stderr:\n{err}" ); - let rev_off = src.find("rev = 2").expect("repro contains `rev = 2`"); + let fld_off = src.find("fld = 2").expect("repro contains `fld = 2`"); assert_eq!( - start, rev_off, + start, fld_off, "span start must equal `r` byte, stderr:\n{err}" ); } @@ -210,14 +215,14 @@ fn single_line_body_span_unchanged() { // Sanity: when no newline rewriting happens, spans must stay // identical to pre-fix behaviour. This pins the no-drift case so a // future refactor that breaks the identity branch surfaces here. - let src = "main>n;rev = 1\n"; + let src = "main>n;fld = 1\n"; let path = write_tmp("single-line", src); let err = run_err_json_file(&path); let (line, start) = first_error_line_and_start(&err); assert_eq!(line, 1, "single-line fault on line 1, got stderr:\n{err}"); - let rev_off = src.find("rev = 1").expect("repro contains `rev = 1`"); + let fld_off = src.find("fld = 1").expect("repro contains `fld = 1`"); assert_eq!( - start, rev_off, + start, fld_off, "span start must equal `r` byte, stderr:\n{err}" ); } diff --git a/tests/regression_script_scope_hint.rs b/tests/regression_script_scope_hint.rs new file mode 100644 index 00000000..05daad33 --- /dev/null +++ b/tests/regression_script_scope_hint.rs @@ -0,0 +1,93 @@ +// ILO-546: grade-calculator failed 3/3 in the N=5 benchmark on the +// one-line glued script shape: `sts=[..];ws=[..];grd a:n>t;..;@s sts{..}` +// makes everything after `grd a:n>t;` part of grd's body (same-line = body, +// the documented script-mode rule), so `sts` is genuinely out of scope and +// ILO-T004 fires — but the old hint suggested an enclosing-fn lambda, which +// sends the model the wrong way. The hint now names the one-edit fix. +// +// The multi-line interleaved form was never broken (the ticket's original +// diagnosis) — pinned here so it stays that way. + +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(), + ) +} + +/// Multi-line: bindings, decl mid-stream, loop over earlier bindings — +/// one shared implicit main scope, decl registers as a sibling. +#[test] +fn multiline_interleaved_decl_shares_scope() { + let src = + "sts=[85, 92, 78]\nws=[0.5, 0.3, 0.2]\ngrd a:n>t;>=a 90 \"A\";\"B\"\n@s sts{prnt grd s}"; + let (ok, out, err) = run(&[src]); + assert!(ok, "stderr={err}"); + assert_eq!(out.trim(), "B\nA\nB"); +} + +/// Fn declared later in the file is callable from earlier script statements +/// (decls are file-scope siblings, order-independent). +#[test] +fn decl_after_statements_is_callable() { + let src = "xs=[1, 2, 3]\n@x xs{r=dbl x;prnt r}\ndbl n:n>n;*n 2"; + let (ok, out, err) = run(&[src]); + assert!(ok, "stderr={err}"); + assert_eq!(out.trim(), "2\n4\n6"); +} + +/// Multiple decls interleaved between statement runs — still one main scope. +#[test] +fn multiple_interleaved_decls() { + let src = "a=10\ninc n:n>n;+n 1\nb=inc a\ndbl n:n>n;*n 2\nprnt dbl b"; + let (ok, out, err) = run(&[src]); + assert!(ok, "stderr={err}"); + assert_eq!(out.trim(), "22"); +} + +/// One-line glued form: T004 fires (correct — same-line joins the body) and +/// the hint names the top-level binding + the own-line fix, NOT the +/// enclosing-fn lambda advisory. +#[test] +fn glued_form_hint_names_the_unglue_fix() { + let src = "sts=[85, 92, 78];ws=[0.5, 0.3, 0.2];grd a:n>t;>=a 90 \"A\";\"B\";@s sts{prnt grd s}"; + let (ok, _out, err) = run(&[src]); + assert!(!ok, "glued form must still error"); + assert!(err.contains("ILO-T004"), "stderr={err}"); + assert!( + err.contains("bound at top level"), + "hint should name the top-level binding: {err}" + ); + assert!( + err.contains("its own line"), + "hint should name the own-line fix: {err}" + ); + assert!( + !err.contains("enclosing fn"), + "lambda advisory is the wrong steer for this shape: {err}" + ); +} + +/// The ILO-504 lambda advisory still fires when the undefined name is NOT a +/// top-level binding (genuine capture attempt). +#[test] +fn lambda_advisory_survives_for_non_main_names() { + let (ok, _out, err) = run(&["f val:n>n;+val vall"]); + assert!(!ok); + assert!(err.contains("ILO-T004"), "stderr={err}"); + assert!( + err.contains("enclosing fn"), + "ILO-504 advisory should remain for non-main names: {err}" + ); +} diff --git a/tests/regression_top_level_chain_hint.rs b/tests/regression_top_level_chain_hint.rs index 383c8bad..f2c3bf3f 100644 --- a/tests/regression_top_level_chain_hint.rs +++ b/tests/regression_top_level_chain_hint.rs @@ -180,19 +180,30 @@ fn check_normal_fn_decl_unaffected(engine: &str) { ); } -// --- Negative: a builtin-shadowing name keeps its precise ILO-P011 hint ---- +// --- Negative: a builtin-shadowing name is a normal binding, not P102 ----- // -// `map=...` at the top level still hits the existing ILO-P011 guard for -// builtin shadowing — the P102 generic guard must not eclipse it. +// `map=...` at the top level used to hit an ILO-P011 builtin-shadow guard; +// 69565d44 made builtin names legal as bindings (they shadow in value +// position). The point this test still carries is that the generic P102 +// top-level-chain guard must not swallow the shape: it is a plain script +// binding and must simply run. -const MAP_SHADOW: &str = "map=[1 2 3];prnt map"; +const MAP_SHADOW: &str = "map=[1 2 3]\nprnt map"; -fn check_builtin_shadow_keeps_p011(engine: &str) { - let (_ok, stdout, stderr) = run_capture(engine, MAP_SHADOW, "main"); - let combined = format!("{stdout}{stderr}"); +fn check_builtin_shadow_runs(engine: &str) { + let (ok, stdout, stderr) = run_capture(engine, MAP_SHADOW, "main"); assert!( - combined.contains("ILO-P011"), - "{engine}: `map=` should keep its builtin-shadow ILO-P011 hint, got: {combined}" + ok, + "{engine}: builtin-named binding should run. stderr={stderr}" + ); + assert_eq!( + stdout.trim(), + "[1, 2, 3]", + "{engine}: use site should resolve to the local. stderr={stderr}" + ); + assert!( + !stderr.contains("ILO-P102"), + "{engine}: P102 must not fire on a plain script binding: {stderr}" ); } @@ -220,7 +231,7 @@ fn check_all(engine: &str) { check_slurp_into_prior_fn(engine); check_main_wrapper_runs(engine); check_normal_fn_decl_unaffected(engine); - check_builtin_shadow_keeps_p011(engine); + check_builtin_shadow_runs(engine); } #[test]