postcard-schema: nalgebra schema is incorrect

[jamesmunns]

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 u8s should be 10 bytes. However this test fails:

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());
}

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:

impl<T: Schema, const N: usize> Schema for [T; N] {
    const SCHEMA: &'static NamedType = &NamedType {
        name: "[T; N]",
        ty: &DataModelType::Tuple(&[T::SCHEMA; N]),
    };
}

[avsaase]

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!

[jamesmunns]

So it turns out this is very frustrating to fix, this is what I've come up with:

#[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 :)

[max-heller]

    /// 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

[max-heller]

/// 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`.

Since we only need to get a &[T], not a &[T; R * C], this is possible: #188

[jamesmunns]

@max-heller - re the guarantees of Schema, I'm actually not sure what the answer should be for this.

In my opinion, it MUST match "what is on the wire", so [[T; R]; C] OR [T; R * C] would be "acceptable", but &[T] would NOT be. But I'm not sure if [[T; R]; C] is "more right" (it matches the type, or at least the interface to the type), or if [T; R * C] is "more right" (it matches what the serializer/deserializer does, and the internal representation of the type in memory).

In my opinion, we can limit the scope to "within Postcard's data model", which is why I think either of the two "good" cases are acceptable.

Really, a driving cause right now is that "postcard-dyn must work", but I haven't formed opinions on the further details.

[jamesmunns]

Opened #189 to discuss this in more detail