diff --git a/contributors/people.toml b/contributors/people.toml index 681a4dd..0099324 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -54,3 +54,6 @@ sync_github_name = true [people.GhostMitra] sync_github_name = true +[people.shrshk] +real_name = "Shirish Veerabattini" +sync_github_name = true diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 91a81b3..e13cbbd 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,28 @@ 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!`. 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| { + if bad.is_none() + && let gridcraft_formula::Expr::Call(name, args) = e + && let Some(spec) = gridcraft_functions::lookup(name) + && !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)); + } + }); + 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_on(s, p, "cell", sheet)? { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index eeb94ac..9f6ec14 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..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)", "Counts whole years, months or days between two dates.", 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), 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) 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()); +}