shifathosn
/
gridcraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #409 from shrshk/fix/formula-arg-count-363 

Refuse formulas with the wrong argument count on entry (#363)
This commit is contained in:
Brandon Thomas 2026-10-11 18:05:18 -04:00
parent e2bcb712fd 3936f3b79c
commit a96f19f02b
7 changed files with 106 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
contributors/people.toml
View File

@ -54,3 +54,6 @@ sync_github_name = true
[people.GhostMitra]
sync_github_name = true
[people.shrshk]
real_name = "Shirish Veerabattini"
sync_github_name = true

23
crates/engine/src/cmd/edit.rs
View File

@ -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<Json> {
let sheet = target_sheet(s, p)?;
let at = match cell_param_on(s, p, "cell", sheet)? {

39
crates/engine/src/tests.rs
View File

@ -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();

11
crates/functions/src/date.rs
View File

@ -496,7 +496,16 @@ pub(crate) fn specs() -> Vec<FnSpec> {
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),

7
crates/functions/src/lib.rs
View File

@ -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)

7
crates/ui-egui/src/formula_bar.rs
View File

@ -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());
}
}
});
});

18
crates/ui-egui/tests/formula_popup.rs
View File

@ -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());
}