fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix: add depth limit to formula evaluation, propagate errors

Browse Source 
Circular or self-referencing formulas now return CellValue::Error
instead of stack overflowing. eval_expr uses Result<f64, String>
internally so errors (circular refs, div/0, missing refs) propagate
immediately through the expression tree via ?. The depth limit (16)
is checked per evaluate_depth call — normal 1-2 level chains are
unaffected.

Also adds CellValue::Error variant for displaying ERR:reason in the
grid, and handles it in format, persistence, and search.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-04-09 01:35:05 -07:00
parent 32d215f3d6
commit 737d14a5c0
5 changed files with 1324 additions and 139 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/model/cell.rs
View File

@ -62,15 +62,21 @@ impl std::fmt::Display for CellKey {
pub enum CellValue {
Number(f64),
Text(String),
/// Evaluation error (circular reference, depth overflow, etc.)
Error(String),
}
impl CellValue {
pub fn as_f64(&self) -> Option<f64> {
match self {
CellValue::Number(n) => Some(*n),
CellValue::Text(_) => None,
_ => None,
}
}
pub fn is_error(&self) -> bool {
matches!(self, CellValue::Error(_))
}
}
impl std::fmt::Display for CellValue {
@ -84,6 +90,7 @@ impl std::fmt::Display for CellValue {
}
}
CellValue::Text(s) => write!(f, "{s}"),
CellValue::Error(msg) => write!(f, "ERR:{msg}"),
}
}
}

163
src/model/types.rs
View File

