Merge pull request #202 from fiddlerwoaroof/fix/6yvg-dz8c-json-float

Every JSON parser reads numbers exactly; derive the float_roundtrip check (improvise-6yvg, improvise-dz8c)

Cargo.toml

@@ -40,7 +40,9 @@ improvise-command = { path = "crates/improvise-command" }
 ratatui = "0.30"
 crossterm = "0.29"
 serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
+# because each build stands alone (improvise-6yvg, context/code/protocol.md).
+serde_json = { version = "1", features = ["float_roundtrip"] }
 anyhow = "1"
 thiserror = "2"
 indexmap = { version = "2", features = ["serde"] }

context/code/protocol.md

@@ -756,28 +756,54 @@ coming back one ULP off, both in `CellDisplay::value` (host to client, so `yy`
 copied a neighbour) and in a `SetCell` (client to host, so a typed or pasted
 number landed as a neighbour).
 
-The feature is on for **every crate that parses a frame**: `improvise-ws-server`
-(`transport.rs`), `improvise-worker-server` (`worker.rs`) and
-`improvise-wasm-client` (`socket.rs`). Each crate's own manifest names it,
-because each browser bundle is built from one crate alone. The `wasm` CI job
-and `wasm-pack` both invoke per crate, and feature unification cannot reach
-across separate invocations (`context/repo-map.md`, `## Wasm`). Enabling it in
-one crate and relying on the others to inherit it works in `cargo test` and
-nowhere else.
+The feature is on for **every workspace crate with a normal dependency on
+serde_json** (improvise-dz8c). Each crate's own manifest names it, because each
+build is made from one crate alone: the `wasm` CI job and `wasm-pack` invoke
+per crate, and feature unification cannot reach across separate invocations
+(`context/repo-map.md`, `## Wasm`). Enabling it in one crate and relying on the
+others to inherit it works in `cargo test` and nowhere else.
 
-That is also why the end-to-end tests cannot guard this alone. They run in one
-unified build with `improvise-ws-server` as a dev-dependency, so they stay
-green with the feature removed from the browser bundle.
-`every_build_that_parses_a_frame_enables_float_roundtrip` asks `cargo tree`
-per crate over normal edges instead. A new crate that parses frames belongs in
-its list.
+**Why "can parse" and not "parses a frame".** The first version of the rule
+covered only the three crates that parse frames and listed them by hand.
+Frames are not the only place a user's number crosses a JSON parser.
+`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import
+(`StartImportWizard`, `ImportJsonHeadless`) and the root binary's
+`improvise import` all parsed with the lossy default, and in the TUI binary
+nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285`
+stored as `…283`). The worker bundle happened to be exact through unification,
+so one import gave different numbers in the two front ends. "Parses JSON input"
+cannot be read off a manifest, but "has a normal serde_json dependency" can, and
+it over-approximates in the cheap direction: the feature changes only parsing,
+so a crate that only serializes pays nothing for it. Dev-dependencies are out,
+because a test build ships nowhere. That also leaves out `gp-tree`, whose only
+serde_json is a dev-dependency (its example CLI reads a saved model). Every
+number it persists is an `f32`, and 3M random `f32`s all read back exactly
+through the lossy parser, against about 30% of random `f64`s.
 
-Not covered, and filed separately rather than widened into this change
-(improvise-6yvg): `improvise-io`'s JSON import and `improvise-command`'s JSON
-reads also parse numbers with the default parser. They are file formats, not
-frames. In the TUI binary nothing enables the feature. In the worker bundle
-unification happens to reach them, so one import can give different numbers
-in the two front ends.
+**Why a declaration and not the build graph.** The first check asked
+`cargo tree` whether each listed crate's *build* had the feature. That answer
+can be yes by accident. `improvise-command` depends on `improvise-io`, so with
+the feature removed from command's manifest, command's own test build still
+has it, and its import test stays green. The rule requires the crate's own
+manifest to say it, so a later change to a dependency cannot take it away.
+
+**Why not enable it once, in `improvise-protocol`.** That removes the list, but
+it would give the protocol a normal serde_json dependency it has no use for
+(its repo-map entry says core and serde only). It would also reach only builds
+that include the protocol, and `improvise-io` does not depend on it. Under this
+rule the protocol's one serde_json dependency, the optional `test-support` one,
+carries the feature too. That is off in every shipped build, so it costs
+nothing.
+
+`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`)
+reads `cargo metadata --no-deps` and names every package, and every
+target-specific entry, whose normal serde_json dependency lacks the feature. A
+new crate is covered the day it declares the dependency. It does not reach
+`harness/opfs-contract`, which is its own workspace and only serializes.
+
+The end-to-end tests cannot guard this alone. They run in one unified build
+with `improvise-ws-server` as a dev-dependency, so they stay green with the
+feature removed from the browser bundle.
 
 ### `AxisFrame` — the grid's frame, and why it is *one* type
 

