fix(core): a non-finite cell value crosses the wire, in the spelling the format already uses (improvise-55v2)
serde_json writes a non-finite f64 as null, and null does not read back as one,
so CellValue::Number(INFINITY) serialized to {"Number":null} and the frame
carrying it was unreadable. The websocket reader skips an unparseable frame and
the worker logs a console error, so one poisoned cell silently dropped a whole
batch — and to_string returned Ok, so the send site could not notice either.
The ticket offered two answers and the format had already chosen. Measured
before changing anything: inf, -inf and nan all round-trip losslessly through
.improv — format_number writes them, the grammar's bare_value accepts them, the
walker maps them back. So refusing them before sending would make a legal saved
document unprojectable to a browser, which is a new failure rather than a fix.
It also settles the ticket's second location: resolve.rs's typed() accepting inf
is correct, not half a defect.
Number now serializes through an untagged adaptor: a JSON number when finite,
one of the three tokens when not. A finite cell's bytes are unchanged, and a
JSON integer still reads as a number rather than as a token.
The tokens are one home — nonfinite_token/parse_nonfinite beside CellValue,
called by the adaptor and by persistence/{format,parse}.rs. Three copies stood
before and a fourth was about to be written. format_f64 is deliberately not
folded in: it is the display layer and these three are a format.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -1060,3 +1060,41 @@ through `display_text` and therefore through the view's format, as it always
|
||||
has. A document with no explicit `format:` line now exports two decimals. That
|
||||
is pre-existing behaviour meeting a new default, not a new coupling.
|
||||
|
||||
|
||||
## 16. A non-finite cell value has one spelling, and the format chose it (improvise-55v2)
|
||||
|
||||
`serde_json` writes a non-finite `f64` as `null`, and `null` does not read back
|
||||
as one. So `CellValue::Number(f64::INFINITY)` crossed the wire as
|
||||
`{"Number":null}` and the frame carrying it was **unreadable**: the websocket
|
||||
reader skips an unparseable frame and the worker logs a console error, so one
|
||||
poisoned cell silently dropped a whole batch. `to_string` returned `Ok`, so the
|
||||
send site could not notice either.
|
||||
|
||||
The ticket offered two answers — an explicit representation, or a refusal before
|
||||
sending — and **the `.improv` format had already chosen**. It carries non-finite
|
||||
numbers as first-class values: `format_number` writes `inf`, `-inf` and `nan`,
|
||||
the grammar's `bare_value` accepts them, and the walker maps them back. All
|
||||
three round-trip losslessly, measured before anything was changed. A refusal
|
||||
would therefore make a legal saved document unprojectable to a browser, which is
|
||||
a new failure rather than a fix. It also settles the second location the ticket
|
||||
named: `resolve.rs`'s `typed()` accepting `inf` is correct, not half a defect.
|
||||
|
||||
So `Number` serializes through an untagged adaptor — a JSON number when finite,
|
||||
one of the three tokens when not. A finite cell's bytes are unchanged, which is
|
||||
the whole of the compatibility story, and a JSON integer still reads as a number
|
||||
rather than being taken for a token.
|
||||
|
||||
**The tokens are one home**: `nonfinite_token` / `parse_nonfinite` beside
|
||||
`CellValue`, called by the adaptor, by `persistence/format.rs` and by
|
||||
`persistence/parse.rs`. Three copies stood before and a fourth was about to be
|
||||
written, which is the shape gotcha 34 named for pipe quoting. `format_f64` is
|
||||
deliberately **not** folded in: it is the display layer (gotcha 8) and may want a
|
||||
different glyph one day, where these three are a *format*.
|
||||
|
||||
What makes this checkable rather than asserted is that the guard compares the
|
||||
**two carriers to each other** —
|
||||
`a_nonfinite_cell_is_spelled_the_same_on_disk_and_on_the_wire` reads the token
|
||||
off the `## Data` line and off `serde_json` and requires them equal. A second
|
||||
home that happens to agree is harmless; one that drifts fails there, and
|
||||
re-copying the tokens into `format.rs` with one letter changed was injected to
|
||||
prove it does.
|
||||
|
||||
@@ -1217,3 +1217,54 @@ view a fresh workbook creates. Making `Workbook::create_view` refuse or rename
|
||||
a name it already holds passes this test and breaks records mode, because both
|
||||
rebuilders depend on the replacement — which is why the fix is a helper beside
|
||||
`create_view` and not a change to it.
|
||||
|
||||
### model::cell::nonfinite_cell_values::a_nonfinite_number_survives_the_json_wire
|
||||
|
||||
improvise-55v2. `serde_json` writes a non-finite `f64` as `null`, and `null`
|
||||
cannot be read back as one — so `CellValue::Number(f64::INFINITY)` serialized to
|
||||
`{"Number":null}` and the frame carrying it was unreadable at the far end. The
|
||||
websocket reader **skips** an unparseable frame and the worker reports only a
|
||||
console error, so one poisoned cell silently dropped a whole batch. `to_string`
|
||||
returned `Ok`, so nothing at the send site could notice either.
|
||||
|
||||
**The design question the ticket left open is already answered by the format**,
|
||||
and measuring that was the useful half of this work. `.improv` carries
|
||||
non-finite numbers as first-class values: `format_number` writes `inf`, `-inf`
|
||||
and `nan`, the grammar's `bare_value` accepts them, and the walker maps them
|
||||
back — all three round-trip losslessly, verified before anything was changed.
|
||||
So "refuse them before sending" would make a legal saved document unprojectable
|
||||
to a browser, and the remaining option is the one taken: give the wire an
|
||||
explicit representation. It also means `resolve.rs`'s `typed()` accepting `inf`
|
||||
is correct rather than the second half of a defect.
|
||||
|
||||
The tokens are therefore **one home**, `nonfinite_token`/`parse_nonfinite` in
|
||||
`improvise-core`, called by the serde adaptor, by `persistence/format.rs` and by
|
||||
`persistence/parse.rs` — where three separate copies stood before and a fourth
|
||||
was about to be written. `format_f64` is deliberately not folded in: it is the
|
||||
display layer (gotcha 8) and may legitimately want a different glyph.
|
||||
|
||||
Three rivals:
|
||||
|
||||
- **A plain `derive` on the `f64`** — the state at HEAD — fails this test at
|
||||
`left: "{\"Number\":null}"` and also fails the io-side agreement test.
|
||||
- **Drifting the shared token** (`-inf` → `-infinity`) fails this test, the
|
||||
agreement test, and `generated_file_roundtrips`, because the writer and the
|
||||
reader stop agreeing.
|
||||
- **Re-copying the tokens into `persistence/format.rs`** so there are two homes
|
||||
again, drifted, leaves *this* test passing and fails the agreement test and
|
||||
the grammar round-trip. That is the rival the agreement test exists for: a
|
||||
second home that happens to agree is harmless, and one that does not is what
|
||||
must fail.
|
||||
|
||||
The companion `an_ordinary_number_is_still_a_json_number` is the direction the
|
||||
fix could quietly break: it pins `{"Number":1.5}` exactly, and that a bare JSON
|
||||
integer still reads as a number rather than being mistaken for a token.
|
||||
|
||||
### persistence::nonfinite_agreement::a_nonfinite_cell_is_spelled_the_same_on_disk_and_on_the_wire
|
||||
|
||||
improvise-55v2's other half. It writes each of the three values through
|
||||
`format_md`, reads the token off the `## Data` line, reads the token off
|
||||
`serde_json`, and requires them equal — then reparses and requires the value
|
||||
back. It is the guard that makes "one home" checkable rather than asserted: the
|
||||
two carriers are compared against each other, so a future second copy fails here
|
||||
exactly when it drifts, which is the only time it matters.
|
||||
|
||||
@@ -60,9 +60,62 @@ impl std::fmt::Display for CellKey {
|
||||
}
|
||||
}
|
||||
|
||||
/// The three tokens a non-finite cell value is spelled with, shared by the
|
||||
/// `.improv` writer/parser and the JSON wire — `context/code/core.md` §16.
|
||||
pub fn nonfinite_token(n: f64) -> Option<&'static str> {
|
||||
match (n.is_nan(), n.is_infinite(), n.is_sign_positive()) {
|
||||
(true, _, _) => Some("nan"),
|
||||
(_, true, true) => Some("inf"),
|
||||
(_, true, false) => Some("-inf"),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// The inverse of [`nonfinite_token`]: a token back to the value it names.
|
||||
pub fn parse_nonfinite(token: &str) -> Option<f64> {
|
||||
match token {
|
||||
"nan" => Some(f64::NAN),
|
||||
"inf" => Some(f64::INFINITY),
|
||||
"-inf" => Some(f64::NEG_INFINITY),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// serde_json writes a non-finite `f64` as `null`, which cannot be read back as
|
||||
/// one — so a cell the `.improv` format carries killed the frame (improvise-55v2).
|
||||
mod wire_f64 {
|
||||
use serde::{Deserialize, Deserializer, Serialize, Serializer};
|
||||
|
||||
#[derive(Serialize, Deserialize)]
|
||||
#[serde(untagged)]
|
||||
enum Repr<'a> {
|
||||
Finite(f64),
|
||||
Nonfinite(&'a str),
|
||||
}
|
||||
|
||||
pub fn serialize<S: Serializer>(n: &f64, s: S) -> Result<S::Ok, S::Error> {
|
||||
match super::nonfinite_token(*n) {
|
||||
Some(token) => Repr::Nonfinite(token).serialize(s),
|
||||
None => Repr::Finite(*n).serialize(s),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<f64, D::Error> {
|
||||
match Repr::deserialize(d)? {
|
||||
Repr::Finite(n) => Ok(n),
|
||||
Repr::Nonfinite(token) => super::parse_nonfinite(token).ok_or_else(|| {
|
||||
serde::de::Error::custom(format!(
|
||||
"{token:?} is not a cell value: a non-numeric one is spelled \
|
||||
\"inf\", \"-inf\" or \"nan\""
|
||||
))
|
||||
}),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub enum CellValue {
|
||||
Number(f64),
|
||||
Number(#[serde(with = "wire_f64")] f64),
|
||||
Text(String),
|
||||
/// Evaluation error (circular reference, depth overflow, etc.)
|
||||
Error(String),
|
||||
@@ -372,6 +425,65 @@ impl DataStore {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod nonfinite_cell_values {
|
||||
use super::CellValue;
|
||||
|
||||
#[test]
|
||||
fn a_nonfinite_number_survives_the_json_wire() {
|
||||
for (token, v) in nonfinite_cases() {
|
||||
let json = serde_json::to_string(&CellValue::Number(v)).expect("serializes");
|
||||
assert_eq!(json, format!("{{\"Number\":\"{token}\"}}"));
|
||||
let back: CellValue = serde_json::from_str(&json).expect("reads back");
|
||||
match back {
|
||||
CellValue::Number(n) if v.is_nan() => assert!(n.is_nan(), "{token}"),
|
||||
CellValue::Number(n) => assert_eq!(n, v, "{token}"),
|
||||
other => panic!("{token} read back as {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_ordinary_number_is_still_a_json_number() {
|
||||
for v in [0.0, -0.0, 1.0, -2.5, 1e308, f64::MIN_POSITIVE] {
|
||||
let json = serde_json::to_string(&CellValue::Number(v)).expect("serializes");
|
||||
assert_eq!(
|
||||
serde_json::from_str::<CellValue>(&json).expect("reads back"),
|
||||
CellValue::Number(v),
|
||||
"{v}"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
serde_json::to_string(&CellValue::Number(1.5)).unwrap(),
|
||||
"{\"Number\":1.5}",
|
||||
"the shape a finite cell has always had"
|
||||
);
|
||||
assert_eq!(
|
||||
serde_json::from_str::<CellValue>("{\"Number\":3}").expect("a JSON integer"),
|
||||
CellValue::Number(3.0),
|
||||
"an integer literal is still a number, not a token"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_string_that_is_not_one_of_the_three_tokens_is_refused() {
|
||||
let e = serde_json::from_str::<CellValue>("{\"Number\":\"Infinity\"}")
|
||||
.expect_err("not a cell value");
|
||||
assert!(
|
||||
e.to_string().contains("Infinity") || e.to_string().contains("data did not match"),
|
||||
"the refusal names what it saw: {e}"
|
||||
);
|
||||
}
|
||||
|
||||
fn nonfinite_cases() -> [(&'static str, f64); 3] {
|
||||
[
|
||||
("inf", f64::INFINITY),
|
||||
("-inf", f64::NEG_INFINITY),
|
||||
("nan", f64::NAN),
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod cell_key {
|
||||
use super::CellKey;
|
||||
|
||||
@@ -36,15 +36,10 @@ fn agg_keyword(agg: &AggFunc) -> &'static str {
|
||||
}
|
||||
|
||||
fn format_number(n: f64) -> String {
|
||||
if n.is_infinite() {
|
||||
return if n.is_sign_positive() {
|
||||
"inf".to_string()
|
||||
} else {
|
||||
"-inf".to_string()
|
||||
};
|
||||
}
|
||||
if n.is_nan() {
|
||||
return "nan".to_string();
|
||||
// The token is the model's, not this file's: the wire spells it the same
|
||||
// way and a second copy is a second answer (improvise-55v2).
|
||||
if let Some(token) = crate::model::cell::nonfinite_token(n) {
|
||||
return token.to_string();
|
||||
}
|
||||
if n.fract() == 0.0 && n.abs() < 1e15 {
|
||||
format!("{}", n as i64)
|
||||
|
||||
@@ -366,3 +366,52 @@ mod tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod nonfinite_agreement {
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::workbook::Workbook;
|
||||
|
||||
fn key() -> CellKey {
|
||||
CellKey::new(vec![
|
||||
("R".to_string(), "a".to_string()),
|
||||
("_Measure".to_string(), "V".to_string()),
|
||||
])
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_nonfinite_cell_is_spelled_the_same_on_disk_and_on_the_wire() {
|
||||
for v in [f64::INFINITY, f64::NEG_INFINITY, f64::NAN] {
|
||||
let mut wb = Workbook::new("P");
|
||||
wb.model.add_category("R").expect("under the limit");
|
||||
wb.model
|
||||
.category_mut("R")
|
||||
.expect("just added")
|
||||
.add_item("a");
|
||||
wb.model
|
||||
.category_mut("_Measure")
|
||||
.expect("always exists")
|
||||
.add_item("V");
|
||||
wb.model.set_cell(key(), CellValue::Number(v));
|
||||
|
||||
let text = super::format_md(&wb);
|
||||
let on_disk = text
|
||||
.lines()
|
||||
.find_map(|l| l.strip_prefix("R=a, _Measure=V = "))
|
||||
.expect("the data line");
|
||||
let on_wire = serde_json::to_value(CellValue::Number(v))
|
||||
.expect("serializes")
|
||||
.get("Number")
|
||||
.and_then(|n| n.as_str().map(String::from))
|
||||
.expect("a non-finite value is a token on the wire");
|
||||
assert_eq!(on_disk, on_wire, "one spelling, two carriers");
|
||||
|
||||
let back = super::parse_md(&text).expect("reparses");
|
||||
match back.model.data.get(&key()) {
|
||||
Some(CellValue::Number(n)) if v.is_nan() => assert!(n.is_nan()),
|
||||
Some(CellValue::Number(n)) => assert_eq!(*n, v),
|
||||
other => panic!("{v} reparsed as {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -265,12 +265,13 @@ fn walk_cell_value(value_pair: Pair<'_, Rule>) -> Result<CellValue> {
|
||||
let inner = next(&mut value_pair.into_inner(), "pipe_quoted")?;
|
||||
CellValue::Text(unescape_pipe_body(inner.as_str()))
|
||||
}
|
||||
Rule::bare_value => match value_pair.as_str().trim() {
|
||||
"inf" => CellValue::Number(f64::INFINITY),
|
||||
"-inf" => CellValue::Number(f64::NEG_INFINITY),
|
||||
"nan" => CellValue::Number(f64::NAN),
|
||||
s => CellValue::Text(s.to_string()),
|
||||
},
|
||||
Rule::bare_value => {
|
||||
let s = value_pair.as_str().trim();
|
||||
match crate::model::cell::parse_nonfinite(s) {
|
||||
Some(n) => CellValue::Number(n),
|
||||
None => CellValue::Text(s.to_string()),
|
||||
}
|
||||
}
|
||||
_ => CellValue::Text(value_pair.as_str().to_string()),
|
||||
};
|
||||
Ok(value)
|
||||
|
||||
Reference in New Issue
Block a user