Repository: jamesmunns/postcard (language: rust)

Issue:
postcard-schema: `nalgebra` schema is incorrect
Hey @avsaase - I was wondering how you chose `Seq(T)` for the schema of `nalgebra::Matrix`. A `Seq` is a length prefixed item, so a 3x3 matrix of `u8`s should be 10 bytes. However this test fails:

```rust
use nalgebra_v0_33::{Const, Matrix};
#[test]
fn smoke() {
    let x = nalgebra_v0_33::Matrix::<u8, Const<3>, Const<3>, _>::new(
        1, 2, 3,
        4, 5, 6,
        7, 8, 9,
    );
    let y = postcard::to_stdvec(&x).unwrap();
    assert_eq!(&[9, 1, 4, 7, 2, 5, 8, 3, 6, 9], y.as_slice());
}
```

```txt
test impls::nalgebra_v0_33::test::smoke ... FAILED

failures:

---- impls::nalgebra_v0_33::test::smoke stdout ----
thread 'impls::nalgebra_v0_33::test::smoke' panicked at source/postcard-schema/src/impls/nalgebra_v0_33.rs:32:9:
assertion `left == right` failed
  left: [9, 1, 4, 7, 2, 5, 8, 3, 6, 9]
 right: [1, 4, 7, 2, 5, 8, 3, 6, 9]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

Was the choice of `Seq(T)` a guess? Or were you basing this on something else? Just making sure I understand the original choice before I change it.

It appears to me that the actual serialized form is actually `[u8; 9]`, which is *not* a length prefixed, but is actually modeled as a tuple of size N, which avoids serializing a fixed size len:

```rust
impl<T: Schema, const N: usize> Schema for [T; N] {
    const SCHEMA: &'static NamedType = &NamedType {
        name: "[T; N]",
        ty: &DataModelType::Tuple(&[T::SCHEMA; N]),
    };
}
```


Discussion / hints:
Hi, I don't recall exactly why I picked a `Seq(T)`. I guess I thought this would be the correct format but I should have added this test myself to confirm. Sorry about that!
So it turns out this is very frustrating to fix, this is what I've come up with:

```rust
#[cfg_attr(docsrs, doc(cfg(feature = "nalgebra-v0_33")))]
impl<T, const R: usize, const C: usize> Schema
    for nalgebra_v0_33::Matrix<
        T,
        nalgebra_v0_33::Const<R>,
        nalgebra_v0_33::Const<C>,
        nalgebra_v0_33::ArrayStorage<T, R, C>,
    >
