fix: formula correctness and command validation bugs
- model.rs: division by zero now returns Empty instead of 0 so the cell visually signals the error rather than silently showing a wrong value. Updated the test to assert Empty. - parser.rs: missing closing ')' in aggregate functions (SUM, AVG, etc.) and IF(...) now returns a parse error instead of silently succeeding, preventing malformed formulas from evaluating with unexpected results. - dispatch.rs: SetCell validates that every category in the coords exists before mutating anything; previously a typo in a category name silently wrote an orphaned cell (stored but never visible in any grid) and returned ok. Now returns an error immediately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@ -36,12 +36,16 @@ pub fn dispatch(model: &mut Model, cmd: &Command) -> CommandResult {
|
|||||||
let kv: Vec<(String, String)> = coords.iter()
|
let kv: Vec<(String, String)> = coords.iter()
|
||||||
.map(|pair| (pair[0].clone(), pair[1].clone()))
|
.map(|pair| (pair[0].clone(), pair[1].clone()))
|
||||||
.collect();
|
.collect();
|
||||||
// Ensure items exist
|
// Validate all categories exist before mutating anything
|
||||||
for (cat_name, item_name) in &kv {
|
for (cat_name, _) in &kv {
|
||||||
if let Some(cat) = model.category_mut(cat_name) {
|
if model.category(cat_name).is_none() {
|
||||||
cat.add_item(item_name);
|
return CommandResult::err(format!("Category '{cat_name}' not found"));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Ensure items exist within their categories
|
||||||
|
for (cat_name, item_name) in &kv {
|
||||||
|
model.category_mut(cat_name).unwrap().add_item(item_name);
|
||||||
|
}
|
||||||
let key = CellKey::new(kv);
|
let key = CellKey::new(kv);
|
||||||
let cell_value = match value {
|
let cell_value = match value {
|
||||||
CellValueArg::Number { number } => CellValue::Number(*number),
|
CellValueArg::Number { number } => CellValue::Number(*number),
|
||||||
@ -136,7 +140,8 @@ pub fn dispatch(model: &mut Model, cmd: &Command) -> CommandResult {
|
|||||||
|
|
||||||
Command::Load { path } => {
|
Command::Load { path } => {
|
||||||
match persistence::load(std::path::Path::new(path)) {
|
match persistence::load(std::path::Path::new(path)) {
|
||||||
Ok(loaded) => {
|
Ok(mut loaded) => {
|
||||||
|
loaded.normalize_view_state();
|
||||||
*model = loaded;
|
*model = loaded;
|
||||||
CommandResult::ok_msg(format!("Loaded from {path}"))
|
CommandResult::ok_msg(format!("Loaded from {path}"))
|
||||||
}
|
}
|
||||||
|
|||||||
@ -255,6 +255,8 @@ fn parse_primary(tokens: &[Token], pos: &mut usize) -> Result<Expr> {
|
|||||||
// expect )
|
// expect )
|
||||||
if *pos < tokens.len() && tokens[*pos] == Token::RParen {
|
if *pos < tokens.len() && tokens[*pos] == Token::RParen {
|
||||||
*pos += 1;
|
*pos += 1;
|
||||||
|
} else {
|
||||||
|
return Err(anyhow!("Expected ')' to close aggregate function"));
|
||||||
}
|
}
|
||||||
return Ok(Expr::Agg(func, Box::new(inner), filter));
|
return Ok(Expr::Agg(func, Box::new(inner), filter));
|
||||||
}
|
}
|
||||||
@ -268,7 +270,11 @@ fn parse_primary(tokens: &[Token], pos: &mut usize) -> Result<Expr> {
|
|||||||
let then = parse_add_sub(tokens, pos)?;
|
let then = parse_add_sub(tokens, pos)?;
|
||||||
if *pos < tokens.len() && tokens[*pos] == Token::Comma { *pos += 1; }
|
if *pos < tokens.len() && tokens[*pos] == Token::Comma { *pos += 1; }
|
||||||
let else_ = parse_add_sub(tokens, pos)?;
|
let else_ = parse_add_sub(tokens, pos)?;
|
||||||
if *pos < tokens.len() && tokens[*pos] == Token::RParen { *pos += 1; }
|
if *pos < tokens.len() && tokens[*pos] == Token::RParen {
|
||||||
|
*pos += 1;
|
||||||
|
} else {
|
||||||
|
return Err(anyhow!("Expected ')' to close IF(...)"));
|
||||||
|
}
|
||||||
return Ok(Expr::If(Box::new(cond), Box::new(then), Box::new(else_)));
|
return Ok(Expr::If(Box::new(cond), Box::new(then), Box::new(else_)));
|
||||||
}
|
}
|
||||||
Ok(Expr::Ref(name))
|
Ok(Expr::Ref(name))
|
||||||
|
|||||||
@ -123,6 +123,16 @@ impl Model {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Reset all view scroll offsets to zero.
|
||||||
|
/// Call this after loading or replacing a model so stale offsets don't
|
||||||
|
/// cause the grid to render an empty area.
|
||||||
|
pub fn normalize_view_state(&mut self) {
|
||||||
|
for view in self.views.values_mut() {
|
||||||
|
view.row_offset = 0;
|
||||||
|
view.col_offset = 0;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Return all category names
|
/// Return all category names
|
||||||
pub fn category_names(&self) -> Vec<&str> {
|
pub fn category_names(&self) -> Vec<&str> {
|
||||||
self.categories.keys().map(|s| s.as_str()).collect()
|
self.categories.keys().map(|s| s.as_str()).collect()
|
||||||
@ -182,7 +192,7 @@ impl Model {
|
|||||||
"+" => lv + rv,
|
"+" => lv + rv,
|
||||||
"-" => lv - rv,
|
"-" => lv - rv,
|
||||||
"*" => lv * rv,
|
"*" => lv * rv,
|
||||||
"/" => if rv == 0.0 { 0.0 } else { lv / rv },
|
"/" => { if rv == 0.0 { return None; } lv / rv }
|
||||||
"^" => lv.powf(rv),
|
"^" => lv.powf(rv),
|
||||||
_ => return None,
|
_ => return None,
|
||||||
})
|
})
|
||||||
@ -482,7 +492,7 @@ mod formula_tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn division_by_zero_yields_zero() {
|
fn division_by_zero_yields_empty() {
|
||||||
let mut m = Model::new("Test");
|
let mut m = Model::new("Test");
|
||||||
m.add_category("Measure").unwrap();
|
m.add_category("Measure").unwrap();
|
||||||
m.add_category("Region").unwrap();
|
m.add_category("Region").unwrap();
|
||||||
@ -494,7 +504,8 @@ mod formula_tests {
|
|||||||
m.set_cell(coord(&[("Measure", "Revenue"), ("Region", "East")]), CellValue::Number(100.0));
|
m.set_cell(coord(&[("Measure", "Revenue"), ("Region", "East")]), CellValue::Number(100.0));
|
||||||
m.set_cell(coord(&[("Measure", "Zero"), ("Region", "East")]), CellValue::Number(0.0));
|
m.set_cell(coord(&[("Measure", "Zero"), ("Region", "East")]), CellValue::Number(0.0));
|
||||||
m.add_formula(parse_formula("Result = Revenue / Zero", "Measure").unwrap());
|
m.add_formula(parse_formula("Result = Revenue / Zero", "Measure").unwrap());
|
||||||
assert_eq!(m.evaluate(&coord(&[("Measure", "Result"), ("Region", "East")])), CellValue::Number(0.0));
|
// Division by zero must yield Empty, not 0, so the user sees a blank not a misleading zero.
|
||||||
|
assert_eq!(m.evaluate(&coord(&[("Measure", "Result"), ("Region", "East")])), CellValue::Empty);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user