context/test-evidence/clients.md

@@ -4590,24 +4590,12 @@ before #178 too, as the review noted.
 **Fault injected:** the same pre-fix tree. Fails: the host stored
 `4598108823619341467` in South/Cost.
 
-### `every_build_that_parses_a_frame_enables_float_roundtrip`
-
-The two tests above run in **one** feature-unified test build, which includes
-`improvise-ws-server` and `improvise-command` as dev-dependencies. So they pass
-whenever **any** crate in that build enables the feature. The shipped browser
-bundle is built from one crate alone, so it would still be lossy. That was
-demonstrated: with the feature removed from `improvise-wasm-client` only, both
-tests stayed green while `cargo tree` showed no `float_roundtrip` in the wasm
-bundle. This test asks cargo instead, once per crate that parses a frame:
-`cargo tree --locked --edges normal,features --invert serde_json --package
-<crate>`, with `--target wasm32-unknown-unknown` for the two bundles. It uses
-normal edges only, because a dev edge would unify the feature back in
-(`context/repo-map.md`, `## Wasm`).
-
-**Faults injected:** the feature removed from `improvise-wasm-client` alone,
-which fails naming that crate while the two tests above pass. Then removed from
-`improvise-ws-server` alone, which fails naming it. The native path runs
-without `--target`, so the two arms are different invocations.
+**Retired: `every_build_that_parses_a_frame_enables_float_roundtrip`.** It asked
+`cargo tree` about three crates listed by hand. improvise-dz8c replaced it with
+`every_crate_that_can_parse_json_enables_float_roundtrip`, whose entry is in
+`tooling.md`. The injections recorded for it still hold: with the feature
+removed from `improvise-wasm-client` alone, the two tests above stay green, so
+they cannot guard the bundle alone.
 
 ### resolve::tests::every_grid_move_lands_where_its_binding_says
 

context/test-evidence/command.md

@@ -5459,3 +5459,18 @@ one context, which is why the old single-context law was vacuous, and differ
 across two independently built ones. If a future refactor made the fixture
 share one map, or share one `RandomState`, the second assertion fails. Without
 this test the law would then pass everything and look healthy.
+
+## JSON import numbers (improvise-6yvg)
+
+### `a_json_import_stores_its_numbers_bit_for_bit`
+
+`ImportJsonHeadless` imports `[{"Region":"East","Revenue":0.24815714285714285}]`
+and the test reads Region=East, _Measure=Revenue back by `f64::to_bits`.
+`StartImportWizard` parses through the same `serde_json::from_slice` call, so
+one test covers the crate's feature.
+
+**Red at `1158c23`** in `cargo test -p improvise-command`: stored
+`0.24815714285714283`. That reproduces improvise-6yvg. **Limitation:** this test
+cannot see the feature removed from `improvise-command` alone, because
+`improvise-io`, a normal dependency, enables it in the same build. That case is
+held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`).

context/test-evidence/formula-and-io.md

@@ -1434,3 +1434,17 @@ rather than defaulted: the alternative — expanding the group in the export —
 reintroduce the iot6 divergence between what the user sees and what they export.
 On the pre-6qet tree the body is empty (the group had no row, and pruning left
 nothing), so the test fails there.
+
+## Legacy JSON numbers (improvise-6yvg)
+
+### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`
+
+A workbook with Food/Jan holding `0.24815714285714285` is serialized with
+`serde_json` and loaded through `load_from`. A leading `{` sends it down the
+legacy JSON path. The test first asserts that the text holds the exact decimal,
+so what it measures is the parse. It compares `f64::to_bits`. Found by the
+review of PR #190, not by the ticket.
+
+**Red at `1158c23`** in `cargo test -p improvise-io`: loaded
+`0.24815714285714283`. Green once `improvise-io`'s own manifest enables
+`float_roundtrip`, and red again with only that line reverted.

context/test-evidence/tooling.md

@@ -681,3 +681,41 @@ reproduction.
 alone fails, exiting 0. All 44 others pass. That is the review's point: the
 older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it
 cannot see this rival.
