Rc<> gets deserialized incorrectly

[shahn]

The way serde currently treats ref-counted data structures means that it creates copies of the references objects. This is bad for several reasons:

• Potentially increased memory usage
• Equality comparison that relies on comparison of address breaks
• Interior mutability is not reflected in copies
  and maybe more that I can't think of right away. I think there should at least be big warnings in the documentation about this behaviour, but the support should be either fixed or deprecated, imo.

[dylanede]

IMO this is kind of a big deal. Any word on progress towards dealing with this?

[shahn]

I agree. I think serde either needs to drop Rc support or add an annotation that users can specify (or something so people are aware of this issue). It's pretty hard to come up with a general solution that doesn't have other issues. In any case, this is a big backwards compatibility hazard because this will require breaking changes to fix.

[dtolnay]

I am open to dropping Serialize and Deserialize impls for Rc in the next breaking release, and requiring explicit serialize_with and deserialize_with to opt into serializing them.

[alexbool]

Whoa, I personally find Rc/Arc serialization scheme perfectly valid, probably we just have to stress this moment in the docs?

My application will suffer from removal of these implementations. I use serde for deserializing plain-old-data configs with Arc<String> and then clone that Arc throughout the application.
With this impl broken, I will have to either use the hypothetical opt-in mechanism (I don't know what it will be like), or deserialize plain String, then destructore config struct, move the string into Arc - the code is becoming more complex.

On the other hand, one can always write his own personal fancy deserializer if he wants interning.

[oli-obk]

@alexbool: the reverse argument is that you could use #[serde(deserialize_with="val_to_arc")]

Maybe we should provide a #[serde(deserialize_map="Arc::new")]

[alexbool]

@oli-obk while this is clearly possible, I still think this is not worth breaking projects

[oli-obk]

@alexbool It's not a silent breaking change, so everyone will notice and it will be in a major version bump which will break projects most likely anyway.

The reverse is a silent source of bugs. The advantage of breaking it is that in the future we can re-add it when/if we figure out a way to serialize/deserialize the duplication.

[alexbool]

@oli-obk what do you call "not silent"? Do we have a deprecation that is a compilation warning? At the moment I doubt so.

[oli-obk]

@alexbool The following will stop compiling, because you can only have fields that implement Serialize and Deserialize in a struct that derives these traits

#[derive(Deserialize, Serialize)]
struct Foo {
    a: i32,
    b: Arc<String>, //~ERROR Arc<String> does not implement Deserialize
}

[alexbool]

@oli-obk In my understanding, this is simply a breaking change, a silent one. No one is expecting that until we can throw a compilation warning some time before the actual breakage will occur.
By the way, is it possible to add such a warning now?

[oli-obk]

By the way, is it possible to add such a warning now?

#[deprecate] doesn't work on impls to the best of my knowledge.

In my understanding, this is simply a breaking change, a silent one

a silent breaking change is a change that only occurs at runtime

No one is expecting that until we can throw a compilation warning some time before the actual breakage will occur.

When you do a major version bump, you should always expect breakage.

[alexbool]

Can we throw a deprcation message during proc-macro expansion?

[shahn]

I think there's a disconnect what "silent" means. The current behaviour is a bug in my book, because it means a serialize/deserialize cycle breaks programs "silently" - it appears to work, but doesn't. A breaking change that breaks the compile is not silent, independent of whether a deprecation note happened earlier or not. Under semver, when you update to a version with breaking changes, you are supposed to read the release notes to learn whether you're affected, then decide whether you want to upgrade and if you decide you want to you deal with the breakage. That seems normal and expected?

[nox]

I'm -1 on removing the impls. Why would one assume that serialising and then deserialising a Rc<T> would share the T inside it?

[oli-obk]

None would assume that, but you might accidentally end up creating code that expects that it would. Generic structures which derive Serialize and Deserialize might get inialized with Rc.... Also, in the few cases you actually want cloning serialization, you can use serialize_with.

[dtolnay]

I put together a proof of concept of an efficient Rc<> DAG serialization/deserialization approach here in case anyone is interested in pursuing this further: see bincode-org/bincode#139 (comment).

[erickt]

One idea I had for this was that we could add an API to Deserializer that provided a typemap-ish store. This would allow an impl for Rc<T> store a map from some unique id to the deduplicated pointer. This might also be useful for some zero-copy situations, where the creator of the deserializer could prepopulate the typemap with shared data that was used in the Deserialize impl.

This does have some smell to it though.

[dtolnay]

@erickt take a look at the implementation in bincode-org/bincode#139 (comment) (the hashmap could be swapped out for a typemap). I think this should be a separate library on top of Serde. It does not need to be baked into the Deserializer API.

[dtolnay]

I am still torn on this. I understand the concern that implicitly duplicating Rc data may be unexpected and is not aligned with Rust's tendency to make expensive operations explicit. I also know there are use cases where serializing an Rc the current way is exactly what is desired and working around a missing impl would be obnoxious.

@shahn @dylanede or others, does anyone have an example of a library using Rc and Serde where serializing copies of an Rc would lead to undesirable behavior?

Perhaps putting Rc impls in Serde behind a cfg would be an acceptable middle ground?

[nox]

@alexbool: the reverse argument is that you could use #[serde(deserialize_with="val_to_arc")]

What about Rc<T> in other types? Option<Rc<T>>? HashSet<Rc<T>>?

[shahn]

I don't have an example for a public crate, but this ticket exists because I was using serde to serialize the state of a toy compiler and deserializing broke it completely (I was using pointer equality to check for equality of two objects behind an Rc, nested into a much larger struct). Since Serde allowed me to just derive Serialize, I assumed it would work correctly.

[nox]

I was using pointer equality to check for equality of two objects behind an Rc

This is not the default behaviour of PartialEq<Rc<T>> for Rc<T>, so allow me to argue that this breakage wasn't the serde impls' fault.

[shahn]

I don't think the point you're making is correct. I did not modify the way Rc checks equality at all (I could not do that in fact, because of coherence). Rc works for any T, completely independently of whether T's PartialEq impl is derived, missing, or custom-defined.

[dtolnay]

Thanks for the valuable feedback @shahn. Let me know whether you think #864 would have set more correct expectations around Serde's handling of Rc<T> and Arc<T>.

[shahn]

Yes definitely. Putting this behind a cfg transports the message that this might be what you want, but it might also break your assumptions. I think that's the right tradeoff. Plus it's a very easily fixed breaking change, which I hope won't be too hard for people to adapt to.

[dtolnay]

Cool. I am moving forward with the cfg and closing this issue but we have some time before this will be released, so I am open to continuing the discussion here if other people have strong opinions.

[whitequark]

@dtolnay Question. Given how cargo deduplicates dependencies, couldn't one accidentally opt into this feature by having a dependency that, as its implementation detail, requests the rc feature from serde?

[dtolnay]

That is correct. This is just a way to improve the chance that people understand what they are doing.

[shahn]

hm, that would be sad if a popular dependency used that feature. Are there better alternative ideas?

[edit]Ah. serde 1.0 is out, so this is probably it for now :)

[nox]

Yeah it shipped so it's too late. I don't understand why you think it would be sad, the existence of this feature is what is sad. :)

[oli-obk]

As I see it, enabling the feature by default is not a breaking change, so the feature is the most sensible choice in the presence of uncertainty. If you want to make sure you don't accidentally end up with a derived impl containing an rc, open a clippy issue.