where
    T: Schema + nalgebra_v0_33::Scalar,
{
    /// Warning! This is not TECHNICALLY correct. nalgebra actually serializes the
    /// ArrayStorage as `[T; R * C]`, however there is no way to express this below,
    /// as we cannot use generics from the outer context in const context to do
    /// what we really want here, which is `<[T; R * C]>::SCHEMA.ty`. For the
    /// purposes of postcard, these two are actually equivalent with respect to
    /// size and other meaning, but crates like `postcard-dyn` may make this
    /// difference visible, and give the user the nested arrays instead of the
    /// flat array.
    const SCHEMA: &'static NamedType = &NamedType {
        name: "nalgebra::Matrix<T, R, C, ArrayStorage<T, R, C>>",
        ty: <[[T; R]; C]>::SCHEMA.ty,
    };
}
```

This will need to be a breaking change to `postcard-schema`, since changing the schema will break users of postcard-rpc (if they update the HOST but not the MCU, then they will end up unable to use endpoints that contain nalgebra types).

I definitely need to raise the bar for required testing to add schema types!

Also no worries @avsaase, I made the same mistake with `Uuid` before I released, it's pretty annoying that it is necessary to do this manually.

So many things to learn :)
> ```rust
>     /// Warning! This is not TECHNICALLY correct. nalgebra actually serializes the
>     /// ArrayStorage as `[T; R * C]`, however there is no way to express this below,
>     /// as we cannot use generics from the outer context in const context to do
>     /// what we really want here, which is `<[T; R * C]>::SCHEMA.ty`. For the
>     /// purposes of postcard, these two are actually equivalent with respect to
>     /// size and other meaning, but crates like `postcard-dyn` may make this
>     /// difference visible, and give the user the nested arrays instead of the
>     /// flat array.
>     const SCHEMA: &'static NamedType = &NamedType {
>         name: "nalgebra::Matrix<T, R, C, ArrayStorage<T, R, C>>",
>         ty: <[[T; R]; C]>::SCHEMA.ty,
>     };
> ```

This gets to a question about what `Schema` should mean:
- Should `Schema` promise to mirror the exact sequence of serializer calls made by the `Serialize` implementation of a type (call this a "perfect schema")? In this case, the implementation should use `[T; R * C]` once this is possible to compile.
- Or, should `Schema` only promise that the schema is equivalent to the type's "perfect schema" **given postcard's serialization format**. In this case, `[[T; R]; C]` is a correct implementation, and may be more intuitive because it mirrors the user-facing structure of `ArrayStorage<T, R, C>`.
  
For use-cases involving `postcard-dyn`, I could see it being easier to work with matrices in the array of columns form rather than the flattened form. Round-trips from bytes through `postcard-dyn` back to bytes would still work, but the looser contract for Schema would mean that paths like [`T` -> bytes -> `postcard-dyn` -> json -> `T`] wouldn't work

Fix patch:
diff --git a/ci.sh b/ci.sh
index 7ca57a7..5b723b1 100755
--- a/ci.sh
+++ b/ci.sh
@@ -29,7 +29,7 @@ cargo_test() {
   cargo test --all "$@"
 }
 
-cargo_test --features=alloc,experimental-derive,use-std,use-crc,derive
+cargo_test --features=alloc,experimental-derive,use-std,use-crc,derive,nalgebra-v0_33
 
 # NOTE: we exclude postcard-dyn for these checks because it is std-only
 
diff --git a/source/postcard-schema/Cargo.toml b/source/postcard-schema/Cargo.toml
index 4a8e7ef..72d1f53 100644
--- a/source/postcard-schema/Cargo.toml
+++ b/source/postcard-schema/Cargo.toml
@@ -68,6 +68,12 @@ path = "../postcard"
 version = "1.0"
 features = ["use-std"]
 
+[dev-dependencies.nalgebra_v0_33]
+package = "nalgebra"
+version = "0.33.0"
+default-features = false
+features = ["serde-serialize-no-std"]
+
 [features]
 default = []
 use-std = ["serde/std"]
diff --git a/source/postcard-schema/src/impls/nalgebra_v0_33.rs b/source/postcard-schema/src/impls/nalgebra_v0_33.rs
index 61de71e..3f9ceb7 100644
--- a/source/postcard-schema/src/impls/nalgebra_v0_33.rs
+++ b/source/postcard-schema/src/impls/nalgebra_v0_33.rs
@@ -18,6 +18,31 @@ where
 {
     const SCHEMA: &'static NamedType = &NamedType {
         name: "nalgebra::Matrix<T, R, C, ArrayStorage<T, R, C>>",
-        ty: &DataModelType::Seq(T::SCHEMA),
+        ty: &DataModelType::Tuple(flatten(&[[T::SCHEMA; R]; C])),
     };
 }
+
+/// Const version of the const-unstable [`<[[T; N]]>::as_flattened()`]
+const fn flatten<T, const N: usize>(slice: &[[T; N]]) -> &[T] {
+    const {
+        assert!(size_of::<T>() != 0);
+    }
+    // SAFETY: `self.len() * N` cannot overflow because `self` is
+    // already in the address space.
+    let len = unsafe { slice.len().unchecked_mul(N) };
+    // SAFETY: `[T]` is layout-identical to `[T; N]`
+    unsafe { core::slice::from_raw_parts(slice.as_ptr().cast(), len) }
+}
+
+#[test]
+fn flattened() {
+    type T = nalgebra_v0_33::SMatrix<u8, 3, 3>;
+    assert_eq!(T::SCHEMA.ty, <[u8; 9]>::SCHEMA.ty);
+}
+
+#[test]
+fn smoke() {
+    let x = nalgebra_v0_33::SMatrix::<u8, 3, 3>::new(1, 2, 3, 4, 5, 6, 7, 8, 9);
+    let y = postcard::to_stdvec(&x).unwrap();
+    assert_eq!(&[1, 4, 7, 2, 5, 8, 3, 6, 9], y.as_slice());
+}


Test patch: