mirror of
https://github.com/storytold/gridcraft.git
synced 2026-10-30 13:46:47 -06:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5.5
parent
49fe41afbf
commit
0b4cc5618f
@@ -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),
|
||||
},
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<String>, &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")] {
|
||||
|
||||
@@ -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};
|
||||
|
||||
@@ -525,7 +525,7 @@ pub(super) fn go_to(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
} 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()));
|
||||
}
|
||||
|
||||
@@ -269,13 +269,7 @@ pub(crate) fn target_sheet(s: &Session, p: &Json) -> Result<usize> {
|
||||
Ok(d.wb.active_sheet)
|
||||
}
|
||||
|
||||
/// `'My Sheet'!A1` → (`Some("My Sheet")`, `A1`).
|
||||
pub(crate) fn split_sheet(r: &str) -> (Option<String>, &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
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<String>, &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;
|
||||
|
||||
@@ -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"));
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -413,15 +413,15 @@ fn transpose(args: &[Arg], _c: &mut dyn Ctx) -> R<Value> {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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);
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -378,10 +378,7 @@ fn resolve_source(wb: &Workbook, text: &str, default_sheet: usize, depth: u32) -
|
||||
let xml = if valid { format!("<worksheetSource name=\"{}\"/>", 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)?,
|
||||
|
||||
Reference in New Issue
Block a user