From 71e04a7dc66237efbf250ba7a228ce6074f41bb6 Mon Sep 17 00:00:00 2001 From: Daniel Morris Date: Thu, 13 Aug 2026 09:18:10 +0100 Subject: [PATCH 1/5] fix(verify): steer T004 to the unglue fix for main-level bindings (ILO-546) When a one-line script glues statements after a fn header with ';', those statements join the fn body (documented script-mode rule) and lose access to top-level bindings. T004 then fired with the ILO-504 lambda advisory, which steers a repairing model toward captures instead of the one-edit fix. Track main's top-level Let names in VerifyContext and, when the undefined name is one of them, name the real fix: put the statement on its own unindented line. Multi-line interleaved decls were never broken (ticket's original diagnosis was wrong); regression tests pin both shapes plus the surviving lambda advisory for genuine capture attempts. --- src/verify.rs | 53 ++++++++++++--- tests/regression_script_scope_hint.rs | 93 +++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 tests/regression_script_scope_hint.rs diff --git a/src/verify.rs b/src/verify.rs index 1d5c5060..10d918a4 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 @@ -4800,6 +4810,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 +4855,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(); @@ -6451,14 +6475,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", 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}" + ); +} From f81611020c7f5fa56545d4b0a448d38e80d6cdcc Mon Sep 17 00:00:00 2001 From: Daniel Morris Date: Thu, 13 Aug 2026 09:45:34 +0100 Subject: [PATCH 2/5] ci: run Rust workflow on release/** branches rust.yml fired only on main/next, so every PR into release/26.8 merged with no build, test, fmt or clippy gate - the empty check list on #789 and #790. Release branches are the live integration line for a version; they need the same gate as main. --- .github/workflows/rust.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From 3a3c1502c61b6e70da7bd610cb391da7e58df233 Mon Sep 17 00:00:00 2001 From: Daniel Morris Date: Thu, 13 Aug 2026 10:39:35 +0100 Subject: [PATCH 3/5] fix(parser): accept paren-form calls in call-argument position `prnt fmt2(3.14, 2)` and `prnt at([5 6 7], 1)` failed with ILO-P003 'expected `)`, got `,`': parse_call_arg handled zero-arg `name()` but not the adjacent paren-form arg list, so the arity-driven positional loop parsed the group as a parenthesised expression and died on the comma. User-fn calls were unaffected, which is why it read as a builtin-only bug. Adds the same adjacency branch the statement-head and operand positions already carry, and extends trailing operands inline up to the known arity (ILO-544's `at([5 6 7])1`) rather than via paren_call_atom, which only parse_call_or_atom consumes and would leak into an unrelated atom here. --- src/parser/mod.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) 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; From a67386a51f9d279ed0040733ced108d6feb2d57e Mon Sep 17 00:00:00 2001 From: Daniel Morris Date: Thu, 13 Aug 2026 10:39:35 +0100 Subject: [PATCH 4/5] fix: clear clippy -D warnings on the 26.8 line Eight lint failures had accumulated while the Rust workflow was not running on release branches. Mostly mechanical (identity map, unused bindings, needless refs, useless format!). Two were real: - The 2-arg `rou x digits` verifier arm was unreachable because `rou` also appeared in the 1-arg math-builtin arm, so the digits argument was never type-checked. `rou 3.1 "x"` now reports ILO-T013. - Duplicated min/max and rou branches in the arity-hint chain collapsed into one matches!. --- src/constrain.rs | 5 +---- src/main.rs | 2 +- src/verify.rs | 15 ++++++++------- 3 files changed, 10 insertions(+), 12 deletions(-) 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/verify.rs b/src/verify.rs index 10d918a4..231b27ec 100644 --- a/src/verify.rs +++ b/src/verify.rs @@ -1506,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) { @@ -5640,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), ); } @@ -6591,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() From 736ca9fbbb5b55cf1438391d062d023f98c60e67 Mon Sep 17 00:00:00 2001 From: Daniel Morris Date: Thu, 13 Aug 2026 10:39:48 +0100 Subject: [PATCH 5/5] tests: realign suites that pinned the pre-shadowing contract 69565d44 made builtin names legal as bindings (they shadow the builtin in value position, call position still dispatches it) but updated no tests, and the Rust workflow was not running on release branches, so the breakage stayed invisible. Five suites asserted the old ILO-P011 rejection: - coverage_parser: the two builtin-binding cases now assert acceptance. - regression_builtin_binding_name: rewritten around the new contract. The original persona bug (use site mis-dispatching to the builtin and reporting a bogus ILO-T006) is still pinned, now by asserting the use site resolves to the local. Adds a case for call position still dispatching the builtin, the half most likely to be broken later. - regression_multi_line_body_span_drift: fixtures used `rev =` purely as an error vehicle for the span assertions; swapped to `fld`, which is still hard-reserved, so the spans are what is under test again. - regression_listlit_builtin_call_hint: fmt2 has gained a known arity, so the list-literal shape parses correctly and ILO-P101 no longer fires for it. Asserts the parse; `fmt` (variadic) still covers P101. - regression_top_level_chain_hint: the map= case now asserts the binding runs and that P102 does not swallow it. --- tests/coverage_parser.rs | 11 +- tests/regression_builtin_binding_name.rs | 234 +++++++----------- tests/regression_listlit_builtin_call_hint.rs | 34 +-- .../regression_multi_line_body_span_drift.rs | 57 +++-- tests/regression_top_level_chain_hint.rs | 31 ++- 5 files changed, 157 insertions(+), 210 deletions(-) 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_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]