@ -261,11 +261,22 @@ impl Model {
/// Evaluate a computed value at a given key, considering formulas.
/// Returns None when the cell is empty (no stored value, no applicable formula).
/// Maximum formula evaluation depth. Circular references return None
/// instead of stack-overflowing.
const MAX_EVAL_DEPTH: u8 = 16;
pub fn evaluate(&self, key: &CellKey) -> Option<CellValue> {
self.evaluate_depth(key, Self::MAX_EVAL_DEPTH)
}
fn evaluate_depth(&self, key: &CellKey, depth: u8) -> Option<CellValue> {
if depth == 0 {
return Some(CellValue::Error("circular".into()));
}
for formula in &self.formulas {
if let Some(item_val) = key.get(&formula.target_category) {
if item_val == formula.target {
return self.eval_formula(formula, key);
return self.eval_formula_depth(formula, key, depth - 1);
}
}
}
@ -326,6 +337,15 @@ impl Model {
}
fn eval_formula(&self, formula: &Formula, context: &CellKey) -> Option<CellValue> {
self.eval_formula_depth(formula, context, Self::MAX_EVAL_DEPTH)
}
fn eval_formula_depth(
&self,
formula: &Formula,
context: &CellKey,
depth: u8,
) -> Option<CellValue> {
use crate::formula::{AggFunc, Expr};
// Check WHERE filter first
@ -348,42 +368,50 @@ impl Model {
None
}
/// Evaluate an expression, returning Ok(f64) or Err(reason).
/// Errors propagate immediately — a circular reference in any
/// sub-expression short-circuits the entire formula.
fn eval_expr(
expr: &Expr,
context: &CellKey,
model: &Model,
target_category: &str,
) -> Option<f64> {
depth: u8,
) -> Result<f64, String> {
match expr {
Expr::Number(n) => Some(*n),
Expr::Number(n) => Ok(*n),
Expr::Ref(name) => {
let cat = find_item_category(model, name)?;
let cat = find_item_category(model, name)
.ok_or_else(|| format!("ref:{name}"))?;
let new_key = context.clone().with(cat, name);
model.evaluate(&new_key).and_then(|v| v.as_f64())
match model.evaluate_depth(&new_key, depth) {
Some(CellValue::Number(n)) => Ok(n),
Some(CellValue::Error(e)) => Err(e),
_ => Err(format!("ref:{name}")),
}
}
Expr::BinOp(op, l, r) => {
use crate::formula::BinOp;
let lv = eval_expr(l, context, model, target_category)?;
let rv = eval_expr(r, context, model, target_category)?;
Some(match op {
BinOp::Add => lv + rv,
BinOp::Sub => lv - rv,
BinOp::Mul => lv * rv,
let lv = eval_expr(l, context, model, target_category, depth)?;
let rv = eval_expr(r, context, model, target_category, depth)?;
match op {
BinOp::Add => Ok(lv + rv),
BinOp::Sub => Ok(lv - rv),
BinOp::Mul => Ok(lv * rv),
BinOp::Div => {
if rv == 0.0 {
return None;
Err("div/0".into())
} else {
Ok(lv / rv)
}
lv / rv
}
BinOp::Pow => lv.powf(rv),
// Comparison operators are handled by eval_bool; reaching
// here means a comparison was used where a number is expected.
BinOp::Pow => Ok(lv.powf(rv)),
BinOp::Eq | BinOp::Ne | BinOp::Lt | BinOp::Gt | BinOp::Le | BinOp::Ge => {
return None
Err("type".into())
}
})
}
}
Expr::UnaryMinus(e) => Some(-eval_expr(e, context, model, target_category)?),
Expr::UnaryMinus(e) => Ok(-eval_expr(e, context, model, target_category, depth)?),
Expr::Agg(func, inner, agg_filter) => {
let mut partial = context.without(target_category);
if let Expr::Ref(item_name) = inner.as_ref() {
@ -401,25 +429,25 @@ impl Model {
.filter_map(|v| v.as_f64())
.collect();
match func {
AggFunc::Sum => Some(values.iter().sum()),
AggFunc::Sum => Ok(values.iter().sum()),
AggFunc::Avg => {
if values.is_empty() {
None
Err("empty".into())
} else {
Some(values.iter().sum::<f64>() / values.len() as f64)
Ok(values.iter().sum::<f64>() / values.len() as f64)
}
}
AggFunc::Min => values.iter().cloned().reduce(f64::min),
AggFunc::Max => values.iter().cloned().reduce(f64::max),
AggFunc::Count => Some(values.len() as f64),
AggFunc::Min => values.iter().cloned().reduce(f64::min).ok_or_else(|| "empty".into()),
AggFunc::Max => values.iter().cloned().reduce(f64::max).ok_or_else(|| "empty".into()),
AggFunc::Count => Ok(values.len() as f64),
}
}
Expr::If(cond, then, else_) => {
let cv = eval_bool(cond, context, model, target_category)?;
let cv = eval_bool(cond, context, model, target_category, depth)?;
if cv {
eval_expr(then, context, model, target_category)
eval_expr(then, context, model, target_category, depth)
} else {
eval_expr(else_, context, model, target_category)
eval_expr(else_, context, model, target_category, depth)
}
}
}
@ -430,30 +458,33 @@ impl Model {
context: &CellKey,
model: &Model,
target_category: &str,
) -> Option<bool> {
depth: u8,
) -> Result<bool, String> {
use crate::formula::BinOp;
match expr {
Expr::BinOp(op, l, r) => {
let lv = eval_expr(l, context, model, target_category)?;
let rv = eval_expr(r, context, model, target_category)?;
Some(match op {
BinOp::Eq => (lv - rv).abs() < 1e-10,
BinOp::Ne => (lv - rv).abs() >= 1e-10,
BinOp::Lt => lv < rv,
BinOp::Gt => lv > rv,
BinOp::Le => lv <= rv,
BinOp::Ge => lv >= rv,
// Arithmetic operators are not comparisons
let lv = eval_expr(l, context, model, target_category, depth)?;
let rv = eval_expr(r, context, model, target_category, depth)?;
match op {
BinOp::Eq => Ok((lv - rv).abs() < 1e-10),
BinOp::Ne => Ok((lv - rv).abs() >= 1e-10),
BinOp::Lt => Ok(lv < rv),
BinOp::Gt => Ok(lv > rv),
BinOp::Le => Ok(lv <= rv),
BinOp::Ge => Ok(lv >= rv),
BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Pow => {
return None
Err("type".into())
}
})
}
}
_ => None,
_ => Err("type".into()),
}
}
eval_expr(&formula.expr, context, self, &formula.target_category).map(CellValue::Number)
match eval_expr(&formula.expr, context, self, &formula.target_category, depth) {
Ok(n) => Some(CellValue::Number(n)),
Err(e) => Some(CellValue::Error(e)),
}
}
}
@ -862,10 +893,10 @@ mod formula_tests {
CellValue::Number(0.0),
);
m.add_formula(parse_formula("Result = Revenue / Zero", "Measure").unwrap());
// Division by zero must yield Empty, not 0, so the user sees a blank not a misleading zero.
// Division by zero yields an error, not a blank or misleading zero.
assert_eq!(
m.evaluate(&coord(&[("Measure", "Result"), ("Region", "East")])),
None
Some(CellValue::Error("div/0".into()))
);
}
@ -904,7 +935,47 @@ mod formula_tests {
cat.add_item("Ghost");
}
let k = coord(&[("Measure", "Ghost"), ("Region", "East")]);
assert_eq!(m.evaluate(&k), None);
assert!(
matches!(m.evaluate(&k), Some(CellValue::Error(_))),
"missing ref should produce an error, got: {:?}",
m.evaluate(&k)
);
}
/// Circular formula references must produce an error, not stack overflow.
#[test]
fn circular_formula_returns_error() {
let mut m = Model::new("Test");
m.add_category("Measure").unwrap();
if let Some(cat) = m.category_mut("Measure") {
cat.add_item("A");
cat.add_item("B");
}
m.add_formula(parse_formula("A = B + 1", "Measure").unwrap());
m.add_formula(parse_formula("B = A + 1", "Measure").unwrap());
let result = m.evaluate(&coord(&[("Measure", "A")]));
assert!(
matches!(result, Some(CellValue::Error(_))),
"circular reference should produce an error, got: {:?}",
result
);
}
/// Self-referencing formula must produce an error.
#[test]
fn self_referencing_formula_returns_error() {
let mut m = Model::new("Test");
m.add_category("Measure").unwrap();
if let Some(cat) = m.category_mut("Measure") {
cat.add_item("X");
}
m.add_formula(parse_formula("X = X + 1", "Measure").unwrap());
let result = m.evaluate(&coord(&[("Measure", "X")]));
assert!(
matches!(result, Some(CellValue::Error(_))),
"self-reference should produce an error, got: {:?}",
result
);
}
#[test]