serde-json-core icon indicating copy to clipboard operation
serde-json-core copied to clipboard

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

Open ryan-summers opened this issue 4 years ago • 10 comments

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.

ryan-summers avatar Feb 13 '21 06:02 ryan-summers

(The particular issue of newtype has been fixed in https://github.com/rust-embedded-community/serde-json-core/commit/cce045cf76eb4b60d08241cf0a2a2458b49189c1; not sure whether there are any others left.)

dnadlinger avatar Feb 13 '21 13:02 dnadlinger

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

jordens avatar Feb 19 '21 09:02 jordens

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 avatar Feb 19 '21 09:02 ryan-summers

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

ryan-summers avatar Jul 26 '23 10:07 ryan-summers

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"");

jordens avatar Jul 26 '23 11:07 jordens

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

gdemenibus avatar Dec 09 '23 17:12 gdemenibus

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?

jordens avatar Dec 09 '23 18:12 jordens

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.

ryan-summers avatar Dec 13 '23 10:12 ryan-summers

The title should probably reflect that then.

jordens avatar Dec 13 '23 12:12 jordens