ron-rs/ron

`ron`'s output is very odd when combined with `typetag`

Closed this issue · 13 comments

Hi! I was experimenting with typetag, and I found that there's some oddities with what ron outputs when using it.

Here's the code I'm using:

/Cargo.toml
[package]
name = "serde_ron_tag_test"
version = "0.1.0"
edition = "2021"

[dependencies]
	[dependencies.ron]
	version = "0.8"
	
	[dependencies.serde]
	version = "1"
	features = ["derive"]
	
	[dependencies.typetag]
	version = "0.2"
/src/main.rs
use serde::{Deserialize, Serialize};

#[typetag::serde]
trait Event {
	fn inspect(&self);
}

#[derive(Serialize, Deserialize)]
struct Message {
	data: String,
}

#[typetag::serde]
impl Event for Message {
	fn inspect(&self) {
		println!("type A with data `{}`", self.data);
	}
}

fn main() {
	let req_ser = ron::ser::to_string_pretty(
		&Message { data: "abcd".to_string() } as &dyn Event,
		ron::ser::PrettyConfig::default()
	).unwrap();
	
	println!("{}", req_ser);
	
	let req_de: Box<dyn Event> = ron::de::from_str(req_ser.as_str()).unwrap();
	
	req_de.inspect();
}

I would expect it to output something along the lines of this:

Message(
	data: "abcd",
)

since typetag is described as effectively creating an enum containing all types that implement that trait, and this is how ron normally outputs enums.

However, it instead outputs this mess:

{
	"Message": (
		data: "abcd",
	),
}

which looks very ugly and hard to understand when saved to a file. Unfortunately, I haven't found a way to make it output in the format I expect.

I was hoping this could be looked into? I'm unsure if this is an issue with ron or with typetag, but I felt it was worth bringing up either way.


P.S. The problem also exists for enums:

/src/main.rs
use serde::{Deserialize, Serialize};

#[typetag::serde]
trait Event {
	fn inspect(&self);
}

#[derive(Serialize, Deserialize)]
enum Message {
	A { data: String },
	B { data: String },
}

#[typetag::serde]
impl Event for Message {
	fn inspect(&self) {
		match self {
			Self::A { data } => println!("type A with data `{data}`"),
			Self::B { data } => println!("type B with data `{data}`"),
		}
	}
}

fn main() {
	let req_ser = ron::ser::to_string_pretty(
		&Message::A { data: "abcd".to_string() } as &dyn Event,
		ron::ser::PrettyConfig::default()
	).unwrap();
	
	println!("{}", req_ser);
	
	let req_de: Box<dyn Event> = ron::de::from_str(req_ser.as_str()).unwrap();
	
	req_de.inspect();
}
Output
{
	"Message": A(
		data: "abcd",
	),
}
juntyr commented

This unfortunately seems to be an issue in typetag, see https://github.com/dtolnay/typetag/blob/145f27249ee5901793095d308fcbd8f3b84e879e/src/externally.rs#L18

From briefly scanning the typetag codebase, the “enums” are not actually serialised as enums but as maps using the encoding that JSON uses for enums. Therefore, RON sees only that, in your case, a map with a single key is serialised. Unfortunately, this appears to be an issue with serde and it’s adjacent crates often assuming that every format is like JSON and can only treat enums as maps.

I think this would need to be fixed in typetag, sorry. I hope I was able to help you :)

No worries. I'll report this to typetag then (though it'll be in a day or two), see if they can change it on their end.

Do you want this issue kept open for the time being? If not, I can close it.

juntyr commented

I'll close it for now, feel free to reopen if something new comes up with regard to supporting typetag

If you're interested, I've created the report over at typetag's repo. dtolnay/typetag#58

juntyr commented

I'll keep this issue open until we've at least resolved whose fault it is

If you don't mind me asking, is there any status update on this issue?

juntyr commented

Unfortunately, nothing has changed in this area. Since typetag just gives us a map and we have no other type information, I'm not sure if ron can do much more at this point. However, if anyone can come up with a (generalisable) workaround, I'd be delighted to include it.

Perhaps you could try opening a new issue in the typetag repo, asking about having typetag give you an enum instead of a map? (or whatever would be applicable?)

juntyr commented

Ok, I have looked at this again, in particular how everything is implemented under the hood, and I think that there might be a possibility to fix it. I'll see if I can submit a PR.

juntyr commented

I unfortunately found a new roadblock, so I'll summarise my thoughts here:

  • a problem with enums is that formats are allowed to only serialise the variant index and skip the variant name altogether. Since typetag's internal registry might have a different iteration order if more types are registered later, the indices may no longer match up.
  • enum deserialisation could in theory be done as typetag already collects the registry of type names into a &'static Vec<&'static str>, so all valid variants are available as a &'static [&'static str] already.
  • the entire typetag crate codebase could be massively simplified if it shared the enum serialisation code with serde.

Unfortunately, the first issue seems to rule out typetag switching to enums, as they would need a way to either guarantee the name order / come up with collision-resistant variant indices / force Serializers to include the variant name.

juntyr commented

I don't think it is even possible to add special-case support for typetag to RON (and even if it were, we may not necessarily want to add it) since we get no type information from typetag (i.e. we get no struct name or similar which we could use to check if this call might come from typetag). Thus, I'll close this as not-possible for now. I too had hoped that were would be a way to fix this, so if you find a workaround, please post still it here.

I probably won't be working on a workaround for this issue - after I saw how dtolnay reacted in the typetag repo, and the refusal to respond at all after closing the issue, I'm not interested in using typetag in the first place.

Still, thank you for investigating the issue. Hopefully someone else can come up with a workaround.

juntyr commented

I can definitely understand that. Thank you again for bringing up the issue - it was fun to investigate :)