+
+## Every JSON parser reads numbers exactly (improvise-dz8c)
+
+### `every_crate_that_can_parse_json_enables_float_roundtrip`
+
+It replaces a check over three crates listed by hand. It reads
+`cargo metadata --no-deps` and fails naming every workspace package, and
+every target-specific entry, whose **normal** serde_json dependency lacks
+`float_roundtrip`. The rule and why it was chosen over the alternatives are in
+`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`,
+`improvise-command`, `improvise-io` and `improvise-protocol`.
+
+**Faults injected, each run:**
+
+- **The feature removed from `improvise-io` alone.** The check fails naming
+  `improvise-io (all targets)`, and so does
+  `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`.
+  `a_json_import_stores_its_numbers_bit_for_bit` stays green, because
+  `improvise-command` enables the feature and unification reaches io in its
+  build.
+- **The feature removed from `improvise-command` alone.** The check fails naming
+  `improvise-command (all targets)`. The command crate's own import test
+  **stays green**: `cargo tree -p improvise-command` shows io's declaration
+  switching the feature on. So the manifest check is the only guard that
+  sees this case. It is also the case the retired build-graph check would
+  have passed.
+- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on
+  `improvise-protocol` and `serde_json` and no feature (the lockfile updated,
+  as a real new crate would be). The check fails naming
+  `zz-throwaway (all targets)`. With the unconditional dependency carrying the
+  feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails
+  naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than
+  the build needs, since a wasm build unifies the two entries, and cheap to
+  satisfy. The crate was then removed.
+
+The `checked.len() >= 2` assertion is the positive signal. If a cargo change
+renamed the metadata fields, the check would find no serde_json dependency and
+pass vacuously without it.

crates/improvise-command/Cargo.toml

@@ -14,7 +14,9 @@ improvise-io = { path = "../improvise-io" }
 improvise-gp = { path = "../improvise-gp" }
 anyhow = "1"
 serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
+# because each build stands alone (improvise-6yvg, context/code/protocol.md).
+serde_json = { version = "1", features = ["float_roundtrip"] }
 unicode-width = "^0.2"
 # Instant::now() panics on wasm32-unknown-unknown; web_time re-exports
 # std::time off-wasm and uses performance.now() on it (improvise-ywd).

crates/improvise-command/src/ui/effect.rs

@@ -3545,6 +3545,38 @@ mod tests {
         assert_eq!(app.view_state.status_msg, "Imported successfully");
     }
 
+    #[test]
+    fn a_json_import_stores_its_numbers_bit_for_bit() {
+        const LOSSY: f64 = 0.248_157_142_857_142_85;
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("data.json");
+        std::fs::write(
+            &path,
+            r#"[{"Region":"East","Revenue":0.24815714285714285}]"#,
+        )
+        .unwrap();
+
+        let mut app = test_app();
+        ImportJsonHeadless {
+            path,
+            model_name: None,
+            array_path: None,
+        }
+        .apply(&mut app);
+
+        let key = CellKey::new(vec![
+            ("Region".to_string(), "East".to_string()),
+            ("_Measure".to_string(), "Revenue".to_string()),
+        ]);
+        let Some(CellValue::Number(n)) = app.model_state.workbook.model.get_cell(&key) else {
+            panic!(
+                "the import must store the number; status {:?}",
+                app.view_state.status_msg
+            );
+        };
+        assert_eq!(n.to_bits(), LOSSY.to_bits(), "stored {n:?}, not {LOSSY:?}");
+    }
+
     // ── json_import_records (improvise-oaq helper) ──────────────────────
 
     #[test]

crates/improvise-io/Cargo.toml

@@ -17,7 +17,9 @@ indexmap = { version = "2", features = ["serde"] }
 pest = "2.9.2"
 pest_derive = "2.9.2"
 serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate,
+# because each build stands alone (improvise-6yvg, context/code/protocol.md).
+serde_json = { version = "1", features = ["float_roundtrip"] }
 
 # The browser backend. Only the wasm build pulls these in; the native
 # binary never sees them (improvise-i34).

crates/improvise-io/src/persistence/mod.rs

@@ -205,6 +205,28 @@ mod tests {
         assert_eq!(m2.model.name, "Budget");
     }
 
+    #[test]
+    fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() {
+        use crate::storage::{MemoryFs, Storage};
+        const LOSSY: f64 = 0.248_157_142_857_142_85;
+        let key = coord(&[("Type", "Food"), ("Month", "Jan")]);
+        let mut wb = two_cat_model();
+        wb.model.set_cell(key.clone(), CellValue::Number(LOSSY));
+        let json = serde_json::to_string(&wb).unwrap();
+        assert!(
+            json.contains("0.24815714285714285"),
+            "the file holds the exact text"
+        );
+        let fs = MemoryFs::new();
+        let path = std::path::Path::new("legacy.improv");
+        fs.write(path, json.as_bytes()).unwrap();
+        let loaded = super::load_from(&fs, path).unwrap();
+        let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else {
+            panic!("the cell must load as a number");
+        };
+        assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}");
+    }
+
     // ── save/load roundtrip via file ────────────────────────────────────
 
     #[test]

crates/improvise-protocol/Cargo.toml

