From 56f784473d0bb14d8ea19b50bd2180d9a409540d Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 13:29:14 -0500 Subject: [PATCH 1/6] 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), From d0ff4265c78b9b8113246d2bfd8ad160f1a8c336 Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 13:29:14 -0500 Subject: [PATCH 2/6] contributors: add shrshk Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- contributors/people.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contributors/people.toml b/contributors/people.toml index 7b69da3..f9d773e 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -50,3 +50,7 @@ sync_github_name = false [people.Losminek] sync_github_name = true + +[people.shrshk] +real_name = "Shirish Veerabattini" +sync_github_name = true From 993c1086607ad19c491d9b43dd860e9fce6110d7 Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 15:37:09 -0500 Subject: [PATCH 3/6] functions: share the argument-count rule between call and entry FnSpec::accepts holds the arity check (including Excel's 255-argument limit) that functions::call and the entry check both use, so entry also refuses 256 arguments to a variadic function. The evaluator's own check in calc is unchanged. Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- crates/engine/src/cmd/edit.rs | 2 +- crates/engine/src/tests.rs | 6 ++++++ crates/functions/src/lib.rs | 7 ++++++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 9f48a87..b487947 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -200,7 +200,7 @@ fn check_arg_counts(expr: &gridcraft_formula::Expr) -> Result<()> { 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) + && !spec.accepts(args.len()) { let few = args.len() < spec.min_args; bad = Some(format!("{} has too {} arguments. Use {}", spec.name, if few { "few" } else { "many" }, spec.signature)); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 6e6c9d4..620dfcb 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -219,6 +219,12 @@ fn formulas_with_the_wrong_argument_count_are_refused_on_entry() { assert!(e.contains(&format!("too {word} arguments")), "{input}: {e}"); assert_eq!(v(&s, "A1"), Value::text("kept"), "{input} must not replace the cell"); } + // Excel's 255-argument limit applies to variadic functions too. + let many = format!("=SUM({})", vec!["1"; 256].join(",")); + let e = s.execute("cell.set", json!({"cell": "A1", "input": many})).unwrap_err().to_string(); + assert!(e.contains("too many arguments"), "{e}"); + s.execute("cell.set", json!({"cell": "A2", "input": format!("=SUM({})", vec!["1"; 255].join(","))})).unwrap(); + assert_eq!(v(&s, "A2"), Value::Number(255.0)); 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 diff --git a/crates/functions/src/lib.rs b/crates/functions/src/lib.rs index 1e4415e..282b230 100644 --- a/crates/functions/src/lib.rs +++ b/crates/functions/src/lib.rs @@ -160,6 +160,11 @@ impl FnSpec { None => self.scalar_args.last().copied().unwrap_or(false), } } + + /// Whether a call with `n` arguments is valid (Excel allows at most 255). + pub fn accepts(&self, n: usize) -> bool { + (self.min_args..=self.max_args).contains(&n) && n <= 255 + } } /// Variadic marker for `max_args`. @@ -215,7 +220,7 @@ pub fn all() -> &'static [FnSpec] { /// Calls a function with array lifting applied. Wrong arity gives `#VALUE!`. pub fn call(spec: &FnSpec, args: &[Arg], ctx: &mut dyn Ctx) -> Value { - if args.len() < spec.min_args || args.len() > spec.max_args || args.len() > 255 { + if !spec.accepts(args.len()) { return Value::Error(CellError::Value); } lift::call_lifted(spec, args, ctx) From 1d1d27f9a471770ac570edd6aeea54142cdf064b Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 15:37:09 -0500 Subject: [PATCH 4/6] ui: argument hint shows the function's description Inside DATEDIF( the hint showed only the signature, so the values unit takes weren't visible while typing it (#363). The hint now adds the function's description under the signature, wrapped to the hint's width, the same text the autocomplete list shows (translated where a Spanish entry exists). Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- crates/ui-egui/src/formula_bar.rs | 7 +++++++ crates/ui-egui/tests/formula_popup.rs | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/crates/ui-egui/src/formula_bar.rs b/crates/ui-egui/src/formula_bar.rs index 7404a61..83278b1 100644 --- a/crates/ui-egui/src/formula_bar.rs +++ b/crates/ui-egui/src/formula_bar.rs @@ -392,6 +392,13 @@ pub fn editor_widget(app: &mut SheetApp, ui: &mut egui::Ui, id: egui::Id, font: } ui.label(egui::RichText::new(")").font(theme::ui_font(12.0))); }); + // The description too, so values an argument takes (DATEDIF's units, #363) show + // while it's typed; wrapped to the hint's width. + if let Some(desc) = crate::formula_locale::description(&name, ed.locale) { + ui.set_max_width(ui.min_rect().width().max(220.0)); + ui.separator(); + ui.add(egui::Label::new(egui::RichText::new(desc).small()).wrap()); + } } }); }); diff --git a/crates/ui-egui/tests/formula_popup.rs b/crates/ui-egui/tests/formula_popup.rs index 6734d30..9488614 100644 --- a/crates/ui-egui/tests/formula_popup.rs +++ b/crates/ui-egui/tests/formula_popup.rs @@ -142,3 +142,21 @@ fn cell_editor_and_helper_stay_below_the_frozen_top_row() { frames(&ctx, &mut app); assert!(visible(&ctx, "gridcraft.cell_editor"), "scrolling back restores the helper"); } + +#[test] +fn argument_hint_shows_the_function_description() { + // #363: inside DATEDIF( the hint lists the units the third argument takes. + let ctx = Context::default(); + let mut session = Session::new(); + session.new_workbook(); + let mut app = SheetApp::new(session, Default::default()); + frames(&ctx, &mut app); + app.begin_edit(Some("=DATEDIF(A1,B1,".into()), false); + frames(&ctx, &mut app); + let output = frame(&ctx, &mut app); + let texts: Vec<&egui::epaint::TextShape> = + output.shapes.iter().filter_map(|s| if let egui::Shape::Text(t) = &s.shape { Some(t) } else { None }).collect(); + assert!(texts.iter().any(|t| t.galley.text() == "DATEDIF("), "signature painted"); + let desc = texts.iter().find(|t| t.galley.text().contains("YM or YD")).expect("description with the units painted"); + assert!(desc.galley.rect.width() <= 400.0, "description wraps instead of widening the hint: {}", desc.galley.rect.width()); +} From 478f01d2363775dcdbc2536619cd9b5b45cd0f55 Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 15:57:33 -0500 Subject: [PATCH 5/6] functions: keep DATEDIF's original description, add the units Restores "Counts whole years, months or days between two dates." and appends the units, in the multi-line form rustfmt gives a long entry. Drops the 256-argument test case: the parser already refuses more than 255 arguments, so it never reached the entry check. (993c108's message says entry "also refuses" them; it already did.) Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- crates/engine/src/tests.rs | 6 ------ crates/functions/src/date.rs | 11 ++++++++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 620dfcb..6e6c9d4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -219,12 +219,6 @@ fn formulas_with_the_wrong_argument_count_are_refused_on_entry() { assert!(e.contains(&format!("too {word} arguments")), "{input}: {e}"); assert_eq!(v(&s, "A1"), Value::text("kept"), "{input} must not replace the cell"); } - // Excel's 255-argument limit applies to variadic functions too. - let many = format!("=SUM({})", vec!["1"; 256].join(",")); - let e = s.execute("cell.set", json!({"cell": "A1", "input": many})).unwrap_err().to_string(); - assert!(e.contains("too many arguments"), "{e}"); - s.execute("cell.set", json!({"cell": "A2", "input": format!("=SUM({})", vec!["1"; 255].join(","))})).unwrap(); - assert_eq!(v(&s, "A2"), Value::Number(255.0)); 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 diff --git a/crates/functions/src/date.rs b/crates/functions/src/date.rs index 48218ef..29aeb47 100644 --- a/crates/functions/src/date.rs +++ b/crates/functions/src/date.rs @@ -496,7 +496,16 @@ 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)", "Years, months or days apart. unit: Y, M, D, MD, YM or YD.", datedif), + f!( + "DATEDIF", + 3, + 3, + DateTime, + S, + "DATEDIF(start_date, end_date, unit)", + "Counts whole years, months or days between two dates. 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), From 3936f3b79ce0b0aa1dd1e09f88a81634ea8119cb Mon Sep 17 00:00:00 2001 From: Shirish Veerabattini Date: Sun, 11 Oct 2026 16:10:04 -0500 Subject: [PATCH 6/6] engine: correct which functions the entry check skips IF, IFERROR, CHOOSE, SWITCH and INDEX are in the registry and are checked; the comment listed IF as skipped. Claude-Session: https://claude.ai/code/session_01BGBBGBC2gRpQ7GDuwQ6WPt --- crates/engine/src/cmd/edit.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index b487947..e13cbbd 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -192,8 +192,9 @@ pub(crate) fn input_to_cell(input: &str, sheet: usize, at: CellRef, wb: &mut gri } /// 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. +/// refused when it's entered, rather than stored to show `#VALUE!`. Functions the registry doesn't +/// hold (special forms like LET, LAMBDA, OFFSET, INDIRECT, SUBTOTAL), defined LAMBDA names and +/// unknown functions are skipped. fn check_arg_counts(expr: &gridcraft_formula::Expr) -> Result<()> { let mut bad = None; expr.walk(&mut |e| {