From 0b4cc5618f5c5a651cb9aa643896bdb1c3da40d0 Mon Sep 17 00:00:00 2001 From: Eduardo Garcia Date: Thu, 15 Oct 2026 11:20:26 -0300 Subject: [PATCH] fix #1458: one Excel sheet-name quoting rule for formulas, pivot sources and ADDRESS Sheet names starting with '.' or reading as R1C1 (R, C, RC, R1C1...) were saved unquoted, so Excel could not open the file; PivotTable sources and ADDRESS left cell-like names (A1) bare. The quoting rule, unquoting and sheet splitting now live once in gridcraft_core::addr and replace the copies in the formula printer, pivot, ADDRESS, chart, xlsx, Solver, Go To and the Name Box (which now reads '' as '). Co-Authored-By: Claude Sonnet 5.5 --- crates/calc/src/eval.rs | 2 +- crates/chart/src/resolve.rs | 2 +- crates/core/src/addr.rs | 97 +++++++++++++++++++++++ crates/core/src/lib.rs | 2 +- crates/engine/src/cmd/edit.rs | 2 +- crates/engine/src/cmd/mod.rs | 8 +- crates/engine/src/cmd/pivot.rs | 2 +- crates/engine/src/pivot.rs | 25 +----- crates/engine/tests/sheet_name_quoting.rs | 51 ++++++++++++ crates/formula/src/printer.rs | 17 +--- crates/functions/src/lookup.rs | 19 +++-- crates/ui-egui/src/lib.rs | 2 +- crates/ui-egui/tests/interaction.rs | 13 +++ crates/xlsx/src/array_formula.rs | 2 +- crates/xlsx/src/pivot.rs | 5 +- 15 files changed, 187 insertions(+), 62 deletions(-) create mode 100644 crates/engine/tests/sheet_name_quoting.rs diff --git a/crates/calc/src/eval.rs b/crates/calc/src/eval.rs index 0f9cfb03d..dec60b426 100644 --- a/crates/calc/src/eval.rs +++ b/crates/calc/src/eval.rs @@ -262,7 +262,7 @@ impl<'h> Evaluator<'h> { let wb = self.host.workbook(); // Sheet-qualified `Sheet1!Name`. let (scope_sheet, bare) = match n.split_once('!') { - Some((s, b)) => match wb.sheet_index(s.trim_matches('\'')) { + Some((s, b)) => match wb.sheet_index(&gridcraft_core::unquote_sheet(s)) { Some(i) => (i, b), None => return err(CellError::Name), }, diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index c007e3e90..13b7aa5c1 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -41,7 +41,7 @@ fn parse_ref(wb: &Workbook, sheet: usize, formula: &str) -> Option<(usize, Range let (sh, addr) = match f.rfind('!') { Some(i) => { let name = f.get(..i)?.trim(); - let name = name.strip_prefix('\'').and_then(|n| n.strip_suffix('\'')).map(|n| n.replace("''", "'")).unwrap_or_else(|| name.to_string()); + let name = gridcraft_core::unquote_sheet(name); (wb.sheet_index(&name)?, f.get(i + 1..)?) } None => (sheet, f), diff --git a/crates/core/src/addr.rs b/crates/core/src/addr.rs index 2c087f22e..1ef9bc6b8 100644 --- a/crates/core/src/addr.rs +++ b/crates/core/src/addr.rs @@ -237,6 +237,59 @@ impl fmt::Display for RangeRef { } } +/// Whether `name` reads as an R1C1 reference: `R`, `C`, `RC`, `R1`, `C1`, `R1C`, `RC1`, `R1C1` +/// in any case. +fn is_r1c1_token(name: &str) -> bool { + let mut chars = name.chars().peekable(); + let mut letters = 0; + for letter in ['r', 'c'] { + if chars.peek().is_some_and(|c| c.to_ascii_lowercase() == letter) { + chars.next(); + letters += 1; + while chars.peek().is_some_and(|c| c.is_ascii_digit()) { + chars.next(); + } + } + } + letters > 0 && chars.next().is_none() +} + +/// Whether Excel quotes a sheet name in a reference (`'My Sheet'!A1`): it starts with a digit or +/// `.`, holds anything but letters (any script), digits, `_` and `.`, reads as an A1 cell within +/// the sheet or as an R1C1 reference, or is a boolean. +pub fn sheet_needs_quotes(name: &str) -> bool { + let Some(first) = name.chars().next() else { + return true; + }; + if first.is_ascii_digit() || first == '.' || name.chars().any(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) { + return true; + } + CellRef::parse(name).is_some() || is_r1c1_token(name) || name.eq_ignore_ascii_case("TRUE") || name.eq_ignore_ascii_case("FALSE") +} + +/// A sheet name as it is written in a reference: quoted, with `'` doubled, when Excel quotes it. +pub fn quote_sheet(name: &str) -> String { + if sheet_needs_quotes(name) { format!("'{}'", name.replace('\'', "''")) } else { name.to_string() } +} + +/// The inverse of [`quote_sheet`]: takes the quotes off a sheet name and reads `''` as `'`. +/// Text that isn't quoted is returned as it is. +pub fn unquote_sheet(text: &str) -> String { + match text.trim().strip_prefix('\'').and_then(|t| t.strip_suffix('\'')) { + Some(inner) => inner.replace("''", "'"), + None => text.trim().to_string(), + } +} + +/// `'My Sheet'!A1` → (`Some("My Sheet")`, `A1`). Splits at the last `!`, so a quoted name may +/// hold one (`'a!b'!A1`). +pub fn split_sheet(reference: &str) -> (Option, &str) { + match reference.rsplit_once('!') { + Some((sheet, body)) => (Some(unquote_sheet(sheet)), body), + None => (None, reference), + } +} + #[cfg(test)] mod tests { use super::*; @@ -253,6 +306,50 @@ mod tests { assert_eq!((r.start.row, r.end.row), (0, 2)); } + #[test] + fn sheet_quoting_follows_excel() { + for quoted in [ + "A1", + "XFD1", + "AB1048576", + "R1", + "C1", + "RC1", + "R", + "C", + "RC", + "R1C", + "R1C1", + "r2c3", + "CR1", + ".x", + "TRUE", + "false", + "1abc", + "My Sheet", + "Bob's", + "a-b", + "", + ] { + assert!(sheet_needs_quotes(quoted), "{quoted:?} should be quoted"); + } + for bare in ["Sheet1", "XFE1", "AB1048577", "Ärger", "日本", "Σ", "Sheet.1", "_x", "A1A", "Data", "RR", "CRX"] { + assert!(!sheet_needs_quotes(bare), "{bare:?} should stay bare"); + } + } + + #[test] + fn quote_unquote_and_split_sheet() { + assert_eq!(quote_sheet("Bob's"), "'Bob''s'"); + assert_eq!(quote_sheet("Data"), "Data"); + assert_eq!(unquote_sheet("'Bob''s'"), "Bob's"); + assert_eq!(unquote_sheet("Data"), "Data"); + assert_eq!(unquote_sheet("'"), "'"); + assert_eq!(split_sheet("'a!b'!A1"), (Some("a!b".to_string()), "A1")); + assert_eq!(split_sheet("'Bob''s'!$A$1:$B$2"), (Some("Bob's".to_string()), "$A$1:$B$2")); + assert_eq!(split_sheet("A1"), (None, "A1")); + } + #[test] fn letters_roundtrip() { for (c, s) in [(0, "A"), (25, "Z"), (26, "AA"), (51, "AZ"), (52, "BA"), (701, "ZZ"), (702, "AAA"), (16383, "XFD")] { diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index d5adbc07e..833dd06e2 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -8,6 +8,6 @@ pub mod date; pub mod parse; pub mod value; -pub use addr::{CellRef, MAX_COLS, MAX_ROWS, RangeRef, col_to_letters, letters_to_col}; +pub use addr::{CellRef, MAX_COLS, MAX_ROWS, RangeRef, col_to_letters, letters_to_col, quote_sheet, sheet_needs_quotes, split_sheet, unquote_sheet}; pub use date::DateSystem; pub use value::{Array, CellError, Value, compare, compare_numbers, compare_text, number_to_text, sort_compare}; diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index fe7f1fe8b..353953852 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -525,7 +525,7 @@ pub(super) fn go_to(s: &mut Session, p: &Json) -> Result { } else if let Some((si, ti)) = d.wb.table(reference) { // As in Excel, a table's name stands for its data, without the header and totals rows. let t = d.wb.sheet(si).and_then(|sh| sh.tables.get(ti)).map(|t| t.data_range().unwrap_or(t.range).a1()).unwrap_or_default(); - format!("'{}'!{}", d.wb.sheet(si).map(|s| s.name.clone()).unwrap_or_default(), t) + format!("{}!{}", gridcraft_core::quote_sheet(&d.wb.sheet(si).map(|s| s.name.clone()).unwrap_or_default()), t) } else { return Err(EngineError::Other("Reference isn't valid.".into())); } diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 50865a2b7..e18bae20b 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -269,13 +269,7 @@ pub(crate) fn target_sheet(s: &Session, p: &Json) -> Result { Ok(d.wb.active_sheet) } -/// `'My Sheet'!A1` → (`Some("My Sheet")`, `A1`). -pub(crate) fn split_sheet(r: &str) -> (Option, &str) { - match r.rsplit_once('!') { - Some((sh, body)) => (Some(sh.trim_matches('\'').replace("''", "'")), body), - None => (None, r), - } -} +pub(crate) use gridcraft_core::split_sheet; /// A single-cell parameter. Absent or null is `Ok(None)` (callers default it); a value that is not /// a cell reference, or that names a sheet other than the active one, is an error rather than a diff --git a/crates/engine/src/cmd/pivot.rs b/crates/engine/src/cmd/pivot.rs index 06050f7e8..6205a510a 100644 --- a/crates/engine/src/cmd/pivot.rs +++ b/crates/engine/src/cmd/pivot.rs @@ -1392,7 +1392,7 @@ fn chart_pivot(s: &Session, p: &Json) -> Result<(usize, usize)> { let (sheet, addr) = match t.rsplit_once('!') { Some((sh, a)) => { let sh = sh.trim(); - let sh = sh.strip_prefix('\'').and_then(|x| x.strip_suffix('\'')).map(|x| x.replace("''", "'")).unwrap_or_else(|| sh.to_string()); + let sh = gridcraft_core::unquote_sheet(sh); (d.wb.sheet_index(&sh), a) } None => (Some(d.wb.active_sheet), t), diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 6146bd4f9..eaa8ab4db 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -9,7 +9,8 @@ use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; use gridcraft_core::date::datetime_from_serial; -use gridcraft_core::{CellError, CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value}; +pub use gridcraft_core::quote_sheet; +use gridcraft_core::{CellError, CellRef, MAX_COLS, MAX_ROWS, RangeRef, Value, split_sheet}; use gridcraft_model::*; use crate::pivot_calc::CalcSet; @@ -47,26 +48,6 @@ impl Source { } } -/// `'My Sheet'!A1` → (`Some("My Sheet")`, `A1`). -fn split_sheet(r: &str) -> (Option, &str) { - match r.rsplit_once('!') { - Some((sh, body)) => (Some(sh.trim_matches('\'').replace("''", "'")), body), - None => (None, r), - } -} - -/// Quotes a sheet name for a reference when needed. -pub fn quote_sheet(name: &str) -> String { - if !name.is_empty() - && name.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '.') - && !name.chars().next().is_some_and(|c| c.is_ascii_digit()) - { - name.to_string() - } else { - format!("'{}'", name.replace('\'', "''")) - } -} - /// Absolute reference text for a range on a sheet: `Sheet1!$A$1:$E$20`. pub fn range_text(sheet_name: &str, r: RangeRef) -> String { let abs = |c: CellRef| format!("${}${}", gridcraft_core::col_to_letters(c.col), c.row as u64 + 1); @@ -1511,7 +1492,7 @@ pub fn parse_chart_link(link: &str) -> Option<(String, String)> { }; let (sheet, name) = t.rsplit_once('!')?; let sheet = sheet.trim(); - let sheet = sheet.strip_prefix('\'').and_then(|x| x.strip_suffix('\'')).map(|x| x.replace("''", "'")).unwrap_or_else(|| sheet.to_string()); + let sheet = gridcraft_core::unquote_sheet(sheet); let name = name.trim(); if sheet.is_empty() || name.is_empty() { return None; diff --git a/crates/engine/tests/sheet_name_quoting.rs b/crates/engine/tests/sheet_name_quoting.rs new file mode 100644 index 000000000..c7ba317bb --- /dev/null +++ b/crates/engine/tests/sheet_name_quoting.rs @@ -0,0 +1,51 @@ +//! #1458: sheet names are quoted in references the way Excel quotes them, in formulas, saved +//! files, PivotTable sources and ADDRESS. + +use gridcraft_engine::Session; +use gridcraft_engine::core::{CellRef, Value}; +use serde_json::{Value as Json, json}; + +fn run(s: &mut Session, command: &str, params: Json) -> Json { + s.execute(command, params).unwrap() +} + +fn formula(s: &Session, cell: &str) -> String { + s.doc().unwrap().wb.active().unwrap().cell(CellRef::parse(cell).unwrap()).unwrap().formula.as_ref().unwrap().text.clone() +} + +#[test] +fn formulas_keep_the_quotes_excel_needs_through_a_save() { + let mut s = Session::new(); + s.new_workbook(); + for name in [".x", "R1C1", "A1", "Bob's"] { + run(&mut s, "home.insertSheet", json!({"name": name})); + } + run(&mut s, "sheet.activate", json!({"sheet": "Sheet1"})); + let input = "='.x'!A1+'R1C1'!A1+'A1'!B1+'Bob''s'!A1"; + run(&mut s, "cell.set", json!({"cell": "A1", "input": input})); + assert_eq!(formula(&s, "A1"), input.trim_start_matches('=')); + + let saved = run(&mut s, "file.saveBytes", json!({"format": "xlsx"})); + let mut reopened = Session::new(); + run(&mut reopened, "file.open", json!({"name": "q.xlsx", "base64": saved["base64"]})); + run(&mut reopened, "sheet.activate", json!({"sheet": "Sheet1"})); + assert_eq!(formula(&reopened, "A1"), input.trim_start_matches('=')); +} + +#[test] +fn address_and_pivot_source_quote_a_sheet_named_like_a_cell() { + let mut s = Session::new(); + s.new_workbook(); + run(&mut s, "home.insertSheet", json!({"name": "A1"})); + run(&mut s, "sheet.activate", json!({"sheet": "A1"})); + for (cell, input) in [("A1", "K"), ("B1", "V"), ("A2", "a"), ("B2", "1"), ("A3", "b"), ("B3", "2")] { + run(&mut s, "cell.set", json!({"cell": cell, "input": input})); + } + run(&mut s, "cell.set", json!({"cell": "D1", "input": "=ADDRESS(1,1,1,TRUE,\"A1\")"})); + let d1 = s.doc().unwrap().wb.active().unwrap().value(CellRef::parse("D1").unwrap()); + assert_eq!(d1, Value::from("'A1'!$A$1")); + + run(&mut s, "selection.set", json!({"range": "A2"})); + let pivot = run(&mut s, "insert.pivotTable", json!({"rows": ["K"], "values": ["V"]})); + assert_eq!(pivot["source"], json!("'A1'!$A$1:$B$3")); +} diff --git a/crates/formula/src/printer.rs b/crates/formula/src/printer.rs index 3975a7c61..055b0f145 100644 --- a/crates/formula/src/printer.rs +++ b/crates/formula/src/printer.rs @@ -6,22 +6,7 @@ use gridcraft_core::{CellRef, col_to_letters, number_to_text}; use crate::ast::*; -/// Whether a sheet name needs quotes in a reference. -pub fn needs_quotes(name: &str) -> bool { - if name.is_empty() { - return true; - } - let first = name.chars().next().unwrap_or('_'); - if first.is_ascii_digit() || name.chars().any(|c| !(c.is_alphanumeric() || c == '_' || c == '.')) { - return true; - } - // Looks like a cell ref (`A1`, `R1C1`) or a boolean. - gridcraft_core::CellRef::parse(name).is_some() || name.eq_ignore_ascii_case("TRUE") || name.eq_ignore_ascii_case("FALSE") -} - -pub fn quote_sheet(name: &str) -> String { - if needs_quotes(name) { format!("'{}'", name.replace('\'', "''")) } else { name.to_string() } -} +pub use gridcraft_core::{quote_sheet, sheet_needs_quotes as needs_quotes}; fn sheet_prefix(s: &SheetSel) -> String { match s { diff --git a/crates/functions/src/lookup.rs b/crates/functions/src/lookup.rs index 56739c933..f9c0b6f44 100644 --- a/crates/functions/src/lookup.rs +++ b/crates/functions/src/lookup.rs @@ -413,15 +413,15 @@ fn transpose(args: &[Arg], _c: &mut dyn Ctx) -> R { } } -/// Sheet text for ADDRESS, quoted when it needs to be. A `[Book]Sheet` prefix is judged by its +/// Sheet text for ADDRESS, quoted when Excel quotes it. A `[Book]Sheet` prefix is judged by its /// parts (Microsoft's ADDRESS examples: `"[Book1]Sheet1"` stays bare, `"EXCEL SHEET"` becomes -/// `'EXCEL SHEET'`). +/// `'EXCEL SHEET'`). Unlike in a formula, `TRUE` and `FALSE` stay bare. fn quote_sheet(s: &str) -> String { - let word = |p: &str| p.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '.'); - let bare_sheet = |p: &str| !p.is_empty() && word(p) && !p.chars().next().is_some_and(|c| c.is_ascii_digit()); + let bare = |p: &str| p.eq_ignore_ascii_case("TRUE") || p.eq_ignore_ascii_case("FALSE") || !gridcraft_core::sheet_needs_quotes(p); + let word = |p: &str| !p.is_empty() && p.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '.'); let plain = match s.strip_prefix('[').and_then(|r| r.split_once(']')) { - Some((book, sheet)) => !book.is_empty() && word(book) && bare_sheet(sheet), - None => bare_sheet(s), + Some((book, sheet)) => word(book) && bare(sheet), + None => bare(s), }; if plain { s.to_string() } else { format!("'{}'", s.replace('\'', "''")) } } @@ -791,6 +791,13 @@ mod tests { for (r, c, abs, a1, sheet, want) in cases { is_text(ev("ADDRESS", vec![n(r), n(c), n(abs), b(a1), t(sheet)]), want); } + // Sheet names that read as a cell or an R1C1 reference, or start with a dot, are quoted; + // off-sheet cells and booleans are not (#1458). + for (sheet, want) in + [("A1", "'A1'!$A$1"), ("R1C1", "'R1C1'!$A$1"), ("RC", "'RC'!$A$1"), (".x", "'.x'!$A$1"), ("XFE1", "XFE1!$A$1"), ("TRUE", "TRUE!$A$1")] + { + is_text(ev("ADDRESS", vec![n(1.0), n(1.0), n(1.0), b(true), t(sheet)]), want); + } is_text(ev("ADDRESS", vec![n(1048576.0), n(16384.0)]), "$XFD$1048576"); is_err(ev("ADDRESS", vec![n(0.0), n(1.0)]), CellError::Value); is_err(ev("ADDRESS", vec![n(1.0), n(16385.0)]), CellError::Value); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index f549aec2b..890ad6f62 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1264,7 +1264,7 @@ impl SheetApp { for n in &d.wb.names { let target = n.formula.replace('$', ""); let (sheet, body) = match target.rsplit_once('!') { - Some((s, b)) => (Some(s.trim_matches('\'').to_string()), b.to_string()), + Some((s, b)) => (Some(gridcraft_engine::core::unquote_sheet(s)), b.to_string()), None => (None, target.clone()), }; if sheet.as_deref().is_none_or(|s| s.eq_ignore_ascii_case(&sh.name)) && RangeRef::parse(&body) == Some(cur) { diff --git a/crates/ui-egui/tests/interaction.rs b/crates/ui-egui/tests/interaction.rs index 02084ffb8..e316f4830 100644 --- a/crates/ui-egui/tests/interaction.rs +++ b/crates/ui-egui/tests/interaction.rs @@ -227,6 +227,19 @@ fn name_box_enter_goes_to_the_typed_cell_not_the_one_below() { assert_eq!(h.state().session.active().unwrap().selection.active, CellRef::new(1, 0)); } +#[test] +fn name_box_shows_a_name_defined_on_a_sheet_with_an_apostrophe() { + // #1458: the quotes of `'Bob''s'!$A$1:$B$2` were taken off without reading `''` as `'`. + let mut h = harness(blank()); + let session = &mut h.state_mut().session; + session.run("home.insertSheet", json!({"name": "Bob's"})).unwrap(); + session.run("sheet.activate", json!({"sheet": "Bob's"})).unwrap(); + session.run("formulas.defineName", json!({"name": "Box", "refersTo": "='Bob''s'!$A$1:$B$2"})).unwrap(); + session.run("selection.set", json!({"range": "A1:B2"})).unwrap(); + h.run_steps(2); + assert_eq!(h.state().name_box_text(), "Box"); +} + #[test] fn typing_enters_values_and_moves_down() { let mut h = harness(blank()); diff --git a/crates/xlsx/src/array_formula.rs b/crates/xlsx/src/array_formula.rs index caa5e8e9f..81f0aaea6 100644 --- a/crates/xlsx/src/array_formula.rs +++ b/crates/xlsx/src/array_formula.rs @@ -53,7 +53,7 @@ impl Scan<'_> { return *shape; } let (scope, bare) = match n.rsplit_once('!') { - Some((s, name)) => (self.wb.sheet_index(&s.trim_matches('\'').replace("''", "'")).unwrap_or(sheet), name), + Some((s, name)) => (self.wb.sheet_index(&gridcraft_core::unquote_sheet(s)).unwrap_or(sheet), name), None => (sheet, n.as_str()), }; if let Some(def) = self.wb.name(bare, scope) { diff --git a/crates/xlsx/src/pivot.rs b/crates/xlsx/src/pivot.rs index 743a746e7..de0b82238 100644 --- a/crates/xlsx/src/pivot.rs +++ b/crates/xlsx/src/pivot.rs @@ -378,10 +378,7 @@ fn resolve_source(wb: &Workbook, text: &str, default_sheet: usize, depth: u32) - let xml = if valid { format!("", esc_attr(&tb.name)) } else { range_source_xml(&sh.name, r) }; return Some((si, r, xml)); } - let (sheet, body) = match t.rsplit_once('!') { - Some((sh, body)) => (Some(sh.trim_matches('\'').replace("''", "'")), body), - None => (None, t), - }; + let (sheet, body) = gridcraft_core::split_sheet(t); if let Some(r) = RangeRef::parse(body) { let si = match &sheet { Some(n) => wb.sheet_index(n)?,