@@ -14,7 +14,8 @@ serde = { version = "1", features = ["derive"] }
 # `char`, and the TUI has always measured it this way.
 unicode-width = "^0.2"
 # Only for `variants`, which is off unless a test build asks for it.
-serde_json = { version = "1", optional = true }
+# float_roundtrip because every normal serde_json dep carries it (improvise-dz8c).
+serde_json = { version = "1", optional = true, features = ["float_roundtrip"] }
 
 [features]
 # `variants` reaches OUT of this crate: improvise-command bounds its own

crates/improvise-wasm-client/tests/browser_round_trip.rs

@@ -707,44 +707,6 @@ fn a_yanked_number_crosses_the_serialized_wire_exactly() {
     );
 }
 
-/// The two tests around this one share a feature-unified build, so neither can
-/// see ONE frame-parsing crate lacking `float_roundtrip`. Evidence file.
-#[test]
-fn every_build_that_parses_a_frame_enables_float_roundtrip() {
-    for (krate, target) in [
-        ("improvise-wasm-client", Some("wasm32-unknown-unknown")),
-        ("improvise-worker-server", Some("wasm32-unknown-unknown")),
-        ("improvise-ws-server", None),
-    ] {
-        let mut cargo = std::process::Command::new(env!("CARGO"));
-        cargo.current_dir(env!("CARGO_MANIFEST_DIR")).args([
-            "tree",
-            "--locked",
-            "--edges",
-            "normal,features",
-            "--invert",
-            "serde_json",
-            "--package",
-            krate,
-        ]);
-        if let Some(target) = target {
-            cargo.args(["--target", target]);
-        }
-        let out = cargo.output().expect("cargo tree must run");
-        assert!(
-            out.status.success(),
-            "{krate}: cargo tree failed: {}",
-            String::from_utf8_lossy(&out.stderr)
-        );
-        let tree = String::from_utf8(out.stdout).expect("cargo tree prints UTF-8");
-        assert!(
-            tree.contains(r#"serde_json feature "float_roundtrip""#),
-            "{krate} parses frames without float_roundtrip, so its numbers land \
-             one ULP off:\n{tree}"
-        );
-    }
-}
-
 /// A typed number is parsed exactly in the client and re-parsed from JSON by
 /// the host. Evidence file.
 #[test]

tests/json_numbers.rs

@@ -0,0 +1,61 @@
+//! A number read through serde_json must be the number written (improvise-6yvg,
+//! improvise-dz8c). The rule and its derivation: `context/code/protocol.md`.
+
+use serde_json::Value;
+
+/// Every NORMAL serde_json dependency in the workspace, and those lacking
+/// `float_roundtrip`, each spelled `package (target)`.
+fn lossy_json_dependencies(metadata: &Value) -> (Vec<String>, Vec<String>) {
+    let mut checked = vec![];
+    let mut lossy = vec![];
+    let packages = metadata["packages"]
+        .as_array()
+        .expect("metadata lists packages");
+    for package in packages {
+        let name = package["name"].as_str().expect("a package has a name");
+        let deps = package["dependencies"]
+            .as_array()
+            .expect("a package lists deps");
+        for dep in deps {
+            if dep["name"] != "serde_json" || !dep["kind"].is_null() {
+                continue;
+            }
+            let target = dep["target"].as_str().unwrap_or("all targets");
+            let spelled = format!("{name} ({target})");
+            let features = dep["features"].as_array().expect("a dep lists features");
+            if !features.iter().any(|f| f == "float_roundtrip") {
+                lossy.push(spelled.clone());
+            }
+            checked.push(spelled);
+        }
+    }
+    (checked, lossy)
+}
+
+/// Derived from `cargo metadata`, not listed: a new crate that can parse JSON
+/// is covered the day it declares the dependency. Evidence file.
+#[test]
+fn every_crate_that_can_parse_json_enables_float_roundtrip() {
+    let out = std::process::Command::new(env!("CARGO"))
+        .current_dir(env!("CARGO_MANIFEST_DIR"))
+        .args(["metadata", "--no-deps", "--format-version", "1"])
+        .output()
+        .expect("cargo metadata must run");
+    assert!(
+        out.status.success(),
+        "cargo metadata failed: {}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    let metadata: Value = serde_json::from_slice(&out.stdout).expect("metadata is JSON");
+    let (checked, lossy) = lossy_json_dependencies(&metadata);
+    assert!(
+        checked.len() >= 2,
+        "the derivation found {checked:?}: too few to be reading the metadata right"
+    );
+    assert!(
+        lossy.is_empty(),
+        "these parse JSON numbers with serde_json's lossy default, one ULP off \
+         for roughly one decimal in seven; add features = [\"float_roundtrip\"] \
+         to each crate's own manifest: {lossy:?}"
+    );
+}