Library panics when deserializing certain types due to `unreachable!()` statements

[ryan-summers]

When deserializing certain types, the library may hit unreachable!() statements, which results in a panic. This appears to be because serde-json-core does not support deserializing complex enums (e.g. pub struct Test(u32).

That being said, the library should never internally panic. Instead, it should propagate an error out to the user.

[dnadlinger]

(The particular issue of newtype has been fixed in cce045c; not sure whether there are any others left.)

[jordens]

These are all known at compile time. Shouldn't already the derive fail?

[ryan-summers]

The derive is done in serde, which doesn't care about the support of the underlying serialize/deserialize engine. Hence, we are just provided a serde-compatible API at the serde-json-core level. We don't have any proc-macros in this repository, we just have functions that can deserialize/serialize objects that implement serde::Serialize and serde::Deserialize. In other backends for serde, these types are supported.

[ryan-summers]

Closing, as I don't think this has any open defects anymore with newtype resolved.

[jordens]

I think it's still pretty easy to hit, e.g. serde_json_core::to_vec::<_, 10>(&'a'), serde_json_core::from_slice::<char>(b"");

[gdemenibus]

You can also hit an unreachable section by serializing an enum with a tuple element.

[jordens]

I don't think this general behavior is a bug. It seems to be standard practice of serde trait implementations to panic on unimplemented/unimplementable things. What would be the alternative?

[ryan-summers]

I think the core issue is that it's marked as unreachable!() when in reality it's an unimplemented!(). In either case, an internal panic is likely the only solution if we don't want to propogate an error up the stack.

[jordens]

The title should probably reflect that then.

[ryan-summers]

I believe this should be mostly fixed now that #83 has made these functions return Err instead of unreachable!(). There's still some unreachable!() statements in src/de/map.rs.

I think the only case you'll now hit these is if you use some non-string as a map key, such as {0: "Test"}

[sammhicks]

I believe this should be mostly fixed now that #83 has made these functions return Err instead of unreachable!(). There's still some unreachable!() statements in src/de/map.rs.

I think the only case you'll now hit these is if you use some non-string as a map key, such as {0: "Test"}

These can easily be hit using map types (e.g. HashMap or heapless::LinearMap), so I think the logic should be as follows:

• Strings and chars are serialized normally.
• Numerical types, i.e. i32 and u32 are serialized normally, but surrounded by double quotes.
• All other types are rejected with an "key must be a string" error.

This has the benefit that it's more permissive, generally predictable, and mirrors what serde_json does.

I'm happy to submit a pull request if the design is approved.