diff --git a/crates/core/src/eval/functions/statistical/max/mod.rs b/crates/core/src/eval/functions/statistical/max/mod.rs index 6318ef65b..87e1ad249 100644 --- a/crates/core/src/eval/functions/statistical/max/mod.rs +++ b/crates/core/src/eval/functions/statistical/max/mod.rs @@ -3,7 +3,20 @@ use crate::types::{ErrorKind, Value}; /// `MAX(value1, ...)` — largest numeric value in the arguments. /// Direct args: Numbers, Bool (TRUE=1, FALSE=0), parseable text coerced to number. /// Array elements: Numbers only; text/Bool → skip; errors propagate. -/// Empty array arg → #REF!. No numbers → 0.0. +/// +/// "No numbers" is *two* rules, not one: +/// +/// - an **empty** array argument is `#REF!`: `=MAX({})` — the one rule with +/// an in-repo row (statistical.tsv); +/// - a **populated** array holding nothing numeric is 0: `=MAX({"a","b"})` +/// and `=MAX({TRUE,FALSE})` are both 0, even though neither text nor +/// booleans contribute a number in array context. Captured in Google +/// Sheets; the rows land separately, since they fail until this code exists. +/// +/// Everything else numberless — blanks, dates, zoned instants — is unprobed +/// and keeps the `#REF!` MAX has always given it. `array_had_content` is set +/// by text and booleans alone, so it exempts exactly what was captured and +/// makes no claim past it. pub fn max_fn(args: &[Value]) -> Value { if args.is_empty() { return Value::Error(ErrorKind::NA); @@ -15,6 +28,7 @@ pub fn max_fn(args: &[Value]) -> Value { } let mut result: Option = None; let mut had_array = false; + let mut array_had_content = false; let mut skipped_sparkline = false; for arg in args { match arg { @@ -44,7 +58,12 @@ pub fn max_fn(args: &[Value]) -> Value { // Recurse into nested arrays (e.g. a vertical range // materializes as nested one-element row arrays) so every // cell is visited. - if let Err(e) = max_array_into(elems, &mut result, &mut skipped_sparkline) { + if let Err(e) = max_array_into( + elems, + &mut result, + &mut skipped_sparkline, + &mut array_had_content, + ) { return e; } } @@ -54,14 +73,19 @@ pub fn max_fn(args: &[Value]) -> Value { } } // A skipped sparkline is not "nothing usable": the aggregate had something - // in scope, so it answers 0 rather than falling into the numberless-array - // rule below (google.tsv: `=MAX(Data!K1:K1)` is 0). Scoped to a sparkline - // so `=MAX({"a"})` and friends keep their pre-existing `#REF!`. + // in scope, so it answers 0 rather than falling into the rule below + // (google.tsv: `=MAX(Data!K1:K1)` is 0). This runs first, so it decides + // every sparkline case before `array_had_content` is consulted at all. if skipped_sparkline && result.is_none() { return Value::Number(0.0); } - // Empty array with no numbers → Ref - if had_array && result.is_none() { + // An array holding text or booleans answers 0 (`=MAX({"a","b"})` and + // `=MAX({TRUE,FALSE})` are both 0). `array_had_content` is set by exactly + // those two variants and nothing else, so every other numberless array — + // blanks, dates, zoned instants — keeps the long-standing #REF!. Those are + // unprobed; the flag exempts what the capture covers and makes no claim + // beyond it. + if had_array && !array_had_content && result.is_none() { return Value::Error(ErrorKind::Ref); } Value::Number(result.unwrap_or(0.0)) @@ -74,20 +98,29 @@ pub fn max_fn(args: &[Value]) -> Value { /// direct argument or through a range (google.tsv: `=MAX(SPARKLINE({1,2,3}))` /// and `=MAX(Data!K1:K1)` are both 0). The flag is what distinguishes "skipped a /// sparkline" from "saw nothing usable at all", which stay different answers. +/// +/// `had_content` is set by text and booleans *only* — the two variants the +/// capture covers (`=MAX({"a","b"})` and `=MAX({TRUE,FALSE})` are both 0). +/// It is deliberately not a catch-all: every other non-numeric variant, most +/// notably `Date`, leaves it alone and so keeps the `#REF!` MAX has always +/// answered. Listing the variants rather than falling through also stops a +/// future `Value` kind inheriting content-hood by accident. fn max_array_into( elems: &[Value], result: &mut Option, skipped_sparkline: &mut bool, + had_content: &mut bool, ) -> Result<(), Value> { for elem in elems { match elem { Value::Number(n) => { *result = Some(result.map_or(*n, |cur: f64| cur.max(*n))); } + Value::Text(_) | Value::Bool(_) => *had_content = true, Value::Sparkline(_) => *skipped_sparkline = true, Value::Error(e) => return Err(Value::Error(e.clone())), Value::ErrorMsg(e, m) => return Err(Value::ErrorMsg(e.clone(), m.clone())), - Value::Array(inner) => max_array_into(inner, result, skipped_sparkline)?, + Value::Array(inner) => max_array_into(inner, result, skipped_sparkline, had_content)?, _ => {} } } diff --git a/crates/core/src/eval/functions/statistical/max/tests/edge.rs b/crates/core/src/eval/functions/statistical/max/tests/edge.rs index 2026974bf..739698faf 100644 --- a/crates/core/src/eval/functions/statistical/max/tests/edge.rs +++ b/crates/core/src/eval/functions/statistical/max/tests/edge.rs @@ -14,6 +14,88 @@ fn max_text_in_args_returns_value_error() { ); } +#[test] +fn max_empty_array_is_ref_error() { + assert_eq!( + max_fn(&[Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn max_non_empty_array_without_numbers_returns_zero() { + // `=MAX({"a","b"})` and `=MAX({TRUE,FALSE})` are both 0: neither text nor + // booleans contribute a number in array context, but both are enough to + // lift the array out of the #REF! rule. These two variants are the whole + // of the carve-out — nothing else sets `had_content`. + assert_eq!( + max_fn(&[Value::Array(vec![ + Value::Text("a".to_string()), + Value::Text("b".to_string()), + ])]), + Value::Number(0.0) + ); + assert_eq!( + max_fn(&[Value::Array(vec![Value::Bool(true), Value::Bool(false)])]), + Value::Number(0.0) + ); +} + +#[test] +fn max_array_of_only_blanks_is_unchanged_at_ref_error() { + // No captured row covers an all-blank array, so MAX keeps the #REF! it + // has always given. Pinned here because narrowing the rule for text and + // booleans must not disturb this case. + assert_eq!( + max_fn(&[Value::Array(vec![Value::Empty, Value::Empty, Value::Empty])]), + Value::Error(ErrorKind::Ref) + ); + assert_eq!( + max_fn(&[Value::Array(vec![ + Value::Array(vec![Value::Empty]), + Value::Array(vec![Value::Empty]), + ])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn max_array_of_only_dates_is_unchanged_at_ref_error() { + // `max_array_into` folds only `Value::Number`, so a date-only array has + // never produced a result and has always answered #REF!. That is almost + // certainly wrong against Sheets — but it is pre-existing, unprobed, and + // must not be quietly turned into a plausible-looking 0 by the + // text-and-boolean carve-out. `had_content` is set by text and booleans + // only, never by a catch-all, and this pins that. + assert_eq!( + max_fn(&[Value::Array(vec![ + Value::Date(43831.0), + Value::Date(44197.0) + ])]), + Value::Error(ErrorKind::Ref) + ); + // Same for a date arriving through a nested-row range materialization. + assert_eq!( + max_fn(&[Value::Array(vec![Value::Array(vec![Value::Date(43831.0)])])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn max_one_non_blank_lifts_the_array_out_of_the_ref_rule() { + assert_eq!( + max_fn(&[Value::Array(vec![ + Value::Empty, + Value::Text("z".to_string()), + ])]), + Value::Number(0.0) + ); + assert_eq!( + max_fn(&[Value::Array(vec![Value::Empty, Value::Number(4.0)])]), + Value::Number(4.0) + ); +} + #[test] fn max_negative_numbers() { assert_eq!( diff --git a/crates/core/src/eval/functions/statistical/maxa/mod.rs b/crates/core/src/eval/functions/statistical/maxa/mod.rs index 9756b7f3c..43d10a279 100644 --- a/crates/core/src/eval/functions/statistical/maxa/mod.rs +++ b/crates/core/src/eval/functions/statistical/maxa/mod.rs @@ -5,6 +5,7 @@ use crate::types::{ErrorKind, Value}; /// - Booleans coerced: TRUE=1, FALSE=0. /// - Text in direct args → `#VALUE!`. /// - Empty → skip. +/// - Empty array argument → `#REF!`. /// - No args → `#N/A`. pub fn maxa_fn(args: &[Value]) -> Value { if args.is_empty() { @@ -26,6 +27,14 @@ pub fn maxa_fn(args: &[Value]) -> Value { Value::Text(_) => return Value::Error(ErrorKind::Value), Value::Empty => {} Value::Array(inner) => { + // An empty argument is #REF!, as it is for MIN and MAX + // (`=MAXA({})`). Note MAXA reaches the *other* answers by a + // different route: text folds in as 0 rather than being + // skipped, so `=MAXA({"a","b"})` is already 0 without any + // "populated but numberless" rule. + if inner.is_empty() { + return Value::Error(ErrorKind::Ref); + } // In array context: Numbers included, Bool→1/0, Text→0, Empty→skip. // Recurses into nested arrays (e.g. a vertical range // materializes as nested one-element row arrays). diff --git a/crates/core/src/eval/functions/statistical/maxa/tests/edge.rs b/crates/core/src/eval/functions/statistical/maxa/tests/edge.rs index 894de41d1..a1634bcef 100644 --- a/crates/core/src/eval/functions/statistical/maxa/tests/edge.rs +++ b/crates/core/src/eval/functions/statistical/maxa/tests/edge.rs @@ -1,5 +1,5 @@ use super::super::maxa_fn; -use crate::types::Value; +use crate::types::{ErrorKind, Value}; #[test] fn empty_values_skipped() { @@ -37,3 +37,31 @@ fn bool_and_number_mixed() { Value::Number(10.0) ); } + +#[test] +fn maxa_empty_array_is_ref_error() { + // `=MAXA({})` is #REF!, as it is for MIN and MAX. Reached by the + // empty-argument check alone: text folds in as 0 here, so a populated + // array is never numberless in the first place. + assert_eq!( + maxa_fn(&[Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); + assert_eq!( + maxa_fn(&[Value::Number(1.0), Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn maxa_text_only_array_is_zero() { + // `=MAXA({"a","b"})` is 0 — text counts as zero rather than being + // skipped, so this needs no separate rule. + assert_eq!( + maxa_fn(&[Value::Array(vec![ + Value::Text("a".to_string()), + Value::Text("b".to_string()), + ])]), + Value::Number(0.0) + ); +} diff --git a/crates/core/src/eval/functions/statistical/min/mod.rs b/crates/core/src/eval/functions/statistical/min/mod.rs index f7bfbb501..6122d9fab 100644 --- a/crates/core/src/eval/functions/statistical/min/mod.rs +++ b/crates/core/src/eval/functions/statistical/min/mod.rs @@ -3,7 +3,20 @@ use crate::types::{ErrorKind, Value}; /// `MIN(value1, ...)` — smallest numeric value in the arguments. /// Direct args: Numbers, Bool (TRUE=1, FALSE=0), parseable text coerced to number. /// Array elements: Numbers only; text/Bool → skip; errors propagate. -/// No numbers → 0.0. +/// +/// "No numbers" is *two* rules, not one: +/// +/// - an **empty** array argument is `#REF!`: `=MIN({})`. Captured in Google +/// Sheets; the row lands separately, since it fails until this code exists. +/// - a **populated** array holding nothing numeric is 0. The in-repo evidence +/// is indirect but sufficient: statistical.tsv pins +/// `=IFERROR(MIN({"a","b","c"}),"no numbers")` to the *number* 0, so MIN +/// cannot have errored. A direct `=MIN({"a","b"})` row is captured and +/// lands with the others. +/// +/// MIN needs no code for the second rule — it already falls through to 0. +/// An array holding only *blanks* is a third case, unprobed, left at the 0 +/// MIN has always given it. pub fn min_fn(args: &[Value]) -> Value { if args.is_empty() { return Value::Error(ErrorKind::NA); @@ -34,6 +47,13 @@ pub fn min_fn(args: &[Value]) -> Value { } Value::Empty => {} Value::Array(elems) => { + // An explicitly empty argument is fatal on the spot, even if a + // number was already in hand — matching MAX, whose + // `=MAX(SPARKLINE({1,2,3}),{})` row is #REF! despite the + // sparkline that would otherwise answer 0. + if elems.is_empty() { + return Value::Error(ErrorKind::Ref); + } // Recurse into nested arrays (e.g. a vertical range // materializes as nested one-element row arrays) so every // cell is visited. diff --git a/crates/core/src/eval/functions/statistical/min/tests/edge.rs b/crates/core/src/eval/functions/statistical/min/tests/edge.rs index a24575726..9852387be 100644 --- a/crates/core/src/eval/functions/statistical/min/tests/edge.rs +++ b/crates/core/src/eval/functions/statistical/min/tests/edge.rs @@ -14,6 +14,65 @@ fn min_text_in_args_returns_value_error() { ); } +#[test] +fn min_empty_array_is_ref_error() { + // Matches MAX and Google Sheets: an empty array argument is #REF!, + // not a silent 0. + assert_eq!( + min_fn(&[Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn min_empty_array_beside_a_number_is_ref_error() { + assert_eq!( + min_fn(&[Value::Number(1.0), Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); + assert_eq!( + min_fn(&[Value::Array(vec![]), Value::Number(1.0)]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn min_non_empty_array_without_numbers_still_returns_zero() { + // Distinct from the absent-argument rule: the fixtures pin + // `=MIN({"a","b"})` and `=IFERROR(MIN({"a","b","c"}),"no numbers")` to + // the number 0, so a populated-but-numberless array must not become #REF!. + assert_eq!( + min_fn(&[Value::Array(vec![ + Value::Text("a".to_string()), + Value::Text("b".to_string()), + ])]), + Value::Number(0.0) + ); + // Booleans are skipped in array context but still make the array present. + assert_eq!( + min_fn(&[Value::Array(vec![Value::Bool(true), Value::Bool(false)])]), + Value::Number(0.0) + ); +} + +#[test] +fn min_array_of_only_blanks_is_unchanged_at_zero() { + // No captured row covers an all-blank array, so MIN keeps the 0 it has + // always given. Pinned here so a future change to it has to be deliberate + // rather than a side effect of the empty-array rule above. + assert_eq!( + min_fn(&[Value::Array(vec![Value::Empty, Value::Empty, Value::Empty])]), + Value::Number(0.0) + ); + assert_eq!( + min_fn(&[Value::Array(vec![ + Value::Array(vec![Value::Empty]), + Value::Array(vec![Value::Empty]), + ])]), + Value::Number(0.0) + ); +} + #[test] fn min_negative_numbers() { assert_eq!( diff --git a/crates/core/src/eval/functions/statistical/mina/mod.rs b/crates/core/src/eval/functions/statistical/mina/mod.rs index 1d4ab3680..63449e413 100644 --- a/crates/core/src/eval/functions/statistical/mina/mod.rs +++ b/crates/core/src/eval/functions/statistical/mina/mod.rs @@ -5,6 +5,7 @@ use crate::types::{ErrorKind, Value}; /// - Booleans coerced: TRUE=1, FALSE=0. /// - Text in direct args → `#VALUE!`. /// - Empty → skip. +/// - Empty array argument → `#REF!`. /// - No args → `#N/A`. pub fn mina_fn(args: &[Value]) -> Value { if args.is_empty() { @@ -26,6 +27,14 @@ pub fn mina_fn(args: &[Value]) -> Value { Value::Text(_) => return Value::Error(ErrorKind::Value), Value::Empty => {} Value::Array(inner) => { + // An empty argument is #REF!, as it is for MIN and MAX + // (`=MINA({})`). Note MINA reaches the *other* answers by a + // different route: text folds in as 0 rather than being + // skipped, so `=MINA({"a","b"})` is already 0 without any + // "populated but numberless" rule. + if inner.is_empty() { + return Value::Error(ErrorKind::Ref); + } // In array context: Numbers included, Bool→1/0, Text→0, Empty→skip. // Recurses into nested arrays (e.g. a vertical range // materializes as nested one-element row arrays). diff --git a/crates/core/src/eval/functions/statistical/mina/tests/edge.rs b/crates/core/src/eval/functions/statistical/mina/tests/edge.rs index cf7c44d39..09b256883 100644 --- a/crates/core/src/eval/functions/statistical/mina/tests/edge.rs +++ b/crates/core/src/eval/functions/statistical/mina/tests/edge.rs @@ -1,5 +1,5 @@ use super::super::mina_fn; -use crate::types::Value; +use crate::types::{ErrorKind, Value}; #[test] fn empty_values_skipped() { @@ -37,3 +37,31 @@ fn bool_and_number_mixed() { Value::Number(0.0) ); } + +#[test] +fn mina_empty_array_is_ref_error() { + // `=MINA({})` is #REF!, as it is for MIN and MAX. Reached by the + // empty-argument check alone: text folds in as 0 here, so a populated + // array is never numberless in the first place. + assert_eq!( + mina_fn(&[Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); + assert_eq!( + mina_fn(&[Value::Number(1.0), Value::Array(vec![])]), + Value::Error(ErrorKind::Ref) + ); +} + +#[test] +fn mina_text_only_array_is_zero() { + // `=MINA({"a","b"})` is 0 — text counts as zero rather than being + // skipped, so this needs no separate rule. + assert_eq!( + mina_fn(&[Value::Array(vec![ + Value::Text("a".to_string()), + Value::Text("b".to_string()), + ])]), + Value::Number(0.0) + ); +} diff --git a/crates/core/tests/sparkline.rs b/crates/core/tests/sparkline.rs index 916423f3a..98782ba17 100644 --- a/crates/core/tests/sparkline.rs +++ b/crates/core/tests/sparkline.rs @@ -738,12 +738,13 @@ fn an_empty_array_argument_outranks_the_sparkline_skip() { eval("=MAX(SPARKLINE({1,2,3}),{\"a\"})"), Value::Number(0.0) ); - // MIN's counterpart row (`=MIN(SPARKLINE({1,2,3}),{})` → #REF!) is a known - // divergence recorded in bugs.tsv: MIN has no empty-array rule at all, so - // it answers 0 — for `=MIN({})` too, with no sparkline in sight. Fixing - // that would move MIN for inputs unrelated to this work. - assert_eq!(eval("=MIN(SPARKLINE({1,2,3}),{})"), Value::Number(0.0)); - assert_eq!(eval("=MIN({})"), Value::Number(0.0)); + // bugs.tsv: `=MIN(SPARKLINE({1,2,3}),{})` is #REF! — a live Google Sheets + // value this engine used to miss. MIN now carries the same empty-array + // rule, so its counterpart row agrees with MAX's. + assert_eq!( + eval("=MIN(SPARKLINE({1,2,3}),{})"), + Value::Error(ErrorKind::Ref) + ); } // ── Registry surface ────────────────────────────────────────────────────────