From 56f784473d0bb14d8ea19b50bd2180d9a409540d Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 13:29:14 -0500 Subject: [PATCH] engine: refuse formulas with the wrong argument count on entry A formula calling a built-in function with too few or too many arguments (=DATEDIF(a,b), =SUM(), =DATE(2024,1)) was stored and showed #VALUE! with no hint about what was wrong. Entry now refuses it with the function's signature, e.g. "DATEDIF has too few arguments. Use DATEDIF(start_date, end_date, unit)", and leaves the cell unchanged. Only registered functions are checked: special forms the evaluator handles itself (IF, LET, LAMBDA...), LAMBDA names and unknown functions are skipped. Paste and Find & Replace keep turning input that fails entry into text. DATEDIF's description now lists its units. Fixes #363 Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- crates/engine/src/cmd/edit.rs | 22 ++++++++++++++++++++ crates/engine/src/tests.rs | 39 +++++++++++++++++++++++++++++++++++ crates/functions/src/date.rs | 2 +- 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 82c15aa..a65b16a 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -150,6 +150,7 @@ pub(crate) fn input_to_cell(input: &str, sheet: usize, at: CellRef, wb: &mut gri } let expr = parsed.map_err(|e| EngineError::Other(format!("There's a problem with this formula: {e}")))?; let expr = wb.name_call_case(expr); + check_arg_counts(&expr)?; let mut cell = Cell::formula(Formula::from_expr(expr)); cell.style = style; // Formulas whose result is a date/time get a format from functions like TODAY(). @@ -190,6 +191,27 @@ pub(crate) fn input_to_cell(input: &str, sheet: usize, at: CellRef, wb: &mut gri Ok(Some(Cell { value: parsed.value, formula: None, style })) } +/// Like Excel, a formula that calls a built-in function with too few or too many arguments is +/// refused when it's entered, rather than stored to show `#VALUE!`. Special forms the evaluator +/// handles itself (IF, LET, LAMBDA…) and defined LAMBDA names aren't in the registry: skipped. +fn check_arg_counts(expr: &gridcraft_formula::Expr) -> Result<()> { + let mut bad = None; + expr.walk(&mut |e| { + if bad.is_none() + && let gridcraft_formula::Expr::Call(name, args) = e + && let Some(spec) = gridcraft_functions::lookup(name) + && (args.len() < spec.min_args || args.len() > spec.max_args) + { + let few = args.len() < spec.min_args; + bad = Some(format!("{} has too {} arguments. Use {}", spec.name, if few { "few" } else { "many" }, spec.signature)); + } + }); + match bad { + Some(msg) => Err(EngineError::Other(msg)), + None => Ok(()), + } +} + fn cell_set(s: &mut Session, p: &Json) -> Result { let sheet = target_sheet(s, p)?; let at = match cell_param(p, "cell") { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index c0dd7c8..68910fc 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -202,6 +202,45 @@ fn enter_values_and_formulas() { assert_eq!(crate::display::cell_text(&d.wb, sh, CellRef::parse("B1").unwrap()), "12%"); } +#[test] +fn formulas_with_the_wrong_argument_count_are_refused_on_entry() { + // #363: =DATEDIF(start, end) was stored and showed #VALUE!. Like Excel, entry refuses it and + // the message gives the signature, so the missing unit argument is visible. + let mut s = s(); + s.execute("cell.set", json!({"cell": "A1", "input": "kept"})).unwrap(); + for (input, word) in [ + ("=DATEDIF(DATE(2024,1,1),DATE(2024,3,15))", "few"), + ("=1+DATE(2024,1)", "few"), + ("=SUM()", "few"), + ("=IFERROR(1,LEFT(\"a\",1,2,3))", "many"), + ("=datedif(1,2,\"D\",4)", "many"), + ] { + let e = s.execute("cell.set", json!({"cell": "A1", "input": input})).unwrap_err().to_string(); + assert!(e.contains(&format!("too {word} arguments")), "{input}: {e}"); + assert_eq!(v(&s, "A1"), Value::text("kept"), "{input} must not replace the cell"); + } + let e = s.execute("cell.set", json!({"cell": "A1", "input": "=DATEDIF(1,2)"})).unwrap_err().to_string(); + assert!(e.contains("DATEDIF(start_date, end_date, unit)"), "{e}"); + // Correct counts, optional arguments left empty, special forms, LAMBDA names and unknown + // functions (#NAME?) still enter. + s.execute("formulas.defineName", json!({"name": "TWICE", "refersTo": "=LAMBDA(x,x*2)"})).unwrap(); + for (cell, input) in [ + ("B1", "=DATEDIF(DATE(2024,1,1),DATE(2024,3,15),\"D\")"), + ("B2", "=IF(TRUE,1)"), + ("B3", "=LET(a,1,b,2,a+b)"), + ("B4", "=TWICE(4)"), + ("B5", "=NOSUCHFUNCTION(1,2,3)"), + ("B6", "=ROUND(2.5,)"), + ] { + s.execute("cell.set", json!({"cell": cell, "input": input})).unwrap_or_else(|e| panic!("{input}: {e}")); + } + assert_eq!(v(&s, "B1"), Value::Number(74.0)); + assert_eq!(v(&s, "B4"), Value::Number(8.0)); + // Pasted text that isn't a valid formula stays text, as it does for a parse error. + s.execute("edit.paste", json!({"at": "C1", "text": "=DATEDIF(1,2)\n"})).unwrap(); + assert_eq!(v(&s, "C1"), Value::text("=DATEDIF(1,2)")); +} + #[test] fn undo_redo() { let mut s = s(); diff --git a/crates/functions/src/date.rs b/crates/functions/src/date.rs index 258aea6..48218ef 100644 --- a/crates/functions/src/date.rs +++ b/crates/functions/src/date.rs @@ -496,7 +496,7 @@ pub(crate) fn specs() -> Vec { f!("ISOWEEKNUM", 1, 1, DateTime, S, "ISOWEEKNUM(date)", "Returns the ISO 8601 week number of a date.", isoweeknum), f!("EDATE", 2, 2, DateTime, S, "EDATE(start_date, months)", "Returns the date a number of months before or after a date.", edate), f!("EOMONTH", 2, 2, DateTime, S, "EOMONTH(start_date, months)", "Returns the last day of the month a number of months away.", eomonth), - f!("DATEDIF", 3, 3, DateTime, S, "DATEDIF(start_date, end_date, unit)", "Counts whole years, months or days between two dates.", datedif), + f!("DATEDIF", 3, 3, DateTime, S, "DATEDIF(start_date, end_date, unit)", "Years, months or days apart. unit: Y, M, D, MD, YM or YD.", datedif), f!("DATEVALUE", 1, 1, DateTime, S, "DATEVALUE(date_text)", "Converts a date written as text into a date serial number.", datevalue), f!("TIMEVALUE", 1, 1, DateTime, S, "TIMEVALUE(time_text)", "Converts a time written as text into a fraction of a day.", timevalue), f!("DAYS", 2, 2, DateTime, S, "DAYS(end_date, start_date)", "Returns the number of days between two dates.", days),