From 8776a961eedb0b29f430fbc239863038f4c3c859 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:04:49 +0000 Subject: [PATCH 1/2] chore(spreadsheet): remove dead CellRef::parse_inner `parse_inner` is a byte-for-byte duplicate of the earlier iteration of `CellRef::parse` that carried `#[allow(dead_code)]` since a refactor, and no caller in the repository references it. It never ran, so its 60 lines could only ever drag the coverage report down without guarding anything. `parse` remains the single entry point for cell-reference grammar. --- .../spreadsheet-engine/src/formula2.rs | 61 ------------------- 1 file changed, 61 deletions(-) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs index 6ded958..06e322f 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs @@ -98,67 +98,6 @@ impl CellRef { abs_col, }) } - - #[allow(dead_code)] - fn parse_inner(s: &str, abs_col_hint: bool, abs_row_hint: bool) -> Option { - let mut chars = s.chars().peekable(); - let mut abs_col = abs_col_hint; - let mut abs_row = abs_row_hint; - - // Skip leading `$`. - if chars.peek() == Some(&'$') { - abs_col = true; - chars.next(); - } - - // Column letters. - let mut col_str = String::new(); - while let Some(&c) = chars.peek() { - if c.is_ascii_alphabetic() { - col_str.push(c.to_ascii_uppercase()); - chars.next(); - } else { - break; - } - } - if col_str.is_empty() { - return None; - } - - // Check for `$` between col and row. - if chars.peek() == Some(&'$') { - abs_row = true; - chars.next(); - } - - // Row digits. - let mut row_str = String::new(); - while let Some(&c) = chars.peek() { - if c.is_ascii_digit() { - row_str.push(c); - chars.next(); - } else { - return None; // trailing junk - } - } - if row_str.is_empty() { - return None; - } - - let row = row_str.parse::().ok()?; - if row == 0 { - return None; - } - - let col = col_str_to_index(&col_str)?; - - Some(Self { - row: row - 1, - col, - abs_row, - abs_col, - }) - } } /// Format back to A1 notation (e.g. `$A$1`, `B2`). From c301c7a48ffb959ba3236212518649c2183d2704 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:04:49 +0000 Subject: [PATCH 2/2] test(spreadsheet): cover formula2.rs tokenizer/evaluator branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branches the corrected report named, now that every test binary is measured: - Tokenizer: a lone `!` is a parse error; string escapes (`\n`, `\t`, `\\`); a double-dot number (`1.2.3`) is rejected after the tokenizer breaks on the second dot. - FormulaError: `InvalidRef` and `RuntimeError` display strings; the `Display` delegation; `from_display` round-trip for `#REF!...`. - BinOp precedence table, pinned so a shared precedence cannot pass silently. - Value conversions: `to_f64` / `to_bool` / `to_display_string` / `is_error` propagate and render `Value::Error`. - Evaluator: boolean literals; unary `+` (built directly — the parser collapses `+x` to `x`, so the `UnaryOp::Pos` arm only runs on a hand-built AST); single-cell named range; single- and multi-cell range expressions; `SUM(Undefined)` -> UnknownName through `resolve_arg`; `values_equal` text (case-insensitive), boolean, error and mixed-type arms. - evaluate_call: `SQRT(-4)` -> `#DIV/0!`; `LOG10`, `SIN`, `COS`, `TAN`. - evaluate_if: wrong argument count -> `#VALUE:`. formula2.rs 90.11% -> 99.05% lines; engine total 94.38% -> 96.54%. Unit tests 289 -> 308; 9 integration tests unchanged. Deliberately left uncovered: the `invalid number` map_err closure in the tokenizer (an f64 parse of a digit/dot string cannot fail) and the `_ => panic!(...)` arms of existing positive match tests. --- .../spreadsheet-engine/src/formula2.rs | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs index 06e322f..03b54b4 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs @@ -1894,4 +1894,204 @@ mod tests { ctx.set(0, 0, "=A1"); assert_eq!(eval_mock(&ctx, "A1"), "#CYCLE!"); } + + // ── FormulaError display and round-trip (remaining variants) ─────── + + #[test] + fn formula_error_invalid_ref_and_runtime_display() { + assert_eq!( + FormulaError::InvalidRef("A0".into()).to_display(), + "#REF!A0" + ); + assert_eq!( + FormulaError::RuntimeError("boom".into()).to_display(), + "boom" + ); + // `Display` delegates to `to_display`; `format!` exercises it. + assert_eq!( + format!("{}", FormulaError::InvalidRef("A0".into())), + "#REF!A0" + ); + } + + #[test] + fn formula_error_from_display_invalid_ref() { + assert_eq!( + FormulaError::from_display("#REF!ZZ999999"), + Some(FormulaError::InvalidRef("ZZ999999".into())) + ); + } + + // ── BinOp precedence table ───────────────────────────────────────── + + #[test] + fn binop_precedence_table() { + assert_eq!(BinOp::Concat.precedence(), 1); + assert_eq!(BinOp::Gt.precedence(), 2); + assert_eq!(BinOp::Lt.precedence(), 2); + assert_eq!(BinOp::Ge.precedence(), 2); + assert_eq!(BinOp::Le.precedence(), 2); + assert_eq!(BinOp::Eq.precedence(), 2); + assert_eq!(BinOp::Ne.precedence(), 2); + assert_eq!(BinOp::Add.precedence(), 3); + assert_eq!(BinOp::Sub.precedence(), 3); + assert_eq!(BinOp::Mul.precedence(), 4); + assert_eq!(BinOp::Div.precedence(), 4); + } + + // ── Expr::cell_ref convenience constructor ───────────────────────── + + #[test] + fn expr_cell_ref_constructor_evaluates() { + let mut ctx = MockCtx::new(); + ctx.set(0, 0, "42"); + let expr = Expr::cell_ref(0, 0); + assert!(matches!(evaluate(&expr, &ctx), Ok(Value::Number(n)) if n == 42.0)); + } + + // ── Value conversion error arms ──────────────────────────────────── + + #[test] + fn value_conversions_propagate_errors() { + let err = Value::Error(FormulaError::DivByZero); + assert!(matches!(err.to_f64(), Err(FormulaError::DivByZero))); + assert!(matches!(err.to_bool(), Err(FormulaError::DivByZero))); + assert_eq!(err.to_display_string(), "#DIV/0!"); + assert!(err.is_error()); + assert!(!Value::Number(1.0).is_error()); + } + + // ── Tokenizer edge branches ──────────────────────────────────────── + + #[test] + fn tokenize_lone_bang_is_an_error() { + assert!(matches!(tokenize("!"), Err(FormulaError::ParseError(_)))); + assert!(matches!(tokenize("5!3"), Err(FormulaError::ParseError(_)))); + } + + #[test] + fn tokenize_string_escape_sequences() { + assert_eq!( + tokenize("\"a\\nb\"").unwrap(), + vec![Token::Str("a\nb".to_string())] + ); + assert_eq!( + tokenize("\"a\\tb\"").unwrap(), + vec![Token::Str("a\tb".to_string())] + ); + assert_eq!( + tokenize("\"a\\\\b\"").unwrap(), + vec![Token::Str("a\\b".to_string())] + ); + } + + #[test] + fn tokenize_double_dot_number_is_rejected() { + // `1.2.3`: the tokenizer consumes `1.2`, hits a second dot, breaks, + // then rejects the stray dot. + assert!(matches!( + tokenize("1.2.3"), + Err(FormulaError::ParseError(_)) + )); + } + + // ── evaluate: boolean literals, unary plus, ranges ───────────────── + + #[test] + fn eval_boolean_literals() { + let ctx = MockCtx::new(); + assert_eq!(eval_mock(&ctx, "TRUE"), "TRUE"); + assert_eq!(eval_mock(&ctx, "FALSE"), "FALSE"); + } + + #[test] + fn eval_unary_plus() { + let mut ctx = MockCtx::new(); + ctx.set(0, 0, "7"); + // The parser collapses `+x` to `x`; the Unary(Pos, ..) node only + // reaches the evaluator when the AST is built directly. + let expr = Expr::unary(UnaryOp::Pos, Expr::number(5.0)); + assert!(matches!(evaluate(&expr, &ctx), Ok(Value::Number(n)) if n == 5.0)); + assert_eq!(eval_mock(&ctx, "+A1"), "7"); + } + + #[test] + fn eval_single_cell_named_range() { + let mut ctx = MockCtx::new(); + ctx.set(0, 0, "99"); + // MockCtx is exact-match, and the tokenizer uppercases identifiers. + ctx.named.insert("SOLO".to_string(), (0, 0, 0, 0)); + assert_eq!(eval_mock(&ctx, "Solo"), "99"); + } + + #[test] + fn eval_range_expression_single_and_multi_cell() { + let mut ctx = MockCtx::new(); + ctx.set(0, 0, "5"); + ctx.set(0, 1, "6"); + // Single-cell range returns that cell. + assert_eq!(eval_mock(&ctx, "A1:A1"), "5"); + // Multi-cell range in expression context returns the first cell. + assert_eq!(eval_mock(&ctx, "A1:B1"), "5"); + } + + #[test] + fn eval_sum_over_unknown_named_range_errors() { + let ctx = MockCtx::new(); + assert_eq!(eval_mock(&ctx, "SUM(Undefined)"), "#NAME?UNDEFINED"); + } + + // ── values_equal: text, boolean, error, mixed ────────────────────── + + #[test] + fn eq_operator_covers_text_boolean_error_and_mixed() { + let mut ctx = MockCtx::new(); + ctx.set(0, 0, "abc"); + ctx.set(0, 1, "ABC"); + // Text equality is case-insensitive. + assert_eq!(eval_mock(&ctx, "A1=B1"), "TRUE"); + // Boolean equality. + assert_eq!(eval_mock(&ctx, "TRUE=TRUE"), "TRUE"); + assert_eq!(eval_mock(&ctx, "TRUE=FALSE"), "FALSE"); + // Mixed types are not equal. + assert_eq!(eval_mock(&ctx, "1=\"1\""), "FALSE"); + // An error operand propagates instead of comparing. + assert_eq!(eval_mock(&ctx, "1/0=1"), "#DIV/0!"); + } + + #[test] + fn values_equal_error_arm_returns_none() { + let num = Value::Number(1.0); + let err = Value::Error(FormulaError::DivByZero); + assert_eq!(values_equal(&err, &num), None); + assert_eq!(values_equal(&num, &err), None); + } + + // ── evaluate_call: SQRT negative, LOG10, SIN/COS/TAN ─────────────── + + #[test] + fn eval_sqrt_negative_is_div_by_zero() { + let ctx = MockCtx::new(); + assert_eq!(eval_mock(&ctx, "SQRT(-4)"), "#DIV/0!"); + } + + #[test] + fn eval_log10_and_trig_functions() { + let ctx = MockCtx::new(); + assert_eq!(eval_mock(&ctx, "LOG10(100)"), "2"); + assert_eq!(eval_mock(&ctx, "SIN(0)"), "0"); + assert_eq!(eval_mock(&ctx, "COS(0)"), "1"); + assert_eq!(eval_mock(&ctx, "TAN(0)"), "0"); + } + + // ── evaluate_if: wrong argument count ────────────────────────────── + + #[test] + fn eval_if_wrong_argument_count_errors() { + let ctx = MockCtx::new(); + assert_eq!( + eval_mock(&ctx, "IF(1)"), + "#VALUE:IF requires 2 or 3 arguments" + ); + } }